Add test coverage support

[keith]

No description provided.

[keith]

This was only added pretty recently, maybe even 1.60 even, not sure if we should try to gate something about this? Otherwise it was unstable / nightly only

[keith]

So interestingly this is currently failing because of a mismatch in the version variable between clangrt, and rust's builtin libraries. What happens is that the info should be coming from libprofiler_builtins, but gets loaded from libclang_rt instead.

[keith]

This seems to be because the version of clangrt that ships with the latest Xcode version is still based on llvm 13, where rust is using llvm 14, and the expected profile version has changed significantly since there have been changes in the format. But I guess there is always potential for that mismatch so ideally we would solve that by making it prefer the rust library over the clang library. Although I have no idea how that might affect binaries that contained both rust and C++

[keith]

Ok that can be worked around by passing --linkopt=-noprofilelib to make clang not add the library by default. We also have to pass --experimental_use_llvm_covmap, I think on Linux it's slightly different

[keith]

Ok I've pushed an actual working version here, on my macOS machine you can test this in the examples workspace with:

bazel coverage //hello_lib:greeting_test --linkopt=-noprofilelib --experimental_use_llvm_covmap

[keith]

I think this is ready for some light review, some open questions that I'm not sure about:

• is there something we can / should do for the llvm version mismatch problem, maybe banning it, or maybe at least adding the flag for folks
• is there some binary configuration where we need to handle the binaries passed to llvm-cov separately, like maybe if some dynamic binaries are used
• is there some way we can test this on CI?
• the only reason we have to pass experimental_use_llvm_covmap is to make noprofilelib work because it disables --coverage to clang, which is required for this flag to mean anything for our use case, ideally we could remove that need.
• should we be filtering the files to llvm-cov, we do this in the apple rules, but it requires some path manipulation probably

[UebelAndre]

@keith For CI it seems https://github.com/bazelbuild/continuous-integration/blob/master/buildkite/README.md#running-bazel-coverage is how you'd add jobs that test coverage.

[hlopko]

Thank you for working on this! In general looks good to me, but I'd like @krasimirgg to take a look too.

[krasimirgg]

This looks reasonable. I'm not very familiar with the coverage infrastructure, but wanted to ask a few questions anyways:

1. What's the intended high-level way to disable coverage support (e.g., at rust_toolchain level)? I'm thinking of situations where, e.g.,:

• folks use a custom rust compiler that doesn't include the "profiler runtime" (https://doc.rust-lang.org/nightly/rustc/instrument-coverage.html#enable-coverage-profiling-in-the-rust-compiler)
• folks don't want to induce coverage instrumentation overhead to their tests
• the llvm coverage tools are available or not compatible with the rust toolchain on the execution platform.

2. There seem to be at least two "flavours" of coverage infrastructure: the DebugInfo one and the source-based one (which this PR seems to add) (https://doc.rust-lang.org/nightly/rustc/instrument-coverage.html#introduction). I gather that the source-based one is of superior quality and it seems reasonable to have that one as the default. But I could imagine some clients may wanna use the other one, e.g., for compatibility reasons. Are there plans to have some sort of "coverage flavor" option for users to pick?

3. Have you looked into how does this PR approach works out in situations where there is mixed rust and C++ code?

[keith]

1. What's the intended high-level way to disable coverage support (e.g., at rust_toolchain level)? I'm thinking of situations where, e.g.,:

Good question, I think it would require them to not run bazel coverage on rust targets. As far as I know there isn't a way to disable coverage per-language or anything for other languages that support it either. Maybe if you exclude the rust targets from --instrumentation_filter it disables it for them?

2. There seem to be at least two "flavours" of coverage infrastructure: the DebugInfo one and the source-based one (which this PR seems to add) (doc.rust-lang.org/nightly/rustc/instrument-coverage.html#introduction). I gather that the source-based one is of superior quality and it seems reasonable to have that one as the default. But I could imagine some clients may wanna use the other one, e.g., for compatibility reasons. Are there plans to have some sort of "coverage flavor" option for users to pick?

Interesting! I hadn't seen the -Z profile option. That could be nice to allow if folks needed it. I also don't know how this rule set treats unstable flags that require nightly, since luckily the llvm based format was just stabilized.

3. Have you looked into how does this PR approach works out in situations where there is mixed rust and C++ code?

I haven't yet, and that's on my radar since C++ integration is our case as well. One immediate issue is the LLVM version mismatch issue I mentioned above, since I have been testing with clang with LLVM 13. I think we'll have to fix up issues as we hit them, but I imagine not all targets will work out of the box, which will lead to them just not producing any coverage info when you request it. The tests still pass because of this detail bazelbuild/bazel#15420

[UebelAndre]

Friendly ping here, what's still outstanding on this PR?

[keith]

I looked at adding CI, but if all we're doing is running bazel coverage instead of bazel test I'm not sure we're getting much from that. Unless we're validating something about the output (like maybe it being non-empty?), which doesn't seem like something we normally do on bazel CI

[keith]

Besides that I think the other outstanding concerns can probably be dealt with as needed in follow ups.

[UebelAndre]

I looked at adding CI, but if all we're doing is running bazel coverage instead of bazel test I'm not sure we're getting much from that. Unless we're validating something about the output (like maybe it being non-empty?), which doesn't seem like something we normally do on bazel CI

I think there's value in at least running bazel coverage to make sure the code paths don't lead to any breakages. I'd agree passed that though is uncharted territory. This, to me, raises questions about the intended way to check coverage in Bazel. Are there any built-in tools to enforce a particular target or package has some percentage of coverage? This can very likely be a separate conversation but again, still think there's value in adding a job that actually runs bazel coverage //....

[keith]

Sounds good, I pushed them to the existing jobs here, we'll see if it works out.

Are there any built-in tools to enforce a particular target or package has some percentage of coverage?

Not as far as I know, we have something internally for this, and envoy implements some post-processing script for it as well

[UebelAndre]

@hlopko @krasimirgg would you folks be able to take another look here?

[krasimirgg]

This PR looks good to me.

I share @UebelAndre's view that it would be valuable setting up a build bot doing roughly bazel coverage //... at least as something to exercise the coverage-related code paths (and it sounds OK to set something like this as a follow-up).

[krasimirgg]

LGTM

[hlopko]

Looks good to me too, I'm happy to merge after the merge conflict is resolved. I also recommend adding a CI coverage (ba dum tsss) so we don't break it accidentally. That can be done in a followup PR.

[keith]

I did add coverage_targets to the default CI jobs, so it is running coverage over the targets, though it's not validating the output, so it's not 100% useful. Rebased here

[UebelAndre]

I did add coverage_targets to the default CI jobs, so it is running coverage over the targets, though it's not validating the output, so it's not 100% useful. Rebased here

I think it's useful enough in just hitting those starlark code paths. I would say make an issue to add a step to verify coverage percentage. Ideally that would then become an issue in upstream Bazel.

[UebelAndre]

I leave it to @hlopko and @krasimirgg to determine when to merge.

[keith]

if we do merge please squash!

[UebelAndre]

if we do merge please squash!

Squashing happens automatically for every PR

[UebelAndre]

@krasimirgg @hlopko any more to do here?

[krasimirgg]

This looks good and is ready to be merged. @UebelAndre could you merge this (I believe I don't have merge rights yet)? EDIT: nevermind, the PR was out-of-sync, after it got back in sync I was able to merge it.