Revert "test_runner: remove promises returned by test()"

This reverts commit 9671826.

PR-URL: #58282
Fixes: #58227
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>

Author: romainmenke

doc/api/test.md

@@ -1472,11 +1472,6 @@ run({ files: [path.resolve('./tests/test.js')] })
 added:
   - v22.0.0
   - v20.13.0
-changes:
-  - version:
-    - v24.0.0
-    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
@@ -1487,6 +1482,7 @@ changes:
 * `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.
 
@@ -1530,10 +1526,6 @@ added:
   - v18.0.0
   - v16.17.0
 changes:
-  - version:
-    - v24.0.0
-    pr-url: https://github.com/nodejs/node/pull/56664
-    description: This function no longer returns a `Promise`.
   - version:
     - v20.2.0
     - v18.17.0
@@ -1583,6 +1575,8 @@ 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}.
@@ -1591,6 +1585,26 @@ 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

lib/internal/test_runner/harness.js

@@ -325,12 +325,11 @@ 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;
+      return PromiseResolve();
     }
 
-    startSubtestAfterBootstrap(subtest);
+    return startSubtestAfterBootstrap(subtest);
   }
 
   const test = (name, options, fn) => {
@@ -339,7 +338,7 @@ function runInParentContext(Factory) {
       loc: getCallerLocation(),
     };
 
-    run(name, options, fn, overrides);
+    return run(name, options, fn, overrides);
   };
   ArrayPrototypeForEach(['skip', 'todo', 'only'], (keyword) => {
     test[keyword] = (name, options, fn) => {
@@ -349,7 +348,7 @@ function runInParentContext(Factory) {
         loc: getCallerLocation(),
       };
 
-      run(name, options, fn, overrides);
+      return run(name, options, fn, overrides);
     };
   });
   return test;

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());

test/parallel/test-runner-misc.js

@@ -18,8 +18,9 @@ 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

test/parallel/test-runner-typechecking.js

@@ -6,6 +6,7 @@ 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 });
@@ -15,12 +16,21 @@ 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 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);
+  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);
   });
 });