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

Feature request: test runner reporters #45648

Closed
cjihrig opened this issue Nov 27, 2022 · 12 comments · Fixed by #45712
Closed

Feature request: test runner reporters #45648

cjihrig opened this issue Nov 27, 2022 · 12 comments · Fixed by #45712
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 Nov 27, 2022

What is the problem this feature will solve?

The test runner currently only generates TAP output. Users would like a way to author and use reporters in a format other than TAP.

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

Now that the TAP parser has landed, it would be nice to define some type of reporter API built on top of the parser output. Node should probably ship one reporter that is easier on the eyes than TAP, and can serve as an example of how to create a reporter.

I think at a minimum we would want an API that:

  • Generates 'test' events. This would include the test name, result, and meta information such as the test duration, comments/user logs, and error information if the test fails.
  • Properly represents test nesting. This should work for both test() and describe()/it() style tests.
  • Handle warnings that are generated for things like extraneous asynchronous activity.
  • Maybe the test summary - the part that says how many tests passed, failed, etc. We may want to surface this information, but leave it up to the reporter implementer whether they want to use it, or track the counts on their own. Right now, the CLI runner reports how many test files passed or failed. A reporter author could be more interested in counting individual test()s, or may want to count nested tests in different ways.

What alternatives have you considered?

There are some existing reporters on npm already. However, I think they have a few drawbacks:

  • Many seem to be unmaintained or buggy.
  • It is possible for Node to produce valid TAP and have a reporter consume that TAP, but still have less than ideal output. For example, a recent issue comment mentioned seeing confusing output because tap-arc was looking for specific fields in the TAP output. Neither Node nor tap-arc were wrong here and tap-arc has an open issue to work better with Node's test runner. It would be nice to build something that is guaranteed to work with Node's runner.
  • Anything currently on npm needs to use another TAP parser to parse the test runner output. This is wasteful because Node is already doing that work in the test runner CLI.
  • The existing modules generally work by piping the output of Node into another process. It would be nice to be able to run something like node --test --test-reporter=my-reporter and have it just work.
@cjihrig cjihrig added the feature request Issues that request new features to be added to Node.js. label Nov 27, 2022
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 27, 2022

cc: @manekinekko since you expressed interest in this. In the TAP parser PR, you initially had some colorful output that I asked you to remove. I think reporters would be the time and place to introduce that 😄

@cjihrig cjihrig added the test_runner Issues and PRs related to the test runner subsystem. label Nov 27, 2022
@manekinekko
Copy link
Contributor

Thank you @cjihrig for putting this together so quickly. Gonna draft a rough design for the reporter API. Feel free to assign this work to me.

cc: @manekinekko since you expressed interest in this. In the TAP parser PR, you initially had some colorful output that I asked you to remove. I think reporters would be the time and place to introduce that 😄

Of course, I will 😂

@MoLow
Copy link
Member

MoLow commented Nov 29, 2022

+1 for me.

A few thoughts regarding the implementation:

  • we already fire some of the data through tapStream - we can either use that class to fire all the additional data needed for reporters to work, or use a new/different API - in that case, we might want to remove the events from tapStream for the sake of simplicity
  • do we want to support using reporters when using node:test without the --test flag? seems like a valid use case, and we already stream TAP to stdout.
  • another interesting use-case I know of is having multiple reporters streamed to different locations, for example, a spec reporter streamed to stdout and a jUnit reporter to a file. even if we do not support that directly - we should think about what API to expose that will allow that

@MoLow
Copy link
Member

MoLow commented Nov 29, 2022

cc @nodejs/test_runner

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 29, 2022

do we want to support using reporters when using node:test without the --test flag?

Yes, we should. Since both ways produce TAP, it should be feasible.

another interesting use-case I know of is having multiple reporters streamed to different locations

Yes, we should support this too. We can make a new --test-reporter CLI flag that can be specified multiple times.

@MoLow
Copy link
Member

MoLow commented Nov 29, 2022

