test_runner: reland run global after() hook earlier

This commit reverts the revert in
bb52656. It also includes the
fix for the issue that required the revert
(nodejs#49059 (comment))
and an additional common.mustCall() in the added test.

Refs: nodejs#49059
Refs: nodejs#49110
PR-URL: nodejs#49116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Author: cjihrig

lib/internal/test_runner/harness.js

@@ -142,8 +142,8 @@ function setup(root) {
   const rejectionHandler =
     createProcessEventHandler('unhandledRejection', root);
   const coverage = configureCoverage(root, globalOptions);
-  const exitHandler = async () => {
-    await root.run(new ERR_TEST_FAILURE(
+  const exitHandler = () => {
+    root.postRun(new ERR_TEST_FAILURE(
       'Promise resolution is still pending but the event loop has already resolved',
       kCancelledByParent));
 
@@ -152,8 +152,8 @@ function setup(root) {
     process.removeListener('uncaughtException', exceptionHandler);
   };
 
-  const terminationHandler = async () => {
-    await exitHandler();
+  const terminationHandler = () => {
+    exitHandler();
     process.exit();
   };
 

lib/internal/test_runner/test.js

@@ -574,7 +574,7 @@ class Test extends AsyncResource {
     }
   }
 
-  async run(pendingSubtestsError) {
+  async run() {
     if (this.parent !== null) {
       this.parent.activeSubtests++;
     }
@@ -662,9 +662,16 @@ class Test extends AsyncResource {
       }
     }
 
-    // Clean up the test. Then, try to report the results and execute any
-    // tests that were pending due to available concurrency.
-    this.postRun(pendingSubtestsError);
+    if (this.parent !== null || typeof this.hookType === 'string') {
+      // Clean up the test. Then, try to report the results and execute any
+      // tests that were pending due to available concurrency.
+      //
+      // The root test is skipped here because it is a special case. Its
+      // postRun() method is called when the process is getting ready to exit.
+      // This helps catch any asynchronous activity that occurs after the tests
+      // have finished executing.
+      this.postRun();
+    }
   }
 
   postRun(pendingSubtestsError) {
@@ -706,6 +713,18 @@ class Test extends AsyncResource {
       this.parent.addReadySubtest(this);
       this.parent.processReadySubtestRange(false);
       this.parent.processPendingSubtests();
+
+      if (this.parent === this.root &&
+          this.root.activeSubtests === 0 &&
+          this.root.pendingSubtests.length === 0 &&
+          this.root.readySubtests.size === 0 &&
+          this.root.hooks.after.length > 0) {
+        // This is done so that any global after() hooks are run. At this point
+        // all of the tests have finished running. However, there might be
+        // ref'ed handles keeping the event loop alive. This gives the global
+        // after() hook a chance to clean them up.
+        this.root.run();
+      }
     } else if (!this.reported) {
       const {
         diagnostics,

test/fixtures/test-runner/output/async-test-scheduling.mjs

@@ -0,0 +1,13 @@
+import * as common from '../../../common/index.mjs';
+import { describe, test } from 'node:test';
+import { setTimeout } from 'node:timers/promises';
+
+test('test', common.mustCall());
+describe('suite', common.mustCall(async () => {
+  test('test', common.mustCall());
+  await setTimeout(10);
+  test('scheduled async', common.mustCall());
+}));
+
+await setTimeout(10);
+test('scheduled async', common.mustCall());

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

@@ -0,0 +1,37 @@
+TAP version 13
+# Subtest: test
+ok 1 - test
+  ---
+  duration_ms: *
+  ...
+# Subtest: suite
+    # Subtest: test
+    ok 1 - test
+      ---
+      duration_ms: *
+      ...
+    # Subtest: scheduled async
+    ok 2 - scheduled async
+      ---
+      duration_ms: *
+      ...
+    1..2
+ok 2 - suite
+  ---
+  duration_ms: *
+  type: 'suite'
+  ...
+# Subtest: scheduled async
+ok 3 - scheduled async
+  ---
+  duration_ms: *
+  ...
+1..3
+# tests 4
+# suites 1
+# pass 4
+# fail 0
+# cancelled 0
+# skipped 0
+# todo 0
+# duration_ms *

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

@@ -22,7 +22,6 @@ not ok 2 - /test/fixtures/test-runner/output/global_after_should_fail_the_test.j
     *
     *
     *
-    *
   ...
 1..1
 # tests 1

test/parallel/test-runner-output.mjs

@@ -74,6 +74,7 @@ const tests = [
   { name: 'test-runner/output/unresolved_promise.js' },
   { name: 'test-runner/output/default_output.js', transform: specTransform, tty: true },
   { name: 'test-runner/output/arbitrary-output.js' },
+  { name: 'test-runner/output/async-test-scheduling.mjs' },
   !skipForceColors ? {
     name: 'test-runner/output/arbitrary-output-colored.js',
     transform: snapshot.transform(specTransform, replaceTestDuration), tty: true

test/parallel/test-runner-root-after-with-refed-handles.js

@@ -0,0 +1,26 @@
+'use strict';
+const common = require('../common');
+const { before, after, test } = require('node:test');
+const { createServer } = require('node:http');
+
+let server;
+
+before(common.mustCall(() => {
+  server = createServer();
+
+  return new Promise(common.mustCall((resolve, reject) => {
+    server.listen(0, common.mustCall((err) => {
+      if (err) {
+        reject(err);
+      } else {
+        resolve();
+      }
+    }));
+  }));
+}));
+
+after(common.mustCall(() => {
+  server.close(common.mustCall());
+}));
+
+test();