Skip to content

Conversation

@avivkeller
Copy link
Member

@avivkelleravivkeller commented Aug 17, 2024

Fixes#48739

This PR adds support for coverage thresholds.

The following new CLI flags are added:

  • --test-coverage-branches
  • --test-coverage-functions
  • --test-coverage-lines

Notable Change

Node.js now supports requiring code coverage to meet a specific threshold before the process exits successfully. To use this feature, you need to enable the --experimental-test-coverage flag.

You can set thresholds for the following types of coverage:

  • Branch coverage: Use --test-coverage-branches=<threshold>
  • Function coverage: Use --test-coverage-functions=<threshold>
  • Line coverage: Use --test-coverage-lines=<threshold>

<threshold> should be an integer between 0 and 100. If an invalid value is provided, a TypeError will be thrown.

If the code coverage fails to meet the specified thresholds for any category, the process will exit with code 1.

For instance, to enforce a minimum of 80% line coverage and 60% branch coverage, you can run:

$ node --experimental-test-coverage --test-coverage-lines=80 --test-coverage-branches=60 example.js # start of coverage report # ... # ... # ... # end of coverage report # Error: 53.45% line coverage does not meet the threshold of 80%. # Error: 40.30% branch coverage does not meet the threshold of 60%.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 17, 2024
@avivkeller
Copy link
MemberAuthor

To reviewers, please send as any changes as you'd like. This is still a draft, so I'd love lots and lots of feedback

@ljharb
Copy link
Member

there's always 4 thresholds: branch, function, line, and statements. can statements be added also?

@avivkeller
Copy link
MemberAuthor

there's always 4 thresholds: branch, function, line, and statements. can statements be added also?

I don't believe the Node.js coverage reporter handlers statements

@avivkelleravivkeller added semver-minor PRs that contain new features and should be released in the next minor version. cli Issues and PRs related to the Node.js command line interface. coverage Issues and PRs related to native coverage support. test_runner Issues and PRs related to the test runner subsystem. labels Aug 17, 2024
@avivkelleravivkeller marked this pull request as ready for review August 18, 2024 00:43
@avivkeller
Copy link
MemberAuthor

I realize that this is passing local tests, and is not 'incomplete' per say, so it doesn't hurt to keep it in the review state.

@avivkelleravivkeller added notable-change PRs with changes that should be highlighted in changelogs. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 18, 2024
@github-actions
Copy link
Contributor

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @redyetidev.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@marco-ippolito
Copy link
Member

I gave it a try in the past #51370 and it was blocked

@avivkeller
Copy link
MemberAuthor

I gave it a try in the past #51370 and it was blocked

Looking at yours, it looks like a much smarter way to do this than I did. If that didn't land, I doubt this will.

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was ok then and I'm ok now, but yeah, this should probably not land unless someone wants to move it through a @nodejs/tsc vote.

@avivkeller
Copy link
MemberAuthor

I was ok then and I'm ok now, but yeah, this should probably not land unless someone wants to move it through a @nodejs/tsc vote.

+1 with a TSC vote. If the consensus is to ship this feature, I think @marco-ippolito's PR is a better layout of this.

@marco-ippolito
Copy link
Member

I was ok then and I'm ok now, but yeah, this should probably not land unless someone wants to move it through a @nodejs/tsc vote.

+1 with a TSC vote. If the consensus is to ship this feature, I think @marco-ippolito's PR is a better layout of this.

Im happy if you want to takeover

Copy link
Contributor

@ShogunPandaShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tniessen
Copy link
Member

FWIW, there were two previous attempts at this: #51182 and #51370. I am not sure folks are convinced that we want to support this kind of arbitrary threshold.

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statements should be added too

@avivkeller
Copy link
MemberAuthor

avivkeller commented Aug 19, 2024

statements should be added too

IIRC the coverage reporter doesn't support statements (but maybe it should in the future 🤔)

@MoLow
Copy link
Member

there's always 4 thresholds: branch, function, line, and statements. can statements be added also?

@ljharb@mcollina statements coverage are only available when using instrumentation coverage from tools such as isnatbul.
V8 coverage dosent report that, you can see that c8 for instance doesn't have that either

Copy link
Member

@MoLowMoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern with previous attempts was lack of source map support. Now that we have that I thinks this is ok.
Also see my previous comment regarding lack of statements coverage (we might want to document that limitation, but it is a limitation that exists also in the coverage report/event, regardless of thresholds)

@cjihrigcjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Aug 23, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/54429 ✔ Done loading data for nodejs/node/pull/54429 ----------------------------------- PR info ------------------------------------ Title test_runner: add support for coverage thresholds (#54429) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch RedYetiDev:coverage-threshold -> nodejs:main Labels semver-minor, notable-change, cli, author ready, coverage, commit-queue-squash, test_runner Commits 2 - test_runner: add support for coverage thresholds - (skip if inspector disabled) Committers 1 - RedYetiDev <[email protected]> PR-URL: https://github.com/nodejs/node/pull/54429 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54429 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - (skip if inspector disabled) ℹ This PR was created on Sat, 17 Aug 2024 22:18:01 GMT ✔ Approvals: 5 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54429#pullrequestreview-2247072628 ✔ - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/54429#pullrequestreview-2244734218 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/54429#pullrequestreview-2246968179 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/54429#pullrequestreview-2255436655 ✔ - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/54429#pullrequestreview-2255452813 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-08-23T13:36:18Z: https://ci.nodejs.org/job/node-test-pull-request/61378/ - Querying data for job/node-test-pull-request/61378/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10529382777

@nodejs-github-botnodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 23, 2024
@cjihrigcjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 23, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024
@nodejs-github-botnodejs-github-bot merged commit 9edf4a0 into nodejs:mainAug 23, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 9edf4a0

RafaelGSS pushed a commit that referenced this pull request Aug 25, 2024
Co-Authored-By: Marco Ippolito <[email protected]> PR-URL: #54429 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 25, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 PR-URL: TODO
@RafaelGSSRafaelGSS mentioned this pull request Aug 25, 2024
RafaelGSS added a commit that referenced this pull request Aug 25, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 PR-URL: #54560
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
Co-Authored-By: Marco Ippolito <[email protected]> PR-URL: #54429 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 30, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Aug 30, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Aug 31, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Aug 31, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Sep 1, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Sep 2, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Sep 3, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
RafaelGSS added a commit that referenced this pull request Sep 3, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560
@targostargos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) nodejs#54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) nodejs#54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) nodejs#54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) nodejs#54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) nodejs#53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) nodejs#53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) nodejs#54394 PR-URL: nodejs#54560
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) nodejs#54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) nodejs#54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) nodejs#54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) nodejs#54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) nodejs#53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) nodejs#53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) nodejs#54394 PR-URL: nodejs#54560
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.cliIssues and PRs related to the Node.js command line interface.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.coverageIssues and PRs related to native coverage support.dont-land-on-v20.xPRs that should not land on the v20.x-staging branch and should not be released in v20.x.notable-changePRs with changes that should be highlighted in changelogs.semver-minorPRs that contain new features and should be released in the next minor version.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New CLI flag to exit with an error status if test coverage is incomplete

10 participants

@avivkeller@nodejs-github-bot@ljharb@marco-ippolito@tniessen@MoLow@cjihrig@mcollina@ShogunPanda@targos