test_runner: fix timeout in *Each hook failing further tests

PR-URL: https://github.com/nodejs/node/pull/48925
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

lib/internal/test_runner/test.js

@@ -193,8 +193,8 @@ class SuiteContext {
 }
 
 class Test extends AsyncResource {
-  #abortController;
-  #outerSignal;
+  abortController;
+  outerSignal;
   #reportedSubtest;
 
   constructor(options) {
@@ -292,16 +292,16 @@ class Test extends AsyncResource {
       fn = noop;
     }
 
-    this.#abortController = new AbortController();
-    this.#outerSignal = signal;
-    this.signal = this.#abortController.signal;
+    this.abortController = new AbortController();
+    this.outerSignal = signal;
+    this.signal = this.abortController.signal;
 
     validateAbortSignal(signal, 'options.signal');
     if (signal) {
       kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
     }
 
-    this.#outerSignal?.addEventListener(
+    this.outerSignal?.addEventListener(
       'abort',
       this.#abortHandler,
       { __proto__: null, [kResistStopPropagation]: true },
@@ -441,7 +441,7 @@ class Test extends AsyncResource {
   }
 
   #abortHandler = () => {
-    const error = this.#outerSignal?.reason || new AbortError('The test was aborted');
+    const error = this.outerSignal?.reason || new AbortError('The test was aborted');
     error.failureType = kAborted;
     this.#cancel(error);
   };
@@ -459,7 +459,7 @@ class Test extends AsyncResource {
     );
     this.startTime = this.startTime || this.endTime; // If a test was canceled before it was started, e.g inside a hook
     this.cancelled = true;
-    this.#abortController.abort();
+    this.abortController.abort();
   }
 
   createHook(name, fn, options) {
@@ -527,7 +527,7 @@ class Test extends AsyncResource {
     if (this.signal.aborted) {
       return true;
     }
-    if (this.#outerSignal?.aborted) {
+    if (this.outerSignal?.aborted) {
       this.#abortHandler();
       return true;
     }
@@ -639,7 +639,7 @@ class Test extends AsyncResource {
       // Do not abort hooks and the root test as hooks instance are shared between tests suite so aborting them will
       // cause them to not run for further tests.
       if (this.parent !== null) {
-        this.#abortController.abort();
+        this.abortController.abort();
       }
     }
 
@@ -679,7 +679,7 @@ class Test extends AsyncResource {
       this.fail(new ERR_TEST_FAILURE(msg, kSubtestsFailed));
     }
 
-    this.#outerSignal?.removeEventListener('abort', this.#abortHandler);
+    this.outerSignal?.removeEventListener('abort', this.#abortHandler);
     this.mock?.reset();
 
     if (this.parent !== null) {
@@ -795,6 +795,14 @@ class TestHook extends Test {
     super({ __proto__: null, fn, timeout, signal });
   }
   run(args) {
+    if (this.error && !this.outerSignal?.aborted) {
+      this.passed = false;
+      this.error = null;
+      this.abortController.abort();
+      this.abortController = new AbortController();
+      this.signal = this.abortController.signal;
+    }
+
     this.#args = args;
     return super.run();
   }

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

@@ -0,0 +1,63 @@
+// Flags: --no-warnings
+'use strict';
+const { before, beforeEach, describe, it, after, afterEach } = require('node:test');
+
+describe('1 before describe', () => {
+  const ac = new AbortController();
+  before(() => {
+    console.log('before');
+    ac.abort()
+  }, {signal: ac.signal});
+
+  it('test 1', () => {
+    console.log('1.1');
+  });
+  it('test 2', () => {
+    console.log('1.2');
+  });
+});
+
+describe('2 after describe', () => {
+  const ac = new AbortController();
+  after(() => {
+    console.log('after');
+    ac.abort()
+  }, {signal: ac.signal});
+
+  it('test 1', () => {
+    console.log('2.1');
+  });
+  it('test 2', () => {
+    console.log('2.2');
+  });
+});
+
+describe('3 beforeEach describe', () => {
+  const ac = new AbortController();
+  beforeEach(() => {
+    console.log('beforeEach');
+    ac.abort()
+  }, {signal: ac.signal});
+
+  it('test 1', () => {
+    console.log('3.1');
+  });
+  it('test 2', () => {
+    console.log('3.2');
+  });
+});
+
+describe('4 afterEach describe', () => {
+  const ac = new AbortController();
+  afterEach(() => {
+    console.log('afterEach');
+    ac.abort()
+  }, {signal: ac.signal});
+
+  it('test 1', () => {
+    console.log('4.1');
+  });
+  it('test 2', () => {
+    console.log('4.2');
+  });
+});

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

@@ -0,0 +1,188 @@
+before
+2.1
+2.2
+after
+beforeEach
+4.1
+afterEach
+4.2
+TAP version 13
+# Subtest: 1 before describe
+    # Subtest: test 1
+    not ok 1 - test 1
+      ---
+      duration_ms: ZERO
+      failureType: 'cancelledByParent'
+      error: 'test did not finish before its parent and was cancelled'
+      code: 'ERR_TEST_FAILURE'
+      ...
+    # Subtest: test 2
+    not ok 2 - test 2
+      ---
+      duration_ms: ZERO
+      failureType: 'cancelledByParent'
+      error: 'test did not finish before its parent and was cancelled'
+      code: 'ERR_TEST_FAILURE'
+      ...
+    1..2
+not ok 1 - 1 before describe
+  ---
+  duration_ms: *
+  type: 'suite'
+  failureType: 'hookFailed'
+  error: 'This operation was aborted'
+  code: 20
+  name: 'AbortError'
+  stack: |-
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+  ...
+# Subtest: 2 after describe
+    # Subtest: test 1
+    ok 1 - test 1
+      ---
+      duration_ms: *
+      ...
+    # Subtest: test 2
+    ok 2 - test 2
+      ---
+      duration_ms: *
+      ...
+    1..2
+not ok 2 - 2 after describe
+  ---
+  duration_ms: *
+  type: 'suite'
+  failureType: 'hookFailed'
+  error: 'This operation was aborted'
+  code: 20
+  name: 'AbortError'
+  stack: |-
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+    *
+  ...
+# Subtest: 3 beforeEach describe
+    # Subtest: test 1
+    not ok 1 - test 1
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'This operation was aborted'
+      code: 20
+      name: 'AbortError'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        async Promise.all (index 0)
+      ...
+    # Subtest: test 2
+    not ok 2 - test 2
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'This operation was aborted'
+      code: 20
+      name: 'AbortError'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        async Promise.all (index 0)
+      ...
+    1..2
+not ok 3 - 3 beforeEach describe
+  ---
+  duration_ms: *
+  type: 'suite'
+  failureType: 'subtestsFailed'
+  error: '2 subtests failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+# Subtest: 4 afterEach describe
+    # Subtest: test 1
+    not ok 1 - test 1
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'This operation was aborted'
+      code: 20
+      name: 'AbortError'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    # Subtest: test 2
+    not ok 2 - test 2
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'This operation was aborted'
+      code: 20
+      name: 'AbortError'
+      stack: |-
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+        *
+      ...
+    1..2
+not ok 4 - 4 afterEach describe
+  ---
+  duration_ms: *
+  type: 'suite'
+  failureType: 'subtestsFailed'
+  error: '2 subtests failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+1..4
+# tests 8
+# suites 4
+# pass 2
+# fail 4
+# cancelled 2
+# skipped 0
+# todo 0
+# duration_ms *

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

@@ -134,8 +134,6 @@ not ok 3 - after throws
         *
         *
         *
-        async Promise.all (index 0)
-        *
         *
       ...
     1..2
@@ -183,7 +181,6 @@ not ok 4 - beforeEach throws
         *
         *
         *
-        async Promise.all (index 0)
         *
       ...
     1..2
@@ -265,7 +262,6 @@ not ok 6 - afterEach when test fails
         *
         *
         *
-        async Promise.all (index 0)
         *
       ...
     1..2

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

@@ -0,0 +1,46 @@
+const {describe, test, beforeEach, afterEach} = require("node:test");
+const {setTimeout} = require("timers/promises");
+
+
+describe('before each timeout', () => {
+  let i = 0;
+
+  beforeEach(async () => {
+    if (i++ === 0) {
+      console.log('gonna timeout');
+      await setTimeout(700);
+      return;
+    }
+    console.log('not gonna timeout');
+  }, {timeout: 500});
+
+  test('first describe first test', () => {
+    console.log('before each test first ' + i);
+  });
+
+  test('first describe second test', () => {
+    console.log('before each test second ' + i);
+  });
+});
+
+
+describe('after each timeout', () => {
+  let i = 0;
+
+  afterEach(async function afterEach1() {
+    if (i++ === 0) {
+      console.log('gonna timeout');
+      await setTimeout(700);
+      return;
+    }
+    console.log('not gonna timeout');
+  }, {timeout: 500});
+
+  test('second describe first test', () => {
+    console.log('after each test first ' + i);
+  });
+
+  test('second describe second test', () => {
+    console.log('after each test second ' + i);
+  });
+});

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

@@ -0,0 +1,67 @@
+gonna timeout
+TAP version 13
+not gonna timeout
+before each test second 2
+after each test first 0
+gonna timeout
+# Subtest: before each timeout
+    # Subtest: first describe first test
+    not ok 1 - first describe first test
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'failed running beforeEach hook'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        async Promise.all (index 0)
+      ...
+    # Subtest: first describe second test
+    ok 2 - first describe second test
+      ---
+      duration_ms: *
+      ...
+    1..2
+not ok 1 - before each timeout
+  ---
+  duration_ms: *
+  type: 'suite'
+  failureType: 'subtestsFailed'
+  error: '1 subtest failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+after each test second 1
+not gonna timeout
+# Subtest: after each timeout
+    # Subtest: second describe first test
+    not ok 1 - second describe first test
+      ---
+      duration_ms: *
+      failureType: 'hookFailed'
+      error: 'failed running afterEach hook'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        async Promise.all (index 0)
+      ...
+    # Subtest: second describe second test
+    ok 2 - second describe second test
+      ---
+      duration_ms: *
+      ...
+    1..2
+not ok 2 - after each timeout
+  ---
+  duration_ms: *
+  type: 'suite'
+  failureType: 'subtestsFailed'
+  error: '1 subtest failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+1..2
+# tests 4
+# suites 2
+# pass 2
+# fail 2
+# cancelled 0
+# skipped 0
+# todo 0
+# duration_ms *

test/parallel/test-runner-output.mjs

@@ -33,9 +33,11 @@ const specTransform = snapshot
 const tests = [
   { name: 'test-runner/output/abort.js' },
   { name: 'test-runner/output/abort_suite.js' },
+  { name: 'test-runner/output/abort_hooks.js' },
   { name: 'test-runner/output/describe_it.js' },
   { name: 'test-runner/output/describe_nested.js' },
   { name: 'test-runner/output/hooks.js' },
+  { name: 'test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js' },
   { name: 'test-runner/output/hooks-with-no-global-test.js' },
   { name: 'test-runner/output/before-and-after-each-too-many-listeners.js' },
   { name: 'test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js' },