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

Test runner coverage thresholds must show up in red #4504

Open
nicholaswmin opened this issue Nov 18, 2024 · 2 comments
Open

Test runner coverage thresholds must show up in red #4504

nicholaswmin opened this issue Nov 18, 2024 · 2 comments

Comments

@nicholaswmin
Copy link

nicholaswmin commented Nov 18, 2024

  • since v22.8

Subsystem

Test Runner, all OS, all versions

Description

Another small visual tweak.

The spec reporter must show errors on coverage thresholds in red.1
As it is right now, visually - it looks like the tests pass.

I can imagine it can create a lot of back/forth in code reviews.

You'll only realise there's a problem when the CI cycle fails because it correctly exits 1. That sucks because debugging in CI is as pleasant as watching paint dry.

I'm talking about the last line of this:

ℹ duration_ms 60.134125
ℹ start of coverage report
ℹ test/exceptions.test.js | 100.00 |   100.00 |  100.00 | 
ℹ ------------------------------------------------------------------------
ℹ all files               | 100.00 |   100.00 |   95.08 |
ℹ ------------------------------------------------------------------------
ℹ end of coverage report
ℹ Error: 95.08% function coverage does not meet threshold of 100%. <-- this must be more pronounced. It's in blue right now.

here's how it really looks:

screenshot of coverage report showing errors in blue colour

Minimal Reproduction

node --test foo.test.js --experimental-test-coverage --test-coverage-lines=100

Output

No response

Footnotes

  1. I'm aware of the accesibility concerns about using color, not sure how it's dealt or if at all.

@avivkeller avivkeller self-assigned this Nov 18, 2024
@avivkeller
Copy link
Member

SGTM, I'll open a PR in a few hours

@nicholaswmin
Copy link
Author

nicholaswmin commented Dec 16, 2024

hm.. this is interesting:

headers are now red even though all pass..

test results screenshot of terminal showing coverage headers in red color

these are generic design principles:

  • Coloring labels tends to create ambiguity:
    • If there's a problem or a danger, just the quantity (value) itself should be colored.
    • Coloring a label creates ambiguity "there a problem in general with the facility itself."
      Coloring just the quantity sends a clear message that it's the quantity itself that's creating the issue.

In this case it's flat out wrong.
Both the tests and the coverage thresholds pass.
No yellows, orange, or red should appear in the output.

Additionally, that "blue" generic colouring creates issues:

a) it's unnecessary.
I get that colours make the CLI a bit less boring but they
desensitize the user to important messages that could be emphasized by colors.
b) right now it also obscures diagnostics logging:

t.diagnostics('foobar') is also printed in that blue colour but it ends up blending in with the existing blue, ending up hidden.

bit confused here - none of the PRs have been merged so no idea where this change came from

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants