Allow Java coverage collection for external targets. (#17360)

A roll-forward of #16268.

LazyWritePathsFileAction has been changed to accept a custom converter
parameter that can be used to specify exactly what path should be
written out to the file. This is required to support the use case of an
internal rule that still needs root-relative paths written out.

The default case (when no converter is specified) is to output the exec
path, as per the original PR.

PiperOrigin-RevId: 505678128
Change-Id: Ib1d8547dcb359a9e0fb7e462850424ec4218f7fb

Co-authored-by: Googler <[email protected]>

Author: fmeum

src/main/java/com/google/devtools/build/lib/analysis/actions/LazyWritePathsFileAction.java

@@ -29,41 +29,46 @@
 import com.google.devtools.build.lib.util.Fingerprint;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.util.function.Function;
 import javax.annotation.Nullable;
 
 /**
- * Lazily writes the exec path of the given files separated by newline into a specified output file.
+ * Lazily writes the path of the given files separated by newline into a specified output file.
+ *
+ * <p>By default the exec path is written but this behaviour can be customized by providing an
+ * alternative converter function.
  */
 public final class LazyWritePathsFileAction extends AbstractFileWriteAction {
   private static final String GUID = "6be94d90-96f3-4bec-8104-1fb08abc2546";
 
   private final NestedSet<Artifact> files;
   private final ImmutableSet<Artifact> filesToIgnore;
   private final boolean includeDerivedArtifacts;
+  private final Function<Artifact, String> converter;
 
   public LazyWritePathsFileAction(
       ActionOwner owner,
       Artifact output,
       NestedSet<Artifact> files,
       ImmutableSet<Artifact> filesToIgnore,
       boolean includeDerivedArtifacts) {
-    // TODO(ulfjack): It's a bad idea to have these two constructors do slightly different things.
-    super(owner, files, output, false);
-    this.files = files;
-    this.includeDerivedArtifacts = includeDerivedArtifacts;
-    this.filesToIgnore = filesToIgnore;
+    this(owner, output, files, filesToIgnore, includeDerivedArtifacts, Artifact::getExecPathString);
   }
 
   public LazyWritePathsFileAction(
       ActionOwner owner,
       Artifact output,
-      ImmutableSet<Artifact> files,
+      NestedSet<Artifact> files,
       ImmutableSet<Artifact> filesToIgnore,
-      boolean includeDerivedArtifacts) {
+      boolean includeDerivedArtifacts,
+      Function<Artifact, String> converter) {
+    // We don't need to pass the given files as explicit inputs to this action; we don't care about
+    // them, we only need their names, which we already know.
     super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), output, false);
-    this.files = NestedSetBuilder.<Artifact>stableOrder().addAll(files).build();
+    this.files = files;
     this.includeDerivedArtifacts = includeDerivedArtifacts;
     this.filesToIgnore = filesToIgnore;
+    this.converter = converter;
   }
 
   @Override
@@ -94,10 +99,14 @@ private String getContents() {
         continue;
       }
       if (file.isSourceArtifact() || includeDerivedArtifacts) {
-        stringBuilder.append(file.getRootRelativePathString());
+        stringBuilder.append(converter.apply(file));
         stringBuilder.append("\n");
       }
     }
     return stringBuilder.toString();
   }
+
+  public NestedSet<Artifact> getFiles() {
+    return files;
+  }
 }

src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java

@@ -365,7 +365,7 @@ && getJavaConfiguration().experimentalEnableJspecify()
           new LazyWritePathsFileAction(
               ruleContext.getActionOwner(),
               coverageArtifact,
-              sourceFiles,
+              NestedSetBuilder.<Artifact>stableOrder().addAll(sourceFiles).build(),
               /* filesToIgnore= */ ImmutableSet.of(),
               false));
     }

src/test/shell/bazel/bazel_coverage_java_test.sh

@@ -1143,4 +1143,176 @@ end_of_record"
   assert_coverage_result "$coverage_result_num_lib_header" "$coverage_file_path"
 }
 
