Sitelet https://github.com/nodejs/node/pull/48974
Skip to content

test_runner: add error message if custom reporter throws error - #48974

Closed
ocodista wants to merge 0 commit into
nodejs:mainfrom
ocodista:main
Closed

test_runner: add error message if custom reporter throws error#48974
ocodista wants to merge 0 commit into
nodejs:mainfrom
ocodista:main

Conversation

@ocodista

@ocodista ocodista commented Jul 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This goal of this PR is to solve the issue 48937

Where the test silently fails if there's an exception inside the Custom test Reporter.

I created this repo with a minimum reproduction of the bug.

I also created a new test (that's currently failing) to check if the Custom Reporter error message will be shown.

I'm still working on getting the exception from the custom reporter (start step) and displaying it on the test result.

Any help/guidance would be appreciated!

@nodejs-github-bot

nodejs-github-bot commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Jul 30, 2023
@atlowChemi

atlowChemi commented Jul 31, 2023

Copy link
Copy Markdown
Member

Any help/guidance would be appreciated!

@ocodista Is there anything specific you would like help with? 🙂

@atlowChemi atlowChemi linked an issue Jul 31, 2023 that may be closed by this pull request
Throwing an exception on test reporters silently error the test runner #48937
Closed
atlowChemi
atlowChemi reviewed Jul 31, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/custom_reporters/error_reporter.mjs Outdated
@ocodista

ocodista commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

Any help/guidance would be appreciated!

@ocodista Is there anything specific you would like help with? 🙂

I'm still not sure where the call is being made so I can wrap it with a try-catch.

Also, it takes an eternity to build node on my PC, is there a way to make it faster? 😅

This way would be easier to add console logs and find it 😁

@atlowChemi

atlowChemi commented Jul 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

I'm still not sure where the call is being made so I can wrap it with a try-catch.

Reporter details

We create the reporter here: https://github.com/nodejs/node/blob/4b3d964a8c01c4b541fc85d3a419b58524806f04/lib/internal/test_runner/utils.js#L165-L172

Basically we call compose on the TestStream (rootTest.reporter) with the relevant reporters

node/lib/internal/test_runner/test.js

Line 192 in 4b3d964

this.reporter = new TestsStream();

This compose function is a Stream function you can find here:

node/lib/internal/streams/compose.js

Line 24 in 4b3d964

module.exports = function compose(...streams) {

Also, it takes an eternity to build node on my PC, is there a way to make it faster? 😅

This depends on what OS you are using, but I suggest having a look at BUILDING.MD:

node/BUILDING.md

Line 220 in 4b3d964

## Building Node.js on supported platforms

This way would be easier to add console logs and find it 😁

If you are only changing JS modules (and no src c++ code etc) you can avoid additional builds by setting the --node-builtin-modules-path option:

node/BUILDING.md

Lines 548 to 558 in 4b3d964

When modifying only the JS layer in `lib`, it is possible to externally load it
without modifying the executable:
```bash
./configure --node-builtin-modules-path "$(pwd)"
```
The resulting binary won't include any JS files and will try to load them from
the specified directory. The JS debugger of Visual Studio Code supports this
configuration since the November 2020 version and allows for setting
breakpoints.

@ocodista ocodista closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atlowChemi atlowChemi atlowChemi left review comments

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Throwing an exception on test reporters silently error the test runner

3 participants

@ocodista @nodejs-github-bot @atlowChemi