Yes, we should support this too. We can make a new --test-reporter CLI flag that can be specified multiple times.

but how can we specify the target stream for each flag?

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 29, 2022

but how can we specify the target stream for each flag?

To be determined 😅. There are different approaches we could try - maybe part of the CLI flag (--test-reporter=reporter-name,stderr), preloaded modules, etc. Maybe the simplest thing would be to only support multiple reporters via the programmatic config (run()). I'm open to ideas.

@MoLow
Copy link
Member

MoLow commented Nov 29, 2022

it should definitely be supported through run.
my concern is this use case is very common, and we should also provide an even simpler way to do this.

  • maybe we do finally introduce a configuration file, even if it is just for test runner configuration?
  • maybe each reporter can define a default destination
    anyway for a first iteration supporting only run() for multiple reporters sounds good enough

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 29, 2022

Here is one possible way to do it: https://github.com/hapijs/lab/blob/master/API.md#multiple-reporters (also note the Custom Reporters section immediately below it)

@manekinekko
Copy link
Contributor

but how can we specify the target stream for each flag?

We should be flexible and support both --test-reporter=abc --test-reporter=xyz and as @cjihrig mentioned --test-reporter=abc,xyz

maybe we do finally introduce a configuration file, even if it is just for test runner configuration?

Maintaining a configuration file can be hard sometimes. How about reusing package.json and introducing a new property?

@MoLow MoLow mentioned this issue Dec 2, 2022
3 tasks
nodejs-github-bot pushed a commit that referenced this issue Dec 19, 2022
PR-URL: #45712
Fixes: #45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@GeoffreyBooth
Copy link
Member

Maintaining a configuration file can be hard sometimes. How about reusing package.json and introducing a new property?

Hi, just discovering this issue. Going forward, please always tag @nodejs/modules and @nodejs/loaders for any discussion around CLI flags that load files, package.json stuff and config files. These are all hot topics related to module loading and tied into other designs such as the Loaders API.

@benjamingr
Copy link
Member

@GeoffreyBooth sounds like a good idea, as a side note - consider joining @nodejs/test_runner yourself :)

targos pushed a commit that referenced this issue Jan 1, 2023
PR-URL: #45712
Fixes: #45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
RafaelGSS pushed a commit that referenced this issue Jan 4, 2023
PR-URL: #45712
Fixes: #45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Jan 26, 2023
PR-URL: nodejs#45712
Fixes: nodejs#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Jan 26, 2023
PR-URL: nodejs#45712
Fixes: nodejs#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Jan 26, 2023
PR-URL: nodejs#45712
Fixes: nodejs#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow added a commit to MoLow/node that referenced this issue Jan 26, 2023
PR-URL: nodejs#45712
Fixes: nodejs#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ruyadorno pushed a commit that referenced this issue Jan 31, 2023
Backport-PR-URL: #46361
PR-URL: #45712
Fixes: #45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 6, 2023
PR-URL: nodejs/node#45712
Fixes: nodejs/node#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 7, 2023
PR-URL: nodejs/node#45712
Fixes: nodejs/node#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 7, 2023
PR-URL: nodejs/node#45712
Fixes: nodejs/node#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)
MoLow added a commit to MoLow/node-core-test that referenced this issue Feb 7, 2023
PR-URL: nodejs/node#45712
Fixes: nodejs/node#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)
MoLow added a commit to nodejs/node-core-test that referenced this issue Feb 8, 2023
PR-URL: nodejs/node#45712
Fixes: nodejs/node#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
(cherry picked from commit a1b27b25bb01aadd3fd2714e4b136db11b7eb85a)
MoLow added a commit to MoLow/node that referenced this issue Feb 25, 2023
PR-URL: nodejs#45712
Fixes: nodejs#45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
PR-URL: #45712
Backport-PR-URL: #46839
Fixes: #45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
PR-URL: #45712
Backport-PR-URL: #46839
Fixes: #45648
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants