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

RFC: Add native code coverage support in Cargo #3287

Closed

Conversation

ridwanabdillahi
Copy link
Contributor

@ridwanabdillahi ridwanabdillahi commented Jul 1, 2022

This RFC proposes integrating Rust's support for source-based code coverage into Cargo.

Internals Thread
Rendered
rust-lang/cargo#13040

@xd009642
Copy link

xd009642 commented Jul 1, 2022

(Tarpaulin author) Making coverage easier to get for users on any system and reducing the overhead would definitely be a big quality of life improvement!

Just a note on tarpaulin it has alpha support for the llvm instrumentation xd009642/tarpaulin#549, but it also plans in future of using probe-rs and providing options for embedded device coverage. Some way of reducing -Cinstrument-coverage to limit it to certain crates is definitely desired though. Without it the binary size is pretty large and it stops it being suitable to use in embedded (otherwise we could get embedded coverage using minicov.

As well as these changes making the coverage more applicable to more domains, it would benefit crates like cargo-llvm-cov and tarpaulin and enable coverage in more specific environments. Plus making coverage work for 99% of users without needing a 3rd party tool would be a big UX improvement.

@ehuss ehuss added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Jul 2, 2022
@c-git
Copy link

c-git commented Jul 2, 2022

As a new user to rust I want to say that this in my opinion would be a great step in the right direction. It would help make rust adoption simpler as there would be a clear way to check code coverage without needing to check what creates provide this functionality and potentially need to deep dive into them to choose one.

@adlrwbr
Copy link

adlrwbr commented Jul 7, 2022

Bumping this. The Rust community has always heavily invested into its tooling. This is a feature found in many other ecosystems. It’d be great to see an ergonomic cargo flag as a solution.

Copy link

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

You can actually specify which crates to test when giving the cargo test command in a workspace. I think this RFC should talk about this scenario right now instead of leaving it as a future possibility.

And IMO we should remove the "root crate" wording from the RFC because of the above reasons.

@ridwanabdillahi
Copy link
Contributor Author

You can actually specify which crates to test when giving the cargo test command in a workspace. I think this RFC should talk about this scenario right now instead of leaving it as a future possibility.

I'll add this in as opposed to leaving it as a future possibility.

@pksunkara
Copy link

Good update. But I still see some references to "top-level crate" which is a bit confusing with the new changes.

…e default crates instrumented via the `--coverage` command to the current RFC and remove from the future possibilities section.
@ridwanabdillahi
Copy link
Contributor Author

Good update. But I still see some references to "top-level crate" which is a bit confusing with the new changes.

I've removed the references to top-level crate. Thanks for the feedback!

Copy link

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

@epage It's ready for your review I think.

text/0000-cargo-test-coverage.md Show resolved Hide resolved
text/0000-cargo-test-coverage.md Show resolved Hide resolved
text/0000-cargo-test-coverage.md Show resolved Hide resolved
text/0000-cargo-test-coverage.md Show resolved Hide resolved
@c-git
Copy link

c-git commented Jul 23, 2022

I'm very new to rust and not sure if I missed it somewhere higher up but wanted to give my opinion on back-end support from my humble point of view.

Even if in the initial working version we only have LLVM support that is still better than no built-in support. As a new user for me it would clearly identify what is at least a generally considered a good way to do coverage testing without doing a lot of research on the topic. I got the impression this change was mostly targeted at beginners, and I think beginners often don't mind working with defaults to get results to get started. Like using LLVM or whatever other backend is required to get started.

That is not to say it should be included in Future Possibilities. However in the meanwhile, you could use other backends when debugging and only need to use LLVM if you want to test coverage.

@ridwanabdillahi
Copy link
Contributor Author

@epage thank you for taking the time to review this PR, it is greatly appreciated. I've made the updates you requested and added doctests to the initial iteration of this RFC.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

This RFC seems to propose two things: a generic mechanism for how tools (or in this case arguments) are applied to different crates in a workspace, and some specific API for running code coverage which uses that mechanism (unfortunately, the generic mechanism is only described in terms of the specialist use for code coverage :-) ).

I think that the generic mechanism would be useful for more than code coverage and adding support to it to Cargo in a way that it can be used with any flags or by any plugin would be super-useful (certainly some fun questions about the design though). I hope that such a mechanism could be used by coverage plugins to provide a better experience.

On the question of code coverage itself, I strongly agree it is something Cargo should help support, but I don't think it should get built in to Cargo. There are a lot of things a build tool could do (benchmarking, fuzz testing, linting, other static analysis, dynamic analysis (e.g., with MIRI), ...) and if Cargo does all of them it will become a confusing mess. It should be a platform for supporting all things, but only include default support for core functionality. I don't think code coverage makes that mark - some people like it, some people don't. There are many ways to do it using many different tools and I don't think that even if there is consensus that it is useful, there is consensus on the best way to do it. Therefore, supporting it in Cargo risks us either having multiple ways to do it (in the long term, which makes things even more confusing) or becoming outdated and we have to recommend people not to use the built-in support.


### Alternative 4: use existing custom subcommands to run code coverage analysis

Supporting this alternative would mean that there wouldn't need to be any changes to Cargo
Copy link
Member

Choose a reason for hiding this comment

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

Given that these options exist today, I think the tone of the RFC should be more about 'moving an existing plugin from the ecosystem to core Cargo' rather than 'adding a new feature to Cargo'. Concretely, that means justifying why such a move would benefit a large enough subset of users to justify the increased maintenance burden, and what the benefits of core support rather than ecosystem support actually are. Furthermore, if you're proposing something new rather than something which already exists then you need to justify why the things which exist (or at least their API) are less good than the proposal.

I think you should also consider an alternative where we add features to Cargo which enable the ecosystem plugins to be as good as a core feature, i.e., could we add features which let cargo-llvm-cov be as good as the proposal in this RFC?

@ridwanabdillahi
Copy link
Contributor Author

Thank you for your response @nrc

It should be a platform for supporting all things, but only include default support for core functionality. I don't think code coverage makes that mark

I don't necessarily agree with this statement. Code coverage is an essential part of the development lifecycle. This only boldens the argument for writing tests and ensuring tests capture core functionality of any project. The Rust compiler also has a mechanism for gathering coverage data in two ways. Instrumentation based coverage is supported in stable and a gcc coverage implementation is in unstable. Having stable support for instrumentation-based coverage in rustc should ensure that Cargo should not have any risks of this feature becoming "outdated". Also with any feature, having an agreed upon RFC would allow any Rust developer to be able to find the default suggested way of collecting coverage metrics in a Rust project via Cargo. Cargo could support code coverage in the same manner it supports rustdoc, making it very easy for a Rust developer to build their documentation and open it in a browser. This RFC is proposing leveraging the support rustc currently provides along with the components provided by the rust toolchain, i.e. llvm-tools-preview.

@nrc
Copy link
Member

nrc commented Jul 28, 2022

I don't necessarily agree with this statement. Code coverage is an essential part of the development lifecycle.

So, personally, I strongly disagree with this. I think code coverage is a flaky heuristic masquerading as a metric and it encourages useless boilerplate testing at the expense of testing any edge cases which don't show up as branches. BUT, that isn't really important. I do think it is sometimes useful and I do think some people consider it an essential part of their development. However, my understanding of the wider dev community (which is what is important here) is that there is no consensus on it being essential and it isn't in the same tier as unit testing, benchmarking, or documentation. I think (and the Cargo team might have a different opinion) that to qualify for core support in Cargo, something must either have almost unanimous support in the community (like testing) or be something that we are specifically pushing as an advantage of Rust (e.g., Rustdoc). (or possibly be something which is important and fundamentally cannot be supported as a plugin). I don't think coverage meets these requirements, but perhaps it has become a lot more widely accepted recently?

Anyway, I don't think being a plugin is a bad thing! Some really great tools are Cargo plugins rather than having built-in support (Rustfmt, Clippy, cargo audit, etc.). I would love for Rust to have best-in-class support for code coverage (as a plugin) and I'd support changes to Cargo to facilitate that.

The Rust compiler also has a mechanism for gathering coverage data in two ways. Instrumentation based coverage is supported in stable and a gcc coverage implementation is in unstable

It actually kinda worries me that there are already two implementations here. It seems like that means it is more likely for there to be more in the future. They are also closely tied to the compiler backend, so support with a different backend (e.g., Cranelift) or with a different compiler is likely to be different or non-existent. That seems like another good reason for this to be a plugin to Cargo rather than built-in.

@c-git
Copy link

c-git commented Jul 30, 2022

If I'm understanding plugins correctly then Tarpaulin is already a coverage plugin for cargo (among others)
https://lib.rs/crates/cargo-tarpaulin

I found it in the list of plugins for cargo https://lib.rs/development-tools/cargo-plugins

The only concern I have is that it comes back to being a bit confusing for new users which one they should pick

