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.

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

Test Runner: delegate arbitrary output formatting to each reporter #48011

Closed
MoLow opened this issue May 15, 2023 · 7 comments · Fixed by #48045
Closed

Test Runner: delegate arbitrary output formatting to each reporter #48011

MoLow opened this issue May 15, 2023 · 7 comments · Fixed by #48045
Labels
feature request Issues that request new features to be added to Node.js. test_runner

Comments

@MoLow
Copy link
Member

MoLow commented May 15, 2023

What is the problem this feature will solve?

The current implementation of the test runner (--test) is very TAP oriented,
so we have decided (#43525 (comment), #45618) to treat any arbitrary output as a test diagnostic.

I do agree this decision makes sense when using TAP as the reporter, but I think we can provide better DX for console logs or other arbitrary output when using other reporters.

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

add a new event emitted by TestsStream, perhaps test:stdout or test:unknown (or whatever) so each reporter can decide how to treat unknown output.
I think console.log("test") should show the same when running node --test-reporter spec test.js and node --test --test-reporter spec test.js (probably also the same colors etc, but that probably justifies a separate issue)

What alternatives have you considered?

No response

@MoLow MoLow added feature request Issues that request new features to be added to Node.js. test_runner labels May 15, 2023
@MoLow
Copy link
Member Author

MoLow commented May 15, 2023

CC @nodejs/test_runner

@MoLow
Copy link
Member Author

MoLow commented May 15, 2023

one thing that is TAP oriented and might be harder to change is the fact we direct any output on stderr to stdout - but that might make sense in the context of a test runner.

@HinataKah0
Copy link
Contributor

HinataKah0 commented May 15, 2023

Hi @MoLow ,

Is anyone from test runner team working on it? Or is it open for anyone to try?
It seems that it's still being discussed 🤔

@MoLow
Copy link
Member Author

MoLow commented May 15, 2023

@HinataKah0 I first want to gather feedback regarding this issue, then it makes sense to only work on it after #47867 lands

@MoLow
Copy link
Member Author

MoLow commented May 16, 2023

I have discussed this with @cjihrig and he says it makes sense to him.
@HinataKah0 do you want to implement this?

@HinataKah0
Copy link
Contributor

I'll be happy to try 😄
Will send a PR so we can discuss as well (I hope I don't misunderstand anything)

@cjihrig
Copy link
Contributor

cjihrig commented May 17, 2023

one thing that is TAP oriented and might be harder to change is the fact we direct any output on stderr to stdout - but that might make sense in the context of a test runner.

I think it makes sense for the test runner to collect both stdout and stderr from the child processes and direct them to the same place. For example, if the reporter destination is a file, having stdout and stderr both sent to the file is helpful for reviewing the output at a later time. If stderr is not sent to the reporter destination, there would be no way of recovering it later.

nodejs-github-bot pushed a commit that referenced this issue May 23, 2023
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: #48045
Fixes: #48011
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue May 30, 2023
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: #48045
Fixes: #48011
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams pushed a commit that referenced this issue Jul 6, 2023
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: #48045
Fixes: #48011
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this issue Jul 6, 2023
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: nodejs#48045
Fixes: nodejs#48011
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: nodejs#48045
Fixes: nodejs#48011
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
Introduce new `TestsStream` events `test:stderr` and `test:stdout`
to delegate `stderr` and `stdout` (e.g. `console.log()`) formatting
to the reporter. And patch existing reporters to:
- Spec: output the message as it is
- TAP: stay the same with existing `test:diagnostic`

PR-URL: nodejs#48045
Fixes: nodejs#48011
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. test_runner
Projects
Status: Pending Triage
Development

Successfully merging a pull request may close this issue.

3 participants