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

feat(coverage): add option to set coverage threshold #17418

Closed
wants to merge 22 commits into from
Closed

feat(coverage): add option to set coverage threshold #17418

wants to merge 22 commits into from

Conversation

GJZwiers
Copy link
Contributor

@GJZwiers GJZwiers commented Jan 14, 2023

This PR implements part of #9669 by adding a --threshold flag to deno coverage that will cause the program to throw an Error if the collected coverage % is lower. Right now there is no default threshold set (well technically 0%) because I am not sure what it should be or if there should even be a default.

The line, branch and function coverages are compared and the program will throw if any one falls below the set threshold.

@bartlomieju
Copy link
Member

The total coverage % is calculated by getting the average branch and line coverage and applying some weighting to it (20% branches, 80% lines).

Is there a prior art to that?

@GJZwiers
Copy link
Contributor Author

The total coverage % is calculated by getting the average branch and line coverage and applying some weighting to it (20% branches, 80% lines).

Is there a prior art to that?

Nope, I was trying to simulate how Coveralls calculates this but I got it wrong. What they actually use is:
$aggregate\ coverage = (lines\ hit + branches\ hit) / (relevant\ lines + relevant\ branches)$
(source)

Alternatively, nyc/istanbul calculate the individual coverages for lines/branches/functions/statements and then if any of those fall under a threshold it throws. So I guess we could go either way.

@GJZwiers GJZwiers changed the title feat(coverage): add option to throw error when coverage is below threshold feat(coverage): add option to set coverage threshold Jan 14, 2023
@bartlomieju
Copy link
Member

The total coverage % is calculated by getting the average branch and line coverage and applying some weighting to it (20% branches, 80% lines).

Is there a prior art to that?

Nope, I was trying to simulate how Coveralls calculates this but I got it wrong. What they actually use is: aggregate coverage=(lines hit+branches hit)/(relevant lines+relevant branches) (source)

Alternatively, nyc/istanbul calculate the individual coverages for lines/branches/functions/statements and then if any of those fall under a threshold it throws. So I guess we could go either way.

IMO following nyc/istanbul will be least surprising here.

@aapoalas
Copy link
Collaborator

@GJZwiers Will you be working on this to align with nyc/istanbul?

@GJZwiers
Copy link
Contributor Author

@aapoalas I have not had time to work on this, I'm hoping to have some more time in ~1-1.5 months

@@ -654,6 +689,7 @@ pub async fn cover_files(
None => None,
};

let mut report_outputs: Vec<ReportOutput> = vec![];
for script_coverage in script_coverages {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I probably messed up in the merge here and removed the borrow from &script_coverages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I have a spare borrow laying around

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for a magnificent feature!

The code seems good, tests are done, calculation of coverage is implemented according to istanbul which was recommended by @bartlomieju so I see no reason to keep this as a Draft anymore. I'd be willing to merge this as well.

@aapoalas aapoalas marked this pull request as ready for review March 25, 2023 10:01
@GJZwiers
Copy link
Contributor Author

Thanks for approving @aapoalas! I am OK with landing this as is, although I had a few concerns left which is why I kept it as a draft. The current implementation works but is also fairly basic, other implementations have separate flags for each type of coverage e.g. --branches while I used a more general --threshold flag. I can image Deno may want to improve on that in the future, also by somehow collecting statement coverage and including it. Furthermore, I found some comments in an older PR that stated the approach was too tighly coupled to the reporter: #9826 (review), and I think I took more-or-less the same approach. It was two years ago though, so perhaps ideas on that have changed a bit.

@bartlomieju bartlomieju added this to the 1.33 milestone Apr 11, 2023
@GJZwiers GJZwiers closed this Apr 26, 2023
@lowlighter
Copy link
Contributor

lowlighter commented Oct 21, 2023

Any reason for why this was closed without being merged ? It looks close to be shippable

@GJZwiers
Copy link
Contributor Author

@lowlighter I closed this because I decided to do other things, but the code was pretty much ready to be merged. If you want to make a PR and revive this you are welcome to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants