test_runner: print coverage threshold errors in red by avivkeller · Pull Request #55911 · nodejs/node
| 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} ` + |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In the coverage table, colors are used to show whether something has a close to 100% coverage or not
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ☹️ We should not do that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should add a level parameter in diagnostics (i.e debug/info/warn/error) so reporters can implement coloring or other things
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, this PR isn’t needed, and a new one should be open implementing that, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes