-
-
Notifications
You must be signed in to change notification settings - Fork 35.9k
test_runner: add level-based diagnostic handling for reporter #55964
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
7dda539
test_runner: add level parameter to reporter.diagnostic
hpatel292-seneca c618874
test_runner: enhance #handleEvent to handle levels
hpatel292-seneca 2ac841c
test_runner: extend reporterColorMap for levels
hpatel292-seneca 1ca0d45
test_runner: set level for coverage threshold
hpatel292-seneca 038b0f8
test_runner: remove debug from reporterColorMap
hpatel292-seneca b490d9d
doc: update 'test:diagnostic' event to include level parameter
hpatel292-seneca 3e4d5e6
test: add test for spec reporter red color output
hpatel292-seneca 4048621
test: disable no-control-regex for color regex
hpatel292-seneca 33b961d
doc: clarify diagnostic behavior in API docs
hpatel292-seneca b1c8033
test: validate diagnostic events in test runner
hpatel292-seneca c06e9e1
test: Fix Lint error
hpatel292-seneca File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
test_runner: extend reporterColorMap for levels
Enhanced to include colors for the following diagnostic levels: : blue - info : yellow - warn : red - error Refs: #55922
- Loading branch information
commit 2ac841c625a57b9c677388e7fb2bb71a403a9df3
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the debug level. The reporter stream is not a generic logger, and we have other ways (
NODE_DEBUG) of adding debug output.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove that. And I have a question so this CI https://github.com/nodejs/node/actions/runs/11983534043/job/33427085217?pr=55964 failed and it's for
First commit message adheres to guidelines / lint-commit-message (pull_request)so should I change commit history??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_runner: add level to diagnosticsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I am asking if I should change the commit history and force push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Yes, please. You'll need to avoid merge commits too, as the tooling does not handle them well.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmarchini Ok so I am taking reference from this tests
https://github.com/nodejs/node/blob/main/test/fixtures/test-runner/output/coverage-width-80-color.mjs
https://github.com/nodejs/node/blob/main/test/fixtures/test-runner/output/coverage-width-80-color.snapshot
and based on that I wrote this test.
fixtures/test-runner/output/spec_reporter_diagnostic_levels.mjs
and add this in
test-runner-output.mjsnow I am running
tools/test.py test/parallel/test-runner-output.mjs --snapshotbut it's not creating snap for added test.How I can create snaps??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this when I tried using test with

release/nodeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot you were working in a Windows environment. Considering this, I would suggest picking a different approach. You could add a test to
node/test/parallel/test-runner-coverage-thresholds.jsthat forces the colors via the environment variable in thespawn, and check that the error message 'coverage does not meet...' is displayed in red.Note: you need to set the reporter to
spec.This is also because
test-runner-output.mjsis intended for 'e2e' testing of the test runner's output under specific circumstances.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmarchini @cjihrig Thank you so much for your help. It wouldn't be possible without your help.


I ran test cases and here is output
I printed raw output in test case and here is output
and here is test case:
If it looks good to you I can commit it and you can review the whole PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pmarchini @cjihrig @jasnell @MoLow
Could we please land this PR if no change is required??