test_runner: remove Promise return values from test(), suite(), and t.test()

doc/api/test.md

@@ -105,11 +105,11 @@ top level test with two subtests.
 
 ```js
 test('top level test', async (t) => {
-  await t.test('subtest 1', (t) => {
+  t.test('subtest 1', (t) => {
     assert.strictEqual(1, 1);
   });
 
-  await t.test('subtest 2', (t) => {
+  t.test('subtest 2', (t) => {
     assert.strictEqual(2, 2);
   });
 });
@@ -118,12 +118,7 @@ test('top level test', async (t) => {
 > **Note:** `beforeEach` and `afterEach` hooks are triggered
 > between each subtest execution.
 
-In this example, `await` is used to ensure that both subtests have completed.
-This is necessary because tests do not wait for their subtests to
-complete, unlike tests created within suites.
-Any subtests that are still outstanding when their parent finishes
-are cancelled and treated as failures. Any subtest failures cause the parent
-test to fail.
+Any subtest failures cause the parent test to fail.
 
 ## Skipping tests
 
@@ -241,20 +236,20 @@ that are not executed are omitted from the test runner output.
 // The suite's 'only' option is set, so these tests are run.
 test('this test is run', { only: true }, async (t) => {
   // Within this test, all subtests are run by default.
-  await t.test('running subtest');
+  t.test('running subtest');
 
   // The test context can be updated to run subtests with the 'only' option.
   t.runOnly(true);
-  await t.test('this subtest is now skipped');
-  await t.test('this subtest is run', { only: true });
+  t.test('this subtest is now skipped');
+  t.test('this subtest is run', { only: true });
 
   // Switch the context back to execute all tests.
   t.runOnly(false);
-  await t.test('this subtest is now run');
+  t.test('this subtest is now run');
 
   // Explicitly do not run these tests.
-  await t.test('skipped subtest 3', { only: false });
-  await t.test('skipped subtest 4', { skip: true });
+  t.test('skipped subtest 3', { only: false });
+  t.test('skipped subtest 4', { skip: true });
 });
 
 // The 'only' option is not set, so this test is skipped.
@@ -309,13 +304,13 @@ multiple times (e.g. `--test-name-pattern="test 1"`,
 
 ```js
 test('test 1', async (t) => {
-  await t.test('test 2');
-  await t.test('test 3');
+  t.test('test 2');
+  t.test('test 3');
 });
 
 test('Test 4', async (t) => {
-  await t.test('Test 5');
-  await t.test('test 6');
+  t.test('Test 5');
+  t.test('test 6');
 });
 ```
 
@@ -1407,6 +1402,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 +1417,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 +1460,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 +1513,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 +1521,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
@@ -3189,12 +3170,9 @@ before each subtest of the current test.
 ```js
 test('top level test', async (t) => {
   t.beforeEach((t) => t.diagnostic(`about to run ${t.name}`));
-  await t.test(
-    'This is a subtest',
-    (t) => {
-      assert.ok('some relevant assertion here');
-    },
-  );
+  t.test('This is a subtest', (t) => {
+    assert.ok('some relevant assertion here');
+  });
 });
 ```
 
@@ -3252,12 +3230,9 @@ after each subtest of the current test.
 ```js
 test('top level test', async (t) => {
   t.afterEach((t) => t.diagnostic(`finished running ${t.name}`));
-  await t.test(
-    'This is a subtest',
-    (t) => {
-      assert.ok('some relevant assertion here');
-    },
-  );
+  t.test('This is a subtest', (t) => {
+    assert.ok('some relevant assertion here');
+  });
 });
 ```
 
@@ -3472,10 +3447,8 @@ no-op.
 test('top level test', (t) => {
   // The test context can be set to run subtests with the 'only' option.
   t.runOnly(true);
-  return Promise.all([
-    t.test('this subtest is now skipped'),
-    t.test('this subtest is run', { only: true }),
-  ]);
+  t.test('this subtest is now skipped');
+  t.test('this subtest is run', { only: true });
 });
 ```
 
@@ -3547,6 +3520,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:
     - v18.8.0
     - v16.18.0
