Repository navigation
node:test should be able to detect a plan has not been met #56758
Description
Activity
If nobody beats me to it, I'll try to check it out ASAP
Reacted by James Sumners@pmarchini go for it. I think the only tricky part is:
The test should wait until the defined timeout for both asserts to resolve
Tests do not have a default timeout (we received push back over having a default test timeout when originally creating the test runner), so unmet plans would hang indefinitely when they currently do not. The simplest way to implement it without breaking anyone might be by adding a
timeoutorwaitoption tot.plan().Some other implementation notes (at least how I see it in my head):
- There are three places where
plan.actual++;is performed. This used to be a single place. We should create an internalcount()function that does that. - The
this.plan?.check();line should become anif (this.plan !== null)so that we only pay the cost if someone is using a test plan. - Inside that
ifstatement, we would want to do something likeawait SafePromiseRace([xxx, stopPromise]);, wherestopPromisealready exists in the code, andxxxis some Promise related to the test plan (possibly returned fromcheck()).
Reacted by Pietro Marchini- There are three places where
@jsumners, would you be able to provide a minimum repro? I'd then add it directly to our tests
'use strict' const test = require('node:test') const assert = require('node:assert') const http = require('node:http') const server = http.createServer((req, res) => { res.writeHead(200, { 'content-type': 'application/json' }) res.end(JSON.stringify({ data: 'ok' })) }) server.listen(0, '127.0.0.1', (listenError) => { assert.ifError(listenError) test.after(() => { server.close() }) test('with a plan', t => { t.plan(2) doRequest(payload => { t.assert.deepStrictEqual(payload, { data: 'ok' }) }) doRequest(payload => { t.assert.deepStrictEqual(payload, { data: 'ok' }) }) }) test('should pass', async t => { t.plan(2) await new Promise(resolve => { doRequest(payload => { t.assert.deepStrictEqual(payload, { data: 'ok' }) resolve() }) }) await new Promise(resolve => { doRequest(payload => { t.assert.deepStrictEqual(payload, { data: 'ok' }) resolve() }) }) }) }) function doRequest(cb) { const req = http.request( { host: '127.0.0.1', // server.address().address, port: server.address().port }, res => { let data = '' res.on('data', d => { data += d.toString() }) res.on('end', () => cb(JSON.parse(data))) } ) req.end() }
Reacted by Pietro MarchiniThe simplest way to implement it without breaking anyone might be by adding a
timeoutorwaitoption tot.plan().I hope we can define a default value, say
30_000, for this new plan option. Having to supply it for every plan will be a very poor developer experience.Having to supply it for every plan will be a very poor developer experience.
It doesn't seem like it's a very common option that needs to be on by default? My understanding is that Fastify has been using this feature for a while and this is the first time it has come up.
My two cents is that even if it does get a default value of
30_000or whatever, it should first land being disabled and then be enabled in a semver major bump to avoid breaking any existing code. I'll leave it up to the actual users ofplan()to decide on a good default value.Reacted by Pietro MarchiniMost of Fastify's tests that rely on a plan are synchronous and thus do not encounter this issue. It was very surprising to find that the plan is synchronous only.
Reacted by Gürgün Dayıoğlu@pmarchini should this be closed by #56765?
Reacted by Pietro Marchini
This issue stems from the conversation at fastify/fastify#5957 (review)
Given a test like:
The test should wait until the defined timeout for both asserts to resolve. If only one resolves within the time limit, the test should fail due to the plan not being completed. From the linked conversation, this is not happening.