test_runner: cleanup test timeout abort listener

fix nodejs#48475 PR-URL: nodejs#48915 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
rluvaton · Aug 18, 2023 · 3bff1fb · 3bff1fb
1 parent 28602e2
commit 3bff1fb
Show file tree

Hide file tree

Showing 6 changed files with 194 additions and 10 deletions.
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -20,10 +20,12 @@ const {
   SafeSet,
   SafePromiseAll,
   SafePromiseRace,
+  SymbolDispose,
+  ObjectDefineProperty,
   Symbol,
 } = primordials;
+const { addAbortListener } = require('events');
 const { AsyncResource } = require('async_hooks');
-const { once } = require('events');
 const { AbortController } = require('internal/abort_controller');
 const {
   codes: {
@@ -52,7 +54,7 @@ const {
   validateOneOf,
   validateUint32,
 } = require('internal/validators');
-const { setTimeout } = require('timers/promises');
+const { setTimeout } = require('timers');
 const { TIMEOUT_MAX } = require('internal/timers');
 const { availableParallelism } = require('os');
 const { bigint: hrtime } = process.hrtime;
@@ -76,15 +78,42 @@ const { testNamePatterns, testOnlyFlag } = parseCommandLine();
 let kResistStopPropagation;
 
 function stopTest(timeout, signal) {
+  const deferred = createDeferredPromise();
+  const abortListener = addAbortListener(signal, deferred.resolve);
+  let timer;
+  let disposeFunction;
+
   if (timeout === kDefaultTimeout) {
-    return once(signal, 'abort');
+    disposeFunction = abortListener[SymbolDispose];
+  } if (timeout !== kDefaultTimeout) {
+    timer = setTimeout(() => deferred.resolve(), timeout);
+    timer.unref();
+
+    ObjectDefineProperty(deferred, 'promise', {
+      __proto__: null,
+      configurable: true,
+      writable: true,
+      value: PromisePrototypeThen(deferred.promise, () => {
+        throw new ERR_TEST_FAILURE(
+          `test timed out after ${timeout}ms`,
+          kTestTimeoutFailure,
+        );
+      }),
+    });
+
+    disposeFunction = () => {
+      abortListener[SymbolDispose]();
+      timer[SymbolDispose]();
+    };
   }
-  return PromisePrototypeThen(setTimeout(timeout, null, { __proto__: null, ref: false, signal }), () => {
-    throw new ERR_TEST_FAILURE(
-      `test timed out after ${timeout}ms`,
-      kTestTimeoutFailure,
-    );
+
+  ObjectDefineProperty(deferred.promise, SymbolDispose, {
+    __proto__: null,
+    configurable: true,
+    writable: true,
+    value: disposeFunction,
   });
+  return deferred.promise;
 }
 
 class TestContext {
@@ -549,14 +578,16 @@ class Test extends AsyncResource {
       }
     });
 
+    let stopPromise;
+
     try {
       if (this.parent?.hooks.before.length > 0) {
         await this.parent.runHook('before', this.parent.getRunArgs());
       }
       if (this.parent?.hooks.beforeEach.length > 0) {
         await this.parent.runHook('beforeEach', { __proto__: null, args, ctx });
       }
-      const stopPromise = stopTest(this.timeout, this.signal);
+      stopPromise = stopTest(this.timeout, this.signal);
       const runArgs = ArrayPrototypeSlice(args);
       ArrayPrototypeUnshift(runArgs, this.fn, ctx);
 
@@ -603,6 +634,8 @@ class Test extends AsyncResource {
         this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
       }
     } finally {
+      stopPromise?.[SymbolDispose]();
+
       // 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) {
@@ -817,6 +850,7 @@ class Suite extends Test {
   async run() {
     const hookArgs = this.getRunArgs();
 
+    let stopPromise;
     try {
       this.parent.activeSubtests++;
       await this.buildSuite;
@@ -834,7 +868,7 @@ class Suite extends Test {
 
       await this.runHook('before', hookArgs);
 
-      const stopPromise = stopTest(this.timeout, this.signal);
+      stopPromise = stopTest(this.timeout, this.signal);
       const subtests = this.skipped || this.error ? [] : this.subtests;
       const promise = SafePromiseAll(subtests, (subtests) => subtests.start());
 
@@ -848,6 +882,8 @@ class Suite extends Test {
       } else {
         this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
       }
+    } finally {
+      stopPromise?.[SymbolDispose]();
     }
 
     this.postRun();

diff --git a/test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.js b/test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.js
@@ -0,0 +1,8 @@
+'use strict';
+const { beforeEach, afterEach, test} = require("node:test");
+beforeEach(() => {});
+afterEach(() => {});
+
+for (let i = 1; i <= 11; ++i) {
+  test(`${i}`, () => {});
+}
diff --git a/test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.snapshot b/test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.snapshot
@@ -0,0 +1,65 @@
+TAP version 13
+# Subtest: 1
+ok 1 - 1
+  ---
+  duration_ms: *
+  ...
+# Subtest: 2
+ok 2 - 2
+  ---
+  duration_ms: *
+  ...
+# Subtest: 3
+ok 3 - 3
+  ---
+  duration_ms: *
+  ...
+# Subtest: 4
+ok 4 - 4
+  ---
+  duration_ms: *
+  ...
+# Subtest: 5
+ok 5 - 5
+  ---
+  duration_ms: *
+  ...
+# Subtest: 6
+ok 6 - 6
+  ---
+  duration_ms: *
+  ...
+# Subtest: 7
+ok 7 - 7
+  ---
+  duration_ms: *
+  ...
+# Subtest: 8
+ok 8 - 8
+  ---
+  duration_ms: *
+  ...
+# Subtest: 9
+ok 9 - 9
+  ---
+  duration_ms: *
+  ...
+# Subtest: 10
+ok 10 - 10
+  ---
+  duration_ms: *
+  ...
+# Subtest: 11
+ok 11 - 11
+  ---
+  duration_ms: *
+  ...
+1..11
+# tests 11
+# suites 0
+# pass 11
+# fail 0
+# cancelled 0
+# skipped 0
+# todo 0
+# duration_ms *
diff --git a/test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js b/test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js
@@ -0,0 +1,8 @@
+'use strict';
+const { beforeEach, afterEach, test} = require("node:test");
+beforeEach(() => {}, {timeout: 10000});
+afterEach(() => {}, {timeout: 10000});
+
+for (let i = 1; i <= 11; ++i) {
+  test(`${i}`, () => {});
+}
diff --git a/...ixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.snapshot b/...ixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.snapshot
@@ -0,0 +1,65 @@
+TAP version 13
+# Subtest: 1
+ok 1 - 1
+  ---
+  duration_ms: *
+  ...
+# Subtest: 2
+ok 2 - 2
+  ---
+  duration_ms: *
+  ...
+# Subtest: 3
+ok 3 - 3
+  ---
+  duration_ms: *
+  ...
+# Subtest: 4
+ok 4 - 4
+  ---
+  duration_ms: *
+  ...
+# Subtest: 5
+ok 5 - 5
+  ---
+  duration_ms: *
+  ...
+# Subtest: 6
+ok 6 - 6
+  ---
+  duration_ms: *
+  ...
+# Subtest: 7
+ok 7 - 7
+  ---
+  duration_ms: *
+  ...
+# Subtest: 8
+ok 8 - 8
+  ---
+  duration_ms: *
+  ...
+# Subtest: 9
+ok 9 - 9
+  ---
+  duration_ms: *
+  ...
+# Subtest: 10
+ok 10 - 10
+  ---
+  duration_ms: *
+  ...
+# Subtest: 11
+ok 11 - 11
+  ---
+  duration_ms: *
+  ...
+1..11
+# tests 11
+# suites 0
+# pass 11
+# fail 0
+# cancelled 0
+# skipped 0
+# todo 0
+# duration_ms *
diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs
@@ -37,6 +37,8 @@ const tests = [
   { name: 'test-runner/output/describe_nested.js' },
   { name: 'test-runner/output/hooks.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' },
   { name: 'test-runner/output/no_refs.js' },
   { name: 'test-runner/output/no_tests.js' },
   { name: 'test-runner/output/only_tests.js' },