@ridwanabdillahi
Copy link
Contributor Author

ridwanabdillahi commented Aug 1, 2022

If I'm understanding plugins correctly then Tarpaulin is already a coverage plugin for cargo (among others) https://lib.rs/crates/cargo-tarpaulin

Yes, Tarpaulin is a plugin along with llvm-cov.

The only concern I have is that it comes back to being a bit confusing for new users which one they should pick

I agree with your sentiment which is why I introduced this RFC as adding this support directly in Cargo to help alleviate some of those concerns. @nrc makes some good points around how plugins can be a good thing in terms of handling code coverage for Rust projects, but it is also clear that Cargo is lacking some features that would allow these plugins to have the full picture around which crates need to be instrumented.

@ridwanabdillahi
Copy link
Contributor Author

@nrc

As per our discussion, I pushed out a separate RFC to support a generic mechanism for setting rustc flags via Cargo. This allows setting any Rust compiler flag, not just code coverage flags, and enables them only on the current crate being built. Dependencies including transitive dependencies will not have the flags applied. This new feature will make it easier to enable rustc flags regardless of the cargo command being run.

RFC #3310

@ssokolow
Copy link

ssokolow commented Sep 8, 2022

To be honest, I see code coverage as being to cargo test as Clippy (and especially clippy::restriction) is to cargo build.

Yes, it can be abused and misinterpreted, but so can lints, and both are a pain to implement and maintain without support from the compiler.

@ridwanabdillahi
Copy link
Contributor Author

@ssokolow I agree with this sentiment, thus the reason for opening this RFC. In my opinion having support for code coverage integrated directly into Cargo would be extremely helpful for a large group of Rust developers. I understand that there is some disagreement in how beneficial code coverage can be but having builtin stable support for it so those Rust developers that find it extremely essential in elevating code quality are able to easily use it.

@jonhoo
Copy link
Contributor

jonhoo commented Sep 20, 2022

One quick note — I would want to make sure this knows to not clobber between coverage and non-coverage artifacts so that the last two calls in this sequence are no-ops:

$ cargo test --no-run
$ cargo test --coverage --no-run
$ cargo test --no-run
$ cargo test --coverage --no-run

@ridwanabdillahi
Copy link
Contributor Author

One quick note — I would want to make sure this knows to not clobber between coverage and non-coverage artifacts so that the last two calls in this sequence are no-ops:

$ cargo test --no-run
$ cargo test --coverage --no-run
$ cargo test --no-run
$ cargo test --coverage --no-run

That would not be the case. The --coverage flag alters the rustflags enabled for a given crate by forcing cargo to re-compile the root crate to ensure its libraries are instrumented. This has a direct change in the artifacts produced so for the example above, the last two calls would both cause the top-level crate and any of its dependencies to be compiled again since the invocations to rustc will have a different set of flags enabled.

@jonhoo
Copy link
Contributor

jonhoo commented Sep 20, 2022

I understand that that's the case if implemented directly as a way to modify rustflags. But I'd like to have that not be the case with --coverage. If we truly want "native code coverage support in Cargo", then that should (imo) include teaching Cargo to distinguish between the coverage and non-coverage artifacts so that it doesn't let them clobber each other.

@ridwanabdillahi
Copy link
Contributor Author

Ahh I understand your comment now. That is very interesting, my only worry about this adding additional artifacts for each crate that is being instrumented and potentially having 'coverage artifacts' for each target being compiled which can quickly expand the number of artifacts for a simple build. This might be as simple as adding a separate target/out directory when the --coverage flag is enabled.

I do see the benefit of this approach and will certainly add it to the RFC as an Alternative as well as an Unanswered question. I would love to get input from the cargo team on this as well and see what their thought on this approach would be. In the meantime, I'll look into what exactly this will mean for cargo and what changes would be necessary.

Thanks for pointing out this alternative implementation :)

@ehuss
Copy link
Contributor

ehuss commented Sep 27, 2022

Just posting an update: The Cargo team discussed this RFC a bit. I think in the short term we would like to see more experimentation done as third-party commands. We may consider more first-class support in the future, as it would make it more accessible. However, it seems like there is quite a bit of space to explore.

