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

_collect_cc_coverage script can silently fail to produce coverage #18650

Open
UebelAndre opened this issue Jun 13, 2023 · 7 comments
Open

_collect_cc_coverage script can silently fail to produce coverage #18650

UebelAndre opened this issue Jun 13, 2023 · 7 comments
Labels
coverage P2 We'll consider working on this in future. (Assignee optional) team-Rules-Server Issues for serverside rules included with Bazel type: bug

Comments

@UebelAndre
Copy link
Contributor

Description of the bug:

I noticed an issue where rules_rust rules were failing to produce coverage reports when running with --experimental_split_coverage_postprocessing and found it to be caused by runfiles not being available to the llvm-cov call which expects test binaries to be available:
https://github.com/bazelbuild/bazel/blob/6.2.1/tools/test/collect_cc_coverage.sh#L95-L97

The script would fail with a File not found error and no coverage would be reported.

This to me seems like a miss. I can see a test not having any coverage to report if maybe there's no dependencies and --instrument_test_target is not set, but otherwise I fully expected bazel coverage invocations to fail if a genuine error occurs.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Run bazel coverage //... --experimental_split_coverage_postprocessing in rules_rust v0.22.0 on Bazel v6.2.1.

Which operating system are you running Bazel on?

Linux, MacOS

What is the output of bazel info release?

release 6.2.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

https://bazelbuild.slack.com/archives/CA31HN1T3/p1686230133034699 is the thread where this issue was triaged.

Any other information, logs, or outputs that you want to share?

No response

@UebelAndre
Copy link
Contributor Author

#18634 (comment) seems like it would fix the issue of rules_rust not supporting --experimental_split_coverage_postprocessing.

@Pavank1992 Pavank1992 added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Jun 13, 2023
@coeuvre coeuvre added team-Rules-Server Issues for serverside rules included with Bazel and removed team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Jun 13, 2023
@comius
Copy link
Contributor

comius commented Aug 4, 2023

@c-mita please triage

@keith
Copy link
Member

keith commented Aug 4, 2023

I started trying to make that script fail tests (I assume that would have made it more obvious in this case) #15420 but that breaks tests since they're currently relying on that because the host machines I guess aren't setup for coverage

@c-mita c-mita added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Aug 8, 2023
@UebelAndre
Copy link
Contributor Author

Running into this again. Would it be possible to have whatever tests that depend on this use an environment variable or something to toggle the silencing behavior? It'd be extremely valuable to be able to see when generating coverage failed.

@keith
Copy link
Member

keith commented Jan 9, 2024

Good idea, I added a flag to support that and enabled it for bazel CI, I wouldn't think users should ever use this #15420

@keith
Copy link
Member

keith commented Jan 9, 2024

alternatively we could hijack that env var to just set it to true or something, but this is probably more clear

@UebelAndre
Copy link
Contributor Author

IMO the script needs to fail if any problem is encountered. Only the tests you mentioned in #18650 (comment) should use this flag and everything else should fail fast and loud so rules maintainers can start making sure coverage actually works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage P2 We'll consider working on this in future. (Assignee optional) team-Rules-Server Issues for serverside rules included with Bazel type: bug
Projects
None yet
Development

No branches or pull requests

7 participants