Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: pass abort signal to test #43554

Merged
merged 2 commits into from
Jul 20, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 37 additions & 2 deletions doc/api/test.md
Original file line number Diff line number Diff line change
@@ -337,6 +337,7 @@ changes:
* `only` {boolean} If truthy, and the test context is configured to run
`only` tests, then this test will be run. Otherwise, the test is skipped.
**Default:** `false`.
* `signal` {AbortSignal} Allows aborting an in-progress test
* `skip` {boolean|string} If truthy, the test is skipped. If a string is
provided, that string is displayed in the test results as the reason for
skipping the test. **Default:** `false`.
@@ -385,8 +386,9 @@ test('top level test', async (t) => {
does not have a name.
* `options` {Object} Configuration options for the suite.
supports the same options as `test([name][, options][, fn])`
* `fn` {Function} The function under suite.
a synchronous function declaring all subtests and subsuites.
* `fn` {Function|AsyncFunction} The function under suite
declaring all subtests and subsuites.
The first argument to this function is a [`SuiteContext`][] object.
**Default:** A no-op function.
* Returns: `undefined`.

@@ -483,6 +485,20 @@ test('top level test', (t) => {
});
```

### `context.signal`

<!-- YAML
added: REPLACEME
-->

* <AbortSignal> Can be used to abort test subtasks when the test has been aborted.

```js
test('top level test', async (t) => {
await fetch('some/uri', { signal: t.signal });
});
```

### `context.skip([message])`

<!-- YAML
@@ -573,9 +589,28 @@ test('top level test', async (t) => {
});
```

## Class: `SuiteContext`

<!-- YAML
added: REPLACEME
-->

An instance of `SuiteContext` is passed to each suite function in order to
interact with the test runner. However, the `SuiteContext` constructor is not
exposed as part of the API.

### `context.signal`

<!-- YAML
added: REPLACEME
-->

* <AbortSignal> Can be used to abort test subtasks when the test has been aborted.

[TAP]: https://testanything.org/
[`--test-only`]: cli.md#--test-only
[`--test`]: cli.md#--test
[`SuiteContext`]: #class-suitecontext
[`TestContext`]: #class-testcontext
[`test()`]: #testname-options-fn
[describe options]: #describename-options-fn
85 changes: 35 additions & 50 deletions lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
@@ -3,20 +3,18 @@ const {
ArrayFrom,
ArrayPrototypeFilter,
ArrayPrototypeIncludes,
ArrayPrototypeJoin,
ArrayPrototypePush,
ArrayPrototypeSlice,
ArrayPrototypeSort,
Promise,
PromiseAll,
SafeArrayIterator,
SafePromiseAll,
SafeSet,
} = primordials;
const {
prepareMainThreadExecution,
} = require('internal/bootstrap/pre_execution');
const { spawn } = require('child_process');
const { readdirSync, statSync } = require('fs');
const { finished } = require('internal/streams/end-of-stream');
const console = require('internal/console/global');
const {
codes: {
@@ -30,6 +28,7 @@ const {
doesPathMatchFilter,
} = require('internal/test_runner/utils');
const { basename, join, resolve } = require('path');
const { once } = require('events');
const kFilterArgs = ['--test'];

prepareMainThreadExecution(false);
@@ -102,53 +101,39 @@ function filterExecArgv(arg) {
}

function runTestFile(path) {
return test(path, () => {
return new Promise((resolve, reject) => {
const args = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
ArrayPrototypePush(args, path);

const child = spawn(process.execPath, args);
// TODO(cjihrig): Implement a TAP parser to read the child's stdout
// instead of just displaying it all if the child fails.
let stdout = '';
let stderr = '';
let err;

child.on('error', (error) => {
err = error;
});

child.stdout.setEncoding('utf8');
child.stderr.setEncoding('utf8');

child.stdout.on('data', (chunk) => {
stdout += chunk;
});

child.stderr.on('data', (chunk) => {
stderr += chunk;
});

child.once('exit', async (code, signal) => {
if (code !== 0 || signal !== null) {
if (!err) {
await PromiseAll(new SafeArrayIterator([finished(child.stderr), finished(child.stdout)]));
err = new ERR_TEST_FAILURE('test failed', kSubtestsFailed);
err.exitCode = code;
err.signal = signal;
err.stdout = stdout;
err.stderr = stderr;
// The stack will not be useful since the failures came from tests
// in a child process.
err.stack = undefined;
}

return reject(err);
}

resolve();
});
return test(path, async (t) => {
const args = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
ArrayPrototypePush(args, path);

const child = spawn(process.execPath, args, { signal: t.signal, encoding: 'utf8' });
// TODO(cjihrig): Implement a TAP parser to read the child's stdout
// instead of just displaying it all if the child fails.
let err;

child.on('error', (error) => {
err = error;
});

const { 0: { code, signal }, 1: stdout, 2: stderr } = await SafePromiseAll([
once(child, 'exit', { signal: t.signal }),
child.stdout.toArray({ signal: t.signal }),
child.stderr.toArray({ signal: t.signal }),
]);

if (code !== 0 || signal !== null) {
if (!err) {
err = new ERR_TEST_FAILURE('test failed', kSubtestsFailed);
err.exitCode = code;
err.signal = signal;
err.stdout = ArrayPrototypeJoin(stdout, '');
err.stderr = ArrayPrototypeJoin(stderr, '');
// The stack will not be useful since the failures came from tests
// in a child process.
err.stack = undefined;
Comment on lines +130 to +132
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct, we should keep that property as a string – also, you may think that the stack is not useful, but why is that a good reason to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although I agree - this was here before this PR so I think it is out of scope

}

throw err;
}
});
}

113 changes: 87 additions & 26 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
@@ -5,17 +5,23 @@ const {
ArrayPrototypeUnshift,
FunctionPrototype,
Number,
PromisePrototypeThen,
PromiseResolve,
ReflectApply,
SafeMap,
PromiseRace,
SafePromiseAll,
SafePromiseRace,
Symbol,
} = primordials;
const { AsyncResource } = require('async_hooks');
const { once } = require('events');
const { AbortController } = require('internal/abort_controller');
const {
codes: {
ERR_TEST_FAILURE,
},
kIsNodeError,
AbortError,
} = require('internal/errors');
const { getOptionValue } = require('internal/options');
const { TapStream } = require('internal/test_runner/tap_stream');
@@ -25,7 +31,7 @@ const {
kEmptyObject,
} = require('internal/util');
const { isPromise } = require('internal/util/types');
const { isUint32 } = require('internal/validators');
const { isUint32, validateAbortSignal } = require('internal/validators');
const { setTimeout } = require('timers/promises');
const { cpus } = require('os');
const { bigint: hrtime } = process.hrtime;
@@ -43,20 +49,19 @@ const testOnlyFlag = !isTestRunner && getOptionValue('--test-only');
// TODO(cjihrig): Use uv_available_parallelism() once it lands.
const rootConcurrency = isTestRunner ? cpus().length : 1;

const kShouldAbort = Symbol('kShouldAbort');

function testTimeout(promise, timeout) {

function stopTest(timeout, signal) {
if (timeout === kDefaultTimeout) {
return promise;
}
return PromiseRace([
promise,
setTimeout(timeout, null, { ref: false }).then(() => {
throw new ERR_TEST_FAILURE(
`test timed out after ${timeout}ms`,
kTestTimeoutFailure
);
}),
]);
return once(signal, 'abort');
}
return PromisePrototypeThen(setTimeout(timeout, null, { ref: false, signal }), () => {
throw new ERR_TEST_FAILURE(
`test timed out after ${timeout}ms`,
kTestTimeoutFailure
);
});
}

class TestContext {
@@ -66,6 +71,10 @@ class TestContext {
this.#test = test;
}

get signal() {
return this.#test.signal;
}

diagnostic(message) {
this.#test.diagnostic(message);
}
@@ -91,11 +100,14 @@ class TestContext {
}

class Test extends AsyncResource {
#abortController;
#outerSignal;

constructor(options) {
super('Test');

let { fn, name, parent, skip } = options;
const { concurrency, only, timeout, todo } = options;
const { concurrency, only, timeout, todo, signal } = options;

if (typeof fn !== 'function') {
fn = noop;
@@ -148,6 +160,14 @@ class Test extends AsyncResource {
fn = noop;
}

this.#abortController = new AbortController();
this.#outerSignal = signal;
this.signal = this.#abortController.signal;

validateAbortSignal(signal, 'options.signal');
this.#outerSignal?.addEventListener('abort', this.#abortHandler);


this.fn = fn;
this.name = name;
this.parent = parent;
@@ -241,7 +261,8 @@ class Test extends AsyncResource {

// If this test has already ended, attach this test to the root test so
// that the error can be properly reported.
if (this.finished) {
const preventAddingSubtests = this.finished || this.buildPhaseFinished;
if (preventAddingSubtests) {
while (parent.parent !== null) {
parent = parent.parent;
}
@@ -253,7 +274,7 @@ class Test extends AsyncResource {
parent.waitingOn = test.testNumber;
}

if (this.finished) {
if (preventAddingSubtests) {
test.startTime = test.startTime || hrtime();
test.fail(
new ERR_TEST_FAILURE(
@@ -267,18 +288,23 @@ class Test extends AsyncResource {
return test;
}

cancel() {
#abortHandler = () => {
this.cancel(this.#outerSignal?.reason || new AbortError('The test was aborted'));
};

cancel(error) {
if (this.endTime !== null) {
return;
}

this.fail(
this.fail(error ||
new ERR_TEST_FAILURE(
'test did not finish before its parent and was cancelled',
kCancelledByParent
)
);
this.cancelled = true;
this.#abortController.abort();
}

fail(err) {
@@ -329,6 +355,16 @@ class Test extends AsyncResource {
return this.run();
}

[kShouldAbort]() {
if (this.signal.aborted) {
return true;
}
if (this.#outerSignal?.aborted) {
this.cancel(this.#outerSignal.reason || new AbortError('The test was aborted'));
return true;
}
}

getRunArgs() {
const ctx = new TestContext(this);
return { ctx, args: [ctx] };
@@ -338,7 +374,13 @@ class Test extends AsyncResource {
this.parent.activeSubtests++;
this.startTime = hrtime();

if (this[kShouldAbort]()) {
this.postRun();
return;
}

try {
const stopPromise = stopTest(this.timeout, this.signal);
const { args, ctx } = this.getRunArgs();
ArrayPrototypeUnshift(args, this.fn, ctx); // Note that if it's not OK to mutate args, we need to first clone it.

@@ -354,13 +396,19 @@ class Test extends AsyncResource {
'passed a callback but also returned a Promise',
kCallbackAndPromisePresent
));
await testTimeout(ret, this.timeout);
await SafePromiseRace([ret, stopPromise]);
} else {
await testTimeout(promise, this.timeout);
await SafePromiseRace([PromiseResolve(promise), stopPromise]);
}
} else {
// This test is synchronous or using Promises.
await testTimeout(ReflectApply(this.runInAsyncScope, this, args), this.timeout);
const promise = ReflectApply(this.runInAsyncScope, this, args);
await SafePromiseRace([PromiseResolve(promise), stopPromise]);
}

if (this[kShouldAbort]()) {
this.postRun();
return;
}

this.pass();
@@ -409,6 +457,8 @@ class Test extends AsyncResource {
this.fail(new ERR_TEST_FAILURE(msg, kSubtestsFailed));
}

this.#outerSignal?.removeEventListener('abort', this.#abortHandler);

if (this.parent !== null) {
this.parent.activeSubtests--;
this.parent.addReadySubtest(this);
@@ -476,20 +526,21 @@ class Test extends AsyncResource {
class ItTest extends Test {
constructor(opt) { super(opt); } // eslint-disable-line no-useless-constructor
getRunArgs() {
return { ctx: {}, args: [] };
return { ctx: { signal: this.signal }, args: [] };
}
}
class Suite extends Test {
constructor(options) {
super(options);

try {
this.buildSuite = this.runInAsyncScope(this.fn);
const context = { signal: this.signal };
this.buildSuite = this.runInAsyncScope(this.fn, context, [context]);
} catch (err) {
this.fail(new ERR_TEST_FAILURE(err, kTestCodeFailure));
}
this.fn = () => {};
this.finished = true; // Forbid adding subtests to this suite
this.buildPhaseFinished = true;
}

start() {
@@ -504,8 +555,18 @@ class Suite extends Test {
}
this.parent.activeSubtests++;
this.startTime = hrtime();

if (this[kShouldAbort]()) {
this.subtests = [];
this.postRun();
return;
}

const stopPromise = stopTest(this.timeout, this.signal);
const subtests = this.skipped || this.error ? [] : this.subtests;
await SafePromiseAll(subtests, (subtests) => subtests.start());
const promise = SafePromiseAll(subtests, (subtests) => subtests.start());

await SafePromiseRace([promise, stopPromise]);
this.pass();
this.postRun();
}
47 changes: 47 additions & 0 deletions test/message/test_runner_abort.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Flags: --no-warnings
'use strict';
require('../common');
const test = require('node:test');

test('promise timeout signal', { signal: AbortSignal.timeout(1) }, async (t) => {
await Promise.all([
t.test('ok 1', async () => {}),
t.test('ok 2', () => {}),
t.test('ok 3', { signal: t.signal }, async () => {}),
t.test('ok 4', { signal: t.signal }, () => {}),
t.test('not ok 1', () => new Promise(() => {})),
t.test('not ok 2', (t, done) => {}),
t.test('not ok 3', { signal: t.signal }, () => new Promise(() => {})),
t.test('not ok 4', { signal: t.signal }, (t, done) => {}),
t.test('not ok 5', { signal: t.signal }, (t, done) => {
t.signal.addEventListener('abort', done);
}),
]);
});

test('promise abort signal', { signal: AbortSignal.abort() }, async (t) => {
await t.test('should not appear', () => {});
});

test('callback timeout signal', { signal: AbortSignal.timeout(1) }, (t, done) => {
t.test('ok 1', async () => {});
t.test('ok 2', () => {});
t.test('ok 3', { signal: t.signal }, async () => {});
t.test('ok 4', { signal: t.signal }, () => {});
t.test('not ok 1', () => new Promise(() => {}));
t.test('not ok 2', (t, done) => {});
t.test('not ok 3', { signal: t.signal }, () => new Promise(() => {}));
t.test('not ok 4', { signal: t.signal }, (t, done) => {});
t.test('not ok 5', { signal: t.signal }, (t, done) => {
t.signal.addEventListener('abort', done);
});
});

test('callback abort signal', { signal: AbortSignal.abort() }, (t, done) => {
t.test('should not appear', done);
});

// AbortSignal.timeout(1) doesn't prevent process from closing
// thus we have to keep the process open to prevent cancelation
// of the entire test tree
setTimeout(() => {}, 1000);
249 changes: 249 additions & 0 deletions test/message/test_runner_abort.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
TAP version 13
# Subtest: promise timeout signal
# Subtest: ok 1
ok 1 - ok 1
---
duration_ms: *
...
# Subtest: ok 2
ok 2 - ok 2
---
duration_ms: *
...
# Subtest: ok 3
ok 3 - ok 3
---
duration_ms: *
...
# Subtest: ok 4
ok 4 - ok 4
---
duration_ms: *
...
# Subtest: not ok 1
not ok 5 - not ok 1
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 2
not ok 6 - not ok 2
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 3
not ok 7 - not ok 3
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
*
...
# Subtest: not ok 4
not ok 8 - not ok 4
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
*
...
# Subtest: not ok 5
not ok 9 - not ok 5
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
*
...
1..9
not ok 1 - promise timeout signal
---
duration_ms: *
error: 'The operation was aborted due to timeout'
code: 23
stack: |-
*
*
*
*
...
# Subtest: promise abort signal
not ok 2 - promise abort signal
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
...
# Subtest: callback timeout signal
# Subtest: ok 1
ok 1 - ok 1
---
duration_ms: *
...
# Subtest: ok 2
ok 2 - ok 2
---
duration_ms: *
...
# Subtest: ok 3
ok 3 - ok 3
---
duration_ms: *
...
# Subtest: ok 4
ok 4 - ok 4
---
duration_ms: *
...
# Subtest: not ok 1
not ok 5 - not ok 1
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 2
not ok 6 - not ok 2
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 3
not ok 7 - not ok 3
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
*
...
# Subtest: not ok 4
not ok 8 - not ok 4
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
*
...
# Subtest: not ok 5
not ok 9 - not ok 5
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
*
...
1..9
not ok 3 - callback timeout signal
---
duration_ms: *
error: 'The operation was aborted due to timeout'
code: 23
stack: |-
*
*
*
*
...
# Subtest: callback abort signal
not ok 4 - callback abort signal
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
...
1..4
# tests 4
# pass 0
# fail 0
# cancelled 4
# skipped 0
# todo 0
# duration_ms *
27 changes: 27 additions & 0 deletions test/message/test_runner_abort_suite.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Flags: --no-warnings
'use strict';
require('../common');
const { describe, it } = require('node:test');

describe('describe timeout signal', { signal: AbortSignal.timeout(1) }, (t) => {
it('ok 1', async () => {});
it('ok 2', () => {});
it('ok 3', { signal: t.signal }, async () => {});
it('ok 4', { signal: t.signal }, () => {});
it('not ok 1', () => new Promise(() => {}));
it('not ok 2', (done) => {});
it('not ok 3', { signal: t.signal }, () => new Promise(() => {}));
it('not ok 4', { signal: t.signal }, (done) => {});
it('not ok 5', { signal: t.signal }, function(done) {
this.signal.addEventListener('abort', done);
});
});

describe('describe abort signal', { signal: AbortSignal.abort() }, () => {
it('should not appear', () => {});
});

// AbortSignal.timeout(1) doesn't prevent process from closing
// thus we have to keep the process open to prevent cancelation
// of the entire test tree
setTimeout(() => {}, 1000);
99 changes: 99 additions & 0 deletions test/message/test_runner_abort_suite.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
TAP version 13
# Subtest: describe timeout signal
# Subtest: ok 1
ok 1 - ok 1
---
duration_ms: *
...
# Subtest: ok 2
ok 2 - ok 2
---
duration_ms: *
...
# Subtest: ok 3
ok 3 - ok 3
---
duration_ms: *
...
# Subtest: ok 4
ok 4 - ok 4
---
duration_ms: *
...
# Subtest: not ok 1
not ok 5 - not ok 1
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 2
not ok 6 - not ok 2
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 3
not ok 7 - not ok 3
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 4
not ok 8 - not ok 4
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
# Subtest: not ok 5
not ok 9 - not ok 5
---
duration_ms: *
failureType: 'cancelledByParent'
error: 'test did not finish before its parent and was cancelled'
code: 'ERR_TEST_FAILURE'
...
1..9
not ok 1 - describe timeout signal
---
duration_ms: *
error: 'The operation was aborted due to timeout'
code: 23
stack: |-
*
*
*
*
...
# Subtest: describe abort signal
not ok 2 - describe abort signal
---
duration_ms: *
error: 'This operation was aborted'
code: 20
stack: |-
*
*
*
*
*
*
*
*
*
...
1..2
# tests 2
# pass 0
# fail 0
# cancelled 2
# skipped 0
# todo 0
# duration_ms *
6 changes: 3 additions & 3 deletions test/message/test_runner_describe_it.js
Original file line number Diff line number Diff line change
@@ -225,15 +225,15 @@ it('callback fail', (done) => {
});

it('sync t is this in test', function() {
assert.deepStrictEqual(this, {});
assert.deepStrictEqual(this, { signal: this.signal });
});

it('async t is this in test', async function() {
assert.deepStrictEqual(this, {});
assert.deepStrictEqual(this, { signal: this.signal });
});

it('callback t is this in test', function(done) {
assert.deepStrictEqual(this, {});
assert.deepStrictEqual(this, { signal: this.signal });
done();
});

2 changes: 2 additions & 0 deletions test/message/test_runner_describe_it.out
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@ not ok 3 - sync fail todo # TODO
*
*
*
*
...
# Subtest: sync fail todo with message
not ok 4 - sync fail todo with message # TODO this is a failing todo
@@ -41,6 +42,7 @@ not ok 4 - sync fail todo with message # TODO this is a failing todo
*
*
*
*
...
# Subtest: sync skip pass
ok 5 - sync skip pass # SKIP