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.

Sign up for GitHub

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

Jump to bottom

test_runner: report coverage thresholds in test:coverage #54813

Merged
merged 3 commits into from
Sep 13, 2024
Merged

test_runner: report coverage thresholds in test:coverage #54813

merged 3 commits into from
Sep 13, 2024

Conversation

RedYetiDev
Copy link
Member

Fixes #54812

Adds a thresholds object to test:coverage's summary property.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Sep 6, 2024
@RedYetiDev RedYetiDev added the coverage Issues and PRs related to native coverage support. label Sep 6, 2024
@RedYetiDev RedYetiDev mentioned this pull request Sep 6, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Sep 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.90%. Comparing base (ce19715) to head (45db682).
Report is 224 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/test.js 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54813      +/-   ##
==========================================
+ Coverage   87.60%   87.90%   +0.29%     
==========================================
  Files         650      651       +1     
  Lines      182943   183351     +408     
  Branches    35399    35709     +310     
==========================================
+ Hits       160270   161168     +898     
+ Misses      15936    15465     -471     
+ Partials     6737     6718      -19
Files with missing lines Coverage Δ
lib/internal/test_runner/coverage.js 20.69% <100.00%> (+0.97%) ⬆️
lib/internal/test_runner/test.js 96.93% <0.00%> (ø)

... and 87 files with indirect coverage changes

mcollina
mcollina approved these changes Sep 7, 2024
View reviewed changes
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.

lgtm

@RedYetiDev RedYetiDev mentioned this pull request Sep 7, 2024
Merged
@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 7, 2024 •
edited
Loading

This PR also fixes an issue that (I believe) is causing #53937 to have failing tests.

@RedYetiDev RedYetiDev added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 8, 2024
cjihrig
cjihrig requested changes Sep 8, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Requesting changes since this has the approvals to land.

doc/api/test.md Outdated Show resolved Hide resolved
lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev force-pushed the report-coverage-thresholds branch 2 times, most recently from c411a61 to e157067 Compare September 8, 2024 20:54
cjihrig
cjihrig approved these changes Sep 8, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

doc/api/test.md Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@RedYetiDev @cjihrig
Update doc/api/test.md
f86abda 
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 8, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 9, 2024

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/10782754798

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62263/

@RedYetiDev RedYetiDev removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Sep 10, 2024
@RedYetiDev
Copy link
Member Author

The CI appears to have been aborted, can a new one be started in its place?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62367/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62375/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62384/

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Sep 13, 2024 •
edited
Loading

CI is 🟢

@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 Sep 13, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 13, 2024
@nodejs-github-bot nodejs-github-bot merged commit 99bbf80 into nodejs:main Sep 13, 2024
56 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 99bbf80

RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@RedYetiDev @RafaelGSS
test_runner: report coverage thresholds in test:coverage
5fdab31 
PR-URL: #54813
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
@RedYetiDev
Copy link
Member Author

Is this semver-minor and/or notable change? I know it's being released in v22.9.0, but it does add new information, so I wanted to be sure.

RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024
@RedYetiDev @RafaelGSS
test_runner: report coverage thresholds in test:coverage
d0ab1b1 
PR-URL: #54813
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024
@RedYetiDev @RafaelGSS
test_runner: report coverage thresholds in test:coverage
a52c199 
PR-URL: #54813
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 17, 2024
@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 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@RedYetiDev @louwers
test_runner: report coverage thresholds in test:coverage
ac53a9f 
PR-URL: nodejs#54813
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rozzilla rozzilla rozzilla left review comments

@atlowChemi atlowChemi atlowChemi approved these changes

@MoLow MoLow MoLow approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. 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
Milestone
No milestone
9 participants
@RedYetiDev @nodejs-github-bot @mcollina @jasnell @cjihrig @rozzilla @MoLow @atlowChemi @targos