Exclusion Patterns for Lines and Branches

[jcdickinson]

Adds line and branch exclusion, following the names of the vars in lcovrc. The lines are removed during path rewriting (as the full path is resolved during this phase). Resolves #261

Dangling regions at the end of files are permitted for e.g. mod tests {

Also resolves some failing tests as well as compilation errors on Windows (see individual commits).

[codecov-io]

Codecov Report

Merging #416 into master will increase coverage by 0.96%.
The diff coverage is 82.30%.

@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
+ Coverage   68.66%   69.63%   +0.96%     
==========================================
  Files          12       13       +1     
  Lines        4340     4521     +181     
  Branches      977     1009      +32     
==========================================
+ Hits         2980     3148     +168     
+ Misses        604      591      -13     
- Partials      756      782      +26     

Impacted Files	Coverage Δ
src/lib.rs	71.93% <ø> (-0.15%)	⬇️
src/main.rs	73.50% <76.92%> (+0.51%)	⬆️
src/path_rewriting.rs	73.67% <81.41%> (+1.20%)	⬆️
src/file_filter.rs	86.88% <86.88%> (ø)
src/defs.rs	15.78% <0.00%> (-1.52%)	⬇️
src/html.rs	0.00% <0.00%> (ø)
src/parser.rs	65.64% <0.00%> (+0.10%)	⬆️
src/producer.rs	85.83% <0.00%> (+0.18%)	⬆️
... and 2 more

---

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 1040420...7e12d8c. Read the comment docs.

[calixteman]

It's an interesting patch (thanks for doing it)
Would it be possible to multi-thread the filter part ?

[jcdickinson]

Thanks for all the great feedback!

Would it be possible to multi-thread the filter part ?

I had a go at it using Rayon, adding inline comments in a few moments.

[jcdickinson]

FYI: TravisCI doesn't seem to be reporting back on successful builds: https://travis-ci.org/github/mozilla/grcov/builds/670724791

[rivy]

I'm new to gcov, grcov, gcovr, lcov, ...

Could you suggest how this would best be used?

For example, what would be a good method of ignoring assert_eq!(...) incomplete branch coverage?

It looks like the lcov method (used fairly widely) is to use LCOV_EXCL_LINE (or LCOV_EXCL_BR_LINE for the assert) somewhere on the excluded line. It doesn't look like the tools otherwise require any extra option flags.

This patch looks like it's generic without suggesting a similar default implementation. Would including the lcov "standard" keywords by default be a good idea?

ref: https://stackoverflow.com/questions/3555083/how-do-i-tell-gcov-to-ignore-un-hittable-lines-of-c-code
ref: https://manpages.debian.org/jessie/lcov/geninfo.1.en.html

And, thanks for the patch!

[jcdickinson]

And, thanks for the patch!

Always a pleasure!

For example, what would be a good method of ignoring assert_eq!(...) incomplete branch coverage?

In some projects, tests are contained in different directories entirely - those projects already have usable ignores in grcov (just ignore the entire file with the existing file exclusion patterns). The other approach that seems widespread in Rust is to have a mod tests at the bottom of the file. In that scenario, you could ignore every line in your tests by passing --excl-br-start "mod tests \{" --excl-start "mod tests \{". That would begin a range in each file starting at mod tests { and only terminating at the end of the file (because there is no associated -stop argument).

In addition, the derive Rust attribute (e.g. #[derive(Debug, Eq, PartialEq)]) can usually be assumed to be working code, because there are tests in the Rust stdlib that make sure that this derive macro works. This is where the per-line options are relevant --excl-br-line "#\[derive\(" --excl-line "#\[derive\(". This fails to account for lines with good macro hygiene (i.e. fields in structs are still counted as uncovered), but it's a start.

Here's a coverage report demonstrating both of the above, and the Github workflow configuration file that contains the regex patterns.

I probably wouldn't ignore assert_eq! (as that may be used in non-test code), but you could ignore it by passing --excl-br-line "assert_eq!" --excl-line "assert_eq!".

Having two parameters is a bit redundant, but I assumed there is a good reason why lcov differentiates between ignored lines and branches.

It looks like the lcov method (used fairly widely) is to use LCOV_EXCL_LINE somewhere on the excluded line. It doesn't look like the tools otherwise require any extra option flags.

lcov allows you to customize this with a machine-wide lcovrc file - which isn't a great solution in my opinion (as all developer machines have to either have no lcovrc file, or have to have an lcovrc file that agrees for all projects that they work on). It seems to be a better solution to pass these on the CLI so that each Git/whatever repo can decide how best to approach ignores (and use intelligent ignores for idiomatic patterns in the language that they are using, such as my above examples). The names of the CLI args closely follow the names of the values in lcovrc.

If you wanted to use this ignore pattern, you could pass --excl-line LCOV_EXCL_LINE to grcov.

This patch looks like it's generic without suggesting a similar default implementation. Would including the lcov "standard" keywords by default be a good idea?

lcovrc does default to what you indicated, but that means you have ignore patterns whether you want them or not. This smells like an anti-feature to me, but I'm happy to concede to the principle of least surprise and go with what lcov does.

I'll rectify the remainder of your feedback tomorrow. Thanks!

[rivy]

I probably wouldn't ignore assert_eq! (as that may be used in non-test code), but you could ignore it by passing --excl-br-line "assert_eq!" --excl-line "assert_eq!".

So, from some google research and your feedback, it looks like I should just use something like --excl-br-line "assert(_eq|_ne)?!" when calling grcov. That would still count the line(s) for coverage but ignore that the internal branch is not taken (avoiding coverage warnings).

Sound correct?

In addition, the derive Rust attribute (e.g. #[derive(Debug, Eq, PartialEq)]) can usually be assumed to be working code, because there are tests in the Rust stdlib that make sure that this derive macro works. This is where the per-line options are relevant --excl-br-line "#\[derive\(" --excl-line "#\[derive\(". This fails to account for lines with good macro hygiene (i.e. fields in structs are still counted as uncovered), but it's a start.

Would the --excl-br-line "#\[derive\(" be necessary if the line is already ignored with --excl-line "#\[derive\(" ?

Having two parameters is a bit redundant, but I assumed there is a good reason why lcov differentiates between ignored lines and branches.

I'm assuming that we can use the two to discriminate between lines to be completely ignored and lines which need to be counted for coverage but ignore that branches within the line aren't taken.

lcovrc does default to what you indicated, but that means you have ignore patterns whether you want them or not. This smells like an anti-feature to me, but I'm happy to concede to the principle of least surprise and go with what lcov does.

I don't have enough experience here to comment with any authority. Maybe others with some experience could chime in?

I do like drop-in functionality that re-uses prior art and experience. If the thought is that the LCOV_... signals would be used by default, a --no-excl-defaults option could be added to remove the functionality when desired by the user.

[jcdickinson]

Sorry for the late response on this. It has been a crazy week for me.

So, from some google research and your feedback, it looks like I should just use something like --excl-br-line "assert(_eq|_ne)?!" when calling grcov.

That is correct.

Would the --excl-br-line "#[derive(" be necessary if the line is already ignored with --excl-line "#[derive(" ?

Yes, it looks like lcov does this.

If the thought is that the LCOV_... signals would be used by default, a --no-excl-defaults option could be added to remove the functionality when desired by the user.

Happy to do this, if requested.

[calixteman]

Please just fix the nits and it should be ok for merging.

[calixteman]

Thx a lot

[calixteman]

@jcdickinson could you update the README to explain how to use this new feature ?

[TimDiekmann]

I have used --excl-br-line "(#\\[derive\\()|(debug_assert)" which also filters debug assertions, which should never fail, even in production code. This might be added to the README as well.

[marco-c]

@TimDiekmann++

[TimDiekmann]

Actually I reverted it to "#\\[derive\\(" and added -Cdebug-assertions=no to RUSTFLAGS instead.