It sounds like one of the biggest concerns expressed in this RFC with the current third-party approach (such as cargo llvm-cov) is the inability to limit the coverage from being collected from dependencies. We are interested in looking at adding more control over how RUSTFLAGS can be set (such as #3310). In the short term, you can try a hack using profiles, something like:

[profile.codecov]
inherits = "dev"
rustflags = ["-Cinstrument-coverage"]
[profile.codecov.package."*"]
rustflags = []

I understand that is not ideal, and due to a limitation of the current implementation this can't be used via .cargo/config.toml. But it is at least something to experiment with.

Another thing that would be helpful is to include more of a survey of code coverage in other ecosystems. What lessons can we learn from them?

@ridwanabdillahi
Copy link
Contributor Author

@ehuss Thanks for the detailed response!

Yes, that hack should be sufficient enough to use as a workaround for the main issue of using third-party commands to handle collecting code coverage for a specific crate. My biggest concern is that this is not as ergonomic as I would have hoped for collecting code coverage as it adds more onus on the user of the third-party command to set these profile specific rustflags rather than making it ergonomic for the third-party command to handle this.

I agree that having a better way of setting RUSTFLAGS on a crate-by-crate basis would alleviate this scenario and enhance how third-party commands collect coverage metrics and would make it simpler for a plugin to set the specific RUSTFLAG needed to collect coverage for a given crate.

I have started this conversation here.

However, it seems like there is quite a bit of space to explore.

Could you elaborate on what you mean here? It would be great to get this documented to see the full scope of hurdles that would be blocking first-class support for handling code coverage within Cargo.

Comment on lines +308 to +311
Cargo-llvm-cov currently leverages the `cargo-config` subcommand which is still unstable. To do so,
cargo-llvm-cov sets the `RUSTC_BOOTSTRAP` environment variable to allow its usage from a stable
toolchain. This is not a recommended approach especially for Rust developers that want to use
such tools in production code.
Copy link
Member

@taiki-e taiki-e Jan 10, 2023

Choose a reason for hiding this comment

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

cargo-llvm-cov is now using cargo-config2 crate and is no longer depending on the unstable cargo-config subcommand.

This is not a recommended approach especially for Rust developers that want to use
such tools in production code.

Btw, if you think that development tools do this is a problem, you may want to open the issue to rust-analyzer that does similar things to the previous cargo-llvm-cov.

of the generated `profraw` files? This is used to ensure each test run generates unique profiling data as opposed to overwriting
the previous run.

# Future possibilities
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth mentioning here some kinds of tests that are supported by cargo-llvm-cov and do not seem to be supported by this RFC.

  • other (builtin) subcommands (e.g., cargo run)
  • merge coverage of different runs
  • coverage of C/C++ code linked to Rust lib/bin

cargo-llvm-cov also supports the following, but I think they are out of the scope of cargo.

  • external tests
  • third-party test libraries (e.g., trybuild)
  • other (third-party) subcommands (e.g., cargo nextest)

@ehuss
Copy link
Contributor

ehuss commented Dec 11, 2023

I'm going to propose to postpone this RFC. The Cargo team agrees that we would like to see first-party support for coverage support and recognizes that the current options are not great. However, we have concerns about how this ties directly into the llvm tools, and the uncertainty on how that works with various scenarios (different LLVM versions, different codegen backends, different approaches of coverage instrumentation, etc.). Primarily, there isn't anyone on the team who has the capacity at this time to champion this feature.

As a compromise, we may be interested in possibly seeing some unstable experimentation done in Cargo, where we can explore options for the solution. This would be with the intent that an RFC would follow up before stabilization. If anyone is interested in working on that, feel free to reach out to the Cargo team on #t-cargo on Zulip, or on the issue rust-lang/cargo#13040, or come to Office hours. We would probably like to see it broken into smaller pieces, such as adding an option to have Cargo pass the -Cinstrument-coverage flag, but not do anything else. Or maybe more investigation into better RUSTFLAGS support (like #3310).

@rfcbot fcp postpone

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 11, 2023

Team member @ehuss has proposed to postpone this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Dec 11, 2023
@aDogCalledSpot
Copy link

I think it would make sense to add a flag that lets you only generate the profraw files and stop there. This way you could manually merge the coverage of multiple executions of cargo test which you may be invoking for testing different configurations (different features, different targets, etc).

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Feb 28, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Feb 28, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Mar 9, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 9, 2024

The final comment period, with a disposition to postpone, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This is now postponed.

@rfcbot rfcbot added postponed RFCs that have been postponed and may be revisited at a later time. and removed disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Mar 9, 2024
@rfcbot rfcbot closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. T-cargo Relevant to the Cargo team, which will review and decide on the RFC. to-announce
Projects
Archived in project
Status: Final Comment
Development

Successfully merging this pull request may close these issues.