From 9d6fd557bb8c063c0304ae5dc6356f9fbfdba3a4 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Sun, 28 May 2023 17:51:26 +0300
Subject: [PATCH 1/3] test_runner: fix global after hook

---
 lib/internal/test_runner/harness.js           |  7 ++++-
 lib/internal/test_runner/test.js              |  8 +++---
 test/fixtures/test-runner/output/hooks.js     | 27 +++++++++++--------
 .../test-runner/output/hooks.snapshot         |  3 +++
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index 26b85ac0d2d6a5..fd4c2d3288d767 100644
--- a/lib/internal/test_runner/harness.js
+++ b/lib/internal/test_runner/harness.js
@@ -140,7 +140,12 @@ function setup(root) {
   const rejectionHandler =
     createProcessEventHandler('unhandledRejection', root);
   const coverage = configureCoverage(root, globalOptions);
-  const exitHandler = () => {
+  const exitHandler = async () => {
+    try {
+      if (root.hooks.after.length > 0) {
+        await root.runHook('after', root.getRunArgs());
+      }
+    } catch { /* Ignore error. */ }
     root.postRun(new ERR_TEST_FAILURE(
       'Promise resolution is still pending but the event loop has already resolved',
       kCancelledByParent));
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index ea6578e53f666a..420a133f3cf2ae 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -427,7 +427,7 @@ class Test extends AsyncResource {
     validateOneOf(name, 'hook name', kHookNames);
     // eslint-disable-next-line no-use-before-define
     const hook = new TestHook(fn, options);
-    if (name === 'before') {
+    if (name === 'before' || name === 'after') {
       hook.run = runOnce(hook.run);
     }
     ArrayPrototypePush(this.hooks[name], hook);
@@ -526,11 +526,11 @@ class Test extends AsyncResource {
     }
 
     const { args, ctx } = this.getRunArgs();
-    const after = runOnce(async () => {
+    const after = async () => {
       if (this.hooks.after.length > 0) {
         await this.runHook('after', { args, ctx });
       }
-    });
+    };
     const afterEach = runOnce(async () => {
       if (this.parent?.hooks.afterEach.length > 0) {
         await this.parent.runHook('afterEach', { args, ctx });
@@ -579,8 +579,8 @@ class Test extends AsyncResource {
       await after();
       this.pass();
     } catch (err) {
-      try { await after(); } catch { /* Ignore error. */ }
       try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
+      try { await after(); } catch { /* Ignore error. */ }
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.#cancel(err);
diff --git a/test/fixtures/test-runner/output/hooks.js b/test/fixtures/test-runner/output/hooks.js
index a69506bbda5ef7..827da5d5646262 100644
--- a/test/fixtures/test-runner/output/hooks.js
+++ b/test/fixtures/test-runner/output/hooks.js
@@ -5,6 +5,7 @@ const assert = require('assert');
 const { test, describe, it, before, after, beforeEach, afterEach } = require('node:test');
 
 before((t) => t.diagnostic('before 1 called'));
+after((t) => t.diagnostic('after 1 called'));
 
 describe('describe hooks', () => {
   const testArr = [];
@@ -107,17 +108,20 @@ test('test hooks', async (t) => {
     await t.test('nested 2', () => testArr.push('nested 2'));
   });
 
-  assert.deepStrictEqual(testArr, [
-    'before test hooks',
-    'beforeEach 1', '1', 'afterEach 1',
-    'beforeEach 2', '2', 'afterEach 2',
-    'beforeEach nested',
-    'nested before nested',
-    'beforeEach nested 1', 'nested beforeEach nested 1', 'nested1', 'afterEach nested 1', 'nested afterEach nested 1',
-    'beforeEach nested 2', 'nested beforeEach nested 2', 'nested 2', 'afterEach nested 2', 'nested afterEach nested 2',
-    'afterEach nested',
-    'nested after nested',
-  ]);
+  t.after(common.mustCall(() => {
+    assert.deepStrictEqual(testArr, [
+      'before test hooks',
+      'beforeEach 1', '1', 'afterEach 1',
+      'beforeEach 2', '2', 'afterEach 2',
+      'beforeEach nested',
+      'nested before nested',
+      'beforeEach nested 1', 'nested beforeEach nested 1', 'nested1', 'afterEach nested 1', 'nested afterEach nested 1',
+      'beforeEach nested 2', 'nested beforeEach nested 2', 'nested 2', 'afterEach nested 2', 'nested afterEach nested 2',
+      'afterEach nested',
+      'nested after nested',
+      'after test hooks',
+    ]);
+  }));
 });
 
 test('t.before throws', async (t) => {
@@ -164,3 +168,4 @@ test('t.after() is called if test body throws', (t) => {
 });
 
 before((t) => t.diagnostic('before 2 called'));
+after((t) => t.diagnostic('after 2 called'));
diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot
index 1007093e352a88..5b16957ba24dc6 100644
--- a/test/fixtures/test-runner/output/hooks.snapshot
+++ b/test/fixtures/test-runner/output/hooks.snapshot
@@ -97,6 +97,7 @@ not ok 3 - after throws
     *
     *
     *
+    *
   ...
 # Subtest: beforeEach throws
     # Subtest: 1
@@ -544,6 +545,8 @@ not ok 14 - t.after() is called if test body throws
 1..14
 # before 1 called
 # before 2 called
+# after 1 called
+# after 2 called
 # tests 38
 # suites 8
 # pass 14

From 969de024c77ce492a5ea34926ea2e17a2d6b8a99 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Tue, 30 May 2023 19:51:57 +0300
Subject: [PATCH 2/3] CR

---
 lib/internal/test_runner/harness.js           |  7 +---
 lib/internal/test_runner/test.js              | 36 ++++++++++---------
 .../test-runner/output/describe_it.snapshot   |  7 ++--
 .../test-runner/output/hooks.snapshot         |  6 ++--
 .../test-runner/output/output.snapshot        |  3 ++
 .../test-runner/output/output_cli.snapshot    |  3 ++
 .../test-runner/output/spec_reporter.snapshot |  6 ++++
 .../output/spec_reporter_cli.snapshot         |  6 ++++
 8 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index fd4c2d3288d767..98a05babf71297 100644
--- a/lib/internal/test_runner/harness.js
+++ b/lib/internal/test_runner/harness.js
@@ -141,12 +141,7 @@ function setup(root) {
     createProcessEventHandler('unhandledRejection', root);
   const coverage = configureCoverage(root, globalOptions);
   const exitHandler = async () => {
-    try {
-      if (root.hooks.after.length > 0) {
-        await root.runHook('after', root.getRunArgs());
-      }
-    } catch { /* Ignore error. */ }
-    root.postRun(new ERR_TEST_FAILURE(
+    await root.run(new ERR_TEST_FAILURE(
       'Promise resolution is still pending but the event loop has already resolved',
       kCancelledByParent));
 
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 420a133f3cf2ae..3a05182c11fe8f 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -514,7 +514,7 @@ class Test extends AsyncResource {
     }
   }
 
-  async run() {
+  async run(pendingSubtestsError) {
     if (this.parent !== null) {
       this.parent.activeSubtests++;
     }
@@ -526,16 +526,7 @@ class Test extends AsyncResource {
     }
 
     const { args, ctx } = this.getRunArgs();
-    const after = async () => {
-      if (this.hooks.after.length > 0) {
-        await this.runHook('after', { args, ctx });
-      }
-    };
-    const afterEach = runOnce(async () => {
-      if (this.parent?.hooks.afterEach.length > 0) {
-        await this.parent.runHook('afterEach', { args, ctx });
-      }
-    });
+    const afterEach = runOnce(() => this.parent.runHook('afterEach', { args, ctx }));
 
     try {
       if (this.parent?.hooks.before.length > 0) {
@@ -548,7 +539,10 @@ class Test extends AsyncResource {
       const runArgs = ArrayPrototypeSlice(args);
       ArrayPrototypeUnshift(runArgs, this.fn, ctx);
 
-      if (this.fn.length === runArgs.length - 1) {
+      if (this.fn === noop && this.parent === null) {
+        // Avoid dealy of microtasks for root test
+        // to allow calling root.run() after event loop has ended
+      } else if (this.fn.length === runArgs.length - 1) {
         // This test is using legacy Node.js error first callbacks.
         const { promise, cb } = createDeferredCallback();
 
@@ -575,12 +569,20 @@ class Test extends AsyncResource {
         return;
       }
 
-      await afterEach();
-      await after();
+      if (this.parent?.hooks.afterEach.length > 0) {
+        await afterEach();
+      }
+      if (this.hooks.after.length > 0) {
+        await this.runHook('after', { args, ctx });
+      }
       this.pass();
     } catch (err) {
-      try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
-      try { await after(); } catch { /* Ignore error. */ }
+      if (this.parent?.hooks.afterEach.length > 0) {
+        try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
+      }
+      if (this.hooks.after.length > 0) {
+        try { await this.runHook('after', { args, ctx }); } catch { /* Ignore error. */ }
+      }
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.#cancel(err);
@@ -594,7 +596,7 @@ 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();
+    this.postRun(pendingSubtestsError);
   }
 
   postRun(pendingSubtestsError) {
diff --git a/test/fixtures/test-runner/output/describe_it.snapshot b/test/fixtures/test-runner/output/describe_it.snapshot
index e085ff9535ec70..11b47ed2652e72 100644
--- a/test/fixtures/test-runner/output/describe_it.snapshot
+++ b/test/fixtures/test-runner/output/describe_it.snapshot
@@ -40,6 +40,9 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
     *
     *
     *
+    *
+    *
+    *
   ...
 # Subtest: sync skip pass
 ok 5 - sync skip pass # SKIP
@@ -488,10 +491,10 @@ not ok 53 - custom inspect symbol that throws fail
         *
         *
         *
+        new Promise (<anonymous>)
         *
         *
-        *
-        async Promise.all (index 0)
+        Array.map (<anonymous>)
       ...
     1..2
 not ok 54 - subtest sync throw fails
diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot
index 5b16957ba24dc6..7c0b9d0660656b 100644
--- a/test/fixtures/test-runner/output/hooks.snapshot
+++ b/test/fixtures/test-runner/output/hooks.snapshot
@@ -163,9 +163,9 @@ not ok 4 - beforeEach throws
         *
         *
         *
-        *
         async Promise.all (index 0)
         *
+        *
       ...
     # Subtest: 2
     not ok 2 - 2
@@ -182,9 +182,9 @@ not ok 4 - beforeEach throws
         *
         *
         *
-        *
         async Promise.all (index 0)
         *
+        *
       ...
     1..2
 not ok 5 - afterEach throws
@@ -264,9 +264,9 @@ not ok 6 - afterEach when test fails
         *
         *
         *
-        *
         async Promise.all (index 0)
         *
+        *
       ...
     1..2
 not ok 7 - afterEach throws and test fails
diff --git a/test/fixtures/test-runner/output/output.snapshot b/test/fixtures/test-runner/output/output.snapshot
index db19d8ca549a38..17ad4d1af5c5e5 100644
--- a/test/fixtures/test-runner/output/output.snapshot
+++ b/test/fixtures/test-runner/output/output.snapshot
@@ -40,6 +40,9 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
     *
     *
     *
+    *
+    *
+    *
   ...
 # Subtest: sync skip pass
 ok 5 - sync skip pass # SKIP
diff --git a/test/fixtures/test-runner/output/output_cli.snapshot b/test/fixtures/test-runner/output/output_cli.snapshot
index fe192625e1f8b6..6d657668efb62c 100644
--- a/test/fixtures/test-runner/output/output_cli.snapshot
+++ b/test/fixtures/test-runner/output/output_cli.snapshot
@@ -40,6 +40,9 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
     *
     *
     *
+    *
+    *
+    *
   ...
 # Subtest: sync skip pass
 ok 5 - sync skip pass # SKIP
diff --git a/test/fixtures/test-runner/output/spec_reporter.snapshot b/test/fixtures/test-runner/output/spec_reporter.snapshot
index 768204177014af..c52e428a810d54 100644
--- a/test/fixtures/test-runner/output/spec_reporter.snapshot
+++ b/test/fixtures/test-runner/output/spec_reporter.snapshot
@@ -19,6 +19,9 @@
       *
       *
       *
+      *
+      *
+      *
 
  sync skip pass (*ms) # SKIP
  sync skip pass with message (*ms) # SKIP
@@ -326,6 +329,9 @@
       *
       *
       *
+      *
+      *
+      *
 
  sync throw fail (*ms)
   Error: thrown from sync throw fail
diff --git a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
index e4e08764fd4925..1bd115bc2b51c7 100644
--- a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
+++ b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
@@ -19,6 +19,9 @@
       *
       *
       *
+      *
+      *
+      *
 
  sync skip pass (*ms) # SKIP
  sync skip pass with message (*ms) # SKIP
@@ -326,6 +329,9 @@
       *
       *
       *
+      *
+      *
+      *
 
  sync throw fail (*ms)
   Error: thrown from sync throw fail

From ca8e7bf1a39c9f363dcb5521a9af77649be3f3cb Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Wed, 31 May 2023 12:24:42 +0300
Subject: [PATCH 3/3] CR 1

---
 lib/internal/test_runner/harness.js           |  4 +--
 lib/internal/test_runner/test.js              | 32 +++++++++----------
 .../test-runner/output/describe_it.snapshot   |  7 ++--
 .../test-runner/output/hooks.snapshot         |  6 ++--
 .../test-runner/output/output.snapshot        |  3 --
 .../test-runner/output/output_cli.snapshot    |  3 --
 .../test-runner/output/spec_reporter.snapshot |  6 ----
 .../output/spec_reporter_cli.snapshot         |  6 ----
 8 files changed, 22 insertions(+), 45 deletions(-)

diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index 98a05babf71297..f150a8f5ed85c2 100644
--- a/lib/internal/test_runner/harness.js
+++ b/lib/internal/test_runner/harness.js
@@ -150,8 +150,8 @@ function setup(root) {
     process.removeListener('uncaughtException', exceptionHandler);
   };
 
-  const terminationHandler = () => {
-    exitHandler();
+  const terminationHandler = async () => {
+    await exitHandler();
     process.exit();
   };
 
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 3a05182c11fe8f..310e794ab2945d 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -526,7 +526,16 @@ class Test extends AsyncResource {
     }
 
     const { args, ctx } = this.getRunArgs();
-    const afterEach = runOnce(() => this.parent.runHook('afterEach', { args, ctx }));
+    const after = async () => {
+      if (this.hooks.after.length > 0) {
+        await this.runHook('after', { args, ctx });
+      }
+    };
+    const afterEach = runOnce(async () => {
+      if (this.parent?.hooks.afterEach.length > 0) {
+        await this.parent.runHook('afterEach', { args, ctx });
+      }
+    });
 
     try {
       if (this.parent?.hooks.before.length > 0) {
@@ -539,10 +548,7 @@ class Test extends AsyncResource {
       const runArgs = ArrayPrototypeSlice(args);
       ArrayPrototypeUnshift(runArgs, this.fn, ctx);
 
-      if (this.fn === noop && this.parent === null) {
-        // Avoid dealy of microtasks for root test
-        // to allow calling root.run() after event loop has ended
-      } else if (this.fn.length === runArgs.length - 1) {
+      if (this.fn.length === runArgs.length - 1) {
         // This test is using legacy Node.js error first callbacks.
         const { promise, cb } = createDeferredCallback();
 
@@ -569,20 +575,12 @@ class Test extends AsyncResource {
         return;
       }
 
-      if (this.parent?.hooks.afterEach.length > 0) {
-        await afterEach();
-      }
-      if (this.hooks.after.length > 0) {
-        await this.runHook('after', { args, ctx });
-      }
+      await afterEach();
+      await after();
       this.pass();
     } catch (err) {
-      if (this.parent?.hooks.afterEach.length > 0) {
-        try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
-      }
-      if (this.hooks.after.length > 0) {
-        try { await this.runHook('after', { args, ctx }); } catch { /* Ignore error. */ }
-      }
+      try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
+      try { await after(); } catch { /* Ignore error. */ }
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.#cancel(err);
diff --git a/test/fixtures/test-runner/output/describe_it.snapshot b/test/fixtures/test-runner/output/describe_it.snapshot
index 11b47ed2652e72..e085ff9535ec70 100644
--- a/test/fixtures/test-runner/output/describe_it.snapshot
+++ b/test/fixtures/test-runner/output/describe_it.snapshot
@@ -40,9 +40,6 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
     *
     *
     *
-    *
-    *
-    *
   ...
 # Subtest: sync skip pass
 ok 5 - sync skip pass # SKIP
@@ -491,10 +488,10 @@ not ok 53 - custom inspect symbol that throws fail
         *
         *
         *
-        new Promise (<anonymous>)
         *
         *
-        Array.map (<anonymous>)
+        *
+        async Promise.all (index 0)
       ...
     1..2
 not ok 54 - subtest sync throw fails
diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot
index 7c0b9d0660656b..5b16957ba24dc6 100644
--- a/test/fixtures/test-runner/output/hooks.snapshot
+++ b/test/fixtures/test-runner/output/hooks.snapshot
@@ -163,8 +163,8 @@ not ok 4 - beforeEach throws
         *
         *
         *
-        async Promise.all (index 0)
         *
+        async Promise.all (index 0)
         *
       ...
     # Subtest: 2
@@ -182,8 +182,8 @@ not ok 4 - beforeEach throws
         *
         *
         *
-        async Promise.all (index 0)
         *
+        async Promise.all (index 0)
         *
       ...
     1..2
@@ -264,8 +264,8 @@ not ok 6 - afterEach when test fails
         *
         *
         *
-        async Promise.all (index 0)
         *
+        async Promise.all (index 0)
         *
       ...
     1..2
diff --git a/test/fixtures/test-runner/output/output.snapshot b/test/fixtures/test-runner/output/output.snapshot
index 17ad4d1af5c5e5..db19d8ca549a38 100644
--- a/test/fixtures/test-runner/output/output.snapshot
+++ b/test/fixtures/test-runner/output/output.snapshot
@@ -40,9 +40,6 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
     *
     *
     *
-    *
-    *
-    *
   ...
 # Subtest: sync skip pass
 ok 5 - sync skip pass # SKIP
diff --git a/test/fixtures/test-runner/output/output_cli.snapshot b/test/fixtures/test-runner/output/output_cli.snapshot
index 6d657668efb62c..fe192625e1f8b6 100644
--- a/test/fixtures/test-runner/output/output_cli.snapshot
+++ b/test/fixtures/test-runner/output/output_cli.snapshot
@@ -40,9 +40,6 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
     *
     *
     *
-    *
-    *
-    *
   ...
 # Subtest: sync skip pass
 ok 5 - sync skip pass # SKIP
diff --git a/test/fixtures/test-runner/output/spec_reporter.snapshot b/test/fixtures/test-runner/output/spec_reporter.snapshot
index c52e428a810d54..768204177014af 100644
--- a/test/fixtures/test-runner/output/spec_reporter.snapshot
+++ b/test/fixtures/test-runner/output/spec_reporter.snapshot
@@ -19,9 +19,6 @@
       *
       *
       *
-      *
-      *
-      *
 
  sync skip pass (*ms) # SKIP
  sync skip pass with message (*ms) # SKIP
@@ -329,9 +326,6 @@
       *
       *
       *
-      *
-      *
-      *
 
  sync throw fail (*ms)
   Error: thrown from sync throw fail
diff --git a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
index 1bd115bc2b51c7..e4e08764fd4925 100644
--- a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
+++ b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot
@@ -19,9 +19,6 @@
       *
       *
       *
-      *
-      *
-      *
 
  sync skip pass (*ms) # SKIP
  sync skip pass with message (*ms) # SKIP
@@ -329,9 +326,6 @@
       *
       *
       *
-      *
-      *
-      *
 
  sync throw fail (*ms)
   Error: thrown from sync throw fail