Make bazel coverage work with minimal mode

This PR solves the problem in a different way that bazelbuild#16475 tries to solve:

1. bazelbuild#16812 allows skyframe read metadata from ActionFS.
2. Use `ActionFileSystem` to check existence of coverage data.
3. Fire event `CoverageReport` in the action after the coverage report is generated and listen to it in `ToplevelArtifactsDownloader` to download the report.

Closes bazelbuild#16556.

PiperOrigin-RevId: 502854552
Change-Id: I2796baaa962857831ff161423be6dffa6eb73e5c

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -2527,6 +2527,16 @@ java_library(
     ],
 )
 
+java_library(
+    name = "test/coverage_report",
+    srcs = ["test/CoverageReport.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/events",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//third_party:guava",
+    ],
+)
+
 java_library(
     name = "test/coverage_report_action_factory",
     srcs = ["test/CoverageReportActionFactory.java"],

src/main/java/com/google/devtools/build/lib/analysis/test/CoverageReport.java

@@ -0,0 +1,31 @@
+// Copyright 2023 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.
+package com.google.devtools.build.lib.analysis.test;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.vfs.Path;
+
+/** This event is used to notify about a successfully generated coverage report. */
+public final class CoverageReport implements ExtendedEventHandler.Postable {
+  private final ImmutableList<Path> files;
+
+  public CoverageReport(ImmutableList<Path> files) {
+    this.files = files;
+  }
+
+  public ImmutableList<Path> getFiles() {
+    return files;
+  }
+}

src/main/java/com/google/devtools/build/lib/bazel/coverage/BUILD

@@ -25,12 +25,14 @@ java_library(
         "//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",
         "//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",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_resolver",
         "//src/main/java/com/google/devtools/build/lib/util",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:auto_value",

src/main/java/com/google/devtools/build/lib/bazel/coverage/CoverageReportActionBuilder.java

@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.bazel.coverage;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -30,6 +32,7 @@
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
+import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.BaseSpawn;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -45,6 +48,7 @@
 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;
+import com.google.devtools.build.lib.analysis.test.CoverageReport;
 import com.google.devtools.build.lib.analysis.test.CoverageReportActionFactory.CoverageReportActionsWrapper;
 import com.google.devtools.build.lib.analysis.test.TestProvider;
 import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams;
@@ -57,6 +61,7 @@
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.exec.SpawnStrategyResolver;
 import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -146,6 +151,12 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
                 .getContext(SpawnStrategyResolver.class)
                 .exec(spawn, actionExecutionContext);
         actionExecutionContext.getEventHandler().handle(Event.info(locationMessage));
+        ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
+        ImmutableList<Path> files =
+            getOutputs().stream()
+                .map(artifact -> pathResolver.convertPath(artifact.getPath()))
+                .collect(toImmutableList());
+        actionExecutionContext.getEventHandler().post(new CoverageReport(files));
         return ActionResult.create(spawnResults);
       } catch (ExecException e) {
         throw ActionExecutionException.fromExecException(e, this);

src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java

@@ -827,7 +827,10 @@ public TestAttemptContinuation execute()
 
       Verify.verify(
           !(testAction.isCoverageMode() && testAction.getSplitCoveragePostProcessing())
-              || testAction.getCoverageData().getPath().exists());
+              || actionExecutionContext
+                  .getPathResolver()
+                  .convertPath(testAction.getCoverageData().getPath())
+                  .exists());
       Verify.verifyNotNull(spawnResults);
       Verify.verifyNotNull(testResultDataBuilder);
 

src/main/java/com/google/devtools/build/lib/remote/BUILD

@@ -201,6 +201,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
+        "//src/main/java/com/google/devtools/build/lib/analysis:test/coverage_report",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/remote/util",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_execution_value",

src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java

@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.analysis.TargetCompleteEvent;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
+import com.google.devtools.build.lib.analysis.test.CoverageReport;
 import com.google.devtools.build.lib.analysis.test.TestAttempt;
 import com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher.Priority;
 import com.google.devtools.build.lib.remote.util.StaticMetadataProvider;
@@ -56,7 +57,8 @@ private enum CommandMode {
     UNKNOWN,
     BUILD,
     TEST,
-    RUN;
+    RUN,
+    COVERAGE;
   }
 
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
@@ -83,6 +85,9 @@ public ToplevelArtifactsDownloader(
       case "run":
         this.commandMode = CommandMode.RUN;
         break;
+      case "coverage":
+        this.commandMode = CommandMode.COVERAGE;
+        break;
       default:
         this.commandMode = CommandMode.UNKNOWN;
     }
@@ -104,36 +109,48 @@ public interface PathToMetadataConverter {
     FileArtifactValue getMetadata(Path path);
   }
 
+  private void downloadTestOutput(Path path) {
+    // Since the event is fired within action execution, the skyframe doesn't know the outputs of
+    // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
+    // of the path is an ActionFileSystem, we use it to get the metadata for this file.
+    //
+    // If the test hit action cache, the filesystem is local filesystem because the actual test
+    // action didn't get the chance to execute. In this case the metadata is null which is fine
+    // because test outputs are already downloaded (otherwise it cannot hit the action cache).
+    FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path);
+    if (metadata != null) {
+      ListenableFuture<Void> future =
+          actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW);
+      addCallback(
+          future,
+          new FutureCallback<Void>() {
+            @Override
+            public void onSuccess(Void unused) {}
+
+            @Override
+            public void onFailure(Throwable throwable) {
+              logger.atWarning().withCause(throwable).log(
+                  "Failed to download test output %s.", path);
+            }
+          },
+          directExecutor());
+    }
+  }
+
   @Subscribe
   @AllowConcurrentEvents
   public void onTestAttempt(TestAttempt event) {
     for (Pair<String, Path> pair : event.getFiles()) {
       Path path = checkNotNull(pair.getSecond());
-      // Since the event is fired within action execution, the skyframe doesn't know the outputs of
-      // test actions yet, so we can't get their metadata through skyframe. However, the fileSystem
-      // of the path is an ActionFileSystem, we use it to get the metadata for this file.
-      //
-      // If the test hit action cache, the filesystem is local filesystem because the actual test
-      // action didn't get the chance to execute. In this case the metadata is null which is fine
-      // because test outputs are already downloaded (otherwise it cannot hit the action cache).
-      FileArtifactValue metadata = pathToMetadataConverter.getMetadata(path);
-      if (metadata != null) {
-        ListenableFuture<Void> future =
-            actionInputPrefetcher.downloadFileAsync(path.asFragment(), metadata, Priority.LOW);
-        addCallback(
-            future,
-            new FutureCallback<Void>() {
-              @Override
-              public void onSuccess(Void unused) {}
+      downloadTestOutput(path);
+    }
+  }
 
-              @Override
-              public void onFailure(Throwable throwable) {
-                logger.atWarning().withCause(throwable).log(
-                    "Failed to download test output %s.", path);
-              }
-            },
-            directExecutor());
-      }
+  @Subscribe
+  @AllowConcurrentEvents
+  public void onCoverageReport(CoverageReport event) {
+    for (var file : event.getFiles()) {
+      downloadTestOutput(file);
     }
   }
 
