-
Notifications
You must be signed in to change notification settings - Fork 121
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
switch to source based coverage #1293
Conversation
bcef6d8
to
caa4e5b
Compare
005df59
to
b2a5939
Compare
@dconnolly okay this is ready to merge probably. I've still got an ongoing ticket in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yaahc I would merge but for it looks like the coverage reported via this is missing the code executed for some of our tests such as unit/proptests such as in zebra_chain:
#main (tarpaulin): https://codecov.io/gh/ZcashFoundation/zebra/src/main/zebra-chain/src/sprout/keys.rs
this branch (-Zinstrument-coverage): https://app.codecov.io/gh/ZcashFoundation/zebra/compare/1293/tree/zebra-chain/src/sprout/keys.rs
43e6fa4
to
6a5da07
Compare
alright @dconnolly, I think i figured out what was causing it to lose coverage for new coverage: https://codecov.io/gh/ZcashFoundation/zebra/src/75c202891cb996b8ecc090f5b4360d87ca93aeec/zebra-chain/src/sprout/keys.rs old coverage: https://codecov.io/gh/ZcashFoundation/zebra/src/main/zebra-chain/src/sprout/keys.rs It seems to be getting a more accurate report now, except for on the |
8f451fa
to
eb4398b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One q: zebra-utils contains mostly tools that are useful to a general zcash/zebra user, vs our coverage script, which is mostly useful only to developers. If it were shorter I would suggest inlining it in the workflow file but I understand why it's not. Is there a better location for it, like a plain scripts
or utils
maybe?
not sure, I think we should probably discuss this as a team, I'll put an item on the agenda for our next zebra sync. |
Motivation
Source-based coverage is more accurate and useful than 'gcov-based' coverage that we currently rely on:
Above: A comparison of the gcov (left) and source-based coverage (right) results. gcov highlights skipped lines, marked with #####, while source-based coverage highlights exact regions of code that were skipped. Note that on line 30, one boolean subexpression is short-circuited. This is surfaced by source-based coverage but not gcov.
https://blog.rust-lang.org/inside-rust/2020/11/12/source-based-code-coverage.html
Solution
This PR switches to using
-Zinstrument-coverage
to calculate coverage using llvm's source based coverage instrumentation. We then upload that report in two different formats. First, we create an html report usingllvm-cov show
, which includes the exact coverage report, including details down to the specific regions in a line that are covered, and coverage per expansion of generic types.A second format is then used to upload the coverage to
codecov
, this is because codecov doesn't currently support region based coverage results, so we have to fall back to an older format that they do understand. This discards some information and shows lines as covered when they're only partially covered, but should still be useful for tracking statistics and for ease of access.The code in this pull request has:
Review
@dconnolly
Related Issues
Follow Up Work