From 5e954c3ce3cce314ef8b96abd5948298c0603d4d Mon Sep 17 00:00:00 2001 From: Colin Ihrig Date: Sun, 19 Feb 2023 20:26:04 -0500 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/46707 Reviewed-By: Antoine du Hamel Reviewed-By: Yagiz Nizipli Reviewed-By: Moshe Atlow Reviewed-By: James M Snell --- lib/internal/test_runner/harness.js | 22 +++++++----- lib/internal/test_runner/test.js | 15 ++------ lib/internal/test_runner/utils.js | 55 +++++++++++++++++++++++++---- 3 files changed, 65 insertions(+), 27 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 3dec42d0c4aa9e..6f196645ff473d 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -16,12 +16,13 @@ const { const { exitCodes: { kGenericUserError } } = internalBinding('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(); @@ -56,8 +57,8 @@ function createProcessEventHandler(eventName, rootTest) { }; } -function configureCoverage(rootTest) { - if (!getOptionValue('--experimental-test-coverage')) { +function configureCoverage(rootTest, globalOptions) { + if (!globalOptions.coverage) { return null; } @@ -98,6 +99,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) { @@ -122,7 +128,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( @@ -142,8 +148,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 index 9fb918f96b828e..0064a48cd334a7 100644 --- 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 index d981c0f6d4381b..ba7dbe57ba1784 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -1,5 +1,6 @@ 'use strict'; const { + ArrayPrototypeMap, ArrayPrototypePush, ObjectGetOwnPropertyDescriptor, SafePromiseAllReturnArrayLike, @@ -148,8 +149,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); @@ -160,15 +180,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 = { @@ -177,5 +219,6 @@ module.exports = { doesPathMatchFilter, isSupportedFileType, isTestFailureError, + parseCommandLine, setupTestReporters, };