From 84de97a61e5c0591a310b5b12cc2be34b6893b36 Mon Sep 17 00:00:00 2001
From: Colin Ihrig <cjihrig@gmail.com>
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 <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>
---
 doc/api/cli.md                                |  9 +++++
 doc/api/test.md                               |  6 ++++
 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, 170 insertions(+), 10 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 d7b514694595b0..b445f6b0e176f1 100644
--- 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
index b22e4348ea8167..661e357c1da528 100644
--- 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
index 190dd8d9ff4714..d055485bf0a0d2 100644
--- 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
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 242e807fa68883..b7ac5756dad195 100644
--- 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,7 +443,17 @@ 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');
@@ -448,6 +461,15 @@ function run(options = kEmptyObject) {
   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
index 80af0f9d978637..1642cfdd15e3c6 100644
--- 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
index fda9f4334aa64e..e812bd197fccfa 100644
--- 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
index 65840ff1be6136..6b4c06b20ad0c0 100644
--- 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
index 2f59f70f8ed60e..5b26458cc6aa29 100644
--- 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
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 8f95de4db16296..2e8a022ea3dd78 100644
--- 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\./
+    });
+  });
+});