Revert "test_runner: automatically wait for subtests to finish"

This reverts commit aa3523ec22.

PR-URL: https://github.com/nodejs/node/pull/58282
Fixes: https://github.com/nodejs/node/issues/58227
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>

lib/internal/test_runner/harness.js

@@ -28,8 +28,6 @@ const {
   setupGlobalSetupTeardownFunctions,
 } = 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 testResources = new SafeMap();
 let globalRoot;
@@ -230,20 +228,11 @@ function setupProcessState(root, globalOptions) {
   const rejectionHandler =
     createProcessEventHandler('unhandledRejection', root);
   const coverage = configureCoverage(root, globalOptions);
-  const exitHandler = async (kill) => {
+  const exitHandler = async () => {
     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));
@@ -263,8 +252,8 @@ function setupProcessState(root, globalOptions) {
     }
   };
 
-  const terminationHandler = async () => {
+  const terminationHandler = () => {
-    await exitHandler(true);
+    exitHandler();
     process.exit();
   };
 

lib/internal/test_runner/test.js

@@ -651,8 +651,6 @@ 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;
@@ -756,11 +754,6 @@ 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) {
@@ -822,7 +815,6 @@ class Test extends AsyncResource {
 
     if (parent.waitingOn === 0) {
       parent.waitingOn = test.childNumber;
-      parent.subtestsPromise = PromiseWithResolvers();
     }
 
     if (preventAddingSubtests) {
@@ -945,7 +937,6 @@ 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();
@@ -1070,10 +1061,6 @@ class Test extends AsyncResource {
 
       this[kShouldAbort]();
 
-      if (this.subtestsPromise !== null) {
-        await SafePromiseRace([this.subtestsPromise.promise, stopPromise]);
-      }
-
       if (this.plan !== null) {
         const planPromise = this.plan?.check();
         // If the plan returns a promise, it means that it is waiting for more assertions to be made before

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

@@ -3,13 +3,13 @@
 [90m﹣ should skip [90m(*ms)[39m # SKIP[39m
 ▶ parent
   [31m✖ should fail [90m(*ms)[39m[39m
-  [32m✔ should pass but parent fail [90m(*ms)[39m[39m
+  [31m✖ 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 2[39m
+[34mℹ pass 1[39m
 [34mℹ fail 3[39m
-[34mℹ cancelled 0[39m
+[34mℹ cancelled 1[39m
 [34mℹ skipped 1[39m
 [34mℹ todo 0[39m
 [34mℹ duration_ms *[39m
@@ -40,3 +40,7 @@
   *[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....
+XXX.....X..X...X....
 .....X...XXX.XX.....
 XXXXXXX...XXXXX
 
@@ -93,6 +93,10 @@ 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
       *

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

@@ -186,8 +186,12 @@ Error [ERR_TEST_FAILURE]: thrown from subtest sync throw fail
 		<testcase name="level 1c" time="*" classname="test"/>
 		<testcase name="level 1d" time="*" classname="test"/>
 	</testsuite>
-	<testsuite name="top level" time="*" disabled="0" errors="0" tests="2" failures="0" skipped="0" hostname="HOSTNAME">
+	<testsuite name="top level" time="*" disabled="0" errors="0" tests="2" failures="1" skipped="0" hostname="HOSTNAME">
-		<testcase name="+long running" time="*" classname="test"/>
+		<testcase name="+long running" time="*" classname="test" failure="test did not finish before its parent and was cancelled">
+			<failure type="cancelledByParent" message="test did not finish before its parent and was cancelled">
+[Error [ERR_TEST_FAILURE]: test did not finish before its parent and was cancelled] { code: 'ERR_TEST_FAILURE', failureType: 'cancelledByParent', cause: 'test did not finish before its parent and was cancelled' }
+			</failure>
+		</testcase>
 		<testsuite name="+short running" time="*" disabled="0" errors="0" tests="1" failures="0" skipped="0" hostname="HOSTNAME">
 			<testcase name="++short running" time="*" classname="test"/>
 		</testsuite>
@@ -512,9 +516,9 @@ Error [ERR_TEST_FAILURE]: test could not be started because its parent finished
 	<!-- Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:269:1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event. -->
 	<!-- tests 75 -->
 	<!-- suites 0 -->
-	<!-- pass 36 -->
+	<!-- pass 34 -->
-	<!-- fail 24 -->
+	<!-- fail 25 -->
-	<!-- cancelled 2 -->
+	<!-- cancelled 3 -->
 	<!-- skipped 9 -->
 	<!-- todo 4 -->
 	<!-- duration_ms * -->

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

@@ -1,23 +1,35 @@
 TAP version 13
 # Subtest: does not keep event loop alive
     # Subtest: +does not keep event loop alive
-    ok 1 - +does not keep event loop alive
+    not ok 1 - +does not keep event loop alive
       ---
       duration_ms: *
       type: 'test'
+      location: '/test/fixtures/test-runner/output/no_refs.js:(LINE):11'
+      failureType: 'cancelledByParent'
+      error: 'Promise resolution is still pending but the event loop has already resolved'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
       ...
     1..1
-ok 1 - does not keep event loop alive
+not ok 1 - does not keep event loop alive
   ---
   duration_ms: *
   type: 'test'
+  location: '/test/fixtures/test-runner/output/no_refs.js:(LINE):1'
+  failureType: 'cancelledByParent'
+  error: 'Promise resolution is still pending but the event loop has already resolved'
+  code: 'ERR_TEST_FAILURE'
+  stack: |-
+    *
   ...
 1..1
 # tests 2
 # suites 0
-# pass 2
+# pass 0
 # fail 0
-# cancelled 0
+# cancelled 2
 # skipped 0
 # todo 0
 # duration_ms *

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

@@ -288,10 +288,14 @@ ok 23 - level 0a
   ...
 # Subtest: top level
     # Subtest: +long running
-    ok 1 - +long running
+    not ok 1 - +long running
       ---
       duration_ms: *
       type: 'test'
+      location: '/test/fixtures/test-runner/output/output.js:(LINE):5'
+      failureType: 'cancelledByParent'
+      error: 'test did not finish before its parent and was cancelled'
+      code: 'ERR_TEST_FAILURE'
       ...
     # Subtest: +short running
         # Subtest: ++short running
@@ -307,10 +311,14 @@ ok 23 - level 0a
       type: 'test'
       ...
     1..2
-ok 24 - top level
+not ok 24 - top level
   ---
   duration_ms: *
   type: 'test'
+  location: '/test/fixtures/test-runner/output/output.js:(LINE):1'
+  failureType: 'subtestsFailed'
+  error: '1 subtest failed'
+  code: 'ERR_TEST_FAILURE'
   ...
 # Subtest: invalid subtest - pass but subtest fails
 ok 25 - invalid subtest - pass but subtest fails
@@ -777,9 +785,9 @@ not ok 62 - invalid subtest fail
 # Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:(LINE):1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event.
 # tests 75
 # suites 0
-# pass 36
+# pass 34
-# fail 24
+# fail 25
-# cancelled 2
+# cancelled 3
 # skipped 9
 # todo 4
 # duration_ms *

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

@@ -288,10 +288,14 @@ ok 23 - level 0a
   ...
 # Subtest: top level
     # Subtest: +long running
-    ok 1 - +long running
+    not ok 1 - +long running
       ---
       duration_ms: *
       type: 'test'
+      location: '/test/fixtures/test-runner/output/output.js:(LINE):5'
+      failureType: 'cancelledByParent'
+      error: 'test did not finish before its parent and was cancelled'
+      code: 'ERR_TEST_FAILURE'
       ...
     # Subtest: +short running
         # Subtest: ++short running
@@ -307,10 +311,14 @@ ok 23 - level 0a
       type: 'test'
       ...
     1..2
-ok 24 - top level
+not ok 24 - top level
   ---
   duration_ms: *
   type: 'test'
+  location: '/test/fixtures/test-runner/output/output.js:(LINE):1'
+  failureType: 'subtestsFailed'
+  error: '1 subtest failed'
+  code: 'ERR_TEST_FAILURE'
   ...
 # Subtest: invalid subtest - pass but subtest fails
 ok 25 - invalid subtest - pass but subtest fails
@@ -791,9 +799,9 @@ ok 63 - last test
 1..63
 # tests 77
 # suites 0
-# pass 38
+# pass 36
-# fail 24
+# fail 25
-# cancelled 2
+# cancelled 3
 # skipped 9
 # todo 4
 # duration_ms *

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

@@ -90,9 +90,9 @@
  Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:269:1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event.
  tests 75
  suites 0
- pass 36
+ pass 34
- fail 24
+ fail 25
- cancelled 2
+ cancelled 3
  skipped 9
  todo 4
  duration_ms *
@@ -203,6 +203,10 @@
  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'
+
 *
  sync skip option is false fail (*ms)
   Error: this should be executed

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

@@ -93,9 +93,9 @@
  Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:269:1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event.
  tests 76
  suites 0
- pass 37
+ pass 35
- fail 24
+ fail 25
- cancelled 2
+ cancelled 3
  skipped 9
  todo 4
  duration_ms *
@@ -206,6 +206,10 @@
  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'
+
 *
  sync skip option is false fail (*ms)
   Error: this should be executed

test/parallel/test-runner-output.mjs

@@ -208,6 +208,10 @@ const tests = [
     name: 'test-runner/output/unfinished-suite-async-error.js',
     flags: ['--test-reporter=tap'],
   },
+  {
+    name: 'test-runner/output/unresolved_promise.js',
+    flags: ['--test-reporter=tap'],
+  },
   { name: 'test-runner/output/default_output.js', transform: specTransform, tty: true },
   {
     name: 'test-runner/output/arbitrary-output.js',