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

Add more options to fail on insufficient coverage #170

Closed
michaelvlach opened this issue May 28, 2022 · 4 comments · Fixed by #173
Closed

Add more options to fail on insufficient coverage #170

michaelvlach opened this issue May 28, 2022 · 4 comments · Fixed by #173
Labels
C-enhancement Category: A new feature or an improvement for an existing one

Comments

@michaelvlach
Copy link
Contributor

Enforcing code coverage is good and the current option --fail-under-lines is a way to do it but unfortunately rather crude one. Percentage is relative and may change due to amount of code changing vis-a-vis covered code. The llvm-cov report does provide absolute values of covered and uncovered for each of the 4 categories:

  • function
  • line
  • region
  • branch

The better way would therefore be to fail coverage on uncovered absolute values rather than percentages which is volatile. That way if new code is added that is fully covered the coverage will pass. However when new code is not fully covered it will also require changing of the threshold(s) which is good thing - it might be justified of course but nevertheless should be visible.

I propose the following options:

--fail-uncovered-functions
--fail-uncovered-lines
--fail-uncovered-branches
--fail-uncovered-regions
@taiki-e taiki-e added the C-enhancement Category: A new feature or an improvement for an existing one label May 29, 2022
@taiki-e
Copy link
Owner

taiki-e commented May 29, 2022

I think referencing absolute values instead of percentages is an interesting idea. I'm fine with adding this as an experimental option.

However when new code is not fully covered it will also require changing of the threshold(s) which is good thing

I think this is not necessarily a good thing when there are multiple PRs, because it easily causes merge conflicts.

@michaelvlach
Copy link
Contributor Author

I think referencing absolute values instead of percentages is an interesting idea. I'm fine with adding this as an experimental option.

Thanks!

However when new code is not fully covered it will also require changing of the threshold(s) which is good thing

I think this is not necessarily a good thing when there are multiple PRs, because it easily causes merge conflicts.

This would happen only if you had multiple PRs that are all worsening coverage. Something nobody would want I presume. And if it happens regularly then enforcing coverage is probably not what is wanted.

@taiki-e
Copy link
Owner

taiki-e commented May 29, 2022

This would happen only if you had multiple PRs that are all worsening coverage.

Note that this can also be caused by PRs that improve coverage (if the current coverage is not 100%).

  • PR A adds a test for function A1 and reduces uncovered lines from 5 to 3.
  • PR B adds a test for function B1 and reduces uncovered lines from 5 to 4.
  • Merging both PRs reduces uncovered lines from 5 to 2, but there is a merge conflict.

EDIT: see #170 (comment)

@taiki-e
Copy link
Owner

taiki-e commented May 29, 2022

Ah, sorry, I somehow misunderstood that the values passed to those options are concrete values. It is thresholds, so my comment above is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A new feature or an improvement for an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants