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

Code Coverage Enhancement #3781

Open
jmayclin opened this issue Jan 26, 2023 · 0 comments
Open

Code Coverage Enhancement #3781

jmayclin opened this issue Jan 26, 2023 · 0 comments
Labels

Comments

@jmayclin
Copy link
Contributor

jmayclin commented Jan 26, 2023

Problem:

PR #3759 Adds basic code coverage support. There are a number of avenues along which we would ultimately like to expand this.

Why Not CodeCov.io

We aren't going with CodeCov.io because the "any line without complete branch coverage does not count as tested" philosophy does not fit well with our codebase. We use a large number of very paranoid safety checking macros. We do not expect all of the branches to be exercised on these macros

ASSERT_NOT_NULL(thing)
routine_operation(thing) <- this is well written code that doesn't make things null
ASSERT_NOT_NULL(thing) <- complain about missing branch coverage

An example of a patten that codecov.io abhors.

That having been documented, what features would we like to add to our system

Fail on CI on certain parameters

The current approach doesn't fail unless the actual build process fails. We should enforce some set of minimum criteria like minimum line coverage per file, etc, and fail the CI job if those status's are not met.

To do this, the summary json exporting functionality is probably quite useful.

llvm-cov export build/lib/libs2n.so \
    -instr-profile=merged.profdata \
    -format=text \
    -summary-only \
    > coverage_summary.json

More Visibly Surface Coverage numbers

Having to click through logs to get to the report is not an ideal user experience. Preferably, we would have a custom PR check that would output coverage information as well as the URL for the full report.

We probably don't want to just comment on the PR, as that is likely to be too noisy if we are commenting for each new commit.

Code Coverage Diff

We should investigate the --baseline file option available for genhtml. It might be possible to

  1. calculate coverage for current commit
  2. calculate coverage for origin/main
  3. diff the coverage
  4. upload coverage to a useful place / do something useful with it

Support detailed coverage reports

There is a --show-details option for genhtml to show more details

   --show-details
     Generate detailed directory view.
         When  this option is enabled, genhtml generates two versions of each file view.
         One containing the standard information plus a link to  a  "detailed"  version.
         The  latter additionally contains information about which test case covered how
         many lines of each source file.

It would be nice if we could use this. Currently it doesn't work because I think llvm discards the trace information when we merge it into a single .info file. It might be possible to

  1. generate profiles for each unit test
  2. "merge" each unit test to it's own .info file
  3. pass all of those to genhtml

There doesn't seem to be a good way to get the coverage profile outputted to a <test_name> file, but when tests are run single threaded the order seems to be stable, so that can likely be leverage to figure out which profile belongs to which test.

Consolidate Fuzz & Unit coverage handling

We use LLVM source based code coverage in two places, fuzzing and unit tests. The fuzzing is currently only run through make, but when those tests are ported over to cmake we should try and consolidate the report handling.

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

No branches or pull requests

2 participants