taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
933 stars 57 forks source link

Add more options to fail on insufficient coverage #170

Closed michaelvlach closed 2 years ago

michaelvlach commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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%).

EDIT: see https://github.com/taiki-e/cargo-llvm-cov/issues/170#issuecomment-1140420839

taiki-e commented 2 years ago

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