Provide arguments to JacocoInstrumenter without using delimiter (#1252)

Currently files provided as arguments for JacocoInstrumenter are separated by '='. That leads to problems if files or target names contain '=' in names.

Author: jauniusb

examples/testing/multi_frameworks_toolchain/BUILD

@@ -56,6 +56,7 @@ declare_deps_provider(
     deps_id = "specs2_junit_classpath",
     visibility = ["//visibility:public"],
     deps = [
+        "@io_bazel_rules_scala//scala:bazel_test_runner_deploy",
         "@io_bazel_rules_scala_org_specs2_specs2_junit",
     ],
 )

scala/private/phases/phase_coverage.bzl

@@ -36,26 +36,24 @@ def _phase_coverage(ctx, p, srcjars):
         output_jar = ctx.actions.declare_file(
             "{}-offline.jar".format(input_jar.basename.split(".")[0]),
         )
-        srcs_paths = [src.path for src in ctx.files.srcs]
-        records = [
-            (input_jar, output_jar, srcs_paths),
-        ]
 
         args = ctx.actions.args()
-        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)
+        args.add(input_jar)
+        args.add(output_jar)
+        args.add_all(ctx.files.srcs)
 
         ctx.actions.run(
             mnemonic = "JacocoInstrumenter",
-            inputs = [record[0] for record in records],
-            outputs = [record[1] for record in records],
+            inputs = [input_jar],
+            outputs = [output_jar],
             executable = ctx.attr._code_coverage_instrumentation_worker.files_to_run,
             execution_requirements = {"supports-workers": "1"},
             arguments = [args],
         )
 
-        replacements = {i: o for (i, o, _) in records}
+        replacements = {input_jar: output_jar}
         provider = _coverage_replacements_provider.create(
             replacements = replacements,
         )
@@ -72,6 +70,3 @@ def _phase_coverage(ctx, p, srcjars):
                 "InstrumentedFilesInfo": instrumented_files_provider,
             },
         )
-
-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

@@ -5,6 +5,7 @@
 import io.bazel.rulesscala.worker.Worker;
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
+import java.io.UnsupportedEncodingException;
 import java.nio.file.FileSystem;
 import java.nio.file.FileSystems;
 import java.nio.file.FileVisitResult;
@@ -17,6 +18,10 @@
 import java.nio.file.attribute.BasicFileAttributes;
 import org.jacoco.core.instr.Instrumenter;
 import org.jacoco.core.runtime.OfflineInstrumentationAccessGenerator;
