test_runner: support forced exit

This commit updates the test runner to allow a forced exit once
all known tests have finished running.

Fixes: #49925
PR-URL: #52038
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

Author: cjihrig

doc/api/cli.md

@@ -1839,6 +1839,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`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+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`
 
 <!-- YAML

doc/api/test.md

@@ -1149,7 +1149,12 @@ added:
   - v18.9.0
   - v16.19.0
 changes:
-  - version: v20.1.0
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/52038
+    description: Added the `forceExit` option.
+  - version:
+    - v20.1.0
+    - v18.17.0
     pr-url: https://github.com/nodejs/node/pull/47628
     description: Add a testNamePatterns option.
 -->
@@ -1165,6 +1170,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,

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.

lib/internal/test_runner/harness.js

@@ -207,6 +207,7 @@ function setup(root) {
     },
     counters: null,
     shouldColorizeTestFiles: false,
+    teardown: exitHandler,
   };
   root.harness.resetCounters();
   root.startTime = hrtime();

lib/internal/test_runner/runner.js

@@ -156,8 +156,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)}`);
   }
@@ -490,14 +493,33 @@ 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');
   }
   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');
   }
@@ -553,7 +575,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;

lib/internal/test_runner/test.js

@@ -78,7 +78,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;
 
@@ -722,6 +727,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) {

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,

src/node_options.cc

@@ -179,6 +179,9 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* 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");
     }
@@ -617,6 +620,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);

src/node_options.h

@@ -166,6 +166,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<std::string> test_name_pattern;
   std::vector<std::string> test_reporter;
   std::vector<std::string> test_reporter_destination;

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');

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 *

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');
+});

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/);

test/parallel/test-runner-output.mjs

@@ -105,6 +105,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' },

test/parallel/test-runner-run.mjs

@@ -526,3 +526,21 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
     assert.match(stderr, /Warning: node:test run\(\) is being called recursively/);
   });
 });
+
+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\./
+    });
+  });
+});