Fix source pathing for code coverage (#1006)

* Fix source pathing for code coverage

* lint fixes hopefully?

* lint? idk what's going on here

* Rename in_out_pair to records

* Remove bad assumptions about how bazel works

* Clean up lingering srcs

Clean up lingering srcs

* this commit should fail tests

* this commit should pass tests

* let this test actually fail the CI

* update comments

* Refactor src_paths

* spelling is hard

scala/private/phases/phase_coverage.bzl

@@ -36,25 +36,26 @@ def _phase_coverage(ctx, p, srcjars):
         output_jar = ctx.actions.declare_file(
             "{}-offline.jar".format(input_jar.basename.split(".")[0]),
         )
-        in_out_pairs = [
-            (input_jar, output_jar),
+        srcs_paths = [src.path for src in ctx.files.srcs]
+        records = [
+            (input_jar, output_jar, srcs_paths),
         ]
 
         args = ctx.actions.args()
-        args.add_all(in_out_pairs, map_each = _jacoco_offline_instrument_format_each)
+        args.add_all(records, map_each = _jacoco_offline_instrument_format_each)
         args.set_param_file_format("multiline")
         args.use_param_file("@%s", use_always = True)
 
         ctx.actions.run(
             mnemonic = "JacocoInstrumenter",
-            inputs = [in_out_pair[0] for in_out_pair in in_out_pairs],
-            outputs = [in_out_pair[1] for in_out_pair in in_out_pairs],
+            inputs = [record[0] for record in records],
+            outputs = [record[1] for record in records],
             executable = ctx.attr._code_coverage_instrumentation_worker.files_to_run,
             execution_requirements = {"supports-workers": "1"},
             arguments = [args],
         )
 
-        replacements = {i: o for (i, o) in in_out_pairs}
+        replacements = {i: o for (i, o, _) in records}
         provider = _coverage_replacements_provider.create(
             replacements = replacements,
         )
@@ -72,5 +73,5 @@ def _phase_coverage(ctx, p, srcjars):
             },
         )
 
-def _jacoco_offline_instrument_format_each(in_out_pair):
-    return (["%s=%s" % (in_out_pair[0].path, in_out_pair[1].path)])
+def _jacoco_offline_instrument_format_each(records):
+    return (["%s=%s=%s" % (records[0].path, records[1].path, ",".join(records[2]))])

src/java/io/bazel/rulesscala/coverage/instrumenter/JacocoInstrumenter.java

@@ -52,11 +52,12 @@ public void processRequest(List < String > args) {
 
     private void processArg(Instrumenter jacoco, String arg) throws Exception {
         String[] parts = arg.split("=");
-        if (parts.length != 2) {
-            throw new Exception("expected `in_path=out_path` form for argument: " + arg);
+        if (parts.length != 3) {
+            throw new Exception("expected `in_path=out_path=srcs` form for argument: " + arg);	
         }
         Path inPath = Paths.get(parts[0]);
         Path outPath = Paths.get(parts[1]);
+        String srcs = parts[2];
         try (
             FileSystem inFS = FileSystems.newFileSystem(inPath, null); FileSystem outFS = FileSystems.newFileSystem(
                 URI.create("jar:" + outPath.toUri()), Collections.singletonMap("create", "true"));
@@ -69,6 +70,21 @@ private void processArg(Instrumenter jacoco, String arg) throws Exception {
                     throw new RuntimeException(e);
                 }
             });
+
+            /* 
+            * https://github.com/bazelbuild/bazel/blob/567ca633d016572f5760bfd027c10616f2b8c2e4/src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoCoverageRunner.java#L411
+            * 
+            * Bazel / JacocoCoverageRunner will look for any file that ends with '-paths-for-coverage.txt' within the JAR to be later used for reconstructing the path for source files.
+            * This is a fairly undocumented feature within bazel at this time, but in essence, it opens all the jars, searches for all files matching '-paths-for-coverage.txt'
+            * and then adds them to a single in memory set. 
+            * 
+            * https://github.com/bazelbuild/bazel/blob/567ca633d016572f5760bfd027c10616f2b8c2e4/src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoLCOVFormatter.java#L70
+            * Which is then used in the formatter to find the corresponding source file from the set of sources we wrote in all the JARs.
+            */
+            Files.write(
+                outFS.getPath("-paths-for-coverage.txt"),
+                srcs.replace(",", "\n").getBytes(java.nio.charset.StandardCharsets.UTF_8)
+            );
         }
     }
 

test/coverage/A1.scala