+import java.net.URLDecoder;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
 
 public final class JacocoInstrumenter implements Worker.Interface {
 
@@ -27,19 +32,17 @@ public static void main(String[] args) throws Exception {
     @Override
     public void work(String[] args) throws Exception {
         Instrumenter jacoco = new Instrumenter(new OfflineInstrumentationAccessGenerator());
-        for (String arg : args) {
-            processArg(jacoco, arg);
-	}
+        processArg(jacoco, args);
     }
 
-    private void processArg(Instrumenter jacoco, String arg) throws Exception {
-        String[] parts = arg.split("=");
-        if (parts.length != 3) {
-            throw new Exception("expected `in_path=out_path=srcs` form for argument: " + arg);	
+    private void processArg(Instrumenter jacoco, String[] args) throws Exception {
+        if (args.length < 3) {
+            throw new Exception("expected format `in_path out_path src1 src2 ... srcN`  for arguments: " + Arrays.asList(args));
         }
-        Path inPath = Paths.get(parts[0]);
-        Path outPath = Paths.get(parts[1]);
-        String srcs = parts[2];
+
+        Path inPath = Paths.get(args[0]);
+        Path outPath = Paths.get(args[1]);
+        String[] srcs = Arrays.copyOfRange(args, 2, args.length);
 
         // Use a directory for coverage metadata that is unique to each built jar. Avoids
         // multiple threads performing read/write/delete actions on the instrumented classes directory.
@@ -73,7 +76,7 @@ private void processArg(Instrumenter jacoco, String arg) throws Exception {
             Path pathsForCoverage = instrumentedClassesDirectory.resolve("-paths-for-coverage.txt");
             Files.write(
                 pathsForCoverage,
-                srcs.replace(",", "\n").getBytes(java.nio.charset.StandardCharsets.UTF_8)
+                String.join("\n", srcs).getBytes(java.nio.charset.StandardCharsets.UTF_8)
             );
 
             jarCreator.addEntry(instrumentedClassesDirectory.relativize(pathsForCoverage).toString(), pathsForCoverage);

test/coverage_filename_encoding/A1.scala

@@ -0,0 +1,7 @@
+package coverage_filename_encoding
+
+object A1 {
+  def a1(flag: Boolean): String =
+    if (flag) "B1"
+    else sys.error("oh noes")
+}

test/coverage_filename_encoding/BUILD.bazel

@@ -0,0 +1,20 @@
+load("//scala:scala.bzl", "scala_library", "scala_specs2_junit_test")
+
+scala_specs2_junit_test(
+    name = "name-with-equals",
+    size = "small",
+    srcs = ["Test.scala"],
+    prefixes = ["Test"],
+    deps = [
+        "a1%3D=%3D%2Bagg",
+    ],
+)
+
+scala_library(
+    name = "a1%3D=%3D%2Bagg",
+    srcs = [
+        "A1.scala",
+    ],
+    deps = [
+    ],
+)

test/coverage_filename_encoding/Test.scala

@@ -0,0 +1,10 @@
+package coverage_filename_encoding
+
+import org.specs2.mutable.SpecWithJUnit
+import org.specs2.specification.Scope
+
+class Test extends SpecWithJUnit {
+  "testA1" in new Scope {
+    A1.a1(true) must_!= 1
+  }
+}

test/coverage_filename_encoding/expected-coverage.dat

@@ -0,0 +1,21 @@
+SF:test/coverage_filename_encoding/A1.scala
+FN:-1,coverage_filename_encoding/A1$::<clinit> ()V
+FN:3,coverage_filename_encoding/A1$::<init> ()V
+FN:5,coverage_filename_encoding/A1$::a1 (Z)Ljava/lang/String;
+FN:-1,coverage_filename_encoding/A1::a1 (Z)Ljava/lang/String;
+FNDA:1,coverage_filename_encoding/A1$::<clinit> ()V
+FNDA:1,coverage_filename_encoding/A1$::<init> ()V
+FNDA:1,coverage_filename_encoding/A1$::a1 (Z)Ljava/lang/String;
+FNDA:0,coverage_filename_encoding/A1::a1 (Z)Ljava/lang/String;
+FNF:4
+FNH:3
+BA:5,2
+BRF:1
+BRH:1
+DA:3,1
+DA:5,4
+DA:6,1
+DA:7,4
+LH:4
+LF:4
+end_of_record

test/coverage_specs2_with_junit/BUILD

@@ -11,6 +11,7 @@ scala_specs2_junit_test(
         ":a2",
         ":b1",
         ":d1",
+        ":e1",
     ],
 )
 
@@ -64,3 +65,15 @@ scala_library(
         "D1.scala",
     ],
 )
+
+scala_library(
+    name = "e1",
+    srcs = [
+        "A1.scala",
+        "D1.scala",
+        "E1.scala",
+    ],
+    deps = [
+        ":b1",
+    ],
+)

test/coverage_specs2_with_junit/E1.scala

@@ -0,0 +1,6 @@
+package coverage_specs2_with_junit
+
+object E1 {
+  def e1(input: String): String =
+    input.reverse
+}

test/coverage_specs2_with_junit/TestWithSpecs2WithJUnit.scala

@@ -15,4 +15,8 @@ class TestWithSpecs2WithJUnit extends SpecWithJUnit {
   "testD1" in new Scope {
     D1.veryLongFunctionNameIsHereAaaaaaaaa()
   }
+
+  "testE1" in new Scope {
+    E1.e1("test")
+  }
 }