Add a flag to declare coverage dir as tree artifact

This adds an experimental flag that changes the behavior of tests when
run with coverage such that the _coverage directory is declared as a
tree artifact. This makes it significantly easier to debug coverage
runs when sandboxing (the default) or remote execution are enabled.
Without this, the files are implicitly ignored, and only the
coverage.dat file is brought back into the output tree. If for some
reason the additional steps to generate the coverage.dat file fail, then
that the file is empty or non-existent.

This also provides a workaround for languages that cannot currently
generate lcov format coverage data. By bringing back the individual
files, the user can use a post-process to merge the data.

It's not perfect, but it's simple and effective. I used it myself to
debug a coverage issue due to incorrect file paths w/ gcc on MacOS.

Unfortunately, the coverage collector script generates a number of
temporary files in the _coverage directory which we generally do not
want (or need) to bring back into the output tree. This will have to be
addressed separately.

PiperOrigin-RevId: 291940271

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

@@ -382,10 +382,15 @@ private TestParams createTestAction(int shards) {
             ruleContext.getPackageRelativeArtifact(dir.getRelative("test.cache_status"), root);
 
         Artifact.DerivedArtifact coverageArtifact = null;
+        Artifact coverageDirectory = null;
         if (collectCodeCoverage) {
           coverageArtifact =
               ruleContext.getPackageRelativeArtifact(dir.getRelative("coverage.dat"), root);
           coverageArtifacts.add(coverageArtifact);
+          if (testConfiguration.fetchAllCoverageOutputs()) {
+            coverageDirectory =
+                ruleContext.getPackageRelativeTreeArtifact(dir.getRelative("_coverage"), root);
+          }
         }
 
         boolean cancelConcurrentTests =
@@ -422,6 +427,7 @@ private TestParams createTestAction(int shards) {
                 testLog,
                 cacheStatus,
                 coverageArtifact,
+                coverageDirectory,
                 testProperties,
                 extraTestEnv,
                 executionSettings,

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

@@ -241,6 +241,16 @@ public static class TestOptions extends FragmentOptions {
     )
     public Label coverageReportGenerator;
 
+    @Option(
+        name = "experimental_fetch_all_coverage_outputs",
+        defaultValue = "false",
+        documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+        effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOADING_AND_ANALYSIS},
+        help =
+            "If true, then Bazel fetches the entire coverage data directory for each test during a "
+                + "coverage run.")
+    public boolean fetchAllCoverageOutputs;
+
     @Override
     public FragmentOptions getHost() {
       TestOptions hostOptions = (TestOptions) getDefault();
@@ -338,6 +348,10 @@ public boolean cancelConcurrentTests() {
     return options.cancelConcurrentTests;
   }
 
+  public boolean fetchAllCoverageOutputs() {
+    return options.fetchAllCoverageOutputs;
+  }
+
   /**
    * Option converter that han handle two styles of value for "--runs_per_test":
    *

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

@@ -115,6 +115,7 @@ public class TestRunnerAction extends AbstractAction
   private final PathFragment testInfrastructureFailure;
   private final PathFragment baseDir;
   private final Artifact coverageData;
+  @Nullable private final Artifact coverageDirectory;
   private final TestTargetProperties testProperties;
   private final TestTargetExecutionSettings executionSettings;
   private final int shardNum;
@@ -163,6 +164,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
       Artifact testLog,
       Artifact cacheStatus,
       Artifact coverageArtifact,
+      @Nullable Artifact coverageDirectory,
       TestTargetProperties testProperties,
       Map<String, String> extraTestEnv,
       TestTargetExecutionSettings executionSettings,
@@ -178,7 +180,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
         NestedSetBuilder.wrap(Order.STABLE_ORDER, tools),
         inputs,
         runfilesSupplier,
-        nonNullAsSet(testLog, cacheStatus, coverageArtifact),
+        nonNullAsSet(testLog, cacheStatus, coverageArtifact, coverageDirectory),
         configuration.getActionEnvironment());
     Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null));
     this.testSetupScript = testSetupScript;
@@ -190,6 +192,7 @@ private static ImmutableSet<Artifact> nonNullAsSet(Artifact... artifacts) {
     this.testLog = testLog;
     this.cacheStatus = cacheStatus;
     this.coverageData = coverageArtifact;
+    this.coverageDirectory = coverageDirectory;
     this.shardNum = shardNum;
     this.runNumber = runNumber;
     this.testProperties = Preconditions.checkNotNull(testProperties);
@@ -257,7 +260,10 @@ public List<ActionInput> getSpawnOutputs() {
     outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsManifestPath()));
     outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsAnnotationsPath()));
     if (isCoverageMode()) {
-      outputs.add(getCoverageData());
+      outputs.add(coverageData);
+      if (coverageDirectory != null) {
+        outputs.add(coverageDirectory);
+      }
     }
     return outputs;
   }
@@ -702,11 +708,18 @@ public boolean isCoverageMode() {
    * execution with exception of the locally generated *.gcda files. Those are stored separately
    * using relative path within coverage directory.
    *
-   * <p>The directory name for the given test runner action is constructed as: {@code
+   * <p>If the coverageDirectory field is set, then its exec path is returned. This is a tree
+   * artifact, meaning that all files in the corresponding directories are returned from sandboxed
+   * or remote execution.
+   *
+   * <p>Otherwise, the directory name for the given test runner action is constructed as: {@code
    * _coverage/target_path/test_log_name} where {@code test_log_name} is usually a target name but
    * potentially can include extra suffix, such as a shard number (if test execution was sharded).
    */
   public PathFragment getCoverageDirectory() {
+    if (coverageDirectory != null) {
+      return coverageDirectory.getExecPath();
+    }
     return COVERAGE_TMP_ROOT.getRelative(
         FileSystemUtils.removeExtension(getTestLog().getRootRelativePath()));
   }

src/test/shell/bazel/bazel_coverage_sh_test.sh

@@ -533,4 +533,28 @@ end_of_record"
   assert_coverage_result "$coverage_result_orange_lib" "$coverage_file_path"
 }
 
+function test_coverage_as_tree_artifact() {
+  cat <<'EOF' > BUILD
+sh_test(
+    name = "pull",
+    srcs = ["pull-test.sh"],
+)
+EOF
+  cat <<'EOF' > pull-test.sh
+#!/bin/bash
+touch $COVERAGE_DIR/foo.txt
+# We need a non-empty coverage.dat file for the checks below to work.
+echo "FN:2,com/google/orange/orangeLib::<init> ()V" > $COVERAGE_OUTPUT_FILE
+exit 0
+EOF
+  chmod +x pull-test.sh
+
+  bazel coverage --test_output=all --experimental_fetch_all_coverage_outputs //:pull &>$TEST_log \
+      || fail "Coverage failed"
+
+  local coverage_file_path="$(dirname $( get_coverage_file_path_from_test_log ))/_coverage/foo.txt"
+  [[ -e $coverage_file_path ]] || fail "Cannot find extra file"
+}
+
+
 run_suite "test tests"