@@ -3591,14 +3568,13 @@ 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.
 
 This function is used to create subtests under the current test. This function
 behaves in the same fashion as the top level [`test()`][] function.
 
 ```js
 test('top level test', async (t) => {
-  await t.test(
+  t.test(
     'This is a subtest',
     { only: false, skip: false, concurrency: 1, todo: false, plan: 1 },
     (t) => {

lib/internal/test_runner/harness.js

@@ -27,6 +27,8 @@ const {
   shouldColorizeTestFiles,
 } = require('internal/test_runner/utils');
 const { queueMicrotask } = require('internal/process/task_queues');
+const { TIMEOUT_MAX } = require('internal/timers');
+const { clearInterval, setInterval } = require('timers');
 const { bigint: hrtime } = process.hrtime;
 const resolvedPromise = PromiseResolve();
 const testResources = new SafeMap();
@@ -212,11 +214,20 @@ function setupProcessState(root, globalOptions) {
   const rejectionHandler =
     createProcessEventHandler('unhandledRejection', root);
   const coverage = configureCoverage(root, globalOptions);
-  const exitHandler = async () => {
+  const exitHandler = async (kill) => {
     if (root.subtests.length === 0 && (root.hooks.before.length > 0 || root.hooks.after.length > 0)) {
       // Run global before/after hooks in case there are no tests
       await root.run();
     }
+
+    if (kill !== true && root.subtestsPromise !== null) {
+      // Wait for all subtests to finish, but keep the process alive in case
+      // there are no ref'ed handles left.
+      const keepAlive = setInterval(() => {}, TIMEOUT_MAX);
+      await root.subtestsPromise.promise;
+      clearInterval(keepAlive);
+    }
+
     root.postRun(new ERR_TEST_FAILURE(
       'Promise resolution is still pending but the event loop has already resolved',
       kCancelledByParent));
@@ -231,8 +242,8 @@ function setupProcessState(root, globalOptions) {
     }
   };
 
-  const terminationHandler = () => {
-    exitHandler();
+  const terminationHandler = async () => {
+    await exitHandler(true);
     process.exit();
   };
 
@@ -301,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) => {
@@ -314,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) => {
@@ -324,7 +336,7 @@ function runInParentContext(Factory) {
         loc: getCallerLocation(),
       };
 
-      return run(name, options, fn, overrides);
+      run(name, options, fn, overrides);
     };
   });
   return test;

lib/internal/test_runner/test.js

@@ -304,7 +304,7 @@ class TestContext {
       Test, name, options, fn, overrides,
     );
 
-    return subtest.start();
+    subtest.start();
   }
 
   before(fn, options) {
@@ -579,6 +579,8 @@ class Test extends AsyncResource {
     this.activeSubtests = 0;
     this.pendingSubtests = [];
     this.readySubtests = new SafeMap();
+    this.unfinishedSubtests = new SafeSet();
+    this.subtestsPromise = null;
     this.subtests = [];
     this.waitingOn = 0;
     this.finished = false;
@@ -682,6 +684,11 @@ class Test extends AsyncResource {
 
   addReadySubtest(subtest) {
     this.readySubtests.set(subtest.childNumber, subtest);
+
+    if (this.unfinishedSubtests.delete(subtest) &&
+        this.unfinishedSubtests.size === 0) {
+      this.subtestsPromise.resolve();
+    }
   }
 
   processReadySubtestRange(canSend) {
@@ -743,6 +750,7 @@ class Test extends AsyncResource {
 
     if (parent.waitingOn === 0) {
       parent.waitingOn = test.childNumber;
+      parent.subtestsPromise = PromiseWithResolvers();
     }
 
     if (preventAddingSubtests) {
@@ -865,6 +873,7 @@ class Test extends AsyncResource {
     // If there is enough available concurrency to run the test now, then do
     // it. Otherwise, return a Promise to the caller and mark the test as
     // pending for later execution.
+    this.parent.unfinishedSubtests.add(this);
     this.reporter.enqueue(this.nesting, this.loc, this.name, this.reportedType);
     if (this.root.harness.buildPromise || !this.parent.hasConcurrency()) {
       const deferred = PromiseWithResolvers();
@@ -982,6 +991,11 @@ class Test extends AsyncResource {
       }
 
       this[kShouldAbort]();
+
+      if (this.subtestsPromise !== null) {
+        await SafePromiseRace([this.subtestsPromise.promise, stopPromise]);
+      }
+
       this.plan?.check();
       this.pass();
       await afterEach();

test/fixtures/test-runner/output/default_output.snapshot

@@ -24,15 +24,13 @@
     *[39m
     *[39m
 
-  [31m✖ should pass but parent fail [90m(*ms)[39m[39m
-    [32m'test did not finish before its parent and was cancelled'[39m
-
+  [32m✔ should pass but parent fail [90m(*ms)[39m[39m
 [31m✖ parent [90m(*ms)[39m[39m
 [34mℹ tests 6[39m
 [34mℹ suites 0[39m
-[34mℹ pass 1[39m
+[34mℹ pass 2[39m
 [34mℹ fail 3[39m
-[34mℹ cancelled 1[39m
+[34mℹ cancelled 0[39m
 [34mℹ skipped 1[39m
 [34mℹ todo 0[39m
 [34mℹ duration_ms *[39m
@@ -63,7 +61,3 @@
   *[39m
   *[39m
   *[39m
-
-*
-[31m✖ should pass but parent fail [90m(*ms)[39m[39m
-  [32m'test did not finish before its parent and was cancelled'[39m

test/fixtures/test-runner/output/dot_reporter.snapshot

@@ -1,5 +1,5 @@
 ..XX...X..XXX.X.....
-XXX.....X..X...X....
+XXX............X....
 .....X...XXX.XX.....
 XXXXXXX...XXXXX
 
@@ -93,10 +93,6 @@ Failed tests:
   '1 subtest failed'
 ✖ sync throw non-error fail (*ms)
   Symbol(thrown symbol from sync throw non-error fail)
-✖ +long running (*ms)
-  'test did not finish before its parent and was cancelled'
-✖ top level (*ms)
-  '1 subtest failed'
 ✖ sync skip option is false fail (*ms)
   Error: this should be executed
       *
@@ -158,8 +154,6 @@ Failed tests:
       *
       *
       *
-      *
-      *
 ✖ subtest sync throw fails (*ms)
   '2 subtests failed'
 ✖ timed out async test (*ms)