From e054dc8a2540cd5575467f1c343bc71f5fb03ad9 Mon Sep 17 00:00:00 2001 From: Colin Ihrig Date: Wed, 13 Mar 2024 06:49:15 -0400 Subject: [PATCH] test_runner: support forced exit This commit updates the test runner to allow a forced exit once all known tests have finished running. Fixes: https://github.com/nodejs/node/issues/49925 PR-URL: https://github.com/nodejs/node/pull/52038 Reviewed-By: Moshe Atlow Reviewed-By: Chemi Atlow Reviewed-By: Raz Luvaton Reviewed-By: Benjamin Gruenbaum --- doc/api/cli.md | 9 +++++ doc/api/test.md | 10 +++++- doc/node.1 | 4 +++ lib/internal/test_runner/harness.js | 1 + lib/internal/test_runner/runner.js | 36 +++++++++++++++++-- lib/internal/test_runner/test.js | 28 +++++++++++---- lib/internal/test_runner/utils.js | 2 ++ src/node_options.cc | 6 ++++ src/node_options.h | 1 + .../fixtures/test-runner/output/force_exit.js | 27 ++++++++++++++ .../test-runner/output/force_exit.snapshot | 16 +++++++++ .../test-runner/throws_sync_and_async.js | 10 ++++++ .../test-runner-force-exit-failure.js | 15 ++++++++ test/parallel/test-runner-output.mjs | 1 + test/parallel/test-runner-run.mjs | 18 ++++++++++ 15 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 test/fixtures/test-runner/output/force_exit.js create mode 100644 test/fixtures/test-runner/output/force_exit.snapshot create mode 100644 test/fixtures/test-runner/throws_sync_and_async.js create mode 100644 test/parallel/test-runner-force-exit-failure.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 25d64f2c8c1073..859b68234ed2e1 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1824,6 +1824,15 @@ added: v20.10.0 The maximum number of test files that the test runner CLI will execute concurrently. The default value is `os.availableParallelism() - 1`. +### `--test-force-exit` + + + +Configures the test runner to exit the process once all known tests have +finished executing even if the event loop would otherwise remain active. + ### `--test-name-pattern` @@ -1139,6 +1144,9 @@ changes: **Default:** `false`. * `files`: {Array} An array containing the list of files to run. **Default** matching files from [test runner execution model][]. + * `forceExit`: {boolean} Configures the test runner to exit the process once + all known tests have finished executing even if the event loop would + otherwise remain active. **Default:** `false`. * `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, diff --git a/doc/node.1 b/doc/node.1 index 2966c14e1c3f5e..8dda6c13622171 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -425,6 +425,10 @@ Starts the Node.js command line test runner. The maximum number of test files that the test runner CLI will execute concurrently. . +.It Fl -test-force-exit +Configures the test runner to exit the process once all known tests have +finished executing even if the event loop would otherwise remain active. +. .It Fl -test-name-pattern A regular expression that configures the test runner to only execute tests whose name matches the provided pattern. diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index f2b8bdde666d80..b13ca112e5cc27 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -195,6 +195,7 @@ function setup(root) { suites: 0, }, shouldColorizeTestFiles: false, + teardown: exitHandler, }; root.startTime = hrtime(); return root; diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index d07d49296c21ea..0a18d8abe3458a 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -155,8 +155,11 @@ function filterExecArgv(arg, i, arr) { !ArrayPrototypeSome(kFilterArgValues, (p) => arg === p || (i > 0 && arr[i - 1] === p) || StringPrototypeStartsWith(arg, `${p}=`)); } -function getRunArgs(path, { inspectPort, testNamePatterns, only }) { +function getRunArgs(path, { forceExit, inspectPort, testNamePatterns, only }) { const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv); + if (forceExit === true) { + ArrayPrototypePush(argv, '--test-force-exit'); + } if (isUsingInspector()) { ArrayPrototypePush(argv, `--inspect-port=${getInspectPort(inspectPort)}`); } @@ -484,7 +487,17 @@ function run(options) { options = kEmptyObject; } let { testNamePatterns, shard } = options; - const { concurrency, timeout, signal, files, inspectPort, watch, setup, only } = options; + const { + concurrency, + timeout, + signal, + files, + forceExit, + inspectPort, + watch, + setup, + only, + } = options; if (files != null) { validateArray(files, 'options.files'); @@ -492,6 +505,15 @@ function run(options) { if (watch != null) { validateBoolean(watch, 'options.watch'); } + if (forceExit != null) { + validateBoolean(forceExit, 'options.forceExit'); + + if (forceExit && watch) { + throw new ERR_INVALID_ARG_VALUE( + 'options.forceExit', watch, 'is not supported with watch mode', + ); + } + } if (only != null) { validateBoolean(only, 'options.only'); } @@ -545,7 +567,15 @@ function run(options) { let postRun = () => root.postRun(); let filesWatcher; - const opts = { __proto__: null, root, signal, inspectPort, testNamePatterns, only }; + const opts = { + __proto__: null, + root, + signal, + inspectPort, + testNamePatterns, + only, + forceExit, + }; if (watch) { filesWatcher = watchFiles(testFiles, opts); postRun = undefined; diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index f70fd77a13e0c2..2d389772a2ecf4 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -75,7 +75,12 @@ const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']); const kUnwrapErrors = new SafeSet() .add(kTestCodeFailure).add(kHookFailure) .add('uncaughtException').add('unhandledRejection'); -const { sourceMaps, testNamePatterns, testOnlyFlag } = parseCommandLine(); +const { + forceExit, + sourceMaps, + testNamePatterns, + testOnlyFlag, +} = parseCommandLine(); let kResistStopPropagation; let findSourceMap; @@ -720,6 +725,16 @@ class Test extends AsyncResource { // This helps catch any asynchronous activity that occurs after the tests // have finished executing. this.postRun(); + } else if (forceExit) { + // This is the root test, and all known tests and hooks have finished + // executing. If the user wants to force exit the process regardless of + // any remaining ref'ed handles, then do that now. It is theoretically + // possible that a ref'ed handle could asynchronously create more tests, + // but the user opted into this behavior. + this.reporter.once('close', () => { + process.exit(); + }); + this.harness.teardown(); } } @@ -770,12 +785,11 @@ class Test extends AsyncResource { if (this.parent === this.root && this.root.activeSubtests === 0 && this.root.pendingSubtests.length === 0 && - this.root.readySubtests.size === 0 && - this.root.hooks.after.length > 0) { - // This is done so that any global after() hooks are run. At this point - // all of the tests have finished running. However, there might be - // ref'ed handles keeping the event loop alive. This gives the global - // after() hook a chance to clean them up. + this.root.readySubtests.size === 0) { + // At this point all of the tests have finished running. However, there + // might be ref'ed handles keeping the event loop alive. This gives the + // global after() hook a chance to clean them up. The user may also + // want to force the test runner to exit despite ref'ed handles. this.root.run(); } } else if (!this.reported) { diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index 583757d1d4524e..6306ddfa0689d3 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -199,6 +199,7 @@ function parseCommandLine() { const isTestRunner = getOptionValue('--test'); const coverage = getOptionValue('--experimental-test-coverage'); + const forceExit = getOptionValue('--test-force-exit'); const sourceMaps = getOptionValue('--enable-source-maps'); const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child'; const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8'; @@ -251,6 +252,7 @@ function parseCommandLine() { __proto__: null, isTestRunner, coverage, + forceExit, sourceMaps, testOnlyFlag, testNamePatterns, diff --git a/src/node_options.cc b/src/node_options.cc index e5dc0c517e0907..4bc8961f0ea4f6 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -179,6 +179,9 @@ void EnvironmentOptions::CheckOptions(std::vector* errors, } else if (force_repl) { errors->push_back("either --watch or --interactive " "can be used, not both"); + } else if (test_runner_force_exit) { + errors->push_back("either --watch or --test-force-exit " + "can be used, not both"); } else if (!test_runner && (argv->size() < 1 || (*argv)[1].empty())) { errors->push_back("--watch requires specifying a file"); } @@ -611,6 +614,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { AddOption("--test-concurrency", "specify test runner concurrency", &EnvironmentOptions::test_runner_concurrency); + AddOption("--test-force-exit", + "force test runner to exit upon completion", + &EnvironmentOptions::test_runner_force_exit); AddOption("--test-timeout", "specify test runner timeout", &EnvironmentOptions::test_runner_timeout); diff --git a/src/node_options.h b/src/node_options.h index a62d0ae32c44d1..b56cf21fc4bf59 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -165,6 +165,7 @@ class EnvironmentOptions : public Options { uint64_t test_runner_concurrency = 0; uint64_t test_runner_timeout = 0; bool test_runner_coverage = false; + bool test_runner_force_exit = false; std::vector test_name_pattern; std::vector test_reporter; std::vector test_reporter_destination; diff --git a/test/fixtures/test-runner/output/force_exit.js b/test/fixtures/test-runner/output/force_exit.js new file mode 100644 index 00000000000000..a629f972a2f0ef --- /dev/null +++ b/test/fixtures/test-runner/output/force_exit.js @@ -0,0 +1,27 @@ +// Flags: --test-force-exit --test-reporter=spec +'use strict'; +const { after, afterEach, before, beforeEach, test } = require('node:test'); + +before(() => { + console.log('BEFORE'); +}); + +beforeEach(() => { + console.log('BEFORE EACH'); +}); + +after(() => { + console.log('AFTER'); +}); + +afterEach(() => { + console.log('AFTER EACH'); +}); + +test('passes but oops', () => { + setTimeout(() => { + throw new Error('this should not have a chance to be thrown'); + }, 1000); +}); + +test('also passes'); diff --git a/test/fixtures/test-runner/output/force_exit.snapshot b/test/fixtures/test-runner/output/force_exit.snapshot new file mode 100644 index 00000000000000..b45901f80c5be8 --- /dev/null +++ b/test/fixtures/test-runner/output/force_exit.snapshot @@ -0,0 +1,16 @@ +BEFORE +BEFORE EACH +AFTER EACH +BEFORE EACH +AFTER EACH +AFTER +✔ passes but oops (*ms) +✔ also passes (*ms) +ℹ tests 2 +ℹ suites 0 +ℹ pass 2 +ℹ fail 0 +ℹ cancelled 0 +ℹ skipped 0 +ℹ todo 0 +ℹ duration_ms * diff --git a/test/fixtures/test-runner/throws_sync_and_async.js b/test/fixtures/test-runner/throws_sync_and_async.js new file mode 100644 index 00000000000000..50ed81b4acf511 --- /dev/null +++ b/test/fixtures/test-runner/throws_sync_and_async.js @@ -0,0 +1,10 @@ +'use strict'; +const { test } = require('node:test'); + +test('fails and schedules more work', () => { + setTimeout(() => { + throw new Error('this should not have a chance to be thrown'); + }, 1000); + + throw new Error('fails'); +}); diff --git a/test/parallel/test-runner-force-exit-failure.js b/test/parallel/test-runner-force-exit-failure.js new file mode 100644 index 00000000000000..1fff8f30d7e038 --- /dev/null +++ b/test/parallel/test-runner-force-exit-failure.js @@ -0,0 +1,15 @@ +'use strict'; +require('../common'); +const { match, doesNotMatch, strictEqual } = require('node:assert'); +const { spawnSync } = require('node:child_process'); +const fixtures = require('../common/fixtures'); +const fixture = fixtures.path('test-runner/throws_sync_and_async.js'); +const r = spawnSync(process.execPath, ['--test', '--test-force-exit', fixture]); + +strictEqual(r.status, 1); +strictEqual(r.signal, null); +strictEqual(r.stderr.toString(), ''); + +const stdout = r.stdout.toString(); +match(stdout, /error: 'fails'/); +doesNotMatch(stdout, /this should not have a chance to be thrown/); diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index 870b79c6e591f8..77a3f4e2a0263b 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -101,6 +101,7 @@ const tests = [ { name: 'test-runner/output/global-hooks-with-no-tests.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/force_exit.js', transform: specTransform }, { name: 'test-runner/output/global_after_should_fail_the_test.js' }, { name: 'test-runner/output/no_refs.js' }, { name: 'test-runner/output/no_tests.js' }, diff --git a/test/parallel/test-runner-run.mjs b/test/parallel/test-runner-run.mjs index 77e0b21439327c..8ee89529c4a921 100644 --- a/test/parallel/test-runner-run.mjs +++ b/test/parallel/test-runner-run.mjs @@ -507,3 +507,21 @@ describe('require(\'node:test\').run', { concurrency: true }, () => { for await (const _ of stream); }); }); + +describe('forceExit', () => { + it('throws for non-boolean values', () => { + [Symbol(), {}, 0, 1, '1', Promise.resolve([])].forEach((forceExit) => { + assert.throws(() => run({ forceExit }), { + code: 'ERR_INVALID_ARG_TYPE', + message: /The "options\.forceExit" property must be of type boolean\./ + }); + }); + }); + + it('throws if enabled with watch mode', () => { + assert.throws(() => run({ forceExit: true, watch: true }), { + code: 'ERR_INVALID_ARG_VALUE', + message: /The property 'options\.forceExit' is not supported with watch mode\./ + }); + }); +});