test_runner: support defining test reporter in NODE_OPTIONS by SRHerzog · Pull Request #46688 · nodejs/node

SRHerzog · 2023-02-16T20:01:26Z

Adds --test-reporter and --test-reporter-destination as allowable options in NODE_OPTIONS.

Fixes: #46484

nodejs-github-bot · 2023-02-16T20:01:30Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/runner.js

MoLow · 2023-02-25T20:30:11Z

doc/api/cli.md

 The destination for the corresponding test reporter. See the documentation on
 [test reporters][] for more details.

+### `--test-child-process`


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

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.

Adds --test-reporter and --test-reporter-destination as allowable options in NODE_OPTIONS. Also adds the CLI flag --test-child-process to allow forcing the default test-reporter for inter-process communication. Fixes: #46484

aduh95 · 2023-03-07T10:39:13Z

doc/api/cli.md

+### `--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.
+


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.

aduh95

We would need to document TEST_CONTEXT in

node/doc/api/cli.md

Line 1933 in 00c2225

## Environment variables

Also, consider renaming it to e.g. NODE_TEST_CONTEXT.

src/node_options.h

cjihrig

LGTM with some small comments.

cjihrig · 2023-03-13T01:49:57Z

lib/internal/test_runner/utils.js

+  let destinations = getOptionValue('--test-reporter-destination');
+  let reporters = getOptionValue('--test-reporter');


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

cjihrig · 2023-03-13T01:59:55Z

doc/api/cli.md

+output will be sent to stdout in the TAP format. This is intended to facilitate
+parsing and aggregating test output by a parent process that spawns one or more
+children.


The last sentence reads a bit like an implementation detail that users shouldn't need to worry about. If others feel it's valuable to include, then you can disregard this comment.

+1 on removing

benjamingr

LGTM with a preference to both Colin's comments being fixed but lgtm either way

MoLow · 2023-03-13T16:37:14Z

Please fix lint issue

nodejs-github-bot · 2023-03-13T17:36:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50359/

nodejs-github-bot · 2023-03-14T08:37:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50370/

nodejs-github-bot · 2023-03-14T10:52:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50376/

nodejs-github-bot · 2023-03-14T16:46:49Z

Commit Queue failed

