Make bazel coverage work with minimal mode

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

@@ -2536,6 +2536,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,19 @@
+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,11 @@ 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

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
@@ -50,8 +51,10 @@
 import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.server.FailureDetails.TestAction;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.FileOutErr;
+import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -65,8 +68,10 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.time.Duration;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.TreeMap;
 
 /** Runs TestRunnerAction actions. */
@@ -714,14 +719,22 @@ private TestAttemptResult runTestAttempt(
         closeSuppressed(e, fileOutErr);
         throw e;
       }
-      actionExecutionContext
-          .getMetadataHandler()
-          .getMetadata(testAction.getCoverageDirectoryTreeArtifact());
-      ImmutableSet<? extends ActionInput> expandedCoverageDir =
+      Path coverageDir =
           actionExecutionContext
-              .getMetadataHandler()
-              .getTreeArtifactChildren(
-                  (SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact());
+              .getPathResolver()
+              .convertPath(testAction.getCoverageDirectoryTreeArtifact().getPath());
+      Set<ActionInput> expandedCoverageDir = new HashSet<>();
+      TreeArtifactValue.visitTree(
+          coverageDir,
+          ((parentRelativePath, type) -> {
+            if (type == Dirent.Type.DIRECTORY) {
+              return;
+            }
+            expandedCoverageDir.add(
+                TreeFileArtifact.createTreeOutput(
+                    (SpecialArtifact) testAction.getCoverageDirectoryTreeArtifact(),
+                    parentRelativePath));
+          }));
       Spawn coveragePostProcessingSpawn =
           createCoveragePostProcessingSpawn(
               actionExecutionContext,
@@ -771,7 +784,10 @@ private TestAttemptResult runTestAttempt(
 
     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

@@ -202,6 +202,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//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:test/coverage_report",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value",
         "//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/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

@@ -1583,4 +1583,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"