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
Show file tree
Hide file tree
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
Expand Up @@ -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`.
Expand Down Expand Up @@ -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`.

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
85 changes: 35 additions & 50 deletions lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
});
}

Expand Down
Loading