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

jest-reporters: Use global coverage thresholds as high watermarks #9416

Merged
merged 7 commits into from
Jan 16, 2020

Conversation

Zache
Copy link
Contributor

@Zache Zache commented Jan 16, 2020

Summary

When Istanbul generates the some reports (like html & lcov) it uses watermarks to to provide visual feedback with colors. The default watermarks are 50% and 80%. Which in the html reports means that coverage under 50% is red and coverage over 80% is green. But we have a coverage threshold of 100%, so using the report to find any file with coverage between 80% and 100% can be quite difficult as @nemoDreamer describes here: #6578.

My solution is to provide istanbul with watermarks where the high watermarks are set to the global coverage threshold of each coverage category (branches, functions, lines, and statements)

Test plan

I added unit tests for my use case and for not having any global coverage threshold and tested my changes using yarn link

coverage

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@codecov-io
Copy link

codecov-io commented Jan 16, 2020

Codecov Report

Merging #9416 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9416      +/-   ##
=========================================
+ Coverage   64.77%   64.8%   +0.02%     
=========================================
  Files         280     281       +1     
  Lines       11988   11998      +10     
  Branches     2957    2959       +2     
=========================================
+ Hits         7765    7775      +10     
  Misses       3591    3591              
  Partials      632     632
Impacted Files Coverage Δ
packages/jest-reporters/src/coverage_reporter.ts 51.21% <ø> (ø) ⬆️
packages/jest-reporters/src/get_watermarks.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e5db14...a5a4a0e. Read the comment docs.

@SimenB SimenB merged commit 17f6c83 into jestjs:master Jan 16, 2020
@SimenB
Copy link
Member

SimenB commented Jan 16, 2020

Thank you!

@jeysal
Copy link
Contributor

jeysal commented Jan 16, 2020

Thanks so much @Zache, I love smart ideas like this that link together existing functionality of tools! 🎉
I realize it's a bit late for a review, but just one question: Have you checked what happens if the threshold / high watermark is lower than the default 50% / low watermark? Just wanna make sure it doesn't cause weird behavior.

@Zache
Copy link
Contributor Author

Zache commented Jan 17, 2020

@jeysal Thank you for your kind feedback. And regarding your question I don't think that would be a problem, I guess you would have my original problem in reverse but nothing should break.

@jeysal
Copy link
Contributor

jeysal commented Jan 17, 2020

Right so based on that it looks like the custom value wouldn't matter then, it'll be red below 50%/low and green above that.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants