-
Notifications
You must be signed in to change notification settings - Fork 648
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
Collect coverage for other languages #3287
Conversation
When using `--combined_report=lcov`, the coverage report for a `go_test` will also include coverage for data dependencies executed by the test, e.g. `java_binary` or `cc_binary` tools run in an integration test. Note: This commit does *not* make it so that coverage is collected for `go_binary`s executed by `go_test`s - Go doesn't provide exit hooks, so this would be rather involved to implement.
d17a02c
to
ceca8e4
Compare
@linzhp @fmeum this caused a regression for us when running in RBE. When running
and
While I can get the second error to go away by manually passing a GCOV path, the first error seems to be caused by this very old outstanding Bazel issue: bazelbuild/bazel#4685 Would it be possible to put this behavior behind some kind of flag? We don't actually need this behavior, and this is preventing us (and likely other remote build execution users) from upgrading to 0.35.0. |
@skevy Do any of the workarounds (e.g. |
@fmeum they do not. I'll continue working with our RBE partner on this but it's definitely a blocker for now (especially given how long that issue has been open). |
@skevy I know it's not going to be easy, but if you can provide me with a reproducer for this issue with the flags I mentioned above, I could actively work on fixing this upstream. |
@linzhp Should we introduce a flag that chooses between the two mergers? |
Let's see if we can fix it first before considering supporting two mergers |
We have the same problem and are blocked on this, want to upgrade to To reproduce any remote cache with |
This reverts commit d94e692.
Coverage collection for both Instead of regressing one feature to support another, I would first like to understand better what exactly doesn't work. @skevy @arjantop-cai Which set of flags make this fail? Are you using:
What about different values of |
@fmeum None of the test cases actually cover |
The following flags make the Bazel integration tests pass:
Could you try them? |
The behavior changed because we (rules_go) are now using Bazel's standard procedure for merging coverage across languages rather than rolling our own, but that procedure isn't fully set up for BwoB. |
@skevy and I are setting
Of the flags we pass for CI these are the relevant ones:
ex output:
It seems related to bazelbuild/bazel#4685 but even with remote_download_outputs=all it still fails. |
@esprehn You are missing |
Why is Also am I to understand that coverage in bazel is just broken with RBE unless you pass that experimental postprocessing flag? :/ |
The name of that flag isn't optimal, it doesn't have anything to do with BwoB and Coverage is broken in Bazel with RBE unless you pass the flag or do what Google does internally: Compile the Java coverage merger tool into a native binary, which doesn't use runfiles and thus works without the flag. |
Ah that makes sense, thanks for explaining! Looking forward to the defaults being flipped to working. :) |
@fmeum we've looked into this and the problem seems to come from the combination of Specifically passing:
the coverage.dat is downloaded but is always zero bytes. If you remove If you remove either of the other two flags coverage crashes in remote mode. Is there a way to preserve the go_cover format support? |
@esprehn What happens if you remove both of the
Could you try what happens if you create this target in your own workspace and pass its label to Bazel via |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
When using
--combined_report=lcov
, the coverage report for ago_test
will also include coverage for data dependencies executed by the test, e.g.java_binary
orcc_binary
tools run in an integration test.Note: This commit does not make it so that coverage is collected for
go_binary
s executed bygo_test
s - Go doesn't provide exit hooks, so this would be rather involved to implement.