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: allow coverage threshold to be enforced #25675

Closed
wants to merge 1 commit into from
Closed

test: allow coverage threshold to be enforced #25675

wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jan 24, 2019

If COV_ENFORCE_THRESHOLD is set, tests run in coverage mode will
exit with an error, if line coverage is below the percentage
threshold.

will rebase against #25671, once it's ready to land.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@bcoe bcoe added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. blocked PRs that are blocked by other issues or PRs. labels Jan 24, 2019
@bcoe bcoe requested review from refack and mhdawson January 24, 2019 02:17
@nodejs-github-bot
Copy link
Collaborator

@bcoe build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2365/pipeline

@refack
Copy link
Contributor

refack commented Jan 24, 2019

Thinking about the next step of enabling this by default in CI...
Should this get the value from the repo (a file we update from time to time)? Maybe read dynamically from https://coverage.nodejs.org/?
Should we run it daily (and notify some derived list of authors)? Or like "Codecov" run it for every PR?

@refack refack added the coverage Issues and PRs related to native coverage support. label Jan 24, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Jan 24, 2019

@refack, I think for starters @mhdawson was planning on making this a configuration variable, which would be set on the build server he's configuring here.

@mhdawson
Copy link
Member

Will test this out on the build I'm trying out and the approve if looks good. Would like #25671 to land first as well.

@mhdawson
Copy link
Member

#25671 landed, if you can rebase I'll run it against the coverage sanity job just to double check and then we'd be good to land.

@mhdawson
Copy link
Member

I have found one other test that is failing under coverage on the docker ubuntu 16 machines we'll use for this job and am testing a PR now, but it should not conflict with this one once rebased.

@bcoe bcoe force-pushed the thresholds branch 2 times, most recently from c9408bf to 79ffeec Compare January 30, 2019 23:49
@bcoe
test: allow coverage threshold to be enforced
62decf4 
If COV_ENFORCE_THRESHOLD is set, tests run in coverage mode will
exit with an error, if line coverage is below the percentage
threshold.
@bcoe
Copy link
Contributor Author

bcoe commented Jan 31, 2019

@mhdawson rebased 👍

@mhdawson
Copy link
Member

run of coverage sanity job we are working on with this update: https://ci.nodejs.org/view/all/job/node-test-commit-coverage/24/

@mhdawson
Copy link
Member

New CI with naming fixed: https://ci.nodejs.org/view/all/job/node-test-commit-coverage/26/

mhdawson
mhdawson approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM as long as CI runs pass

@bcoe
Copy link
Contributor Author

bcoe commented Jan 31, 2019 •
edited
Loading

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

@bcoe bcoe added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 1, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Feb 1, 2019

Landed in a861add

@bcoe bcoe closed this Feb 1, 2019
@bcoe bcoe deleted the thresholds branch February 1, 2019 18:21
bcoe added a commit that referenced this pull request Feb 1, 2019
@bcoe
test: allow coverage threshold to be enforced
a861add 
If COV_ENFORCE_THRESHOLD is set, tests run in coverage mode will
exit with an error, if line coverage is below the percentage
threshold.

PR-URL: #25675
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 1, 2019
@bcoe @addaleax
test: allow coverage threshold to be enforced
704a440 
If COV_ENFORCE_THRESHOLD is set, tests run in coverage mode will
exit with an error, if line coverage is below the percentage
threshold.

PR-URL: #25675
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@refack refack refack 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. build Issues and PRs related to build files or the CI. coverage Issues and PRs related to native coverage support. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bcoe @nodejs-github-bot @refack @mhdawson