Skip to content

Produce code coverage in CI #3327

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

Merged
merged 4 commits into from
Jul 24, 2020
Merged

Produce code coverage in CI #3327

merged 4 commits into from
Jul 24, 2020

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Jul 6, 2020

Sadly, this can't see a lot of test code, but is better than nothing.

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • automation/all_tests.sh runs to completion and produces no failures
    • Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@f021b56). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3327   +/-   ##
=======================================
  Coverage        ?   56.99%           
=======================================
  Files           ?      230           
  Lines           ?    30204           
  Branches        ?     7321           
=======================================
  Hits            ?    17215           
  Misses          ?     7333           
  Partials        ?     5656           

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 f021b56...e28b987. Read the comment docs.

@thomcc thomcc changed the title Try and run cargo-tarpaulin in CI Produce code coverage in CI Jul 14, 2020
@thomcc
Copy link
Contributor Author

thomcc commented Jul 14, 2020

Note: Coverage is low because of NSS CI build error :( Will dig later.

@thomcc
Copy link
Contributor Author

thomcc commented Jul 21, 2020

Update: I believe the reasons for the failures here are that -Clink-dead-code (required for accurate coverage) is causing code that's usually DCEd to be linked in, and that code has linker errors.

@thomcc
Copy link
Contributor Author

thomcc commented Jul 23, 2020

This is ready for review!

The comments in emit_coverage_info.sh should help explain where we ended up here, but TLDR:

  • Uses grcov, not tarpaulin. This produces worse coverage information, but gives us a path for . It also works on macos... In theory. In practice we need to have Update to core-foundation 0.9.0 & core-foundation-sys 0.8.0? kornelski/rust-security-framework#104 for that to work, and possibly some other patches -- in the meantime if you need to test coverage emission on macos, I've used https://github.com/thomcc/core-foundation-rs/tree/fix-grcov which can be enabled by:

    [patch.crates-io]
    core-foundation = { git = "https://github.com/thomcc/core-foundation-rs", branch = "fix-grcov" }
    core-foundation-sys = { git = "https://github.com/thomcc/core-foundation-rs", branch = "fix-grcov" }

    inside Cargo.toml.

    However, as I found out many times here, it working on macOS is absolutely not a guarantee that it will work in CI -- it probably won't!

  • Doesn't use the recommended -Clink-dead-code arg to rustc, this causes NSS to fail to link, which isn't something I feel like dealing with right now, and doesn't seem to mess our coverage up that badly. This seems like a bridge we can cross in the future.

  • Ignores all errors that happen during testing, under the assumption that these tests must pass in other branches. This is something we could work on in the future, but a lot of them are obscure errors that don't really feel related to what we're doing, so much as the coverage infra.

  • Marks coverage as informational -- in Fix intermittent CI errors rusqlite/rusqlite#758 I found that by default, codecov.io considers it a build failure if coverage is reduced. This would be defensible if the coverage infrastructure were a bit more mature and robust -- as it is it's completely possible for coverage to... wobble a bit. We would have -0.02% changes here and there that really didn't seem to come from anywhere.

Generate code coverage:
docker:
- image: circleci/rust:latest
resource_class: large
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this would increase our CI cost substantially (but it is way worth it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like large instead of xlarge works...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made normal tests use large too -- that's not really necessary to do in this PR, but it seems good to land the "reduce CI costs" patch at the same time as the "increase CI costs" patch.

Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Excited to see this in action and iterate on it over time, thank you for pushing through on all the details here.

cargo +nightly run -p protobuf-gen -- tools/protobuf_files.toml || true
cargo +nightly run -p systest || true

env RUSTFLAGS="--cfg __appsvc_ci_hack $RUSTFLAGS" cargo +nightly test --all --all-features --no-fail-fast || true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is __appsvc_ci_hack? (I mean I can kind of guess from the name, but I can't find an other uses of it in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because it isn't from this PR, but from #3343.

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

Successfully merging this pull request may close these issues.

4 participants