From e9a7a7fff35b7e3fc26b6e3fdc025efd69f0ba49 Mon Sep 17 00:00:00 2001 From: Pietro Marchini Date: Fri, 14 Feb 2025 10:03:17 +0100 Subject: [PATCH 01/10] test_runner: refactor testPlan counter increse --- lib/internal/test_runner/test.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 489490baef32c6..52ffea589afef2 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -190,6 +190,10 @@ class TestPlan { ); } } + + increaseActualCount() { + this.actual++; + } } class TestContext { @@ -249,7 +253,7 @@ class TestContext { map.forEach((method, name) => { assert[name] = (...args) => { if (plan !== null) { - plan.actual++; + plan.increaseActualCount(); } return ReflectApply(method, this, args); }; @@ -260,7 +264,7 @@ class TestContext { // stacktrace from the correct starting point. function ok(...args) { if (plan !== null) { - plan.actual++; + plan.increaseActualCount(); } innerOk(ok, args.length, ...args); } @@ -296,7 +300,7 @@ class TestContext { const { plan } = this.#test; if (plan !== null) { - plan.actual++; + plan.increaseActualCount(); } const subtest = this.#test.createSubtest( From 9735384e63844baca05819dab171df27f54b0b0b Mon Sep 17 00:00:00 2001 From: Pietro Marchini Date: Fri, 14 Feb 2025 10:03:46 +0100 Subject: [PATCH 02/10] test_runner: add timeout support to test plan --- lib/internal/test_runner/test.js | 58 +++++++++++++--- .../output/test-runner-plan-timeout.js | 34 ++++++++++ .../output/test-runner-plan-timeout.snapshot | 68 +++++++++++++++++++ test/parallel/test-runner-output.mjs | 4 ++ 4 files changed, 154 insertions(+), 10 deletions(-) create mode 100644 test/fixtures/test-runner/output/test-runner-plan-timeout.js create mode 100644 test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 52ffea589afef2..98748af9daa0f9 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -123,7 +123,7 @@ function lazyAssertObject(harness) { return assertObj; } -function stopTest(timeout, signal) { +function stopTest(timeout, signal, reason) { const deferred = PromiseWithResolvers(); const abortListener = addAbortListener(signal, deferred.resolve); let timer; @@ -141,7 +141,7 @@ function stopTest(timeout, signal) { writable: true, value: PromisePrototypeThen(deferred.promise, () => { throw new ERR_TEST_FAILURE( - `test timed out after ${timeout}ms`, + reason || `test timed out after ${timeout}ms`, kTestTimeoutFailure, ); }), @@ -176,10 +176,26 @@ function testMatchesPattern(test, patterns) { } class TestPlan { - constructor(count) { + #timeoutPromise; + #testSignal; + + constructor(count, options = kEmptyObject) { validateUint32(count, 'count'); this.expected = count; this.actual = 0; + this.timeout = options.timeout; + + if (options.signal) { + this.#testSignal = options.signal; + } + if (this.timeout !== undefined) { + validateNumber(this.timeout, 'options.timeout', 0, TIMEOUT_MAX); + this.#timeoutPromise = stopTest( + this.timeout, + this.#testSignal, + `plan timed out after ${this.timeout}ms with ${this.actual} assertions when expecting ${this.expected}`, + ); + } } check() { @@ -194,6 +210,14 @@ class TestPlan { increaseActualCount() { this.actual++; } + + get timeoutPromise() { + return this.#timeoutPromise; + } + + hasTimeout() { + return this.#timeoutPromise !== undefined; + } } class TestContext { @@ -232,7 +256,7 @@ class TestContext { this.#test.diagnostic(message); } - plan(count) { + plan(count, options) { if (this.#test.plan !== null) { throw new ERR_TEST_FAILURE( 'cannot set plan more than once', @@ -240,7 +264,11 @@ class TestContext { ); } - this.#test.plan = new TestPlan(count); + this.#test.plan = new TestPlan(count, { + __proto__: null, + ...options, + signal: this.#test.signal, + }); } get assert() { @@ -972,11 +1000,12 @@ class Test extends AsyncResource { const runArgs = ArrayPrototypeSlice(args); ArrayPrototypeUnshift(runArgs, this.fn, ctx); + const promises = []; if (this.fn.length === runArgs.length - 1) { - // This test is using legacy Node.js error first callbacks. + // This test is using legacy Node.js error-first callbacks. const { promise, cb } = createDeferredCallback(); - ArrayPrototypePush(runArgs, cb); + const ret = ReflectApply(this.runInAsyncScope, this, runArgs); if (isPromise(ret)) { @@ -984,16 +1013,24 @@ class Test extends AsyncResource { 'passed a callback but also returned a Promise', kCallbackAndPromisePresent, )); - await SafePromiseRace([ret, stopPromise]); + ArrayPrototypePush(promises, ret); } else { - await SafePromiseRace([PromiseResolve(promise), stopPromise]); + ArrayPrototypePush(promises, PromiseResolve(promise)); } } else { // This test is synchronous or using Promises. const promise = ReflectApply(this.runInAsyncScope, this, runArgs); - await SafePromiseRace([PromiseResolve(promise), stopPromise]); + ArrayPrototypePush(promises, PromiseResolve(promise)); + } + + ArrayPrototypePush(promises, stopPromise); + if (this.plan?.hasTimeout()) { + ArrayPrototypePush(promises, this.plan.timeoutPromise); } + // Wait for the race to finish + await SafePromiseRace(promises); + this[kShouldAbort](); if (this.subtestsPromise !== null) { @@ -1018,6 +1055,7 @@ class Test extends AsyncResource { try { await after(); } catch { /* Ignore error. */ } } finally { stopPromise?.[SymbolDispose](); + this.plan?.timeoutPromise?.[SymbolDispose](); // Do not abort hooks and the root test as hooks instance are shared between tests suite so aborting them will // cause them to not run for further tests. diff --git a/test/fixtures/test-runner/output/test-runner-plan-timeout.js b/test/fixtures/test-runner/output/test-runner-plan-timeout.js new file mode 100644 index 00000000000000..f962d6f9d940ae --- /dev/null +++ b/test/fixtures/test-runner/output/test-runner-plan-timeout.js @@ -0,0 +1,34 @@ +'use strict'; +const { describe, it } = require('node:test'); +const { setTimeout } = require('node:timers/promises'); + +describe('planning with timeout', () => { + it(`planning should pass if plan it's correct`, async (t) => { + t.plan(1, { timeout: 500_000_000 }); + t.assert.ok(true); + }); + + it(`planning should fail if plan it's incorrect`, async (t) => { + t.plan(1, { timeout: 500_000_000 }); + t.assert.ok(true); + t.assert.ok(true); + }); + + it('planning with timeout', async (t) => { + t.plan(1, { timeout: 2000 }); + + while (true) { + await setTimeout(5000); + } + }); + + it('nested planning with timeout', async (t) => { + t.plan(1, { timeout: 2000 }); + + t.test('nested', async (t) => { + while (true) { + await setTimeout(5000); + } + }); + }); +}); diff --git a/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot b/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot new file mode 100644 index 00000000000000..854117979ba2f0 --- /dev/null +++ b/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot @@ -0,0 +1,68 @@ +TAP version 13 +# Subtest: planning with timeout + # Subtest: planning should pass if plan it's correct + ok 1 - planning should pass if plan it's correct + --- + duration_ms: * + type: 'test' + ... + # Subtest: planning should fail if plan it's incorrect + not ok 2 - planning should fail if plan it's incorrect + --- + duration_ms: * + type: 'test' + location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):3' + failureType: 'testCodeFailure' + error: 'plan expected 1 assertions but received 2' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: planning with timeout + not ok 3 - planning with timeout + --- + duration_ms: * + type: 'test' + location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):3' + failureType: 'testTimeoutFailure' + error: 'plan timed out after 2000ms with 0 assertions when expecting 1' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: nested planning with timeout + # Subtest: nested + not ok 1 - nested + --- + duration_ms: * + type: 'test' + location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):7' + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + 1..1 + not ok 4 - nested planning with timeout + --- + duration_ms: * + type: 'test' + location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):3' + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' + ... + 1..4 +not ok 1 - planning with timeout + --- + duration_ms: * + type: 'suite' + location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):1' + failureType: 'subtestsFailed' + error: '3 subtests failed' + code: 'ERR_TEST_FAILURE' + ... +1..1 +# tests 5 +# suites 1 +# pass 1 +# fail 2 +# cancelled 2 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index df45456a7eb428..424337ca186393 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -239,6 +239,10 @@ const tests = [ name: 'test-runner/output/test-runner-watch-spec.mjs', transform: specTransform, }, + { + name: 'test-runner/output/test-runner-plan-timeout.js', + flags: ['--test-reporter=tap', '--test-force-exit'], + }, process.features.inspector ? { name: 'test-runner/output/coverage_failure.js', flags: ['--test-reporter=tap', '--test-coverage-exclude=!test/**'], From c942eb9cc1bcb7384dd0de5938cb55ff9ee16d76 Mon Sep 17 00:00:00 2001 From: Pietro Marchini Date: Fri, 14 Feb 2025 10:03:46 +0100 Subject: [PATCH 03/10] test_runner: add input validation tests and refactor planning with wait --- lib/internal/test_runner/test.js | 123 +++++++++++++----- .../output/test-runner-plan-timeout.js | 81 ++++++++---- .../output/test-runner-plan-timeout.snapshot | 81 +++++++----- .../test-runner/output/test-runner-plan.js | 33 ++++- .../output/test-runner-plan.snapshot | 55 +++++--- 5 files changed, 267 insertions(+), 106 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 98748af9daa0f9..582b76d7e14e85 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -176,47 +176,104 @@ function testMatchesPattern(test, patterns) { } class TestPlan { - #timeoutPromise; - #testSignal; + #waitIndefinitely = false; constructor(count, options = kEmptyObject) { validateUint32(count, 'count'); + validateObject(options, 'options'); + this.expected = count; this.actual = 0; - this.timeout = options.timeout; - if (options.signal) { - this.#testSignal = options.signal; + switch (typeof options.wait) { + case 'boolean': + this.wait = options.wait; + this.#waitIndefinitely = true; + break; + + case 'number': + validateNumber(options.wait, 'options.wait', 0, TIMEOUT_MAX); + this.wait = options.wait; + break; + + default: + if (options.wait !== undefined) { + throw new ERR_INVALID_ARG_TYPE('options.wait', ['boolean', 'number'], options.wait); + } } - if (this.timeout !== undefined) { - validateNumber(this.timeout, 'options.timeout', 0, TIMEOUT_MAX); - this.#timeoutPromise = stopTest( - this.timeout, - this.#testSignal, - `plan timed out after ${this.timeout}ms with ${this.actual} assertions when expecting ${this.expected}`, - ); + } + + #planMet() { + return this.actual === this.expected; + } + + #startPoller() { + const { promise, resolve, reject } = PromiseWithResolvers(); + const noError = Symbol(); + let pollerId; + let timeoutId; + const interval = 50; + + const done = (err, result) => { + clearTimeout(pollerId); + timeoutId ?? clearTimeout(timeoutId); + + if (err === noError) { + resolve(result); + } else { + reject(err); + } + }; + + // If the plan has a maximum wait time, then we need to set a timeout + // Otherwise, the plan will wait indefinitely + if (!this.#waitIndefinitely) { + timeoutId = setTimeout(() => { + const err = new ERR_TEST_FAILURE( + `plan timed out after ${this.wait}ms with ${this.actual} assertions when expecting ${this.expected}`, + kTestTimeoutFailure, + ); + // The pooler has timed out, the test should fail + done(err); + }, this.wait); } + + const poller = async () => { + if (this.#planMet()) { + done(noError); + } else { + pollerId = setTimeout(poller, interval); + } + }; + + poller(); + return promise; } check() { - if (this.actual !== this.expected) { + if (this.#planMet()) { + // If the plan has been met, then there is no need to check for a timeout. + return; + } + // The plan has not been met + if (!this.hasTimeout()) { + // If the plan doesn't have a timeout, then the test should fail immediately. throw new ERR_TEST_FAILURE( `plan expected ${this.expected} assertions but received ${this.actual}`, kTestCodeFailure, ); } + // If the plan has a timeout, then the test is still running + // we need to pool until the timeout is reached or the plan is met + return this.#startPoller(); } - increaseActualCount() { + count() { this.actual++; } - get timeoutPromise() { - return this.#timeoutPromise; - } - hasTimeout() { - return this.#timeoutPromise !== undefined; + return this.wait !== undefined; } } @@ -256,7 +313,7 @@ class TestContext { this.#test.diagnostic(message); } - plan(count, options) { + plan(count, options = kEmptyObject) { if (this.#test.plan !== null) { throw new ERR_TEST_FAILURE( 'cannot set plan more than once', @@ -264,11 +321,7 @@ class TestContext { ); } - this.#test.plan = new TestPlan(count, { - __proto__: null, - ...options, - signal: this.#test.signal, - }); + this.#test.plan = new TestPlan(count, options); } get assert() { @@ -281,7 +334,7 @@ class TestContext { map.forEach((method, name) => { assert[name] = (...args) => { if (plan !== null) { - plan.increaseActualCount(); + plan.count(); } return ReflectApply(method, this, args); }; @@ -292,7 +345,7 @@ class TestContext { // stacktrace from the correct starting point. function ok(...args) { if (plan !== null) { - plan.increaseActualCount(); + plan.count(); } innerOk(ok, args.length, ...args); } @@ -328,7 +381,7 @@ class TestContext { const { plan } = this.#test; if (plan !== null) { - plan.increaseActualCount(); + plan.count(); } const subtest = this.#test.createSubtest( @@ -1024,9 +1077,6 @@ class Test extends AsyncResource { } ArrayPrototypePush(promises, stopPromise); - if (this.plan?.hasTimeout()) { - ArrayPrototypePush(promises, this.plan.timeoutPromise); - } // Wait for the race to finish await SafePromiseRace(promises); @@ -1037,7 +1087,15 @@ class Test extends AsyncResource { await SafePromiseRace([this.subtestsPromise.promise, stopPromise]); } - this.plan?.check(); + if (this.plan !== null) { + const checkPollPromise = this.plan?.check(); + // If the plan returns a promise, then the plan is polling and we need to wait for it to finish + // or the test to stop + if (checkPollPromise) { + await SafePromiseRace([checkPollPromise, stopPromise]); + } + } + this.pass(); await afterEach(); await after(); @@ -1055,7 +1113,6 @@ class Test extends AsyncResource { try { await after(); } catch { /* Ignore error. */ } } finally { stopPromise?.[SymbolDispose](); - this.plan?.timeoutPromise?.[SymbolDispose](); // Do not abort hooks and the root test as hooks instance are shared between tests suite so aborting them will // cause them to not run for further tests. diff --git a/test/fixtures/test-runner/output/test-runner-plan-timeout.js b/test/fixtures/test-runner/output/test-runner-plan-timeout.js index f962d6f9d940ae..ed712c5c820ea6 100644 --- a/test/fixtures/test-runner/output/test-runner-plan-timeout.js +++ b/test/fixtures/test-runner/output/test-runner-plan-timeout.js @@ -1,34 +1,65 @@ 'use strict'; const { describe, it } = require('node:test'); -const { setTimeout } = require('node:timers/promises'); +const { platformTimeout } = require('../../../common'); -describe('planning with timeout', () => { - it(`planning should pass if plan it's correct`, async (t) => { - t.plan(1, { timeout: 500_000_000 }); - t.assert.ok(true); +describe('planning with wait', () => { + it('planning with wait and passing', async (t) => { + t.plan(1, { wait: platformTimeout(5000) }); + + const asyncActivity = () => { + setTimeout(() => { + t.assert.ok(true); + }, platformTimeout(250)); + }; + + asyncActivity(); }); - - it(`planning should fail if plan it's incorrect`, async (t) => { - t.plan(1, { timeout: 500_000_000 }); - t.assert.ok(true); - t.assert.ok(true); + + it('planning with wait and failing', async (t) => { + t.plan(1, { wait: platformTimeout(5000) }); + + const asyncActivity = () => { + setTimeout(() => { + t.assert.ok(false); + }, platformTimeout(250)); + }; + + asyncActivity(); + }); + + it('planning wait time expires before plan is met', async (t) => { + t.plan(2, { wait: platformTimeout(500) }); + + const asyncActivity = () => { + setTimeout(() => { + t.assert.ok(true); + }, platformTimeout(50_000_000)); + }; + + asyncActivity(); }); - - it('planning with timeout', async (t) => { - t.plan(1, { timeout: 2000 }); - - while (true) { - await setTimeout(5000); - } + + it(`planning with wait "options.wait : true" and passing`, async (t) => { + t.plan(1, { wait: true }); + + const asyncActivity = () => { + setTimeout(() => { + t.assert.ok(true); + }, platformTimeout(250)); + }; + + asyncActivity(); }); - it('nested planning with timeout', async (t) => { - t.plan(1, { timeout: 2000 }); - - t.test('nested', async (t) => { - while (true) { - await setTimeout(5000); - } - }); + it(`planning with wait "options.wait : true" and failing`, async (t) => { + t.plan(1, { wait: true }); + + const asyncActivity = () => { + setTimeout(() => { + t.assert.ok(false); + }, platformTimeout(250)); + }; + + asyncActivity(); }); }); diff --git a/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot b/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot index 854117979ba2f0..32d7c244303ee9 100644 --- a/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot +++ b/test/fixtures/test-runner/output/test-runner-plan-timeout.snapshot @@ -1,54 +1,73 @@ TAP version 13 -# Subtest: planning with timeout - # Subtest: planning should pass if plan it's correct - ok 1 - planning should pass if plan it's correct +# Subtest: planning with wait + # Subtest: planning with wait and passing + ok 1 - planning with wait and passing --- duration_ms: * type: 'test' ... - # Subtest: planning should fail if plan it's incorrect - not ok 2 - planning should fail if plan it's incorrect + # Subtest: planning with wait and failing + not ok 2 - planning with wait and failing --- duration_ms: * type: 'test' location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):3' - failureType: 'testCodeFailure' - error: 'plan expected 1 assertions but received 2' - code: 'ERR_TEST_FAILURE' + failureType: 'uncaughtException' + error: |- + The expression evaluated to a falsy value: + + t.assert.ok(false) + + code: 'ERR_ASSERTION' + name: 'AssertionError' + expected: true + actual: false + operator: '==' + stack: |- + * + * + * ... - # Subtest: planning with timeout - not ok 3 - planning with timeout + # Subtest: planning wait time expires before plan is met + not ok 3 - planning wait time expires before plan is met --- duration_ms: * type: 'test' location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):3' failureType: 'testTimeoutFailure' - error: 'plan timed out after 2000ms with 0 assertions when expecting 1' + error: 'plan timed out after 500ms with 0 assertions when expecting 2' code: 'ERR_TEST_FAILURE' ... - # Subtest: nested planning with timeout - # Subtest: nested - not ok 1 - nested - --- - duration_ms: * - type: 'test' - location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):7' - failureType: 'cancelledByParent' - error: 'test did not finish before its parent and was cancelled' - code: 'ERR_TEST_FAILURE' - ... - 1..1 - not ok 4 - nested planning with timeout + # Subtest: planning with wait "options.wait : true" and passing + ok 4 - planning with wait "options.wait : true" and passing + --- + duration_ms: * + type: 'test' + ... + # Subtest: planning with wait "options.wait : true" and failing + not ok 5 - planning with wait "options.wait : true" and failing --- duration_ms: * type: 'test' location: '/test/fixtures/test-runner/output/test-runner-plan-timeout.js:(LINE):3' - failureType: 'subtestsFailed' - error: '1 subtest failed' - code: 'ERR_TEST_FAILURE' + failureType: 'uncaughtException' + error: |- + The expression evaluated to a falsy value: + + t.assert.ok(false) + + code: 'ERR_ASSERTION' + name: 'AssertionError' + expected: true + actual: false + operator: '==' + stack: |- + * + * + * ... - 1..4 -not ok 1 - planning with timeout + 1..5 +not ok 1 - planning with wait --- duration_ms: * type: 'suite' @@ -60,9 +79,9 @@ not ok 1 - planning with timeout 1..1 # tests 5 # suites 1 -# pass 1 +# pass 2 # fail 2 -# cancelled 2 +# cancelled 1 # skipped 0 # todo 0 # duration_ms * diff --git a/test/fixtures/test-runner/output/test-runner-plan.js b/test/fixtures/test-runner/output/test-runner-plan.js index 2f4cce48d54fd5..4d24ec6b27cd05 100644 --- a/test/fixtures/test-runner/output/test-runner-plan.js +++ b/test/fixtures/test-runner/output/test-runner-plan.js @@ -1,7 +1,36 @@ 'use strict'; -const { test } = require('node:test'); +const { test, suite } = require('node:test'); const { Readable } = require('node:stream'); +suite('input validation', () => { + test('throws if options is not an object', (t) => { + t.assert.throws(() => { + t.plan(1, null); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "options" argument must be of type object/, + }); + }); + + test('throws if options.wait is not a number or a boolean', (t) => { + t.assert.throws(() => { + t.plan(1, { wait: 'foo' }); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "options\.wait" property must be one of type boolean or number\. Received type string/, + }); + }); + + test('throws if count is not a number', (t) => { + t.assert.throws(() => { + t.plan('foo'); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "count" argument must be of type number/, + }); + }); +}); + test('test planning basic', (t) => { t.plan(2); t.assert.ok(true); @@ -76,4 +105,4 @@ test('planning with streams', (t, done) => { stream.on('end', () => { done(); }); -}) +}); diff --git a/test/fixtures/test-runner/output/test-runner-plan.snapshot b/test/fixtures/test-runner/output/test-runner-plan.snapshot index bbd718663f2438..0300384d31ec91 100644 --- a/test/fixtures/test-runner/output/test-runner-plan.snapshot +++ b/test/fixtures/test-runner/output/test-runner-plan.snapshot @@ -1,12 +1,37 @@ TAP version 13 +# Subtest: input validation + # Subtest: throws if options is not an object + ok 1 - throws if options is not an object + --- + duration_ms: * + type: 'test' + ... + # Subtest: throws if options.wait is not a number or a boolean + ok 2 - throws if options.wait is not a number or a boolean + --- + duration_ms: * + type: 'test' + ... + # Subtest: throws if count is not a number + ok 3 - throws if count is not a number + --- + duration_ms: * + type: 'test' + ... + 1..3 +ok 1 - input validation + --- + duration_ms: * + type: 'suite' + ... # Subtest: test planning basic -ok 1 - test planning basic +ok 2 - test planning basic --- duration_ms: * type: 'test' ... # Subtest: less assertions than planned -not ok 2 - less assertions than planned +not ok 3 - less assertions than planned --- duration_ms: * type: 'test' @@ -16,7 +41,7 @@ not ok 2 - less assertions than planned code: 'ERR_TEST_FAILURE' ... # Subtest: more assertions than planned -not ok 3 - more assertions than planned +not ok 4 - more assertions than planned --- duration_ms: * type: 'test' @@ -33,7 +58,7 @@ not ok 3 - more assertions than planned type: 'test' ... 1..1 -ok 4 - subtesting +ok 5 - subtesting --- duration_ms: * type: 'test' @@ -46,7 +71,7 @@ ok 4 - subtesting type: 'test' ... 1..1 -ok 5 - subtesting correctly +ok 6 - subtesting correctly --- duration_ms: * type: 'test' @@ -59,13 +84,13 @@ ok 5 - subtesting correctly type: 'test' ... 1..1 -ok 6 - correctly ignoring subtesting plan +ok 7 - correctly ignoring subtesting plan --- duration_ms: * type: 'test' ... # Subtest: failing planning by options -not ok 7 - failing planning by options +not ok 8 - failing planning by options --- duration_ms: * type: 'test' @@ -75,7 +100,7 @@ not ok 7 - failing planning by options code: 'ERR_TEST_FAILURE' ... # Subtest: not failing planning by options -ok 8 - not failing planning by options +ok 9 - not failing planning by options --- duration_ms: * type: 'test' @@ -88,13 +113,13 @@ ok 8 - not failing planning by options type: 'test' ... 1..1 -ok 9 - subtest planning by options +ok 10 - subtest planning by options --- duration_ms: * type: 'test' ... # Subtest: failing more assertions than planned -not ok 10 - failing more assertions than planned +not ok 11 - failing more assertions than planned --- duration_ms: * type: 'test' @@ -104,15 +129,15 @@ not ok 10 - failing more assertions than planned code: 'ERR_TEST_FAILURE' ... # Subtest: planning with streams -ok 11 - planning with streams +ok 12 - planning with streams --- duration_ms: * type: 'test' ... -1..11 -# tests 15 -# suites 0 -# pass 11 +1..12 +# tests 18 +# suites 1 +# pass 14 # fail 4 # cancelled 0 # skipped 0 From 927ffdb2a606381e8d59bb1f675f005626b0ce2f Mon Sep 17 00:00:00 2001 From: Pietro Marchini Date: Fri, 14 Feb 2025 10:03:46 +0100 Subject: [PATCH 04/10] test_runner: refactor TestPlan --- lib/internal/test_runner/test.js | 57 ++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 582b76d7e14e85..011b562987fb7f 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -179,23 +179,27 @@ class TestPlan { #waitIndefinitely = false; constructor(count, options = kEmptyObject) { - validateUint32(count, 'count'); - validateObject(options, 'options'); - + this.#validateConstructorArgs(count, options); this.expected = count; this.actual = 0; + this.#setWaitOption(options); + } + #validateConstructorArgs(count, options) { + validateUint32(count, 'count'); + validateObject(options, 'options'); + } + + #setWaitOption(options) { switch (typeof options.wait) { case 'boolean': this.wait = options.wait; this.#waitIndefinitely = true; break; - case 'number': validateNumber(options.wait, 'options.wait', 0, TIMEOUT_MAX); this.wait = options.wait; break; - default: if (options.wait !== undefined) { throw new ERR_INVALID_ARG_TYPE('options.wait', ['boolean', 'number'], options.wait); @@ -211,12 +215,12 @@ class TestPlan { const { promise, resolve, reject } = PromiseWithResolvers(); const noError = Symbol(); let pollerId; - let timeoutId; + let maxWaitingTimeId; const interval = 50; const done = (err, result) => { clearTimeout(pollerId); - timeoutId ?? clearTimeout(timeoutId); + maxWaitingTimeId ?? clearTimeout(maxWaitingTimeId); if (err === noError) { resolve(result); @@ -225,17 +229,10 @@ class TestPlan { } }; - // If the plan has a maximum wait time, then we need to set a timeout - // Otherwise, the plan will wait indefinitely + // If the plan has a maximum wait time, set a timeout. + // Otherwise, the plan will wait indefinitely. if (!this.#waitIndefinitely) { - timeoutId = setTimeout(() => { - const err = new ERR_TEST_FAILURE( - `plan timed out after ${this.wait}ms with ${this.actual} assertions when expecting ${this.expected}`, - kTestTimeoutFailure, - ); - // The pooler has timed out, the test should fail - done(err); - }, this.wait); + maxWaitingTimeId = this.#setMaxWaitingTime(done); } const poller = async () => { @@ -250,21 +247,30 @@ class TestPlan { return promise; } + #setMaxWaitingTime(done) { + return setTimeout(() => { + const err = new ERR_TEST_FAILURE( + `Plan timed out after ${this.wait}ms with ${this.actual} assertions when expecting ${this.expected}`, + kTestTimeoutFailure, + ); + // The poller has timed out; the test should fail. + done(err); + }, this.wait); + } + check() { if (this.#planMet()) { - // If the plan has been met, then there is no need to check for a timeout. + // If the plan has been met, there is no need to check for a timeout. return; } - // The plan has not been met - if (!this.hasTimeout()) { - // If the plan doesn't have a timeout, then the test should fail immediately. + // The plan has not been met. + if (!this.#isWaitDefined()) { + // If the plan doesn't have a wait time defined, the test should fail immediately. throw new ERR_TEST_FAILURE( - `plan expected ${this.expected} assertions but received ${this.actual}`, + `Plan expected ${this.expected} assertions but received ${this.actual}`, kTestCodeFailure, ); } - // If the plan has a timeout, then the test is still running - // we need to pool until the timeout is reached or the plan is met return this.#startPoller(); } @@ -272,11 +278,12 @@ class TestPlan { this.actual++; } - hasTimeout() { + #isWaitDefined() { return this.wait !== undefined; } } + class TestContext { #assert; #test; From 066559778e4dd1cb253a5e6d04addcd7d08d7c94 Mon Sep 17 00:00:00 2001 From: Pietro Marchini Date: Fri, 14 Feb 2025 10:03:46 +0100 Subject: [PATCH 05/10] doc: add plan wait option to test runner documentation --- doc/api/test.md | 29 +++++++++++++++++++ lib/internal/test_runner/test.js | 20 ++++++++----- .../output/test-runner-plan-timeout.js | 12 ++++++++ .../output/test-runner-plan-timeout.snapshot | 18 +++++++++--- 4 files changed, 68 insertions(+), 11 deletions(-) diff --git a/doc/api/test.md b/doc/api/test.md index 2014981613fded..2caff91dd1f669 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -3388,6 +3388,10 @@ added: - v22.2.0 - v20.15.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/56765 + description: Add the `options` parameter. - version: - v23.4.0 - v22.13.0 @@ -3396,6 +3400,13 @@ changes: --> * `count` {number} The number of assertions and subtests that are expected to run. +* `options` {Object} _(Optional)_ Additional options for the plan. + * `wait` {boolean|number} The wait time for the plan: + * If `true`, the plan waits indefinitely for all assertions and subtests to run. + * If a number, it specifies the maximum wait time in milliseconds + before timing out while waiting for expected assertions and subtests to be matched. + If the timeout is reached, the test will fail. + **Default:** `false`. This function is used to set the number of assertions and subtests that are expected to run within the test. If the number of assertions and subtests that run does not match the @@ -3434,6 +3445,24 @@ test('planning with streams', (t, done) => { }); ``` +When using the `wait` option, you can control how long the test will wait for the expected assertions. +For example, setting a maximum wait time ensures that the test will wait for asynchronous assertions +to complete within the specified timeframe: + +```js +test('plan with wait: 2000 waits for async assertions', (t) => { + t.plan(1, { wait: 2000 }); // Waits for up to 2 seconds for the assertion to complete. + + const asyncActivity = () => { + setTimeout(() => { + t.assert.ok(true, 'Async assertion completed within the wait time'); + }, 1000); // Completes after 1 second, within the 2-second wait time. + }; + + asyncActivity(); // The test will pass because the assertion is completed in time. +}); +``` + ### `context.runOnly(shouldRunOnlyTests)` * `count` {number} The number of assertions and subtests that are expected to run. -* `options` {Object} _(Optional)_ Additional options for the plan. +* `options` {Object} Additional options for the plan. * `wait` {boolean|number} The wait time for the plan: * If `true`, the plan waits indefinitely for all assertions and subtests to run. + * If `false`, the plan performs an immediate check after the test function completes, + without waiting for any pending assertions or subtests. + Any assertions or subtests that complete after this check will not be counted towards the plan. * If a number, it specifies the maximum wait time in milliseconds before timing out while waiting for expected assertions and subtests to be matched. If the timeout is reached, the test will fail. @@ -3463,6 +3466,8 @@ test('plan with wait: 2000 waits for async assertions', (t) => { }); ``` +Note: If a `wait` timeout is specified, it begins counting down only after the test function finishes executing. + ### `context.runOnly(shouldRunOnlyTests)`