Skip to content
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

Jump to bottom

test_runner: support defining test reporter in NODE_OPTIONS #46688

Merged
merged 7 commits into from
Mar 14, 2023
+36 −17
Merged

test_runner: support defining test reporter in NODE_OPTIONS #46688

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae2d73a
test_runner: support defining test reporter in NODE_OPTIONS
SRHerzog Feb 16, 2023
b51a68a
fix lint errors
SRHerzog Feb 23, 2023
c2edafb
replace CLI flag with environment variable
SRHerzog Mar 3, 2023
78997bd
remove --test-child-process from CLI doc
SRHerzog Mar 7, 2023
e80a78d
rename env var and clean up docs
SRHerzog Mar 10, 2023
86b639c
minor code and doc cleanup
SRHerzog Mar 13, 2023
588f6ea
fix lint error
SRHerzog Mar 13, 2023
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
8 changes: 8 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1434,6 +1434,12 @@ added: v19.6.0
The destination for the corresponding test reporter. See the documentation on
[test reporters][] for more details.

### `--test-child-process`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am -1 on this implementation,
CLI flags which are a public API should not be added to solve an internal node core issue
I would prefer to add an environment variable instead.
in addition, @cjihrig has expressed in the past a concern regarding introducing big differences between node test.js and node --test test.js - which I agree with, but this case might justify it - it makes more sense to me than parsing CLI flags out of NODE_OPTIONS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, an environment variable makes more sense, since this flag would only be used for the specific case of a parent test process spawning a child, there's no reason to add extra options to the CLI. I'll do a quick update.


A flag to identify the process as a child of another test process to ensure
that test reporting is formatted correctly to be parsed by a parent test
process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### `--test-child-process`
A flag to identify the process as a child of another test process to ensure
that test reporting is formatted correctly to be parsed by a parent test
process.

### `--test-only`

<!-- YAML
Expand Down Expand Up @@ -2119,6 +2125,8 @@ Node.js options that are allowed are:
* `--secure-heap`
* `--snapshot-blob`
* `--test-only`
* `--test-reporter-destination`
* `--test-reporter`
* `--throw-deprecation`
* `--title`
* `--tls-cipher-list`
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/test_runner/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ function getRunArgs({ path, inspectPort }) {
ArrayPrototypePush(argv, `--inspect-port=${getInspectPort(inspectPort)}`);
}
ArrayPrototypePush(argv, path);

return argv;
}

Expand Down Expand Up @@ -260,7 +261,7 @@ function runTestFile(path, root, inspectPort, filesWatcher) {
const subtest = root.createSubtest(FileTest, path, async (t) => {
const args = getRunArgs({ path, inspectPort });
const stdio = ['pipe', 'pipe', 'pipe'];
const env = { ...process.env };
const env = { ...process.env, TEST_CONTEXT: 'child' };
if (filesWatcher) {
stdio.push('ipc');
env.WATCH_REPORT_DEPENDENCIES = '1';
Expand Down
35 changes: 21 additions & 14 deletions lib/internal/test_runner/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const {
SafeMap,
Symbol,
} = primordials;

const { basename } = require('path');
const { createWriteStream } = require('fs');
const { pathToFileURL } = require('internal/url');
Expand Down Expand Up @@ -172,25 +173,31 @@ function parseCommandLine() {

const isTestRunner = getOptionValue('--test');
const coverage = getOptionValue('--experimental-test-coverage');
const destinations = getOptionValue('--test-reporter-destination');
const reporters = getOptionValue('--test-reporter');
let destinations = getOptionValue('--test-reporter-destination');
let reporters = getOptionValue('--test-reporter');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the getOptionValue() calls to the else branch on line 185.

const isChildProcess = process.env.TEST_CONTEXT === 'child';
let testNamePatterns;
let testOnlyFlag;

if (reporters.length === 0 && destinations.length === 0) {
ArrayPrototypePush(reporters, kDefaultReporter);
}
if (isChildProcess) {
reporters = [kDefaultReporter];
destinations = [kDefaultDestination];
} else {
if (reporters.length === 0 && destinations.length === 0) {
ArrayPrototypePush(reporters, kDefaultReporter);
}

if (reporters.length === 1 && destinations.length === 0) {
ArrayPrototypePush(destinations, kDefaultDestination);
}
if (reporters.length === 1 && destinations.length === 0) {
ArrayPrototypePush(destinations, kDefaultDestination);
}

if (destinations.length !== reporters.length) {
throw new ERR_INVALID_ARG_VALUE(
'--test-reporter',
reporters,
'must match the number of specified \'--test-reporter-destination\'',
);
if (destinations.length !== reporters.length) {
throw new ERR_INVALID_ARG_VALUE(
'--test-reporter',
reporters,
'must match the number of specified \'--test-reporter-destination\'',
);
}
}

if (isTestRunner) {
Expand Down
6 changes: 4 additions & 2 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,10 +590,12 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::test_name_pattern);
AddOption("--test-reporter",
"report test output using the given reporter",
&EnvironmentOptions::test_reporter);
&EnvironmentOptions::test_reporter,
kAllowedInEnvvar);
AddOption("--test-reporter-destination",
"report given reporter to the given destination",
&EnvironmentOptions::test_reporter_destination);
&EnvironmentOptions::test_reporter_destination,
kAllowedInEnvvar);
AddOption("--test-only",
"run tests with 'only' option set",
&EnvironmentOptions::test_only,
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ class EnvironmentOptions : public Options {
std::vector<std::string> test_name_pattern;
std::vector<std::string> test_reporter;
std::vector<std::string> test_reporter_destination;
bool test_child_process = false;
MoLow marked this conversation as resolved.
Show resolved Hide resolved
bool test_only = false;
bool test_udp_no_try_send = false;
bool throw_deprecation = false;
Expand Down