From 14af24be49365fb8e4e2ffd270e86bababa3820e Mon Sep 17 00:00:00 2001
From: Jithil <jithil@outlook.com>
Date: Thu, 27 Oct 2022 20:26:49 +1100
Subject: [PATCH 1/5] test_runner: fix afterEach not running on test failures

test_runner: fix afterEach not running on test failures
---
 lib/internal/test_runner/test.js | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 75a3a1558924b1..24dbc75a2abb1b 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -482,8 +482,9 @@ class Test extends AsyncResource {
       return;
     }
 
+    const { args, ctx } = this.getRunArgs();
+
     try {
-      const { args, ctx } = this.getRunArgs();
       if (this.parent?.hooks.beforeEach.length > 0) {
         await this.parent[kRunHook]('beforeEach', { args, ctx });
       }
@@ -518,10 +519,6 @@ class Test extends AsyncResource {
         return;
       }
 
-      if (this.parent?.hooks.afterEach.length > 0) {
-        await this.parent[kRunHook]('afterEach', { args, ctx });
-      }
-
       this.pass();
     } catch (err) {
       if (isTestFailureError(err)) {
@@ -533,6 +530,12 @@ class Test extends AsyncResource {
       } else {
         this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
       }
+    } finally {
+      const afterEach = this.parent?.hooks?.afterEach || [];
+      const shouldRunAfterEach = afterEach.length > 0;
+      if (shouldRunAfterEach) {
+        await this.parent[kRunHook]('afterEach', { args, ctx });
+      }
     }
 
     // Clean up the test. Then, try to report the results and execute any

From 30268cde9df3939ffec70cfb71334f85ed8bfe33 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Sat, 5 Nov 2022 20:46:53 +0200
Subject: [PATCH 2/5] fix

---
 lib/internal/test_runner/test.js         | 27 ++++----
 lib/internal/util.js                     |  2 +-
 test/message/test_runner_describe_it.out |  2 -
 test/message/test_runner_hooks.js        | 15 ++++-
 test/message/test_runner_hooks.out       | 78 ++++++++++++++++++++++--
 test/message/test_runner_output.out      |  2 -
 6 files changed, 103 insertions(+), 23 deletions(-)

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 24dbc75a2abb1b..ac595efc9e94d6 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -41,6 +41,7 @@ const {
 const {
   createDeferredPromise,
   kEmptyObject,
+  once: runOnce,
 } = require('internal/util');
 const { isPromise } = require('internal/util/types');
 const {
@@ -483,6 +484,11 @@ class Test extends AsyncResource {
     }
 
     const { args, ctx } = this.getRunArgs();
+    const afterEach = runOnce(async () => {
+      if (this.parent?.hooks.afterEach.length > 0) {
+        await this.parent[kRunHook]('afterEach', { args, ctx });
+      }
+    });
 
     try {
       if (this.parent?.hooks.beforeEach.length > 0) {
@@ -519,8 +525,10 @@ class Test extends AsyncResource {
         return;
       }
 
+      await afterEach();
       this.pass();
     } catch (err) {
+      await afterEach();
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.cancel(err);
@@ -530,12 +538,6 @@ class Test extends AsyncResource {
       } else {
         this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
       }
-    } finally {
-      const afterEach = this.parent?.hooks?.afterEach || [];
-      const shouldRunAfterEach = afterEach.length > 0;
-      if (shouldRunAfterEach) {
-        await this.parent[kRunHook]('afterEach', { args, ctx });
-      }
     }
 
     // Clean up the test. Then, try to report the results and execute any
@@ -731,6 +733,12 @@ class Suite extends Test {
   }
 
   async run() {
+    const hookArgs = this.getRunArgs();
+    const afterEach = runOnce(async () => {
+      if (this.parent?.hooks.afterEach.length > 0) {
+        await this.parent[kRunHook]('afterEach', hookArgs);
+      }
+    });
     try {
       this.parent.activeSubtests++;
       await this.buildSuite;
@@ -742,7 +750,6 @@ class Suite extends Test {
         return;
       }
 
-      const hookArgs = this.getRunArgs();
 
       if (this.parent?.hooks.beforeEach.length > 0) {
         await this.parent[kRunHook]('beforeEach', hookArgs);
@@ -756,13 +763,11 @@ class Suite extends Test {
 
       await SafePromiseRace([promise, stopPromise]);
       await this[kRunHook]('after', hookArgs);
-
-      if (this.parent?.hooks.afterEach.length > 0) {
-        await this.parent[kRunHook]('afterEach', hookArgs);
-      }
+      await afterEach();
 
       this.pass();
     } catch (err) {
+      await afterEach();
       if (isTestFailureError(err)) {
         this.fail(err);
       } else {
diff --git a/lib/internal/util.js b/lib/internal/util.js
index 76d3fd9122383e..4f74f912936611 100644
--- a/lib/internal/util.js
+++ b/lib/internal/util.js
@@ -453,7 +453,7 @@ function once(callback) {
   return function(...args) {
     if (called) return;
     called = true;
-    ReflectApply(callback, this, args);
+    return ReflectApply(callback, this, args);
   };
 }
 
diff --git a/test/message/test_runner_describe_it.out b/test/message/test_runner_describe_it.out
index 7ca922397a52b3..199e834d6f65ae 100644
--- a/test/message/test_runner_describe_it.out
+++ b/test/message/test_runner_describe_it.out
@@ -42,8 +42,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/message/test_runner_hooks.js b/test/message/test_runner_hooks.js
index 4820b01470d0fe..32e1cda1a4f626 100644
--- a/test/message/test_runner_hooks.js
+++ b/test/message/test_runner_hooks.js
@@ -1,6 +1,6 @@
 // Flags: --no-warnings
 'use strict';
-require('../common');
+const common = require('../common');
 const assert = require('assert');
 const { test, describe, it, before, after, beforeEach, afterEach } = require('node:test');
 
@@ -76,6 +76,12 @@ describe('afterEach throws', () => {
   it('2', () => {});
 });
 
+describe('afterEach when test fails', () => {
+  afterEach(common.mustCall(2));
+  it('1', () => { throw new Error('test'); });
+  it('2', () => {});
+});
+
 test('test hooks', async (t) => {
   const testArr = [];
   t.beforeEach((t) => testArr.push('beforeEach ' + t.name));
@@ -111,3 +117,10 @@ test('t.afterEach throws', async (t) => {
   await t.test('1', () => {});
   await t.test('2', () => {});
 });
+
+
+test('afterEach when test fails', async (t) => {
+  t.afterEach(common.mustCall(2));
+  await t.test('1', () => { throw new Error('test'); });
+  await t.test('2', () => {});
+});
diff --git a/test/message/test_runner_hooks.out b/test/message/test_runner_hooks.out
index 57008a44bbb141..a0eda2665861dc 100644
--- a/test/message/test_runner_hooks.out
+++ b/test/message/test_runner_hooks.out
@@ -189,6 +189,39 @@ not ok 5 - afterEach throws
   error: '2 subtests failed'
   code: 'ERR_TEST_FAILURE'
   ...
+# Subtest: afterEach when test fails
+    # Subtest: 1
+    not ok 1 - 1
+      ---
+      duration_ms: *
+      failureType: 'testCodeFailure'
+      error: 'test'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    # Subtest: 2
+    ok 2 - 2
+      ---
+      duration_ms: *
+      ...
+    1..2
+not ok 6 - afterEach when test fails
+  ---
+  duration_ms: *
+  failureType: 'subtestsFailed'
+  error: '1 subtest failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
 # Subtest: test hooks
     # Subtest: 1
     ok 1 - 1
@@ -217,7 +250,7 @@ not ok 5 - afterEach throws
       duration_ms: *
       ...
     1..3
-ok 6 - test hooks
+ok 7 - test hooks
   ---
   duration_ms: *
   ...
@@ -261,7 +294,7 @@ ok 6 - test hooks
         *
       ...
     1..2
-not ok 7 - t.beforeEach throws
+not ok 8 - t.beforeEach throws
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
@@ -308,17 +341,50 @@ not ok 7 - t.beforeEach throws
         *
       ...
     1..2
-not ok 8 - t.afterEach throws
+not ok 9 - t.afterEach throws
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
   error: '2 subtests failed'
   code: 'ERR_TEST_FAILURE'
   ...
-1..8
-# tests 8
+# Subtest: afterEach when test fails
+    # Subtest: 1
+    not ok 1 - 1
+      ---
+      duration_ms: *
+      failureType: 'testCodeFailure'
+      error: 'test'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    # Subtest: 2
+    ok 2 - 2
+      ---
+      duration_ms: *
+      ...
+    1..2
+not ok 10 - afterEach when test fails
+  ---
+  duration_ms: *
+  failureType: 'subtestsFailed'
+  error: '1 subtest failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+1..10
+# tests 10
 # pass 2
-# fail 6
+# fail 8
 # cancelled 0
 # skipped 0
 # todo 0
diff --git a/test/message/test_runner_output.out b/test/message/test_runner_output.out
index 938989b1b34146..96d977b21c5b1a 100644
--- a/test/message/test_runner_output.out
+++ b/test/message/test_runner_output.out
@@ -42,8 +42,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

From c17bb82dfe655ffd3ac7a93e4cf4cdbe91ab5bf1 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Sat, 5 Nov 2022 23:42:08 +0200
Subject: [PATCH 3/5] CR

---
 lib/internal/test_runner/test.js   |   4 +-
 test/message/test_runner_hooks.js  |  12 ++++
 test/message/test_runner_hooks.out | 108 +++++++++++++++++++++++++++--
 3 files changed, 115 insertions(+), 9 deletions(-)

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index ac595efc9e94d6..68a1ded1c52b81 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -528,7 +528,7 @@ class Test extends AsyncResource {
       await afterEach();
       this.pass();
     } catch (err) {
-      await afterEach();
+      await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop);
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.cancel(err);
@@ -767,7 +767,7 @@ class Suite extends Test {
 
       this.pass();
     } catch (err) {
-      await afterEach();
+      await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop);
       if (isTestFailureError(err)) {
         this.fail(err);
       } else {
diff --git a/test/message/test_runner_hooks.js b/test/message/test_runner_hooks.js
index 32e1cda1a4f626..e5e6bdc9d17ffa 100644
--- a/test/message/test_runner_hooks.js
+++ b/test/message/test_runner_hooks.js
@@ -82,6 +82,12 @@ describe('afterEach when test fails', () => {
   it('2', () => {});
 });
 
+describe('afterEach throws and test fails', () => {
+  afterEach(() => { throw new Error('afterEach'); });
+  it('1', () => { throw new Error('test'); });
+  it('2', () => {});
+});
+
 test('test hooks', async (t) => {
   const testArr = [];
   t.beforeEach((t) => testArr.push('beforeEach ' + t.name));
@@ -124,3 +130,9 @@ test('afterEach when test fails', async (t) => {
   await t.test('1', () => { throw new Error('test'); });
   await t.test('2', () => {});
 });
+
+test('afterEach throws and test fails', async (t) => {
+  afterEach(() => { throw new Error('afterEach'); });
+  await t.test('1', () => { throw new Error('test'); });
+  await t.test('2', () => {});
+});
diff --git a/test/message/test_runner_hooks.out b/test/message/test_runner_hooks.out
index a0eda2665861dc..e0b3e91b8c1376 100644
--- a/test/message/test_runner_hooks.out
+++ b/test/message/test_runner_hooks.out
@@ -222,6 +222,53 @@ not ok 6 - afterEach when test fails
   error: '1 subtest failed'
   code: 'ERR_TEST_FAILURE'
   ...
+# Subtest: afterEach throws and test fails
+    # Subtest: 1
+    not ok 1 - 1
+      ---
+      duration_ms: *
+      failureType: 'testCodeFailure'
+      error: 'test'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    # Subtest: 2
+    not ok 2 - 2
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'failed running afterEach hook'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    1..2
+not ok 7 - afterEach throws and test fails
+  ---
+  duration_ms: *
+  failureType: 'subtestsFailed'
+  error: '2 subtests failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
 # Subtest: test hooks
     # Subtest: 1
     ok 1 - 1
@@ -250,7 +297,7 @@ not ok 6 - afterEach when test fails
       duration_ms: *
       ...
     1..3
-ok 7 - test hooks
+ok 8 - test hooks
   ---
   duration_ms: *
   ...
@@ -294,7 +341,7 @@ ok 7 - test hooks
         *
       ...
     1..2
-not ok 8 - t.beforeEach throws
+not ok 9 - t.beforeEach throws
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
@@ -341,7 +388,7 @@ not ok 8 - t.beforeEach throws
         *
       ...
     1..2
-not ok 9 - t.afterEach throws
+not ok 10 - t.afterEach throws
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
@@ -374,17 +421,64 @@ not ok 9 - t.afterEach throws
       duration_ms: *
       ...
     1..2
-not ok 10 - afterEach when test fails
+not ok 11 - afterEach when test fails
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
   error: '1 subtest failed'
   code: 'ERR_TEST_FAILURE'
   ...
-1..10
-# tests 10
+# Subtest: afterEach throws and test fails
+    # Subtest: 1
+    not ok 1 - 1
+      ---
+      duration_ms: *
+      failureType: 'testCodeFailure'
+      error: 'test'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    # Subtest: 2
+    not ok 2 - 2
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'failed running afterEach hook'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    1..2
+not ok 12 - afterEach throws and test fails
+  ---
+  duration_ms: *
+  failureType: 'subtestsFailed'
+  error: '2 subtests failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+1..12
+# tests 12
 # pass 2
-# fail 8
+# fail 10
 # cancelled 0
 # skipped 0
 # todo 0

From a9defa08c8302ccd2d0872159238b90060b5a665 Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Sun, 6 Nov 2022 08:44:10 +0200
Subject: [PATCH 4/5] CR

---
 lib/internal/test_runner/test.js | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 68a1ded1c52b81..4109c39903cf88 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -528,7 +528,8 @@ class Test extends AsyncResource {
       await afterEach();
       this.pass();
     } catch (err) {
-      await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop);
+      // eslint-disable-next-line no-empty
+      try { await afterEach(); } catch {}
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.cancel(err);
@@ -767,7 +768,8 @@ class Suite extends Test {
 
       this.pass();
     } catch (err) {
-      await PromisePrototypeThen(PromiseResolve(afterEach()), noop, noop);
+      // eslint-disable-next-line no-empty
+      try { await afterEach(); } catch {}
       if (isTestFailureError(err)) {
         this.fail(err);
       } else {

From 5880f19d59b85c74945929c04954e6b6c770210a Mon Sep 17 00:00:00 2001
From: Moshe Atlow <moshe@atlow.co.il>
Date: Sun, 6 Nov 2022 22:29:52 +0200
Subject: [PATCH 5/5] CR

---
 lib/internal/test_runner/test.js | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 4109c39903cf88..6d30f3aaa6686a 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -528,8 +528,7 @@ class Test extends AsyncResource {
       await afterEach();
       this.pass();
     } catch (err) {
-      // eslint-disable-next-line no-empty
-      try { await afterEach(); } catch {}
+      try { await afterEach(); } catch { /* test is already failing, let's the error */ }
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.cancel(err);
@@ -768,8 +767,7 @@ class Suite extends Test {
 
       this.pass();
     } catch (err) {
-      // eslint-disable-next-line no-empty
-      try { await afterEach(); } catch {}
+      try { await afterEach(); } catch { /* test is already failing, let's the error */ }
       if (isTestFailureError(err)) {
         this.fail(err);
       } else {