Skip to content

Commit

Permalink
test_runner: cleanup test timeout abort listener
Browse files Browse the repository at this point in the history
fix #48475

PR-URL: nodejs/node#48915
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
  • Loading branch information
sercher committed Apr 25, 2024
1 parent 44483cc commit 41ad677
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 10 deletions.
56 changes: 46 additions & 10 deletions graal-nodejs/lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -817,6 +850,7 @@ class Suite extends Test {
async run() {
const hookArgs = this.getRunArgs();

let stopPromise;
try {
this.parent.activeSubtests++;
await this.buildSuite;
Expand All @@ -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());

Expand All @@ -848,6 +882,8 @@ class Suite extends Test {
} else {
this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
}
} finally {
stopPromise?.[SymbolDispose]();
}

this.postRun();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';
const { beforeEach, afterEach, test} = require("node:test");
beforeEach(() => {});
afterEach(() => {});

for (let i = 1; i <= 11; ++i) {
test(`${i}`, () => {});
}
Original file line number Diff line number Diff line change
@@ -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 *
Original file line number Diff line number Diff line change
@@ -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}`, () => {});
}
Original file line number Diff line number Diff line change
@@ -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 *
2 changes: 2 additions & 0 deletions graal-nodejs/test/parallel/test-runner-output.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand Down

0 comments on commit 41ad677

Please sign in to comment.