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

Update code coverage #3173

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Oct 10, 2022

Changes

Changed code coverage to use grcov.

Coverage value changes

grcov includes the test code as covered lines (where as kcov does not appear to do so). This moderately increases our line coverage.

Unit test and profiling tool chains

Previously we ran tests with the aarch64-unknown-linux-musl and x86_64-unkown-linux-musl tool chains to generate code coverage data then ran the unit tests under the x86_64-unknown-linux-gnu and aarch64-unknown-linux-gnu tool chains. This accomplished testing both both combinations of tool chains covering both our primary musl targets and the commonly used gnu target.

At present aarch64-unknown-linux-musl is not supported with profiling by Rust lang, I opened an issue regarding this rust-lang/rustup#3095 (interestingly x86_64-unknown-linux-musl is supported rust-lang/rust#79556). To accommodate this we need to generate code coverage using the respective gnu tool chains of x86_64-unknown-linux-gnu and aarch64-unknown-linux-gnu and then run unit tests with the musl tool chains of aarch64-unknown-linux-musl and x86_64-unkown-linux-musl. This does shift our code coverage metric generation from musl to gnu although maintains the same actual code coverage for both. I do not believe this represents a decrease in security or safety. Actual test coverage across all tool chains is identical as before and it is extremely unlikely the test coverage will diverge between the gnu and musl tool chains in the future (or until profiling is support by aarch64-unknown-linux-musl at which point we can use the musl tool chains for code coverage generation).

Reason

We currently use cargo_kcov (a wrapper around kcov). cargo_kcov appears unmaintained with the last commit >2 years ago.

grcov is a popular coverage tool for Rust. It does not produce the same error as kcov with #3105. It offers a better solution in the long term.

The primary advantages of grcov over kcov are:

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

  • All commits in this PR are signed (git commit -s).
  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • New unsafe code is documented.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

  • This functionality can be added in rust-vmm.

@JonathanWoollett-Light JonathanWoollett-Light changed the title Updated code coverage Update code coverage Oct 10, 2022
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the grcov branch 13 times, most recently from c4d44ce to 33778c9 Compare October 11, 2022 15:53
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the grcov branch 7 times, most recently from 2279194 to 183d84f Compare October 17, 2022 12:45
@dianpopa
Copy link
Contributor

I do not agree with some of the supporting arguments for switching to grcov:

  1. cargo-kcov is just a wrapper over kcov; I am skeptical the issue steams from there. You can actually try running plain kcov and see if you still receive the error; As far as maintainance goes, kcov has supported us in adding arm support a while ago: Avoid hang on aarch64 SimonKagstrom/kcov#316.
  2. proc-macro2 on its own cannot be the root cause of the problem; we are using it in versionize-derive and we are able to run kcov successfuly: https://buildkite.com/firecracker/versionize-derive-ci/builds/23.

As far as my concern, we do not have enough datapints to affirm that kcov is the problem and we need to identify what part of the code triggers the error.

@JonathanWoollett-Light JonathanWoollett-Light force-pushed the grcov branch 2 times, most recently from d43e5fd to 09bf2d0 Compare October 19, 2022 13:27
@JonathanWoollett-Light JonathanWoollett-Light changed the base branch from main to feature/cpu-templates October 20, 2022 14:11
@JonathanWoollett-Light JonathanWoollett-Light changed the base branch from feature/cpu-templates to feature/aarch64_snapshot October 20, 2022 14:31
@JonathanWoollett-Light JonathanWoollett-Light changed the base branch from feature/aarch64_snapshot to feature/cpu-templates October 20, 2022 14:31
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the grcov branch 3 times, most recently from dd9e289 to 84d03a6 Compare October 21, 2022 09:45
@roypat
Copy link
Contributor

roypat commented Oct 21, 2022

grcov includes the test code as covered lines (where as kcov does not appear to do so). This moderately increases our line coverage.

I think this is because we explicitly exclude test modules for kcov:

exclude_region = "'mod tests {'"

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Oct 21, 2022

@roypat Yes this is why. Excluding tests is a non-optimal solution here. I would propose to evaluate test code coverage and production code coverage separately. With this approach we can get the benefits of evaluating test coverage without the negative of this giving us false confidence in our production code coverage.

But this is outside this PR for now I will make the change to exclude test code.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
@alxiord alxiord merged commit c7a3a54 into firecracker-microvm:feature/cpu-templates Oct 24, 2022
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.

6 participants