test_runner: support forced exit

cjihrig · web-flow · commit 84de97a61e5c · 2024-03-13T10:49:15.000Z
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 <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -1872,6 +1872,15 @@ added:
 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
diff --git a/doc/api/test.md b/doc/api/test.md
@@ -1119,6 +1119,9 @@ added:
   - v18.9.0
   - v16.19.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/52038
+    description: Added the `forceExit` option.
   - version:
     - v20.1.0
     - v18.17.0
@@ -1137,6 +1140,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
@@ -422,6 +422,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
@@ -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
@@ -112,8 +112,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)}`);
   }
@@ -440,14 +443,33 @@ function run(options = kEmptyObject) {
   validateObject(options, 'options');
 
   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');
   }
@@ -501,7 +523,15 @@ function run(options = kEmptyObject) {
 
   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
@@ -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;
 
@@ -748,6 +753,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();
     }
   }
 
@@ -798,12 +813,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
@@ -193,6 +193,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';
@@ -245,6 +246,7 @@ function parseCommandLine() {
     __proto__: null,
     isTestRunner,
     coverage,
+    forceExit,
     sourceMaps,
     testOnlyFlag,
     testNamePatterns,
diff --git a/src/node_options.cc b/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");
     }
@@ -616,6 +619,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
@@ -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;
diff --git a/test/fixtures/test-runner/output/force_exit.js 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -513,3 +513,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\./
+    });
+  });
+});
