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: support using --inspect with --test #44520

Merged
merged 15 commits into from
Sep 10, 2022
4 changes: 4 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,10 @@ added: REPLACEME
fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
* `inspectPort` {number|Function} Sets inspector port of test child process.
MoLow marked this conversation as resolved.
Show resolved Hide resolved
This can be a number, or a function that takes no arguments and returns a
number. By default each process gets its own port, incremented from the
primary's `process.debugPort`.
MoLow marked this conversation as resolved.
Show resolved Hide resolved
* Returns: {TapStream}

```js
Expand Down
14 changes: 12 additions & 2 deletions lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,22 @@ const {
prepareMainThreadExecution,
markBootstrapComplete
} = require('internal/process/pre_execution');
const { run } = require('internal/test_runner/runner');
const { run, isUsingInspector } = require('internal/test_runner/runner');

prepareMainThreadExecution(false);
markBootstrapComplete();

const tapStream = run();
let concurrency = true;
let inspectPort;

if (isUsingInspector()) {
process.emitWarning('Using the inspector with --test forces running at a concurrency of 1. ' +
'Use the inspectPort option to run with concurrency');
concurrency = 1;
inspectPort = process.debugPort;
}

const tapStream = run({ concurrency, inspectPort });
tapStream.pipe(process.stdout);
tapStream.once('test:fail', () => {
process.exitCode = 1;
Expand Down
46 changes: 37 additions & 9 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
'use strict';
const {
ArrayFrom,
ArrayPrototypeConcat,
ArrayPrototypeFilter,
ArrayPrototypeIncludes,
ArrayPrototypeJoin,
ArrayPrototypePush,
ArrayPrototypeSlice,
ArrayPrototypeSome,
ArrayPrototypeSort,
ObjectAssign,
PromisePrototypeThen,
RegExpPrototypeExec,
SafePromiseAll,
SafeSet,
} = primordials;
Expand All @@ -21,7 +23,7 @@ const {
ERR_TEST_FAILURE,
},
} = require('internal/errors');
const { validateArray } = require('internal/validators');
const { validateArray, validatePort } = require('internal/validators');
const { kEmptyObject } = require('internal/util');
const { createTestTree } = require('internal/test_runner/harness');
const { kSubtestsFailed, Test } = require('internal/test_runner/test');
Expand All @@ -32,6 +34,9 @@ const {
const { basename, join, resolve } = require('path');
const { once } = require('events');

const kMinPort = 1024;
ljharb marked this conversation as resolved.
Show resolved Hide resolved
const kMaxPort = 65535;
const kInspectArgRegex = /--inspect(?:-brk|-port)?/;
MoLow marked this conversation as resolved.
Show resolved Hide resolved
const kFilterArgs = ['--test'];

// TODO(cjihrig): Replace this with recursive readdir once it lands.
Expand Down Expand Up @@ -96,15 +101,38 @@ function createTestFileList() {
return ArrayPrototypeSort(ArrayFrom(testFiles));
}

function isUsingInspector() {
return ArrayPrototypeSome(process.execArgv, (arg) => RegExpPrototypeExec(kInspectArgRegex, arg) !== null) ||
RegExpPrototypeExec(kInspectArgRegex, process.env.NODE_OPTIONS) !== null;
}
Copy link
Member

Choose a reason for hiding this comment

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

this kind of seems like it should just be a boolean property available on process or something


function filterExecArgv(arg) {
return !ArrayPrototypeIncludes(kFilterArgs, arg);
}

function runTestFile(path, root) {
let debugPortOffset = 1;
function getRunArgs({ path, inspectPort }) {
const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
if (isUsingInspector()) {
if (typeof inspectPort === 'function') {
inspectPort = inspectPort();
} else if (typeof inspectPort !== 'number') {
MoLow marked this conversation as resolved.
Show resolved Hide resolved
inspectPort = process.debugPort + debugPortOffset;
if (inspectPort > kMaxPort)
inspectPort = inspectPort - kMaxPort + kMinPort - 1;
debugPortOffset++;
}
validatePort(inspectPort);

ArrayPrototypePush(argv, `--inspect-port=${inspectPort}`);
}
ArrayPrototypePush(argv, path);
return argv;
}

function runTestFile(path, root, inspectPort) {
const subtest = root.createSubtest(Test, path, async (t) => {
const args = ArrayPrototypeConcat(
ArrayPrototypeFilter(process.execArgv, filterExecArgv),
path);
const args = getRunArgs({ path, inspectPort });

const child = spawn(process.execPath, args, { signal: t.signal, encoding: 'utf8' });
// TODO(cjihrig): Implement a TAP parser to read the child's stdout
Expand Down Expand Up @@ -145,7 +173,7 @@ function run(options) {
if (options === null || typeof options !== 'object') {
options = kEmptyObject;
}
const { concurrency, timeout, signal, files } = options;
const { concurrency, timeout, signal, files, inspectPort } = options;

if (files != null) {
validateArray(files, 'options.files');
Expand All @@ -154,10 +182,10 @@ function run(options) {
const root = createTestTree({ concurrency, timeout, signal });
const testFiles = files ?? createTestFileList();

PromisePrototypeThen(SafePromiseAll(testFiles, (path) => runTestFile(path, root)),
PromisePrototypeThen(SafePromiseAll(testFiles, (path) => runTestFile(path, root, inspectPort)),
() => root.postRun());

return root.reporter;
}

module.exports = { run };
module.exports = { run, isUsingInspector };
5 changes: 2 additions & 3 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ const kDefaultTimeout = null;
const noop = FunctionPrototype;
const isTestRunner = getOptionValue('--test');
const testOnlyFlag = !isTestRunner && getOptionValue('--test-only');
// TODO(cjihrig): Use uv_available_parallelism() once it lands.
const rootConcurrency = isTestRunner ? MathMax(cpus().length - 1, 1) : 1;
const kShouldAbort = Symbol('kShouldAbort');
const kRunHook = Symbol('kRunHook');
const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']);
Expand Down Expand Up @@ -146,7 +144,7 @@ class Test extends AsyncResource {
}

if (parent === null) {
this.concurrency = rootConcurrency;
this.concurrency = 1;
MoLow marked this conversation as resolved.
Show resolved Hide resolved
this.indent = '';
this.indentString = kDefaultIndent;
this.only = testOnlyFlag;
Expand Down Expand Up @@ -176,6 +174,7 @@ class Test extends AsyncResource {

case 'boolean':
if (concurrency) {
// TODO(cjihrig): Use uv_available_parallelism() once it lands.
this.concurrency = parent === null ? MathMax(cpus().length - 1, 1) : Infinity;
} else {
this.concurrency = 1;
Expand Down
3 changes: 0 additions & 3 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,6 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
errors->push_back("either --test or --watch can be used, not both");
}

if (debug_options_.inspector_enabled) {
errors->push_back("the inspector cannot be used with --test");
}
#ifndef ALLOW_ATTACHING_DEBUGGER_IN_TEST_RUNNER
debug_options_.allow_attaching_debugger = false;
#endif
Expand Down