[6.1.0] Make bazel coverage work with minimal mode (bazelbuild#17397)

* Returns null if filesystem of test outputs is not ActionFS when processing test attempt event. Previously, we assert that the filesystem of test outputs is ActionFS when we are processing test attempt event. However this is not true when the test hits action cache. This CL looses the check to return null. PiperOrigin-RevId: 501023752 Change-Id: I17cbb26e0a2b5fd30cb781818e42172ac672919e * 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 --------- Co-authored-by: kshyanashree <[email protected]>
fmeum · Feb 7, 2023 · 27bc896 · 27bc896
1 parent 0759081
commit 27bc896
Show file tree

Hide file tree

Showing 9 changed files with 192 additions and 28 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/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"],

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/CoverageReport.java b/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;
+  }
+}
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
@@ -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",

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
@@ -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);

diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/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);
 

diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/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",

diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java
@@ -930,11 +930,11 @@ public void executorInit(CommandEnvironment env, BuildRequest request, ExecutorB
               actionInputFetcher,
               (path) -> {
                 FileSystem fileSystem = path.getFileSystem();
-                Preconditions.checkState(
-                    fileSystem instanceof RemoteActionFileSystem,
-                    "fileSystem must be an instance of RemoteActionFileSystem");
-                return ((RemoteActionFileSystem) path.getFileSystem())
-                    .getRemoteMetadata(path.asFragment());
+                if (fileSystem instanceof RemoteActionFileSystem) {
+                  return ((RemoteActionFileSystem) path.getFileSystem())
+                      .getRemoteMetadata(path.asFragment());
+                }
+                return null;
               });
       env.getEventBus().register(toplevelArtifactsDownloader);
     }

diff --git a/src/main/java/com/google/devtools/build/lib/remote/ToplevelArtifactsDownloader.java b/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,32 +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.
-      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);
     }
   }
 
@@ -168,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);

diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/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"