@@ -1,3 +1,5 @@
+package coverage;
+
 object A1 {
   def a1(flag: Boolean): B1.type =
     if (flag) B1

test/coverage/A2.scala

@@ -1,7 +1,8 @@
+package coverage;
+
 object A2 {
   def a2(): Unit = {
-    println("a2: " + 
-      "" // B2.b2_a()
+    println("a2: " + B2.b2_a()
     )
   }
 }

test/coverage/B1.scala

@@ -1,3 +1,5 @@
+package coverage;
+
 object B1 {
 
   def not_called(): Unit = {

test/coverage/B2.java

@@ -1,3 +1,5 @@
+package coverage;
+
 class B2 {
     public static String b2_a() {
         return C2.c2("hello from b2_a");

test/coverage/BUILD

@@ -62,20 +62,10 @@ scala_library(
         "A2.scala",
     ],
     deps = [
-        # TODO :: Understand why referencing a local java library breaks coverage
-        # ":b2",
+        ":b2",
     ],
 )
 
-#
-# As it stands I can't seem to generate coverage for Java libraries pulled into
-# a scala_test target.
-#
-# The java_library is instrumented, but doesn't have the .uninstrumented files
-# that Bazel seems to expect. There are a few code paths for code coverage, so
-# down the road we can explore how to fix this...
-#
-
 java_library(
     name = "b2",
     srcs = [

test/coverage/C2.scala

@@ -1,3 +1,5 @@
+package coverage;
+
 object C2 {
   def c2(input: String): String =
     input.reverse

test/coverage/TestAll.scala

@@ -1,3 +1,4 @@
+package coverage;
 import org.scalatest._
 
 class TestAll extends FlatSpec {

test/coverage/TestB2.java

@@ -1,3 +1,5 @@
+package coverage;
+
 import org.junit.Test;
 import org.junit.Assert.*;
 

test/coverage/expected-coverage.dat

@@ -1,78 +1,110 @@
-SF:/A1.scala
-FN:-1,A1$::<clinit> ()V
-FN:5,A1$::<init> ()V
-FN:3,A1$::a1 (Z)LB1$;
-FN:-1,A1::a1 (Z)LB1$;
-FNDA:1,A1$::<clinit> ()V
-FNDA:1,A1$::<init> ()V
-FNDA:1,A1$::a1 (Z)LB1$;
-FNDA:0,A1::a1 (Z)LB1$;
+SF:test/coverage/A1.scala
+FN:-1,coverage/A1$::<clinit> ()V
+FN:7,coverage/A1$::<init> ()V
+FN:5,coverage/A1$::a1 (Z)Lcoverage/B1$;
+FN:-1,coverage/A1::a1 (Z)Lcoverage/B1$;
+FNDA:1,coverage/A1$::<clinit> ()V
+FNDA:1,coverage/A1$::<init> ()V
+FNDA:1,coverage/A1$::a1 (Z)Lcoverage/B1$;
+FNDA:0,coverage/A1::a1 (Z)Lcoverage/B1$;
 FNF:4
 FNH:3
-BA:3,2
+BA:5,2
 BRF:1
 BRH:1
-DA:3,4
-DA:4,0
-DA:5,5
+DA:5,4
+DA:6,0
+DA:7,5
 LH:2
 LF:3
 end_of_record
-SF:/A2.scala
-FN:-1,A2$::<clinit> ()V
-FN:7,A2$::<init> ()V
-FN:3,A2$::a2 ()V
-FN:-1,A2::a2 ()V
-FNDA:1,A2$::<clinit> ()V
-FNDA:1,A2$::<init> ()V
-FNDA:1,A2$::a2 ()V
-FNDA:0,A2::a2 ()V
+SF:test/coverage/A2.scala
+FN:-1,coverage/A2$::<clinit> ()V
+FN:8,coverage/A2$::<init> ()V
+FN:5,coverage/A2$::a2 ()V
+FN:-1,coverage/A2::a2 ()V
+FNDA:1,coverage/A2$::<clinit> ()V
+FNDA:1,coverage/A2$::<init> ()V
+FNDA:1,coverage/A2$::a2 ()V
+FNDA:0,coverage/A2::a2 ()V
 FNF:4
 FNH:3
-DA:3,4
-DA:7,5
+DA:5,11
+DA:8,5
 LH:2
 LF:2
 end_of_record
-SF:/B1.scala
-FN:-1,B1$::<clinit> ()V
-FN:7,B1$::<init> ()V
-FN:4,B1$::not_called ()V
-FN:-1,B1::not_called ()V
-FNDA:1,B1$::<clinit> ()V
-FNDA:1,B1$::<init> ()V
-FNDA:0,B1$::not_called ()V
-FNDA:0,B1::not_called ()V
+SF:test/coverage/B1.scala
+FN:-1,coverage/B1$::<clinit> ()V
+FN:9,coverage/B1$::<init> ()V
+FN:6,coverage/B1$::not_called ()V
+FN:-1,coverage/B1::not_called ()V
+FNDA:1,coverage/B1$::<clinit> ()V
+FNDA:1,coverage/B1$::<init> ()V
+FNDA:0,coverage/B1$::not_called ()V
+FNDA:0,coverage/B1::not_called ()V
 FNF:4
 FNH:2
-DA:4,0
-DA:7,5
+DA:6,0
+DA:9,5
+LH:1
+LF:2
+end_of_record
+SF:test/coverage/B2.java
+FN:3,coverage/B2::<init> ()V
+FN:5,coverage/B2::b2_a ()Ljava/lang/String;
+FN:9,coverage/B2::b2_b ()V
+FNDA:0,coverage/B2::<init> ()V
+FNDA:1,coverage/B2::b2_a ()Ljava/lang/String;
+FNDA:0,coverage/B2::b2_b ()V
+FNF:3
+FNH:1
+DA:3,0
+DA:5,3
+DA:9,0
+DA:10,0
 LH:1
+LF:4
+end_of_record
+SF:test/coverage/C2.scala
+FN:-1,coverage/C2$::<clinit> ()V
+FN:7,coverage/C2$::<init> ()V
+FN:5,coverage/C2$::c2 (Ljava/lang/String;)Ljava/lang/String;
+FN:-1,coverage/C2::c2 (Ljava/lang/String;)Ljava/lang/String;
+FNDA:1,coverage/C2$::<clinit> ()V
+FNDA:1,coverage/C2$::<init> ()V
+FNDA:1,coverage/C2$::c2 (Ljava/lang/String;)Ljava/lang/String;
+FNDA:1,coverage/C2::c2 (Ljava/lang/String;)Ljava/lang/String;
+FNF:4
+FNH:4
+DA:5,9
+DA:7,5
+LH:2
 LF:2
 end_of_record
-SF:/TestAll.scala
-FN:10,TestAll$$anonfun$1::<init> (LTestAll;)V
-FN:10,TestAll$$anonfun$1::apply ()V
-FN:10,TestAll$$anonfun$1::apply$mcV$sp ()V
-FN:6,TestAll$$anonfun$2::<init> (LTestAll;)V
-FN:6,TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion;
-FN:3,TestAll::<init> ()V
-FNDA:1,TestAll$$anonfun$1::<init> (LTestAll;)V
-FNDA:1,TestAll$$anonfun$1::apply ()V
-FNDA:1,TestAll$$anonfun$1::apply$mcV$sp ()V
-FNDA:1,TestAll$$anonfun$2::<init> (LTestAll;)V
-FNDA:1,TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion;
-FNDA:1,TestAll::<init> ()V
+SF:test/coverage/TestAll.scala
+FN:11,coverage/TestAll$$anonfun$1::<init> (Lcoverage/TestAll;)V
+FN:11,coverage/TestAll$$anonfun$1::apply ()V
+FN:11,coverage/TestAll$$anonfun$1::apply$mcV$sp ()V
+FN:7,coverage/TestAll$$anonfun$2::<init> (Lcoverage/TestAll;)V
+FN:7,coverage/TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion;
+FN:4,coverage/TestAll::<init> ()V
+FNDA:1,coverage/TestAll$$anonfun$1::<init> (Lcoverage/TestAll;)V
+FNDA:1,coverage/TestAll$$anonfun$1::apply ()V
+FNDA:1,coverage/TestAll$$anonfun$1::apply$mcV$sp ()V
+FNDA:1,coverage/TestAll$$anonfun$2::<init> (Lcoverage/TestAll;)V
+FNDA:1,coverage/TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion;
+FNDA:1,coverage/TestAll::<init> ()V
 FNF:6
 FNH:6
-BA:6,2
+BA:7,2
 BRF:1
 BRH:1
-DA:3,2
-DA:5,22
-DA:6,51
-DA:9,23
-DA:10,13
+DA:4,2
+DA:6,22
+DA:7,51
+DA:10,23
+DA:11,13
 LH:5
 LF:5
 end_of_record