Allow Java coverage collection for external targets.

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"