test_runner: fix afterEach not running on test failures

test_runner: fix afterEach not running on test failures

PR-URL: https://github.com/nodejs/node/pull/45204
Fixes: https://github.com/nodejs/node/issues/45192
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

6 files changed, 211 insertions, 22 deletions

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index 75a3a155892..6d30f3aaa66 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 {
@@ -482,8 +483,14 @@ class Test extends AsyncResource {
       return;
     }
 
+    const { args, ctx } = this.getRunArgs();
+    const afterEach = runOnce(async () => {
+      if (this.parent?.hooks.afterEach.length > 0) {
+        await this.parent[kRunHook]('afterEach', { args, ctx });
+      }
+    });
+
     try {
-      const { args, ctx } = this.getRunArgs();
       if (this.parent?.hooks.beforeEach.length > 0) {
         await this.parent[kRunHook]('beforeEach', { args, ctx });
       }
@@ -518,12 +525,10 @@ class Test extends AsyncResource {
         return;
       }
 
-      if (this.parent?.hooks.afterEach.length > 0) {
-        await this.parent[kRunHook]('afterEach', { args, ctx });
-      }
-
+      await afterEach();
       this.pass();
     } catch (err) {
+      try { await afterEach(); } catch { /* test is already failing, let's the error */ }
       if (isTestFailureError(err)) {
         if (err.failureType === kTestTimeoutFailure) {
           this.cancel(err);
@@ -728,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;
@@ -739,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);
@@ -753,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) {
+      try { await afterEach(); } catch { /* test is already failing, let's the error */ }
       if (isTestFailureError(err)) {
         this.fail(err);
       } else {
diff --git a/lib/internal/util.js b/lib/internal/util.js
index 76d3fd91223..4f74f912936 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 7ca922397a5..199e834d6f6 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 4820b01470d..e5e6bdc9d17 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,18 @@ describe('afterEach throws', () => {
   it('2', () => {});
 });
 
+describe('afterEach when test fails', () => {
+  afterEach(common.mustCall(2));
+  it('1', () => { throw new Error('test'); });
+  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));
@@ -111,3 +123,16 @@ 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', () => {});
+});
+
+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 57008a44bbb..e0b3e91b8c1 100644
--- a/test/message/test_runner_hooks.out
+++ b/test/message/test_runner_hooks.out
@@ -189,6 +189,86 @@ 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: 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
@@ -217,7 +297,7 @@ not ok 5 - afterEach throws
       duration_ms: *
       ...
     1..3
-ok 6 - test hooks
+ok 8 - test hooks
   ---
   duration_ms: *
   ...
@@ -261,7 +341,7 @@ ok 6 - test hooks
         *
       ...
     1..2
-not ok 7 - t.beforeEach throws
+not ok 9 - t.beforeEach throws
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
@@ -308,17 +388,97 @@ not ok 7 - t.beforeEach throws
         *
       ...
     1..2
-not ok 8 - t.afterEach throws
+not ok 10 - t.afterEach throws
+  ---
+  duration_ms: *
+  failureType: 'subtestsFailed'
+  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 11 - afterEach when test fails
+  ---
+  duration_ms: *
+  failureType: 'subtestsFailed'
+  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 12 - afterEach throws and test fails
   ---
   duration_ms: *
   failureType: 'subtestsFailed'
   error: '2 subtests failed'
   code: 'ERR_TEST_FAILURE'
   ...
-1..8
-# tests 8
+1..12
+# tests 12
 # pass 2
-# fail 6
+# fail 10
 # cancelled 0
 # skipped 0
 # todo 0
diff --git a/test/message/test_runner_output.out b/test/message/test_runner_output.out
index 938989b1b34..96d977b21c5 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