From 825bc945e4fdd57be7e08409a45951629ebcf8fd Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sun, 19 Jan 2025 19:42:22 -0500 Subject: [PATCH] test_runner: remove promises returned by test() This commit updates the test() and suite() APIs to no longer return a Promise. Fixes: https://github.com/nodejs/node/issues/51292 --- doc/api/test.md | 32 ++++++------------- lib/internal/test_runner/harness.js | 9 +++--- .../test-runner-coverage-source-map.js | 2 +- test/parallel/test-runner-misc.js | 3 +- test/parallel/test-runner-typechecking.js | 24 ++++---------- 5 files changed, 23 insertions(+), 47 deletions(-) diff --git a/doc/api/test.md b/doc/api/test.md index 58a945df02b88f..c165efeb48d1b8 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -1407,6 +1407,11 @@ run({ files: [path.resolve('./tests/test.js')] }) added: - v22.0.0 - v20.13.0 +changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/56664 + description: This function no longer returns a `Promise`. --> * `name` {string} The name of the suite, which is displayed when reporting test @@ -1417,7 +1422,6 @@ added: * `fn` {Function|AsyncFunction} The suite function declaring nested tests and suites. The first argument to this function is a [`SuiteContext`][] object. **Default:** A no-op function. -* Returns: {Promise} Immediately fulfilled with `undefined`. The `suite()` function is imported from the `node:test` module. @@ -1461,6 +1465,10 @@ added: - v18.0.0 - v16.17.0 changes: + - version: + - REPLACEME + pr-url: https://github.com/nodejs/node/pull/56664 + description: This function no longer returns a `Promise`. - version: - v20.2.0 - v18.17.0 @@ -1510,8 +1518,6 @@ changes: to this function is a [`TestContext`][] object. If the test uses callbacks, the callback function is passed as the second argument. **Default:** A no-op function. -* Returns: {Promise} Fulfilled with `undefined` once - the test completes, or immediately if the test runs within a suite. The `test()` function is the value imported from the `test` module. Each invocation of this function results in reporting the test to the {TestsStream}. @@ -1520,26 +1526,6 @@ The `TestContext` object passed to the `fn` argument can be used to perform actions related to the current test. Examples include skipping the test, adding additional diagnostic information, or creating subtests. -`test()` returns a `Promise` that fulfills once the test completes. -if `test()` is called within a suite, it fulfills immediately. -The return value can usually be discarded for top level tests. -However, the return value from subtests should be used to prevent the parent -test from finishing first and cancelling the subtest -as shown in the following example. - -```js -test('top level test', async (t) => { - // The setTimeout() in the following subtest would cause it to outlive its - // parent test if 'await' is removed on the next line. Once the parent test - // completes, it will cancel any outstanding subtests. - await t.test('longer running subtest', async (t) => { - return new Promise((resolve, reject) => { - setTimeout(resolve, 1000); - }); - }); -}); -``` - The `timeout` option can be used to fail the test if it takes longer than `timeout` milliseconds to complete. However, it is not a reliable mechanism for canceling tests because a running test might block the application thread and diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index b4f46ecdec5586..ab8d1c24ae6d52 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -312,11 +312,12 @@ function runInParentContext(Factory) { function run(name, options, fn, overrides) { const parent = testResources.get(executionAsyncId()) || lazyBootstrapRoot(); const subtest = parent.createSubtest(Factory, name, options, fn, overrides); + if (parent instanceof Suite) { - return PromiseResolve(); + return; } - return startSubtestAfterBootstrap(subtest); + startSubtestAfterBootstrap(subtest); } const test = (name, options, fn) => { @@ -325,7 +326,7 @@ function runInParentContext(Factory) { loc: getCallerLocation(), }; - return run(name, options, fn, overrides); + run(name, options, fn, overrides); }; ArrayPrototypeForEach(['skip', 'todo', 'only'], (keyword) => { test[keyword] = (name, options, fn) => { @@ -335,7 +336,7 @@ function runInParentContext(Factory) { loc: getCallerLocation(), }; - return run(name, options, fn, overrides); + run(name, options, fn, overrides); }; }); return test; diff --git a/test/parallel/test-runner-coverage-source-map.js b/test/parallel/test-runner-coverage-source-map.js index e3b0676a557a9f..3b481f457dd3e6 100644 --- a/test/parallel/test-runner-coverage-source-map.js +++ b/test/parallel/test-runner-coverage-source-map.js @@ -122,4 +122,4 @@ describe('Coverage with source maps', async () => { t.assert.strictEqual(spawned.code, 1); }); } -}).then(common.mustCall()); +}); diff --git a/test/parallel/test-runner-misc.js b/test/parallel/test-runner-misc.js index cea115493249a1..28e626d182e899 100644 --- a/test/parallel/test-runner-misc.js +++ b/test/parallel/test-runner-misc.js @@ -18,9 +18,8 @@ if (process.argv[2] === 'child') { assert.strictEqual(signal.aborted, false); testSignal = signal; await setTimeout(50); - })).finally(common.mustCall(() => { - test(() => assert.strictEqual(testSignal.aborted, true)); })); + test(() => assert.strictEqual(testSignal.aborted, true)); // TODO(benjamingr) add more tests to describe + AbortSignal // this just tests the parameter is passed diff --git a/test/parallel/test-runner-typechecking.js b/test/parallel/test-runner-typechecking.js index e96761b1a054bd..8568b4cb39218b 100644 --- a/test/parallel/test-runner-typechecking.js +++ b/test/parallel/test-runner-typechecking.js @@ -6,7 +6,6 @@ require('../common'); const assert = require('assert'); const { test, describe, it } = require('node:test'); -const { isPromise } = require('util/types'); const testOnly = test('only test', { only: true }); const testTodo = test('todo test', { todo: true }); @@ -16,21 +15,12 @@ const testTodoShorthand = test.todo('todo test shorthand'); const testSkipShorthand = test.skip('skip test shorthand'); describe('\'node:test\' and its shorthands should return the same', () => { - it('should return a Promise', () => { - assert(isPromise(testOnly)); - assert(isPromise(testTodo)); - assert(isPromise(testSkip)); - assert(isPromise(testOnlyShorthand)); - assert(isPromise(testTodoShorthand)); - assert(isPromise(testSkipShorthand)); - }); - - it('should resolve undefined', async () => { - assert.strictEqual(await testOnly, undefined); - assert.strictEqual(await testTodo, undefined); - assert.strictEqual(await testSkip, undefined); - assert.strictEqual(await testOnlyShorthand, undefined); - assert.strictEqual(await testTodoShorthand, undefined); - assert.strictEqual(await testSkipShorthand, undefined); + it('should return undefined', () => { + assert.strictEqual(testOnly, undefined); + assert.strictEqual(testTodo, undefined); + assert.strictEqual(testSkip, undefined); + assert.strictEqual(testOnlyShorthand, undefined); + assert.strictEqual(testTodoShorthand, undefined); + assert.strictEqual(testSkipShorthand, undefined); }); });