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

Proposal: always default test runner to use spec reporter #54540

Closed
cjihrig opened this issue Aug 24, 2024 · 3 comments
Closed

Proposal: always default test runner to use spec reporter #54540

cjihrig opened this issue Aug 24, 2024 · 3 comments
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Aug 24, 2024

What is the problem this feature will solve?

The test runner currently uses the more human friendly spec reporter when a TTY output is detected. Otherwise, it defaults to generating TAP output. I think the cons of this behavior outweigh the pros.

In Node core, this would remove occurrences of nested TAP output which can cause issues like #54535. We also have a number of places throughout the codebase, such as the GitHub Actions config, where we force the spec reporter anyway.

In userland, it is a DX improvement (no more TAP output in CI runs). TAP output would still be available via the --test-reporter=tap

What is the feature you are proposing to solve the problem?

Make the default test runner reporter be spec.

Notes:

  • This would be a semver major change.
  • We should try to avoid churn in snapshot output by forcing the output to whatever is currently being used. This will help avoid conflicts in the future and make backporting other changes simpler.

What alternatives have you considered?

No response
@cjihrig cjihrig added the feature request Issues that request new features to be added to Node.js. label Aug 24, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Aug 24, 2024
@targos
Copy link
Member

targos commented Aug 24, 2024

I'm +1, but I've never had a TAP parser in my pipeline so I don't really know if the impact is big.

@RedYetiDev RedYetiDev added the test_runner Issues and PRs related to the test runner subsystem. label Aug 24, 2024
@RedYetiDev RedYetiDev moved this from Awaiting Triage to Triaged in Node.js feature requests Aug 24, 2024
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 24, 2024 •
edited
Loading

The impact would be for people who want TAP output to pass --test-reporter=tap when starting Node. I think that is pretty reasonable for a semver major change.

EDIT: It's worth noting that while this changes a default, passing --test-reporter=tap would be backward compatible to all versions of Node since test reporters were introduced.

cjihrig added a commit to cjihrig/node that referenced this issue Aug 24, 2024
@cjihrig
test: refactor test_runner tests to change default reporter
efd6176 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: nodejs#54540
cjihrig added a commit to cjihrig/node that referenced this issue Aug 24, 2024
@cjihrig
test_runner: always make spec the default reporter
031aba5 
This is a breaking change. Prior to this commit, the test_runner
defaulted to the spec reporter if using a TTY, and the TAP
reporter otherwise. This commit makes spec the default reporter
unconditionally. TAP output is still available via the
--test-reporter=tap CLI flag.

Fixes: nodejs#54540
@cjihrig cjihrig mentioned this issue Aug 24, 2024
Merged
@RedYetiDev RedYetiDev moved this from Triaged to In Progress in Node.js feature requests Aug 24, 2024
@RedYetiDev
Copy link
Member

Fixed by #54548

cjihrig added a commit to cjihrig/node that referenced this issue Aug 25, 2024
@cjihrig
test: refactor test_runner tests to change default reporter
c6c92c2 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: nodejs#54540
cjihrig added a commit to cjihrig/node that referenced this issue Aug 26, 2024
@cjihrig
test: refactor test_runner tests to change default reporter
9b3fab5 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: nodejs#54540
nodejs-github-bot pushed a commit that referenced this issue Aug 27, 2024
@cjihrig
test: refactor test_runner tests to change default reporter
3d5357a 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: #54540
PR-URL: #54547
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this issue Aug 27, 2024
@cjihrig
test_runner: always make spec the default reporter
dfdbbe4 
This is a breaking change. Prior to this commit, the test_runner
defaulted to the spec reporter if using a TTY, and the TAP
reporter otherwise. This commit makes spec the default reporter
unconditionally. TAP output is still available via the
--test-reporter=tap CLI flag.

Fixes: nodejs#54540
RafaelGSS pushed a commit that referenced this issue Aug 30, 2024
@cjihrig @RafaelGSS
test: refactor test_runner tests to change default reporter
5b4368b 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: #54540
PR-URL: #54547
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
RafaelGSS pushed a commit that referenced this issue Aug 30, 2024
@cjihrig @RafaelGSS
test: refactor test_runner tests to change default reporter
846e2b2 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: #54540
PR-URL: #54547
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@cjihrig @louwers
test: refactor test_runner tests to change default reporter
264ef83 
This commit updates the test runner tests in order to switch the
default reporter from tap to spec. This commit can be backported,
while changing the default reporter cannot.

Refs: nodejs#54540
PR-URL: nodejs#54547
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@cjihrig @louwers
test_runner: always make spec the default reporter
3a1276d 
This is a breaking change. Prior to this commit, the test_runner
defaulted to the spec reporter if using a TTY, and the TAP
reporter otherwise. This commit makes spec the default reporter
unconditionally. TAP output is still available via the
--test-reporter=tap CLI flag.

Fixes: nodejs#54540
PR-URL: nodejs#54548
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request Issues that request new features to be added to Node.js. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@targos @cjihrig @RedYetiDev