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: add support for coverage thresholds #54429

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev 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-bot nodejs-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
@RedYetiDev
Copy link
Member Author

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

ljharb commented Aug 17, 2024

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

@RedYetiDev
Copy link
Member Author

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

@RedYetiDev RedYetiDev 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
@RedYetiDev RedYetiDev marked this pull request as ready for review August 18, 2024 00:43
@RedYetiDev
Copy link
Member Author

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.

@RedYetiDev RedYetiDev 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
Copy link
Contributor

The notable-change PRs 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

@RedYetiDev
Copy link
Member Author

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

@mcollina mcollina 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.

@RedYetiDev
Copy link
Member Author

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

@ShogunPanda ShogunPanda 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

@mcollina mcollina 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

@RedYetiDev
Copy link
Member Author

RedYetiDev 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

MoLow commented Aug 20, 2024

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

@MoLow MoLow 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)

@nodejs-github-bot
Copy link
Collaborator

@cjihrig cjihrig 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-bot nodejs-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 <38299977+RedYetiDev@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/54429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
--------------------------------------------------------------------------------
   ⚠  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-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 23, 2024
@cjihrig cjihrig 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-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024
@nodejs-github-bot nodejs-github-bot merged commit 9edf4a0 into nodejs:main Aug 23, 2024
51 checks passed
@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 <marcoippolito54@gmail.com>
PR-URL: #54429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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
@RafaelGSS RafaelGSS 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 <marcoippolito54@gmail.com>
PR-URL: #54429
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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
@targos targos 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. cli Issues and PRs related to the Node.js command line interface. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. coverage Issues and PRs related to native coverage support. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues 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