test_runner: print coverage threshold errors in red#55911
test_runner: print coverage threshold errors in red#55911avivkeller wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55911 +/- ##
==========================================
- Coverage 88.49% 88.00% -0.50%
==========================================
Files 653 653
Lines 187735 187739 +4
Branches 36181 35877 -304
==========================================
- Hits 166141 165223 -918
- Misses 14819 15700 +881
- Partials 6775 6816 +41
|
| reporter.diagnostic(nesting, loc, `Error: ${NumberPrototypeToFixed(actual, 2)}% ${name} coverage does not meet threshold of ${threshold}%.`); | ||
| reporter.diagnostic(nesting, | ||
| loc, | ||
| `${red}Error: ${NumberPrototypeToFixed(actual, 2)}% ${name} ` + |
There was a problem hiding this comment.
Do we emit colors in the reporter events anywhere else? Doesn't doing so defeat the purpose of having a generic reporter interface?
I am in favor of highlighting this text in reporters that use colors though.
There was a problem hiding this comment.
Yes. In the coverage table, colors are used to show whether something has a close to 100% coverage or not
There was a problem hiding this comment.
Oh
There was a problem hiding this comment.
Yes. In the coverage table, colors are used to show whether something has a close to 100% coverage or not
If I'm not mistaken we colorise during the reporting, not directly in the emit.
I think that the problem here is that we shouldn't colorise the content of the emit
There was a problem hiding this comment.
maybe we should add a level parameter in diagnostics (i.e debug/info/warn/error) so reporters can implement coloring or other things
There was a problem hiding this comment.
In that case, this PR isn’t needed, and a new one should be open implementing that, right?
| const { fileURLToPath } = require('internal/url'); | ||
| const { availableParallelism } = require('os'); | ||
| const { innerOk } = require('internal/assert/utils'); | ||
| const { red, reset } = require('internal/util/colors'); |
There was a problem hiding this comment.
Following the previous comment: util/colors was not used here before because the color is added in the reporting phase
5 participants
Fixes nodejs/help#4504 by printing coverage threshold errors in red.