node
node copied to clipboard
nodejs
test_runner: support test plans
I picked up the work from this comment https://github.com/nodejs/node/issues/44125#issuecomment-2007437877 by @cjihrig to add support for test planning
Can you please elaborate on the use cases of this feature? We already error if a test never ends and the event loop is drained - in what scenario should I worry not all assertions will be checked? and what makes assertions different than any other piece of code inside the test?
It makes sure that an assertion is called inside a callback, when the callback might not be invoked. This feature is actually common in a lot of testing libraries such as
- tap https://node-tap.org/basics/#planned-assertion-counts
- jest https://jestjs.io/docs/expect#expectassertionsnumber
- vitest https://vitest.dev/api/expect.html#expect-assertions
test('make sure callback is invoked', (t) => {
t.plan(1)
server.listen(0, (r) => {
// assert something in here
t.ok(true)
});
});
With plan you are sure than the callback was invoked and the assertion was performed, otherwise the test would pass anyways.
I am -0.5 on adding this.
if one wants to assert his code run they can use mock.fn/mock.method and assert it was actually called. I don't think assertions are very different in that aspect
Hey folks, Marco is working with me and I suggested that he works on this feature that is missing from the Node.js test runner and it's necessary in certain specific cases.
The use case is fairly specific and it pertains callback-based APIs. When the callback is called and you need to make assertions there, there is no easy way to do that except telling the test that it should not end before X number of assertions have been made (bar a test timeout).
Let's take this example from the Fastify docs:
const tap = require('tap')
const buildFastify = require('./app')
tap.test('GET `/` route', t => {
t.plan(4)
const fastify = buildFastify()
// At the end of your tests it is highly recommended to call `.close()`
// to ensure that all connections to external services get closed.
t.teardown(() => fastify.close())
fastify.inject({
method: 'GET',
url: '/'
}, (err, response) => {
t.error(err)
t.equal(response.statusCode, 200)
t.equal(response.headers['content-type'], 'application/json; charset=utf-8')
t.same(response.json(), { hello: 'world' })
})
})
Let's forget for a second that fastify.inject also has a promise API. Without t.plan(4) you have no easy way to make sure that the assertions in the callback have been invoked. E.g. if inject due to a bug didn't invoke the callback, this test would pass just fine if you didn't have a way to tell the test that you're expecting 4 assertions.
I hope this clarifies why this feature is necessary.
I think this feature is needed.
@marco-ippolito can you add a test that verifies what happens if there were more assertion than the plan?
This is missing a way for other assertion frameworks to tap into the test planning. If I recall correctly, the need for this was what stopped @cjihrig working on this feature.
This is missing a way for other assertion frameworks to tap into the test planning. If I recall correctly, the need for this was what stopped @cjihrig working on this feature.
I agree, this is needed.
But I think we should reverse the POV of the implementation.
In other words, we should provide a mechanism for frameworks to increase the execution plan count (like a async hooks or similar APIs). This way people interested can work on a PR on the target framework with way more knowledge of the codebase than us. As a side effect, this will allow us not to require to use the t.assert interface as we can modify the assert module with the same API others use.
Do we need to make it extensible from the get go? Can we leave it to a second iteration on the feature? Not sure how much effort it would be to design it in a way that other test frameworks can hook into it.
I would avoid enforcing people to use t.assert and then move away from it (since, imho, is suboptimal DX).
So if we find a way to do that from day zero it would be great.
Note that a very trivial initial implementation based a global symbol and AsynchronousLocalStorage would make the trick.
I was going to avoid weighing in on this issue since my code was taken without permission or even a heads up, but this conversation appears to be going off the rails a bit.
The idea was to have t.assert as the interface for this feature. It is currently populated with methods from node:assert, but in the future users could register other functions (and potentially even unregister existing functions). That is how other assertion libraries could be supported, and why lazyAssertObject() returns a Map. So, in the future, someone could create something like t.assert.expect(), where expect() is a totally different assertion library. There shouldn't be any need for a global symbol or AsynchronousLocalStorage - just an API to update the contents of the Map.
@cjihrig I see your idea and it makes totally sense.
But I suspect that the use of t.assert... will slow down adoption of this feature as it will require code changes, while with automagic support people can just insert the plan count on the test definitions and then they are good to go.
I was going to avoid weighing in on this issue since my code was taken without permission or even a heads up, but this conversation appears to be going off the rails a bit.
The idea was to have
t.assertas the interface for this feature. It is currently populated with methods fromnode:assert, but in the future users could register other functions (and potentially even unregister existing functions). That is how other assertion libraries could be supported, and whylazyAssertObject()returns aMap. So, in the future, someone could create something liket.assert.expect(), whereexpect()is a totally different assertion library. There shouldn't be any need for a global symbol orAsynchronousLocalStorage- just an API to update the contents of theMap.
Sorry for taking your commit, it was done in good faith as I think this feature would be beneficial to the community. I tried to reach out on slack or twitter but I found out it was not possible. Sorry again I'm closing the PR
@marco-ippolito it's OK, and I do think this feature should be added (in the way that this PR implements it). I urge you to consider reopening this.
We could expose a function t.incrementPlanCounter that libraries can use in their custom assertions
Exactly. But since in the library context they have no t object, we also have to provide them a way to obtain one.
Even if it provide a way to expose the increment tool.
I don't see a good DX for external assertion library without monkey-patching or passing around the t context.
As I remembered, the test runner is built for extendible in beginning. But the current one is hard to extend.
For example, I made a tools to remove the await of test in sub-test that requires a terrible monkey-patching of test context.
https://github.com/kaka-ng/nodejs/blob/eccfba087e7a8f1d2515e34a2936d1c8441a6882/packages/unit/lib/index.ts#L84-L171
Personally, I suggest to group all extensibility problem into another PR or issue.
I agree to go ahead with this and bring up extensibility later. I think the API couldn't be any better because it's the one that people are already familiar with.
Failed to start CI
⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8998448587
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@cjihrig for some reason it squashed the commit and set myself as author, I wanted to give you full credit to you on this Amended in https://github.com/nodejs/node/commit/0c83f80ff32de94bb3fbeac7a91ee04c2ab4237a
This PR adds a feature so it should be labeled https://github.com/nodejs/node/labels/semver-minor (this is not supposed to be done by releasers).