@@ -172,8 +189,9 @@ private boolean shouldDownloadToplevelOutputs(ConfiguredTargetKey configuredTarg
       case RUN:
         // Always download outputs of toplevel targets in RUN mode
         return true;
+      case COVERAGE:
       case TEST:
-        // Do not download test binary in test mode.
+        // Do not download test binary in test/coverage mode.
         try {
           var configuredTargetValue =
               (ConfiguredTargetValue) memoizingEvaluator.getExistingValue(configuredTargetKey);

src/test/shell/bazel/remote/build_without_the_bytes_test.sh

@@ -1363,4 +1363,88 @@ EOF
   expect_log "bin-message"
 }
 
+function test_java_rbe_coverage_produces_report() {
+  mkdir -p java/factorial
+
+  JAVA_TOOLS_ZIP="released"
+  COVERAGE_GENERATOR_DIR="released"
+
+  cd java/factorial
+
+  cat > BUILD <<'EOF'
+java_library(
+    name = "fact",
+    srcs = ["Factorial.java"],
+)
+java_test(
+    name = "fact-test",
+    size = "small",
+    srcs = ["FactorialTest.java"],
+    test_class = "factorial.FactorialTest",
+    deps = [
+        ":fact",
+    ],
+)
+EOF
+
+  cat > Factorial.java <<'EOF'
+package factorial;
+public class Factorial {
+  public static int factorial(int x) {
+    return x <= 0 ? 1 : x * factorial(x-1);
+  }
+}
+EOF
+
+  cat > FactorialTest.java <<'EOF'
+package factorial;
+import static org.junit.Assert.*;
+import org.junit.Test;
+public class FactorialTest {
+  @Test
+  public void testFactorialOfZeroIsOne() throws Exception {
+    assertEquals(Factorial.factorial(3),6);
+  }
+}
+EOF
+  cd ../..
+
+  cat $(rlocation io_bazel/src/test/shell/bazel/testdata/jdk_http_archives) >> WORKSPACE
+
+  bazel coverage \
+    --test_output=all \
+    --experimental_fetch_all_coverage_outputs \
+    --experimental_split_coverage_postprocessing \
+    --remote_download_minimal \
+    --combined_report=lcov \
+    --spawn_strategy=remote \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --instrumentation_filter=//java/factorial \
+    //java/factorial:fact-test &> $TEST_log || fail "Shouldn't fail"
+
+  # Test binary shouldn't be downloaded
+  [[ ! -e "bazel-bin/java/factorial/libfact.jar" ]] || fail "bazel-bin/java/factorial/libfact.jar shouldn't exist!"
+
+  local expected_result="SF:java/factorial/Factorial.java
+FN:2,factorial/Factorial::<init> ()V
+FN:4,factorial/Factorial::factorial (I)I
+FNDA:0,factorial/Factorial::<init> ()V
+FNDA:1,factorial/Factorial::factorial (I)I
+FNF:2
+FNH:1
+BRDA:4,0,0,1
+BRDA:4,0,1,1
+BRF:2
+BRH:2
+DA:2,0
+DA:4,1
+LH:1
+LF:2
+end_of_record"
+  cat bazel-testlogs/java/factorial/fact-test/coverage.dat > $TEST_log
+  expect_log "$expected_result"
+  cat bazel-out/_coverage/_coverage_report.dat > $TEST_log
+  expect_log "$expected_result"
+}
+
 run_suite "Build without the Bytes tests"