Skip to content

Commit c2f1830

Browse files
rluvatonUlisesGascon
rluvaton
authored and
UlisesGascon
committed
test_runner: cleanup test timeout abort listener
fix #48475 PR-URL: #48915 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
1 parent 75333f3 commit c2f1830

6 files changed

+194
-10
lines changed

‎lib/internal/test_runner/test.js

+46-10
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@ const {
2020
SafeSet,
2121
SafePromiseAll,
2222
SafePromiseRace,
23+
SymbolDispose,
24+
ObjectDefineProperty,
2325
Symbol,
2426
} = primordials;
27+
const { addAbortListener } = require('events');
2528
const { AsyncResource } = require('async_hooks');
26-
const { once } = require('events');
2729
const { AbortController } = require('internal/abort_controller');
2830
const {
2931
codes: {
@@ -52,7 +54,7 @@ const {
5254
validateOneOf,
5355
validateUint32,
5456
} = require('internal/validators');
55-
const { setTimeout } = require('timers/promises');
57+
const { setTimeout } = require('timers');
5658
const { TIMEOUT_MAX } = require('internal/timers');
5759
const { availableParallelism } = require('os');
5860
const { bigint: hrtime } = process.hrtime;
@@ -76,15 +78,42 @@ const { testNamePatterns, testOnlyFlag } = parseCommandLine();
7678
let kResistStopPropagation;
7779

7880
function stopTest(timeout, signal) {
81+
const deferred = createDeferredPromise();
82+
const abortListener = addAbortListener(signal, deferred.resolve);
83+
let timer;
84+
let disposeFunction;
85+
7986
if (timeout === kDefaultTimeout) {
80-
return once(signal, 'abort');
87+
disposeFunction = abortListener[SymbolDispose];
88+
} if (timeout !== kDefaultTimeout) {
89+
timer = setTimeout(() => deferred.resolve(), timeout);
90+
timer.unref();
91+
92+
ObjectDefineProperty(deferred, 'promise', {
93+
__proto__: null,
94+
configurable: true,
95+
writable: true,
96+
value: PromisePrototypeThen(deferred.promise, () => {
97+
throw new ERR_TEST_FAILURE(
98+
`test timed out after ${timeout}ms`,
99+
kTestTimeoutFailure,
100+
);
101+
}),
102+
});
103+
104+
disposeFunction = () => {
105+
abortListener[SymbolDispose]();
106+
timer[SymbolDispose]();
107+
};
81108
}
82-
return PromisePrototypeThen(setTimeout(timeout, null, { __proto__: null, ref: false, signal }), () => {
83-
throw new ERR_TEST_FAILURE(
84-
`test timed out after ${timeout}ms`,
85-
kTestTimeoutFailure,
86-
);
109+
110+
ObjectDefineProperty(deferred.promise, SymbolDispose, {
111+
__proto__: null,
112+
configurable: true,
113+
writable: true,
114+
value: disposeFunction,
87115
});
116+
return deferred.promise;
88117
}
89118

90119
class TestContext {
@@ -549,14 +578,16 @@ class Test extends AsyncResource {
549578
}
550579
});
551580

581+
let stopPromise;
582+
552583
try {
553584
if (this.parent?.hooks.before.length > 0) {
554585
await this.parent.runHook('before', this.parent.getRunArgs());
555586
}
556587
if (this.parent?.hooks.beforeEach.length > 0) {
557588
await this.parent.runHook('beforeEach', { __proto__: null, args, ctx });
558589
}
559-
const stopPromise = stopTest(this.timeout, this.signal);
590+
stopPromise = stopTest(this.timeout, this.signal);
560591
const runArgs = ArrayPrototypeSlice(args);
561592
ArrayPrototypeUnshift(runArgs, this.fn, ctx);
562593

@@ -603,6 +634,8 @@ class Test extends AsyncResource {
603634
this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
604635
}
605636
} finally {
637+
stopPromise?.[SymbolDispose]();
638+
606639
// Do not abort hooks and the root test as hooks instance are shared between tests suite so aborting them will
607640
// cause them to not run for further tests.
608641
if (this.parent !== null) {
@@ -817,6 +850,7 @@ class Suite extends Test {
817850
async run() {
818851
const hookArgs = this.getRunArgs();
819852

853+
let stopPromise;
820854
try {
821855
this.parent.activeSubtests++;
822856
await this.buildSuite;
@@ -834,7 +868,7 @@ class Suite extends Test {
834868

835869
await this.runHook('before', hookArgs);
836870

837-
const stopPromise = stopTest(this.timeout, this.signal);
871+
stopPromise = stopTest(this.timeout, this.signal);
838872
const subtests = this.skipped || this.error ? [] : this.subtests;
839873
const promise = SafePromiseAll(subtests, (subtests) => subtests.start());
840874

@@ -848,6 +882,8 @@ class Suite extends Test {
848882
} else {
849883
this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
850884
}
885+
} finally {
886+
stopPromise?.[SymbolDispose]();
851887
}
852888

853889
this.postRun();

‎test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.js

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
'use strict';
2+
const { beforeEach, afterEach, test} = require("node:test");
3+
beforeEach(() => {});
4+
afterEach(() => {});
5+
6+
for (let i = 1; i <= 11; ++i) {
7+
test(`${i}`, () => {});
8+
}

‎test/fixtures/test-runner/output/before-and-after-each-too-many-listeners.snapshot

+65
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
TAP version 13
2+
# Subtest: 1
3+
ok 1 - 1
4+
---
5+
duration_ms: *
6+
...
7+
# Subtest: 2
8+
ok 2 - 2
9+
---
10+
duration_ms: *
11+
...
12+
# Subtest: 3
13+
ok 3 - 3
14+
---
15+
duration_ms: *
16+
...
17+
# Subtest: 4
18+
ok 4 - 4
19+
---
20+
duration_ms: *
21+
...
22+
# Subtest: 5
23+
ok 5 - 5
24+
---
25+
duration_ms: *
26+
...
27+
# Subtest: 6
28+
ok 6 - 6
29+
---
30+
duration_ms: *
31+
...
32+
# Subtest: 7
33+
ok 7 - 7
34+
---
35+
duration_ms: *
36+
...
37+
# Subtest: 8
38+
ok 8 - 8
39+
---
40+
duration_ms: *
41+
...
42+
# Subtest: 9
43+
ok 9 - 9
44+
---
45+
duration_ms: *
46+
...
47+
# Subtest: 10
48+
ok 10 - 10
49+
---
50+
duration_ms: *
51+
...
52+
# Subtest: 11
53+
ok 11 - 11
54+
---
55+
duration_ms: *
56+
...
57+
1..11
58+
# tests 11
59+
# suites 0
60+
# pass 11
61+
# fail 0
62+
# cancelled 0
63+
# skipped 0
64+
# todo 0
65+
# duration_ms *

‎test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
'use strict';
2+
const { beforeEach, afterEach, test} = require("node:test");
3+
beforeEach(() => {}, {timeout: 10000});
4+
afterEach(() => {}, {timeout: 10000});
5+
6+
for (let i = 1; i <= 11; ++i) {
7+
test(`${i}`, () => {});
8+
}

‎test/fixtures/test-runner/output/before-and-after-each-with-timeout-too-many-listeners.snapshot

+65
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
TAP version 13
2+
# Subtest: 1
3+
ok 1 - 1
4+
---
5+
duration_ms: *
6+
...
7+
# Subtest: 2
8+
ok 2 - 2
9+
---
10+
duration_ms: *
11+
...
12+
# Subtest: 3
13+
ok 3 - 3
14+
---
15+
duration_ms: *
16+
...
17+
# Subtest: 4
18+
ok 4 - 4
19+
---
20+
duration_ms: *
21+
...
22+
# Subtest: 5
23+
ok 5 - 5
24+
---
25+
duration_ms: *
26+
...
27+
# Subtest: 6
28+
ok 6 - 6
29+
---
30+
duration_ms: *
31+
...
32+
# Subtest: 7
33+
ok 7 - 7
34+
---
35+
duration_ms: *
36+
...
37+
# Subtest: 8
38+
ok 8 - 8
39+
---
40+
duration_ms: *
41+
...
42+
# Subtest: 9
43+
ok 9 - 9
44+
---
45+
duration_ms: *
46+
...
47+
# Subtest: 10
48+
ok 10 - 10
49+
---
50+
duration_ms: *
51+
...
52+
# Subtest: 11
53+
ok 11 - 11
54+
---
55+
duration_ms: *
56+
...
57+
1..11
58+
# tests 11
59+
# suites 0
60+
# pass 11
61+
# fail 0
62+
# cancelled 0
63+
# skipped 0
64+
# todo 0
65+
# duration_ms *

‎test/parallel/test-runner-output.mjs

+2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ const tests = [
3737
{ name: 'test-runner/output/describe_nested.js' },
3838
{ name: 'test-runner/output/hooks.js' },
3939
{ name: 'test-runner/output/hooks-with-no-global-test.js' },
40+
{ name: 'test-runner/output/before-and-after-each-too-many-listeners.js' },
41+
{ name: 'test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js' },
4042
{ name: 'test-runner/output/no_refs.js' },
4143
{ name: 'test-runner/output/no_tests.js' },
4244
{ name: 'test-runner/output/only_tests.js' },

0 commit comments

Comments
 (0)