From f77d7b76fc2257272077691bce67cbad1d70305e Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Mon, 3 Oct 2022 13:11:58 +0300 Subject: [PATCH] test_runner: support using `--inspect` with `--test` PR-URL: https://github.com/nodejs/node/pull/44520 Backport-PR-URL: https://github.com/nodejs/node/pull/44873 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Antoine du Hamel --- doc/api/test.md | 5 + lib/internal/cluster/primary.js | 1 + lib/internal/main/test_runner.js | 13 +- lib/internal/test_runner/runner.js | 59 +++++-- lib/internal/test_runner/test.js | 5 +- lib/internal/util/inspector.js | 42 +++++ src/node_options.cc | 3 - test/common/index.mjs | 3 + test/fixtures/test-runner/run_inspect.js | 40 +++++ .../test-runner/run_inspect_assert.js | 19 ++ test/parallel/test-runner-cli.js | 6 - test/parallel/test-runner-inspect.mjs | 54 ++++++ test/sequential/test-runner-run-inspect.mjs | 164 ++++++++++++++++++ 13 files changed, 391 insertions(+), 23 deletions(-) create mode 100644 test/fixtures/test-runner/run_inspect.js create mode 100644 test/fixtures/test-runner/run_inspect_assert.js create mode 100644 test/parallel/test-runner-inspect.mjs create mode 100644 test/sequential/test-runner-run-inspect.mjs diff --git a/doc/api/test.md b/doc/api/test.md index fcffd932df5..7da51afff4b 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -338,6 +338,11 @@ 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. + This can be a number, or a function that takes no arguments and returns a + number. If a nullish value is provided, each process gets its own port, + incremented from the primary's `process.debugPort`. + **Default:** `undefined`. * Returns: {TapStream} ```js diff --git a/lib/internal/cluster/primary.js b/lib/internal/cluster/primary.js index 3940d094e7b..749f5377349 100644 --- a/lib/internal/cluster/primary.js +++ b/lib/internal/cluster/primary.js @@ -120,6 +120,7 @@ function createWorkerProcess(id, env) { const debugArgRegex = /--inspect(?:-brk|-port)?|--debug-port/; const nodeOptions = process.env.NODE_OPTIONS || ''; + // TODO(MoLow): Use getInspectPort from internal/util/inspector if (ArrayPrototypeSome(execArgv, (arg) => RegExpPrototypeExec(debugArgRegex, arg) !== null) || RegExpPrototypeExec(debugArgRegex, nodeOptions) !== null) { diff --git a/lib/internal/main/test_runner.js b/lib/internal/main/test_runner.js index c965a6a95fb..bf68b6933be 100644 --- a/lib/internal/main/test_runner.js +++ b/lib/internal/main/test_runner.js @@ -2,12 +2,23 @@ const { prepareMainThreadExecution, } = require('internal/bootstrap/pre_execution'); +const { isUsingInspector } = require('internal/util/inspector'); const { run } = 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; diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index a4afbcb0a77..911a700d68d 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -1,18 +1,22 @@ 'use strict'; const { ArrayFrom, - ArrayPrototypeConcat, ArrayPrototypeFilter, ArrayPrototypeIncludes, ArrayPrototypeJoin, + ArrayPrototypePop, + ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSort, ObjectAssign, PromisePrototypeThen, + RegExpPrototypeSymbolSplit, SafePromiseAll, SafeSet, + StringPrototypeEndsWith, } = primordials; +const { Buffer } = require('buffer'); const { spawn } = require('child_process'); const { readdirSync, statSync } = require('fs'); const console = require('internal/console/global'); @@ -22,6 +26,7 @@ const { }, } = require('internal/errors'); const { validateArray } = require('internal/validators'); +const { getInspectPort, isUsingInspector, isInspectorMessage } = require('internal/util/inspector'); const { kEmptyObject } = require('internal/util'); const { createTestTree } = require('internal/test_runner/harness'); const { kSubtestsFailed, Test } = require('internal/test_runner/test'); @@ -100,25 +105,59 @@ function filterExecArgv(arg) { return !ArrayPrototypeIncludes(kFilterArgs, arg); } -function runTestFile(path, root) { +function getRunArgs({ path, inspectPort }) { + const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv); + if (isUsingInspector()) { + ArrayPrototypePush(argv, `--inspect-port=${getInspectPort(inspectPort)}`); + } + ArrayPrototypePush(argv, path); + return argv; +} + +function makeStderrCallback(callback) { + if (!isUsingInspector()) { + return callback; + } + let buffer = Buffer.alloc(0); + return (data) => { + callback(data); + const newData = Buffer.concat([buffer, data]); + const str = newData.toString('utf8'); + let lines = str; + if (StringPrototypeEndsWith(lines, '\n')) { + buffer = Buffer.alloc(0); + } else { + lines = RegExpPrototypeSymbolSplit(/\r?\n/, str); + buffer = Buffer.from(ArrayPrototypePop(lines), 'utf8'); + lines = ArrayPrototypeJoin(lines, '\n'); + } + if (isInspectorMessage(lines)) { + process.stderr.write(lines); + } + }; +} + +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 // instead of just displaying it all if the child fails. let err; + let stderr = ''; child.on('error', (error) => { err = error; }); - const { 0: { 0: code, 1: signal }, 1: stdout, 2: stderr } = await SafePromiseAll([ + child.stderr.on('data', makeStderrCallback((data) => { + stderr += data; + })); + + const { 0: { 0: code, 1: signal }, 1: stdout } = 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) { @@ -128,7 +167,7 @@ function runTestFile(path, root) { exitCode: code, signal: signal, stdout: ArrayPrototypeJoin(stdout, ''), - stderr: ArrayPrototypeJoin(stderr, ''), + stderr, // The stack will not be useful since the failures came from tests // in a child process. stack: undefined, @@ -145,7 +184,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'); @@ -154,7 +193,7 @@ 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; diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 5c33d760dd0..f02c173bff3 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -58,8 +58,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']); @@ -150,7 +148,7 @@ class Test extends AsyncResource { } if (parent === null) { - this.concurrency = rootConcurrency; + this.concurrency = 1; this.indent = ''; this.indentString = kDefaultIndent; this.only = testOnlyFlag; @@ -180,6 +178,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; diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js index 95ef370c3c4..f1bb21d859d 100644 --- a/lib/internal/util/inspector.js +++ b/lib/internal/util/inspector.js @@ -2,12 +2,47 @@ const { ArrayPrototypeConcat, + ArrayPrototypeSome, FunctionPrototypeBind, ObjectDefineProperty, ObjectKeys, ObjectPrototypeHasOwnProperty, + RegExpPrototypeExec, } = primordials; +const { validatePort } = require('internal/validators'); + +const kMinPort = 1024; +const kMaxPort = 65535; +const kInspectArgRegex = /--inspect(?:-brk|-port)?|--debug-port/; +const kInspectMsgRegex = /Debugger listening on ws:\/\/\[?(.+?)\]?:(\d+)\/|Debugger attached|Waiting for the debugger to disconnect\.\.\./; + +let _isUsingInspector; +function isUsingInspector() { + _isUsingInspector ??= + ArrayPrototypeSome(process.execArgv, (arg) => RegExpPrototypeExec(kInspectArgRegex, arg) !== null) || + RegExpPrototypeExec(kInspectArgRegex, process.env.NODE_OPTIONS) !== null; + return _isUsingInspector; +} + +let debugPortOffset = 1; +function getInspectPort(inspectPort) { + if (!isUsingInspector()) { + return null; + } + if (typeof inspectPort === 'function') { + inspectPort = inspectPort(); + } else if (inspectPort == null) { + inspectPort = process.debugPort + debugPortOffset; + if (inspectPort > kMaxPort) + inspectPort = inspectPort - kMaxPort + kMinPort - 1; + debugPortOffset++; + } + validatePort(inspectPort); + + return inspectPort; +} + let session; function sendInspectorCommand(cb, onError) { const { hasInspector } = internalBinding('config'); @@ -22,6 +57,10 @@ function sendInspectorCommand(cb, onError) { } } +function isInspectorMessage(string) { + return isUsingInspector() && RegExpPrototypeExec(kInspectMsgRegex, string) !== null; +} + // Create a special require function for the inspector command line API function installConsoleExtensions(commandLineApi) { if (commandLineApi.require) { return; } @@ -65,7 +104,10 @@ function wrapConsole(consoleFromNode, consoleFromVM) { // Stores the console from VM, should be set during bootstrap. let consoleFromVM; module.exports = { + getInspectPort, installConsoleExtensions, + isInspectorMessage, + isUsingInspector, sendInspectorCommand, wrapConsole, get consoleFromVM() { diff --git a/src/node_options.cc b/src/node_options.cc index 31baaa9e102..3cb77c537bc 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -161,9 +161,6 @@ void EnvironmentOptions::CheckOptions(std::vector* 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 diff --git a/test/common/index.mjs b/test/common/index.mjs index 2b30f499343..e77b1b298cb 100644 --- a/test/common/index.mjs +++ b/test/common/index.mjs @@ -52,6 +52,8 @@ const { spawnPromisified, } = common; +const getPort = () => common.PORT; + export { isMainThread, isWindows, @@ -100,4 +102,5 @@ export { runWithInvalidFD, createRequire, spawnPromisified, + getPort, }; diff --git a/test/fixtures/test-runner/run_inspect.js b/test/fixtures/test-runner/run_inspect.js new file mode 100644 index 00000000000..1586b6aaccf --- /dev/null +++ b/test/fixtures/test-runner/run_inspect.js @@ -0,0 +1,40 @@ +'use strict'; + +const common = require('../../common'); +const fixtures = require('../../common/fixtures'); +const { run } = require('node:test'); +const assert = require('node:assert'); + +const badPortError = { name: 'RangeError', code: 'ERR_SOCKET_BAD_PORT' }; +let inspectPort = 'inspectPort' in process.env ? Number(process.env.inspectPort) : undefined; +let expectedError; + +if (process.env.inspectPort === 'addTwo') { + inspectPort = common.mustCall(() => { return process.debugPort += 2; }); +} else if (process.env.inspectPort === 'string') { + inspectPort = 'string'; + expectedError = badPortError; +} else if (process.env.inspectPort === 'null') { + inspectPort = null; +} else if (process.env.inspectPort === 'bignumber') { + inspectPort = 1293812; + expectedError = badPortError; +} else if (process.env.inspectPort === 'negativenumber') { + inspectPort = -9776; + expectedError = badPortError; +} else if (process.env.inspectPort === 'bignumberfunc') { + inspectPort = common.mustCall(() => 123121); + expectedError = badPortError; +} else if (process.env.inspectPort === 'strfunc') { + inspectPort = common.mustCall(() => 'invalidPort'); + expectedError = badPortError; +} + +const stream = run({ files: [fixtures.path('test-runner/run_inspect_assert.js')], inspectPort }); +if (expectedError) { + stream.on('test:fail', common.mustCall(({ error }) => { + assert.deepStrictEqual({ name: error.cause.name, code: error.cause.code }, expectedError); + })); +} else { + stream.on('test:fail', common.mustNotCall()); +} diff --git a/test/fixtures/test-runner/run_inspect_assert.js b/test/fixtures/test-runner/run_inspect_assert.js new file mode 100644 index 00000000000..daea9b3b665 --- /dev/null +++ b/test/fixtures/test-runner/run_inspect_assert.js @@ -0,0 +1,19 @@ +'use strict'; + +const assert = require('node:assert'); + +const { expectedPort, expectedInitialPort, expectedHost } = process.env; +const debugOptions = + require('internal/options').getOptionValue('--inspect-port'); + +if ('expectedPort' in process.env) { + assert.strictEqual(process.debugPort, +expectedPort); +} + +if ('expectedInitialPort' in process.env) { + assert.strictEqual(debugOptions.port, +expectedInitialPort); +} + +if ('expectedHost' in process.env) { + assert.strictEqual(debugOptions.host, expectedHost); +} diff --git a/test/parallel/test-runner-cli.js b/test/parallel/test-runner-cli.js index 552d64d7c40..8c1f6b3b0be 100644 --- a/test/parallel/test-runner-cli.js +++ b/test/parallel/test-runner-cli.js @@ -104,12 +104,6 @@ const testFixtures = fixtures.path('test-runner'); ['--print', 'console.log("should not print")', '--test'], ]; - if (process.features.inspector) { - flags.push( - ['--inspect', '--test'], - ['--inspect-brk', '--test'], - ); - } flags.forEach((args) => { const child = spawnSync(process.execPath, args); diff --git a/test/parallel/test-runner-inspect.mjs b/test/parallel/test-runner-inspect.mjs new file mode 100644 index 00000000000..67095291e2a --- /dev/null +++ b/test/parallel/test-runner-inspect.mjs @@ -0,0 +1,54 @@ +import * as common from '../common/index.mjs'; +import * as tmpdir from '../common/tmpdir.js'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; +import { NodeInstance } from '../common/inspector-helper.js'; + + +common.skipIfInspectorDisabled(); +tmpdir.refresh(); + +{ + const child = new NodeInstance(['--test', '--inspect-brk=0'], undefined, fixtures.path('test-runner/index.test.js')); + + let stdout = ''; + let stderr = ''; + child.on('stdout', (line) => stdout += line); + child.on('stderr', (line) => stderr += line); + + const session = await child.connectInspectorSession(); + + await session.send([ + { method: 'Runtime.enable' }, + { method: 'Runtime.runIfWaitingForDebugger' }]); + + session.disconnect(); + assert.match(stderr, + /Warning: Using the inspector with --test forces running at a concurrency of 1\. Use the inspectPort option to run with concurrency/); +} + + +{ + const args = ['--test', '--inspect=0', fixtures.path('test-runner/index.js')]; + const { stderr, stdout, code, signal } = await common.spawnPromisified(process.execPath, args); + + assert.match(stderr, + /Warning: Using the inspector with --test forces running at a concurrency of 1\. Use the inspectPort option to run with concurrency/); + assert.match(stdout, /not ok 1 - .+index\.js/); + assert.match(stdout, /stderr: \|-\r?\n\s+Debugger listening on/); + assert.strictEqual(code, 1); + assert.strictEqual(signal, null); +} + + +{ + // File not found. + const args = ['--test', '--inspect=0', 'a-random-file-that-does-not-exist.js']; + const { stderr, stdout, code, signal } = await common.spawnPromisified(process.execPath, args); + + assert.strictEqual(stdout, ''); + assert.match(stderr, /^Could not find/); + assert.doesNotMatch(stderr, /Warning: Using the inspector with --test forces running at a concurrency of 1\. Use the inspectPort option to run with concurrency/); + assert.strictEqual(code, 1); + assert.strictEqual(signal, null); +} diff --git a/test/sequential/test-runner-run-inspect.mjs b/test/sequential/test-runner-run-inspect.mjs new file mode 100644 index 00000000000..35c380c8d4e --- /dev/null +++ b/test/sequential/test-runner-run-inspect.mjs @@ -0,0 +1,164 @@ +import * as common from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'node:assert'; + +common.skipIfInspectorDisabled(); + + +const debuggerPort = common.getPort(); + +async function spawnRunner({ execArgv, expectedPort, expectedHost, expectedInitialPort, inspectPort }) { + const { code, signal } = await common.spawnPromisified( + process.execPath, + ['--expose-internals', '--no-warnings', ...execArgv, fixtures.path('test-runner/run_inspect.js')], { + env: { ...process.env, + expectedPort, + inspectPort, + expectedHost, + expectedInitialPort } + }); + assert.strictEqual(code, 0); + assert.strictEqual(signal, null); +} + +let offset = 0; + +const defaultPortCase = spawnRunner({ + execArgv: ['--inspect'], + expectedPort: 9230, +}); + +await spawnRunner({ + execArgv: ['--inspect=65535'], + expectedPort: 1024, +}); + +let port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + expectedPort: port + 1, +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: ['--inspect', `--inspect-port=${port}`], + expectedPort: port + 1, +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: ['--inspect', `--debug-port=${port}`], + expectedPort: port + 1, +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=0.0.0.0:${port}`], + expectedPort: port + 1, expectedHost: '0.0.0.0', +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=127.0.0.1:${port}`], + expectedPort: port + 1, expectedHost: '127.0.0.1' +}); + +if (common.hasIPv6) { + port = debuggerPort + offset++ * 5; + + await spawnRunner({ + execArgv: [`--inspect=[::]:${port}`], + expectedPort: port + 1, expectedHost: '::' + }); + + port = debuggerPort + offset++ * 5; + + await spawnRunner({ + execArgv: [`--inspect=[::1]:${port}`], + expectedPort: port + 1, expectedHost: '::1' + }); +} + +// These tests check that setting inspectPort in run +// would take effect and override port incrementing behavior + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: port + 2, + expectedPort: port + 2, +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'addTwo', + expectedPort: port + 2, +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'string', +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'null', + expectedPort: port + 1, +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'bignumber', +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'negativenumber', +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'bignumberfunc' +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 'strfunc', +}); + +port = debuggerPort + offset++ * 5; + +await spawnRunner({ + execArgv: [`--inspect=${port}`], + inspectPort: 0, + expectedInitialPort: 0, +}); + +await defaultPortCase; + +port = debuggerPort + offset++ * 5; +await spawnRunner({ + execArgv: ['--inspect'], + inspectPort: port + 2, + expectedInitialPort: port + 2, +});