+function setup_external_java_target() {
+  cat >> WORKSPACE <<'EOF'
+local_repository(
+    name = "other_repo",
+    path = "other_repo",
+)
+EOF
+
+  cat > BUILD <<'EOF'
+java_library(
+    name = "math",
+    srcs = ["src/main/com/example/Math.java"],
+    visibility = ["//visibility:public"],
+)
+EOF
+
+  mkdir -p src/main/com/example
+  cat > src/main/com/example/Math.java <<'EOF'
+package com.example;
+
+public class Math {
+
+  public static boolean isEven(int n) {
+    return n % 2 == 0;
+  }
+}
+EOF
+
+  mkdir -p other_repo
+  touch other_repo/WORKSPACE
+
+  cat > other_repo/BUILD <<'EOF'
+java_library(
+    name = "collatz",
+    srcs = ["src/main/com/example/Collatz.java"],
+    deps = ["@//:math"],
+)
+
+java_test(
+    name = "test",
+    srcs = ["src/test/com/example/TestCollatz.java"],
+    test_class = "com.example.TestCollatz",
+    deps = [":collatz"],
+)
+EOF
+
+  mkdir -p other_repo/src/main/com/example
+  cat > other_repo/src/main/com/example/Collatz.java <<'EOF'
+package com.example;
+
+public class Collatz {
+
+  public static int getCollatzFinal(int n) {
+    if (n == 1) {
+      return 1;
+    }
+    if (Math.isEven(n)) {
+      return getCollatzFinal(n / 2);
+    } else {
+      return getCollatzFinal(n * 3 + 1);
+    }
+  }
+
+}
+EOF
+
+  mkdir -p other_repo/src/test/com/example
+  cat > other_repo/src/test/com/example/TestCollatz.java <<'EOF'
+package com.example;
+
+import static org.junit.Assert.assertEquals;
+import org.junit.Test;
+
+public class TestCollatz {
+
+  @Test
+  public void testGetCollatzFinal() {
+    assertEquals(Collatz.getCollatzFinal(1), 1);
+    assertEquals(Collatz.getCollatzFinal(5), 1);
+    assertEquals(Collatz.getCollatzFinal(10), 1);
+    assertEquals(Collatz.getCollatzFinal(21), 1);
+  }
+
+}
+EOF
+}
+
+function test_external_java_target_can_collect_coverage() {
+  setup_external_java_target
+
+  bazel coverage --test_output=all @other_repo//:test --combined_report=lcov \
+   --instrumentation_filter=// &>$TEST_log \
+      || echo "Coverage for //:test failed"
+
+  local coverage_file_path="$(get_coverage_file_path_from_test_log)"
+  local expected_result_math='SF:src/main/com/example/Math.java
+FN:3,com/example/Math::<init> ()V
+FN:6,com/example/Math::isEven (I)Z
+FNDA:0,com/example/Math::<init> ()V
+FNDA:1,com/example/Math::isEven (I)Z
+FNF:2
+FNH:1
+BRDA:6,0,0,1
+BRDA:6,0,1,1
+BRF:2
+BRH:2
+DA:3,0
+DA:6,1
+LH:1
+LF:2
+end_of_record'
+  local expected_result_collatz="SF:external/other_repo/src/main/com/example/Collatz.java
+FN:3,com/example/Collatz::<init> ()V
+FN:6,com/example/Collatz::getCollatzFinal (I)I
+FNDA:0,com/example/Collatz::<init> ()V
+FNDA:1,com/example/Collatz::getCollatzFinal (I)I
+FNF:2
+FNH:1
+BRDA:6,0,0,1
+BRDA:6,0,1,1
+BRDA:9,0,0,1
+BRDA:9,0,1,1
+BRF:4
+BRH:4
+DA:3,0
+DA:6,1
+DA:7,1
+DA:9,1
+DA:10,1
+DA:12,1
+LH:5
+LF:6
+end_of_record"
+
+  assert_coverage_result "$expected_result_math" "$coverage_file_path"
+  assert_coverage_result "$expected_result_collatz" "$coverage_file_path"
+
+  assert_coverage_result "$expected_result_math" bazel-out/_coverage/_coverage_report.dat
+  assert_coverage_result "$expected_result_collatz" bazel-out/_coverage/_coverage_report.dat
+}
+
+function test_external_java_target_coverage_not_collected_by_default() {
+  setup_external_java_target
+
+  bazel coverage --test_output=all @other_repo//:test --combined_report=lcov &>$TEST_log \
+      || echo "Coverage for //:test failed"
+
+  local coverage_file_path="$(get_coverage_file_path_from_test_log)"
+  local expected_result_math='SF:src/main/com/example/Math.java
+FN:3,com/example/Math::<init> ()V
+FN:6,com/example/Math::isEven (I)Z
+FNDA:0,com/example/Math::<init> ()V
+FNDA:1,com/example/Math::isEven (I)Z
+FNF:2
+FNH:1
+BRDA:6,0,0,1
+BRDA:6,0,1,1
+BRF:2
+BRH:2
+DA:3,0
+DA:6,1
+LH:1
+LF:2
+end_of_record'
+
+  assert_coverage_result "$expected_result_math" "$coverage_file_path"
+  assert_not_contains "SF:external/other_repo/" "$coverage_file_path"
+
+  assert_coverage_result "$expected_result_math" bazel-out/_coverage/_coverage_report.dat
+  assert_not_contains "SF:external/other_repo/" bazel-out/_coverage/_coverage_report.dat
+}
+
 run_suite "test tests"