bazel 5 coverage --combined_report=lcov executes incompatible actions

[philsc]

Description of the bug:

Running bazel coverage --combined_report=lcov does not properly skip incompatible tests.

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

BUILD file:

genrule(
    name = "generate_example_test",
    outs = ["example_test.cc"],
    cmd = "echo 'int main() { return 1; }' > $(OUTS)",
)

cc_test(
    name = "example_test",
    srcs = ["example_test.cc"],
    target_compatible_with = ["@platforms//:incompatible"],
)

Empty WORKSPACE file.

$ bazel --nohome_rc  coverage //:all --java_runtime_version=remotejdk_11 --combined_report=lcov
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/pschrader/.bazelrc
INFO: Using default value for --instrumentation_filter: "^//".
INFO: Override the above default with --instrumentation_filter
INFO: Analyzed 2 targets (0 packages loaded, 0 targets configured).
INFO: Found 1 target and 1 test target...
ERROR: /home/pschrader/work/test_repo/BUILD:7:8: Reporting failed target //:example_test located at /home/pschrader/work/test_repo/BUILD:7:8 failed: Can't build this. This target is incompatible. Please file a bug upstream. caused by //:example_test
INFO: Elapsed time: 0.433s, Critical Path: 0.01s
INFO: 4 processes: 4 internal.
FAILED: Build did NOT complete successfully
//:example_test                                                         SKIPPED

Executed 0 out of 1 test: 1 was skipped.
All tests passed but there were other errors during the build.
FAILED: Build did NOT complete successfully

Which operating system are you running Bazel on?

x86 Ubuntu 18.04

What is the output of bazel info release?

release 5.1.1

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

N/A

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

N/A

Have you found anything relevant by searching the web?

I couldn't find a similar bug report.

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

No response

[philsc]

@gregestren , FYI.

When I next have some free time I'll work on fixing this before the refactor. This is stopping us from upgrading.

[philsc]

I should clarify that this didn't happen in 4.2.2:

$ bazel --nohome_rc  coverage //:all --javabase=@bazel_tools//tools/jdk:remote_jdk11 --combined_report=lcov
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
/home/pschrader/.bazelrc
INFO: Using default value for --instrumentation_filter: "^//".
INFO: Override the above default with --instrumentation_filter
INFO: Build options --java_runtime_version and --javabase have changed, discarding analysis cache.
ERROR: Cannot generate coverage report - no coverage information was collected
INFO: Analyzed 2 targets (4 packages loaded, 405 targets configured).
INFO: Found 1 target and 1 test target...
INFO: Elapsed time: 3.302s, Critical Path: 0.05s
INFO: 2 processes: 1 internal, 1 linux-sandbox.
INFO: Build completed successfully, 2 total actions
//:example_test                                                         SKIPPED

INFO: Build completed successfully, 2 total actions

[JohanLaineTobii]

This bug affects my organization, we might have to downgrade to 4.2.

I just checked and the issue is in 5.0.0 as well, it's not really a "5.1" bug as it says in the title. But I haven't tracked further than that to find a specific commit.

[philsc]

If you compile your own version, I think you can use this as a quick hack to work around the issue:

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 581a655a65..67afcb3ab0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -531,8 +531,9 @@ public class BuildView {

     Set<ConfiguredTarget> allTargetsToTest = null;
     if (testsToRun != null) {
-      // Determine the subset of configured targets that are meant to be run as tests.
+      // Determine the subset of compatible configured targets that are meant to be run as tests.
       allTargetsToTest = filterTestsByTargets(configuredTargets, testsToRun);
+      allTargetsToTest.removeAll(targetsToSkip);
     }

     ImmutableSet.Builder<Artifact> artifactsToBuild = ImmutableSet.builder();

The problem is that this approach doesn't pass the skipped targets information to the final test reporter (TerminalTestResultNotifier). That means it'll skip the incompatible tests, but not report on them. A proper fix would pass the information through farther so the test reporter reports on both executed tests and skipped tests.

[JohanLaineTobii]

Thanks for the update! I don't think it's likely I can get go-ahead on deploying an inofficial Bazel version internally but it's good to hear that this is being worked on and that the problem is understood.

Do you think a fix will be added to a 5.1.2 release in the near future (say, a couple of weeks)?

[c-mita]

I'll start investigating to identify the cause, but I can't judge on the time to fix just yet.

[philsc]

@c-mita , WDYT about #15419?

[c-mita]

I would like to understand the regression better before judging a fix.

Bisecting indicates that 5b216b2 triggered the regression (the flag flip for
--experimental_forward_instrumentation_files_info_by_default).

Unfortunately the flag was removed in 5.0, so there's no way of disabling this behaviour in that version.

This wouldn't be the first time the change activated by that flag has broken something, but at least this gives an indication of where something might be going wrong.

FWIW, the provided reproduction only starts passing properly after ed01f42 (bisecting between 4.0.0 and 4.2.0).

[c-mita]

So the cause of the regression is straight-forward.

When 21179b2 added the check that forwards an InstrumentedFilesInfo, it changed the behaviour of the case of a test that normally provides its own but didn't because it was "incompatible".

This trips up the coverage code down the line.

The fix could either be what you've done in #15419 or to change the check in RuleConfiguredTargetBuilder::build to also check for the existence of an IncompatibleTargetProvider and not create the forwarding InstrumentedFilesInfo in that case.

I think I prefer the former as it strikes me as a little more "robust".