Repository: bazelbuild/bazel
Issue: bazel 5 `coverage --combined_report=lcov` executes incompatible actions
Labels: type: bug
coverage
team-Rules-CPP
untriaged

### 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:
```python
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.
```console
$ 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` ?


```text
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_

Issue comments:
@gregestren , FYI.

When I next have some free time I'll work on fixing this before the refactor. This is stopping us from upgrading.
I should clarify that this didn't happen in 4.2.2:
```console
$ 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
```
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.
If you compile your own version, I think you can use this as a quick hack to work around the issue:
```patch
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.
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)?
I'll start investigating to identify the cause, but I can't judge on the time to fix just yet.
@c-mita , WDYT about #15419?
I would like to understand the regression better before judging a fix.

Bisecting indicates that 5b216b27435aeb9eb9c3bd3c552d6498e1050cc7 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 ed01f42a8ff533967c8850821fd8058d38377ff1 (bisecting between 4.0.0 and 4.2.0).
So the cause of the regression is straight-forward.

When 21179b2a82154b870ad8c5a7ad22b4662067cb29 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 https://github.com/bazelbuild/bazel/pull/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".


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD
index 22091e04506b65..42078ebfd4b87e 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD
@@ -24,6 +24,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:blaze_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+        "//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report_action_factory",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java
index 7b9c6206b23d40..2057361cf26273 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java
@@ -41,6 +41,7 @@
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
 import com.google.devtools.build.lib.analysis.actions.Compression;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
@@ -200,6 +201,10 @@ public CoverageReportActionsWrapper createCoverageActionsWrapper(
     ImmutableList.Builder<Artifact> builder = ImmutableList.<Artifact>builder();
     FilesToRunProvider reportGenerator = null;
     for (ConfiguredTarget target : targetsToTest) {
+      // Skip incompatible tests.
+      if (target.get(IncompatiblePlatformProvider.PROVIDER) != null) {
+        continue;
+      }
       TestParams testParams = target.getProvider(TestProvider.class).getTestParams();
       builder.addAll(testParams.getCoverageArtifacts());
       if (reportGenerator == null) {


Test patch:
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index 9b4cc0f52bc779..dbf578a9521c8d 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -582,6 +582,15 @@ sh_test(
     ],
 )
 
+sh_test(
+    name = "bazel_coverage_compatibility_test",
+    srcs = ["bazel_coverage_compatibility_test.sh"],
+    data = [":test-deps"],
+    tags = [
+        "no_windows",
+    ],
+)
+
 sh_test(
     name = "bazel_cc_code_coverage_test",
     srcs = ["bazel_cc_code_coverage_test.sh"],
diff --git a/src/test/shell/bazel/bazel_coverage_compatibility_test.sh b/src/test/shell/bazel/bazel_coverage_compatibility_test.sh
new file mode 100755
index 00000000000000..cb2089e0657f00
--- /dev/null
+++ b/src/test/shell/bazel/bazel_coverage_compatibility_test.sh
@@ -0,0 +1,67 @@
+#!/bin/bash
+#
+# Copyright 2022 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+set -eu
+
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+function set_up_sh_test_coverage() {
+  cat <<EOF > BUILD
+constraint_setting(name = "incompatible_setting")
+
+constraint_value(
+    name = "incompatible",
+    constraint_setting = ":incompatible_setting",
+)
+
+sh_test(
+    name = "compatible_test",
+    srcs = ["compatible_test.sh"],
+)
+
+sh_test(
+    name = "incompatible_test",
+    srcs = ["incompatible_test.sh"],
+    target_compatible_with = [":incompatible"],
+)
+EOF
+  cat <<EOF > compatible_test.sh
+#!/bin/bash
+exit 0
+EOF
+  cat <<EOF > incompatible_test.sh
+#!/bin/bash
+exit 1
+EOF
+  chmod +x compatible_test.sh
+  chmod +x incompatible_test.sh
+}
+
+# Validates that coverage skips incompatible tests.  This is a regression test for
+# https://github.com/bazelbuild/bazel/issues/15385.
+function test_sh_test_coverage() {
+  set_up_sh_test_coverage
+  bazel coverage --test_output=all --combined_report=lcov //:all &>$TEST_log \
+    || fail "Coverage for //:all failed"
+  expect_log "INFO: Build completed successfully"
+  expect_log "//:compatible_test .* PASSED"
+  expect_log "//:incompatible_test .* SKIPPED"
+}
+
+run_suite "test tests"