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

@@ -2518,6 +2518,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_configuration",
  srcs = ["test/CoverageConfiguration.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

@@ -771,7 +771,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

@@ -203,6 +203,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

@@ -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"