- Loading data for nodejs/node/pull/46688 ✔ Done loading data for nodejs/node/pull/46688 ----------------------------------- PR info ------------------------------------ Title test_runner: support defining test reporter in NODE_OPTIONS (#46688) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch SRHerzog:node-options-test-reporter -> nodejs:main Labels c++, author ready, needs-ci, dont-land-on-v14.x, test_runner Commits 7 - test_runner: support defining test reporter in NODE_OPTIONS - fix lint errors - replace CLI flag with environment variable - remove --test-child-process from CLI doc - rename env var and clean up docs - minor code and doc cleanup - fix lint error Committers 1 - Steve Herzog PR-URL: https://github.com/nodejs/node/pull/46688 Fixes: https://github.com/nodejs/node/issues/46484 Reviewed-By: Moshe Atlow Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46688 Fixes: https://github.com/nodejs/node/issues/46484 Reviewed-By: Moshe Atlow Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 16 Feb 2023 20:01:26 GMT ✔ Approvals: 3 ✔ - Moshe Atlow (@MoLow): https://github.com/nodejs/node/pull/46688#pullrequestreview-1337613800 ✔ - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/46688#pullrequestreview-1336142617 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46688#pullrequestreview-1336867851 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-03-14T10:52:38Z: https://ci.nodejs.org/job/node-test-pull-request/50376/ - Querying data for job/node-test-pull-request/50376/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 46688 From https://github.com/nodejs/node * branch refs/pull/46688/merge -> FETCH_HEAD ✔ Fetched commits as 4ae7c7a00258..588f6eadd8c9 -------------------------------------------------------------------------------- Auto-merging doc/api/cli.md Auto-merging lib/internal/test_runner/runner.js Auto-merging lib/internal/test_runner/utils.js [main 9571984ed6] test_runner: support defining test reporter in NODE_OPTIONS Author: Steve Herzog Date: Thu Feb 16 12:53:18 2023 -0600 5 files changed, 38 insertions(+), 16 deletions(-) Auto-merging lib/internal/test_runner/runner.js [main 23116dc8af] fix lint errors Author: Steve Herzog Date: Thu Feb 23 10:05:23 2023 -0600 2 files changed, 2 insertions(+), 2 deletions(-) Auto-merging lib/internal/test_runner/runner.js Auto-merging lib/internal/test_runner/utils.js [main 38d41acac0] replace CLI flag with environment variable Author: Steve Herzog Date: Fri Mar 3 14:36:43 2023 -0600 3 files changed, 3 insertions(+), 6 deletions(-) Auto-merging doc/api/cli.md [main 4613283ba6] remove --test-child-process from CLI doc Author: Steve Herzog Date: Tue Mar 7 09:49:27 2023 -0600 1 file changed, 6 deletions(-) Auto-merging doc/api/cli.md Auto-merging lib/internal/test_runner/runner.js Auto-merging lib/internal/test_runner/utils.js [main 779484a634] rename env var and clean up docs Author: Steve Herzog Date: Fri Mar 10 12:47:12 2023 -0600 4 files changed, 9 insertions(+), 3 deletions(-) Auto-merging doc/api/cli.md Auto-merging lib/internal/test_runner/utils.js [main 7f73305c57] minor code and doc cleanup Author: Steve Herzog Date: Mon Mar 13 11:30:02 2023 -0500 2 files changed, 5 insertions(+), 5 deletions(-) Auto-merging lib/internal/test_runner/utils.js [main 21a3d72501] fix lint error Author: Steve Herzog Date: Mon Mar 13 11:47:18 2023 -0500 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 7 commits in the PR. Attempting autorebase. Rebasing (2/14)

Executing: git node land --amend --yes
⚠ Found Fixes: #46484, skipping..
--------------------------------- New Message ----------------------------------
test_runner: support defining test reporter in NODE_OPTIONS

Adds --test-reporter and --test-reporter-destination as
allowable options in NODE_OPTIONS. Also adds the CLI flag
--test-child-process to allow forcing the default
test-reporter for inter-process communication.

Fixes: #46484
PR-URL: #46688
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 2edb1a293e] test_runner: support defining test reporter in NODE_OPTIONS
Author: Steve Herzog srherzog@gmail.com
Date: Thu Feb 16 12:53:18 2023 -0600
5 files changed, 38 insertions(+), 16 deletions(-)
Rebasing (3/14)
Rebasing (4/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix lint errors

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD f4efd8f31a] fix lint errors
Author: Steve Herzog srherzog@gmail.com
Date: Thu Feb 23 10:05:23 2023 -0600
2 files changed, 2 insertions(+), 2 deletions(-)
Rebasing (5/14)
Rebasing (6/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
replace CLI flag with environment variable

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 10a716a067] replace CLI flag with environment variable
Author: Steve Herzog srherzog@gmail.com
Date: Fri Mar 3 14:36:43 2023 -0600
3 files changed, 3 insertions(+), 6 deletions(-)
Rebasing (7/14)
Rebasing (8/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
remove --test-child-process from CLI doc

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 8ade1e3b8b] remove --test-child-process from CLI doc
Author: Steve Herzog srherzog@gmail.com
Date: Tue Mar 7 09:49:27 2023 -0600
1 file changed, 6 deletions(-)
Rebasing (9/14)
Rebasing (10/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
rename env var and clean up docs

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD f6c671172c] rename env var and clean up docs
Author: Steve Herzog srherzog@gmail.com
Date: Fri Mar 10 12:47:12 2023 -0600
4 files changed, 9 insertions(+), 3 deletions(-)
Rebasing (11/14)
Rebasing (12/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
minor code and doc cleanup

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 8bb5268926] minor code and doc cleanup
Author: Steve Herzog srherzog@gmail.com
Date: Mon Mar 13 11:30:02 2023 -0500
2 files changed, 5 insertions(+), 5 deletions(-)
Rebasing (13/14)
Rebasing (14/14)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix lint error

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[detached HEAD 95d0360654] fix lint error
Author: Steve Herzog srherzog@gmail.com
Date: Mon Mar 13 11:47:18 2023 -0500
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/4418134815

