test_runner: preserve hook promise when executed twice

PR-URL: #52791
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Author: MoLow

lib/internal/test_runner/test.js

@@ -87,6 +87,8 @@ const {
 let kResistStopPropagation;
 let findSourceMap;
 
+const kRunOnceOptions = { __proto__: null, preserveReturnValue: true };
+
 function lazyFindSourceMap(file) {
   if (findSourceMap === undefined) {
     ({ findSourceMap } = require('internal/source_map/source_map_cache'));
@@ -526,7 +528,7 @@ class Test extends AsyncResource {
     // eslint-disable-next-line no-use-before-define
     const hook = new TestHook(fn, options);
     if (name === 'before' || name === 'after') {
-      hook.run = runOnce(hook.run);
+      hook.run = runOnce(hook.run, kRunOnceOptions);
     }
     if (name === 'before' && this.startTime !== null) {
       // Test has already started, run the hook immediately
@@ -650,7 +652,7 @@ class Test extends AsyncResource {
       if (this.parent?.hooks.afterEach.length > 0 && !this.skipped) {
         await this.parent.runHook('afterEach', hookArgs);
       }
-    });
+    }, kRunOnceOptions);
 
     let stopPromise;
 
@@ -1004,7 +1006,7 @@ class Suite extends Test {
     const hookArgs = this.getRunArgs();
 
     let stopPromise;
-    const after = runOnce(() => this.runHook('after', hookArgs));
+    const after = runOnce(() => this.runHook('after', hookArgs), kRunOnceOptions);
     try {
       this.parent.activeSubtests++;
       await this.buildSuite;

lib/internal/util.js

@@ -511,12 +511,15 @@ function isInsideNodeModules() {
   return false;
 }
 
-function once(callback) {
+function once(callback, { preserveReturnValue = false } = kEmptyObject) {
   let called = false;
+  let returnValue;
   return function(...args) {
-    if (called) return;
+    if (called) return returnValue;
     called = true;
-    return ReflectApply(callback, this, args);
+    const result = ReflectApply(callback, this, args);
+    returnValue = preserveReturnValue ? result : undefined;
+    return result;
   };
 }
 

test/fixtures/test-runner/output/hooks.js

@@ -2,6 +2,7 @@
 const common = require('../../../common');
 const assert = require('assert');
 const { test, describe, it, before, after, beforeEach, afterEach } = require('node:test');
+const { setTimeout } = require('node:timers/promises');
 
 before((t) => t.diagnostic('before 1 called'));
 after((t) => t.diagnostic('after 1 called'));
@@ -148,7 +149,6 @@ test('test hooks', async (t) => {
   }));
 });
 
-
 test('test hooks - no subtests', async (t) => {
   const testArr = [];
 
@@ -253,5 +253,54 @@ describe('run after when before throws', () => {
   it('1', () => {});
 });
 
+
+test('test hooks - async', async (t) => {
+  const testArr = [];
+
+  t.before(async (t) => {
+    testArr.push('before starting ' + t.name);
+    await setTimeout(10);
+    testArr.push('before ending ' + t.name);
+  });
+  t.after(async (t) => {
+    testArr.push('after starting ' + t.name);
+    await setTimeout(10);
+    testArr.push('after ending ' + t.name);
+  });
+  t.beforeEach(async (t) => {
+    testArr.push('beforeEach starting ' + t.name);
+    await setTimeout(10);
+    testArr.push('beforeEach ending ' + t.name);
+  });
+  t.afterEach(async (t) => {
+    testArr.push('afterEach starting ' + t.name);
+    await setTimeout(10);
+    testArr.push('afterEach ending ' + t.name);
+  });
+  await t.test('1', async () => {
+    testArr.push('1 starting');
+    await setTimeout(10);
+    testArr.push('1 ending');
+  });
+  await t.test('2', async () => {
+    testArr.push('2 starting');
+    await setTimeout(10);
+    testArr.push('2 ending');
+  });
+
+  t.after(common.mustCall(() => {
+    assert.deepStrictEqual(testArr, [
+      'before starting test hooks - async', 'before ending test hooks - async',
+      'beforeEach starting 1', 'beforeEach ending 1',
+      '1 starting', '1 ending',
+      'afterEach starting 1', 'afterEach ending 1',
+      'beforeEach starting 2', 'beforeEach ending 2',
+      '2 starting', '2 ending',
+      'afterEach starting 2', 'afterEach ending 2',
+      'after starting test hooks - async', 'after ending test hooks - async',
+    ]);
+  }));
+});
+
 before((t) => t.diagnostic('before 2 called'));
 after((t) => t.diagnostic('after 2 called'));

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

@@ -767,14 +767,30 @@ not ok 24 - run after when before throws
     *
     *
   ...
-1..24
+# Subtest: test hooks - async
+    # Subtest: 1
+    ok 1 - 1
+      ---
+      duration_ms: *
+      ...
+    # Subtest: 2
+    ok 2 - 2
+      ---
+      duration_ms: *
+      ...
+    1..2
+ok 25 - test hooks - async
+  ---
+  duration_ms: *
+  ...
+1..25
 # before 1 called
 # before 2 called
 # after 1 called
 # after 2 called
-# tests 49
+# tests 52
 # suites 12
-# pass 19
+# pass 22
 # fail 27
 # cancelled 3
 # skipped 0

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

@@ -392,13 +392,17 @@
       *
       *
 
+ test hooks - async
+   1 (*ms)
+   2 (*ms)
+ test hooks - async (*ms)
  before 1 called
  before 2 called
  after 1 called
  after 2 called
- tests 49
+ tests 52
  suites 12
- pass 19
+ pass 22
  fail 27
  cancelled 3
  skipped 0