test_runner: centralize CLI option handling

The test runner relies on a few CLI options. That code was spread across a few locations. This commit centralizes that logic. PR-URL: nodejs#46707 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
MoLow · Feb 25, 2023 · 214915c · 214915c
1 parent 2957034
commit 214915c
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 27 deletions.
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
@@ -14,12 +14,13 @@ const {
   },
 } = require('internal/errors');
 const { kEmptyObject } = require('internal/util');
-const { getOptionValue } = require('internal/options');
 const { kCancelledByParent, Test, ItTest, Suite } = require('internal/test_runner/test');
-const { setupTestReporters } = require('internal/test_runner/utils');
+const {
+  parseCommandLine,
+  setupTestReporters,
+} = require('internal/test_runner/utils');
 const { bigint: hrtime } = process.hrtime;
 
-const isTestRunnerCli = getOptionValue('--test');
 const testResources = new SafeMap();
 const wasRootSetup = new SafeWeakSet();
 
@@ -54,8 +55,8 @@ function createProcessEventHandler(eventName, rootTest) {
   };
 }
 
-function configureCoverage(rootTest) {
-  if (!getOptionValue('--experimental-test-coverage')) {
+function configureCoverage(rootTest, globalOptions) {
+  if (!globalOptions.coverage) {
     return null;
   }
 
@@ -96,6 +97,11 @@ function setup(root) {
   if (wasRootSetup.has(root)) {
     return root;
   }
+
+  // Parse the command line options before the hook is enabled. We don't want
+  // global input validation errors to end up in the uncaughtException handler.
+  const globalOptions = parseCommandLine();
+
   const hook = createHook({
     init(asyncId, type, triggerAsyncId, resource) {
       if (resource instanceof Test) {
@@ -120,7 +126,7 @@ function setup(root) {
     createProcessEventHandler('uncaughtException', root);
   const rejectionHandler =
     createProcessEventHandler('unhandledRejection', root);
-  const coverage = configureCoverage(root);
+  const coverage = configureCoverage(root, globalOptions);
   const exitHandler = () => {
     root.coverage = collectCoverage(root, coverage);
     root.postRun(new ERR_TEST_FAILURE(
@@ -140,8 +146,8 @@ function setup(root) {
   process.on('uncaughtException', exceptionHandler);
   process.on('unhandledRejection', rejectionHandler);
   process.on('beforeExit', exitHandler);
-  // TODO(MoLow): Make it configurable to hook when isTestRunnerCli === false.
-  if (isTestRunnerCli) {
+  // TODO(MoLow): Make it configurable to hook when isTestRunner === false.
+  if (globalOptions.isTestRunner) {
     process.on('SIGINT', terminationHandler);
     process.on('SIGTERM', terminationHandler);
   }

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
@@ -1,6 +1,5 @@
 'use strict';
 const {
-  ArrayPrototypeMap,
   ArrayPrototypePush,
   ArrayPrototypeReduce,
   ArrayPrototypeShift,
@@ -31,13 +30,12 @@ const {
   },
   AbortError,
 } = require('internal/errors');
-const { getOptionValue } = require('internal/options');
 const { MockTracker } = require('internal/test_runner/mock');
 const { TestsStream } = require('internal/test_runner/tests_stream');
 const {
-  convertStringToRegExp,
   createDeferredCallback,
   isTestFailureError,
+  parseCommandLine,
 } = require('internal/test_runner/utils');
 const {
   createDeferredPromise,
@@ -65,22 +63,13 @@ const kTestTimeoutFailure = 'testTimeoutFailure';
 const kHookFailure = 'hookFailed';
 const kDefaultTimeout = null;
 const noop = FunctionPrototype;
-const isTestRunner = getOptionValue('--test');
-const testOnlyFlag = !isTestRunner && getOptionValue('--test-only');
-const testNamePatternFlag = isTestRunner ? null :
-  getOptionValue('--test-name-pattern');
-const testNamePatterns = testNamePatternFlag?.length > 0 ?
-  ArrayPrototypeMap(
-    testNamePatternFlag,
-    (re) => convertStringToRegExp(re, '--test-name-pattern')
-  ) : null;
 const kShouldAbort = Symbol('kShouldAbort');
 const kFilename = process.argv?.[1];
 const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']);
 const kUnwrapErrors = new SafeSet()
   .add(kTestCodeFailure).add(kHookFailure)
   .add('uncaughtException').add('unhandledRejection');
-
+const { testNamePatterns, testOnlyFlag } = parseCommandLine();
 
 function stopTest(timeout, signal) {
   if (timeout === kDefaultTimeout) {

diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js
@@ -1,5 +1,6 @@
 'use strict';
 const {
+  ArrayPrototypeMap,
   ArrayPrototypePush,
   ObjectCreate,
   ObjectGetOwnPropertyDescriptor,
@@ -149,8 +150,27 @@ async function getReportersMap(reporters, destinations) {
 
 
 async function setupTestReporters(testsStream) {
+  const { reporters, destinations } = parseCommandLine();
+  const reportersMap = await getReportersMap(reporters, destinations);
+  for (let i = 0; i < reportersMap.length; i++) {
+    const { reporter, destination } = reportersMap[i];
+    compose(testsStream, reporter).pipe(destination);
+  }
+}
+
+let globalTestOptions;
+
+function parseCommandLine() {
+  if (globalTestOptions) {
+    return globalTestOptions;
+  }
+
+  const isTestRunner = getOptionValue('--test');
+  const coverage = getOptionValue('--experimental-test-coverage');
   const destinations = getOptionValue('--test-reporter-destination');
   const reporters = getOptionValue('--test-reporter');
+  let testNamePatterns;
+  let testOnlyFlag;
 
   if (reporters.length === 0 && destinations.length === 0) {
     ArrayPrototypePush(reporters, kDefaultReporter);
@@ -161,15 +181,37 @@ async function setupTestReporters(testsStream) {
   }
 
   if (destinations.length !== reporters.length) {
-    throw new ERR_INVALID_ARG_VALUE('--test-reporter', reporters,
-                                    'must match the number of specified \'--test-reporter-destination\'');
+    throw new ERR_INVALID_ARG_VALUE(
+      '--test-reporter',
+      reporters,
+      'must match the number of specified \'--test-reporter-destination\'',
+    );
   }
 
-  const reportersMap = await getReportersMap(reporters, destinations);
-  for (let i = 0; i < reportersMap.length; i++) {
-    const { reporter, destination } = reportersMap[i];
-    compose(testsStream, reporter).pipe(destination);
+  if (isTestRunner) {
+    testOnlyFlag = false;
+    testNamePatterns = null;
+  } else {
+    const testNamePatternFlag = getOptionValue('--test-name-pattern');
+    testOnlyFlag = getOptionValue('--test-only');
+    testNamePatterns = testNamePatternFlag?.length > 0 ?
+      ArrayPrototypeMap(
+        testNamePatternFlag,
+        (re) => convertStringToRegExp(re, '--test-name-pattern'),
+      ) : null;
   }
+
+  globalTestOptions = {
+    __proto__: null,
+    isTestRunner,
+    coverage,
+    testOnlyFlag,
+    testNamePatterns,
+    reporters,
+    destinations,
+  };
+
+  return globalTestOptions;
 }
 
 module.exports = {
@@ -178,5 +220,6 @@ module.exports = {
   doesPathMatchFilter,
   isSupportedFileType,
   isTestFailureError,
+  parseCommandLine,
   setupTestReporters,
 };