nodejs-github-bot · 2023-03-14T16:59:38Z

Landed in 334bb17

Adds --test-reporter and --test-reporter-destination as allowable options in NODE_OPTIONS. Also adds the CLI flag --test-child-process to allow forcing the default test-reporter for inter-process communication. Fixes: #46484 PR-URL: #46688 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 16, 2023

ljharb requested changes Feb 16, 2023

View reviewed changes

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved

SRHerzog force-pushed the node-options-test-reporter branch from e9fb4e2 to 15803a0 Compare February 23, 2023 14:43

SRHerzog marked this pull request as ready for review February 23, 2023 14:43

MoLow reviewed Feb 25, 2023

View reviewed changes

SRHerzog added 3 commits March 6, 2023 14:21

test_runner: support defining test reporter in NODE_OPTIONS

ae2d73a

Adds --test-reporter and --test-reporter-destination as allowable options in NODE_OPTIONS. Also adds the CLI flag --test-child-process to allow forcing the default test-reporter for inter-process communication. Fixes: #46484

fix lint errors

b51a68a

replace CLI flag with environment variable

c2edafb

SRHerzog force-pushed the node-options-test-reporter branch from 678bb49 to c2edafb Compare March 6, 2023 20:22

aduh95 reviewed Mar 7, 2023

View reviewed changes

remove --test-child-process from CLI doc

78997bd

MoLow reviewed Mar 7, 2023

View reviewed changes

src/node_options.h Outdated Show resolved Hide resolved

rename env var and clean up docs

e80a78d

MoLow approved these changes Mar 12, 2023

View reviewed changes

MoLow requested a review from cjihrig March 12, 2023 06:44

cjihrig approved these changes Mar 13, 2023

View reviewed changes

benjamingr approved these changes Mar 13, 2023

View reviewed changes

minor code and doc cleanup

86b639c

fix lint error

588f6ea

MoLow approved these changes Mar 13, 2023

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 13, 2023

MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2023

github-actions bot mentioned this pull request Mar 14, 2023

CI Reliability 2023-03-14 nodejs/reliability#523

Open

24 tasks

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 14, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 14, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 14, 2023

nodejs-github-bot merged commit 334bb17 into nodejs:main Mar 14, 2023

This was referenced Mar 15, 2023

CI Reliability 2023-03-15 nodejs/reliability#524

Open

CI Reliability 2023-03-16 nodejs/reliability#525

Open

CI Reliability 2023-03-17 nodejs/reliability#526

Open

CI Reliability 2023-03-18 nodejs/reliability#527

Open

github-actions bot mentioned this pull request Mar 19, 2023

CI Reliability 2023-03-19 nodejs/reliability#528

Open

19 tasks

This was referenced Apr 3, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

v19.9.0 proposal #47441

Merged

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

		let destinations = getOptionValue('--test-reporter-destination');
		let reporters = getOptionValue('--test-reporter');

Uh oh!

Conversation

SRHerzog commented Feb 16, 2023

Uh oh!

nodejs-github-bot commented Feb 16, 2023

Uh oh!

Uh oh!

MoLow Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

SRHerzog Feb 26, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Mar 7, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

MoLow commented Mar 13, 2023

Uh oh!

nodejs-github-bot commented Mar 13, 2023

Uh oh!

nodejs-github-bot commented Mar 14, 2023

Uh oh!

nodejs-github-bot commented Mar 14, 2023

Uh oh!

nodejs-github-bot commented Mar 14, 2023

Fixes: #46484 PR-URL: #46688 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688 Fixes: #46484 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688 Fixes: #46484 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688 Fixes: #46484 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688 Fixes: #46484 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688 Fixes: #46484 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688 Fixes: #46484 Reviewed-By: Moshe Atlow moshe@atlow.co.il Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Successfully rebased and updated refs/heads/main.

Uh oh!

nodejs-github-bot commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduh95 left a comment •

edited

Loading

Fixes: #46484
PR-URL: #46688
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

PR-URL: #46688
Fixes: #46484
Reviewed-By: Moshe Atlow moshe@atlow.co.il
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com