-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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: added coverage threshold support for tests #51182
Conversation
Review requested:
|
I think this should also set the exit code to a nonzero value such that CI systems will mark the job as failed. |
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 agree on the exit code; also, coverage always has 4 metrics, lines, functions, branches, and statements.
working on it 👍🏼 |
bfcfe72
to
52100c7
Compare
It’s still missing “statements”. |
I think we don't collect statement coverage summary. @MoLow please confirm about statement summary here and how can we support that. |
PTAL @nodejs/test_runner |
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.
Overall LGTM, should add documentation though
Thanks for review 🙌🏼 . will add documentation.
@atlowChemi Any idea about this? |
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.
There's a reason we don't use such a threshold in this repository -- it tends to be a fragile indicator.
On a side note, the first commit message does not adhere to our requirements and must be amended.
doc/api/cli.md
Outdated
To Specify the minimum percentage threshold value for code coverage. | ||
Used with `--experimental-test-coverage` flag. If the minimum threshold | ||
value is not met, the test will exit with status code 1. |
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.
The documentation doesn't say anything about what coverage metric is meant here.
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.
Indeed; typically in code coverage there are 4 metrics (statements remains missing), and all 4 are independently able to be set to a threshold.
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.
Really good work, I'm not convinced we need/want the feature but the code and everyting else looks solid <3
Thanks @benjamingr, Additionally, it might be beneficial to consider adding a coverage option within the test_runner/run() function. WDYT @nodejs/test_runner |
FWIW, Node.js could always emit coverage data in some standardized format that could then be consumed by a different tool (e.g., codecov or so). Also, "100% code coverage" is a somewhat odd metric. It by no means guarantees the absence of bugs yet is almost impossible to achieve in many cases. |
node already does that https://nodejs.org/api/test.html#event-testcoverage |
This appears to have stalled out and now has conflicts. I'm going to close this for now. |
fixes #48739
Added support for coverage threshold comparison.
example:
code:
output:
cmd:
./node --experimental-test-coverage --experimental-minimal-test-coverage 80 index.mjs
code:
output: