Skip to content

test_runner: remove problematic uses of parseCommandLine() #54353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cjihrig wants to merge 6 commits into from
Closed

test_runner: remove problematic uses of parseCommandLine() #54353

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
73e0f81
test_runner: use run() argument names in parseCommandLine()
cjihrig Aug 7, 2024
488e5bc
test_runner: pass harness object as option to root test
cjihrig Aug 8, 2024
cc2ba8c
test_runner: pass global options to createTestTree()
cjihrig Aug 12, 2024
2d41819
test_runner: return setup() from parseCommandLine()
cjihrig Aug 13, 2024
3ad749c
test_runner: refactor hook creation
cjihrig Aug 13, 2024
44bfa3b
test_runner: remove parseCommandLine() from test.js
cjihrig Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test_runner: return setup() from parseCommandLine() 
Now that parseCommandLine() returns run() compatible arguments,
it makes sense to return setupTestReporters() as the setup()
argument to run(). This also removes another problematic use of
parseCommandLine() in setupTestReporters().
  • Loading branch information
@cjihrig
cjihrig committed Aug 13, 2024
commit 2d41819a61db91746d2dcc1a9db77a56258aa59b
6 changes: 1 addition & 5 deletions lib/internal/main/test_runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ const {
} = require('internal/process/pre_execution');
const { isUsingInspector } = require('internal/util/inspector');
const { run } = require('internal/test_runner/runner');
const {
parseCommandLine,
setupTestReporters,
} = require('internal/test_runner/utils');
const { parseCommandLine } = require('internal/test_runner/utils');
const { exitCodes: { kGenericUserError } } = internalBinding('errors');
let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => {
debug = fn;
Expand All @@ -31,7 +28,6 @@ if (isUsingInspector()) {
options.inspectPort = process.debugPort;
}

options.setup = setupTestReporters;
options.globPatterns = ArrayPrototypeSlice(process.argv, 1);

debug('test runner configuration:', options);
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/test_runner/harness.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ const { kCancelledByParent, Test, Suite } = require('internal/test_runner/test')
const {
parseCommandLine,
reporterScope,
setupTestReporters,
shouldColorizeTestFiles,
} = require('internal/test_runner/utils');
const { queueMicrotask } = require('internal/process/task_queues');
Expand Down Expand Up @@ -231,13 +230,14 @@ function lazyBootstrapRoot() {
__proto__: null,
entryFile: process.argv?.[1],
};
createTestTree(rootTestOptions, parseCommandLine());
const globalOptions = parseCommandLine();
createTestTree(rootTestOptions, globalOptions);
globalRoot.reporter.on('test:fail', (data) => {
if (data.todo === undefined || data.todo === false) {
process.exitCode = kGenericUserError;
}
});
globalRoot.harness.bootstrapPromise = setupTestReporters(globalRoot.reporter);
globalRoot.harness.bootstrapPromise = globalOptions.setup(globalRoot.reporter);
}
return globalRoot;
}
Expand Down
1 change: 1 addition & 0 deletions lib/internal/test_runner/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,7 @@ function run(options = kEmptyObject) {
// parseCommandLine() should not be used here. However, The existing run()
// behavior has relied on it, so removing it must be done in a semver major.
...parseCommandLine(),
setup, // This line can be removed when parseCommandLine() is removed here.
};
const root = createTestTree(rootTestOptions, globalOptions);

Expand Down
20 changes: 10 additions & 10 deletions lib/internal/test_runner/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,6 @@ async function getReportersMap(reporters, destinations) {
}

const reporterScope = new AsyncResource('TestReporterScope');
const setupTestReporters = reporterScope.bind(async (rootReporter) => {
const { reporters, destinations } = parseCommandLine();
const reportersMap = await getReportersMap(reporters, destinations);
for (let i = 0; i < reportersMap.length; i++) {
const { reporter, destination } = reportersMap[i];
compose(rootReporter, reporter).pipe(destination);
}
});

let globalTestOptions;

function parseCommandLine() {
Expand Down Expand Up @@ -281,6 +272,15 @@ function parseCommandLine() {
coverageIncludeGlobs = getOptionValue('--test-coverage-include');
}

const setup = reporterScope.bind(async (rootReporter) => {
const reportersMap = await getReportersMap(reporters, destinations);

for (let i = 0; i < reportersMap.length; i++) {
const { reporter, destination } = reportersMap[i];
compose(rootReporter, reporter).pipe(destination);
}
});

globalTestOptions = {
__proto__: null,
isTestRunner,
Expand All @@ -292,6 +292,7 @@ function parseCommandLine() {
forceExit,
only,
reporters,
setup,
shard,
sourceMaps,
testNamePatterns,
Expand Down Expand Up @@ -480,7 +481,6 @@ module.exports = {
kDefaultPattern,
parseCommandLine,
reporterScope,
setupTestReporters,
shouldColorizeTestFiles,
getCoverageReport,
};