From abd84adb0ecf57c5e3e46c0f56e94e9a996549e9 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Mon, 31 Jul 2023 11:10:36 +0300 Subject: [PATCH] test_runner: cleanup test timeout abort listener fix #48475 PR-URL: https://github.com/nodejs/node/pull/48915 Reviewed-By: Moshe Atlow Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow --- lib/internal/test_runner/test.js | 56 +++++++++++++--- ...efore-and-after-each-too-many-listeners.js | 8 +++ ...and-after-each-too-many-listeners.snapshot | 65 +++++++++++++++++++ ...er-each-with-timeout-too-many-listeners.js | 8 +++ ...h-with-timeout-too-many-listeners.snapshot | 65 +++++++++++++++++++ test/parallel/test-runner-output.mjs | 2 + 6 files changed, 194 insertions(+), 10 deletions(-) create mode 100644 test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.js create mode 100644 test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.snapshot create mode 100644 test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js create mode 100644 test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.snapshot diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 0d8e3dbd6e8d3c..3d4df848cca293 100644 --- 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,6 +578,8 @@ class Test extends AsyncResource { } }); + let stopPromise; + try { if (this.parent?.hooks.before.length > 0) { await this.parent.runHook('before', this.parent.getRunArgs()); @@ -556,7 +587,7 @@ class Test extends AsyncResource { 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 new file mode 100644 index 00000000000000..73857096068f9a --- /dev/null +++ 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 new file mode 100644 index 00000000000000..4300e21a26403f --- /dev/null +++ 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 new file mode 100644 index 00000000000000..87d645d6b0fa82 --- /dev/null +++ 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/test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.snapshot b/test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.snapshot new file mode 100644 index 00000000000000..4300e21a26403f --- /dev/null +++ b/test/fixtures/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 index 76c511117ea091..c4f7ce1d536f73 100644 --- 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' },