Coverage on 2.12 might have issues

[ittaiz]

Issue is vague because we're not really sure what happened.
When running our coverage test on 2.12 we found that the coverage file changed.
When we compared the files we saw significant changes which we can't explain for now.
This is an issue to track progress of understanding whether this is a bug or just different class generation between 2.11 and 2.12
See #1054 for more details.

[ittaiz]

cc @gergelyfabian

[gergelyfabian]

I'm checking on this issue.
It seems the origin of the problem is rather in the direction of what the built jar contains, not how it's then instrumented.

I compared test/coverage/d1-offline.jar for 2.11 and 2.12 and indeed there are "missing" class files for 2.12 in the jar, like D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1.class or D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1.class.uninstrumented.

However the same differences are already visible in test/coverage/d1.jar. Imho, it's not the Jacoco instrumentation that causes these difference between 2.11 and 2.12.

2.12's d1.jar contains:

├── coverage
│   ├── D1.class
│   └── D1$.class
└── META-INF
    └── MANIFEST.MF

2.11's d1.jar contains:

├── coverage
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$10936c17251556f2f03c90b1524bd9b3$$$$pply$29$$anonfun$apply$30$$anonfun$apply$31$$anonfun$apply$32.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$621bf134f561eb6561b13bf774a82312$$$$apply$9$$anonfun$apply$10$$anonfun$apply$11$$anonfun$apply$12$$anonfun$apply$13$$anonfun$apply$14$$anonfun$apply$15$$anonfun$apply$16.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$621bf134f561eb6561b13bf774a82312$$$$apply$9$$anonfun$apply$10$$anonfun$apply$11$$anonfun$apply$12$$anonfun$apply$13$$anonfun$apply$14$$anonfun$apply$15.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$621bf134f561eb6561b13bf774a82312$$$$apply$9$$anonfun$apply$10$$anonfun$apply$11$$anonfun$apply$12$$anonfun$apply$13$$anonfun$apply$14.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$621bf134f561eb6561b13bf774a82312$$$$apply$9$$anonfun$apply$10$$anonfun$apply$11$$anonfun$apply$12$$anonfun$apply$13.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$621bf134f561eb6561b13bf774a82312$$$$apply$9$$anonfun$apply$10$$anonfun$apply$11$$anonfun$apply$12.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$813cbed17422695f842dcc7b538b5fbd$$$$pply$14$$anonfun$apply$15$$anonfun$apply$16$$anonfun$apply$17$$anonfun$apply$18$$anonfun$apply$19$$anonfun$apply$20$$anonfun$apply$21.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$813cbed17422695f842dcc7b538b5fbd$$$$pply$14$$anonfun$apply$15$$anonfun$apply$16$$anonfun$apply$17$$anonfun$apply$18$$anonfun$apply$19$$anonfun$apply$20.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$813cbed17422695f842dcc7b538b5fbd$$$$pply$14$$anonfun$apply$15$$anonfun$apply$16$$anonfun$apply$17$$anonfun$apply$18$$anonfun$apply$19.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$813cbed17422695f842dcc7b538b5fbd$$$$pply$14$$anonfun$apply$15$$anonfun$apply$16$$anonfun$apply$17$$anonfun$apply$18.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$813cbed17422695f842dcc7b538b5fbd$$$$pply$14$$anonfun$apply$15$$anonfun$apply$16$$anonfun$apply$17.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$8376f2f8ad4b1b2c863b51f7a969d2fd$$$$pply$19$$anonfun$apply$20$$anonfun$apply$21$$anonfun$apply$22$$anonfun$apply$23$$anonfun$apply$24$$anonfun$apply$25$$anonfun$apply$26.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$8376f2f8ad4b1b2c863b51f7a969d2fd$$$$pply$19$$anonfun$apply$20$$anonfun$apply$21$$anonfun$apply$22$$anonfun$apply$23$$anonfun$apply$24$$anonfun$apply$25.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$8376f2f8ad4b1b2c863b51f7a969d2fd$$$$pply$19$$anonfun$apply$20$$anonfun$apply$21$$anonfun$apply$22$$anonfun$apply$23$$anonfun$apply$24.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$8376f2f8ad4b1b2c863b51f7a969d2fd$$$$pply$19$$anonfun$apply$20$$anonfun$apply$21$$anonfun$apply$22$$anonfun$apply$23.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$8376f2f8ad4b1b2c863b51f7a969d2fd$$$$pply$19$$anonfun$apply$20$$anonfun$apply$21$$anonfun$apply$22.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$dab1c931f824ef7de5939f126155866$$$$pply$24$$anonfun$apply$25$$anonfun$apply$26$$anonfun$apply$27$$anonfun$apply$28$$anonfun$apply$29$$anonfun$apply$30$$anonfun$apply$31.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$dab1c931f824ef7de5939f126155866$$$$pply$24$$anonfun$apply$25$$anonfun$apply$26$$anonfun$apply$27$$anonfun$apply$28$$anonfun$apply$29$$anonfun$apply$30.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$dab1c931f824ef7de5939f126155866$$$$pply$24$$anonfun$apply$25$$anonfun$apply$26$$anonfun$apply$27$$anonfun$apply$28$$anonfun$apply$29.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$dab1c931f824ef7de5939f126155866$$$$pply$24$$anonfun$apply$25$$anonfun$apply$26$$anonfun$apply$27$$anonfun$apply$28.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$ap$$$$dab1c931f824ef7de5939f126155866$$$$pply$24$$anonfun$apply$25$$anonfun$apply$26$$anonfun$apply$27.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5$$anonfun$apply$6$$anonfun$apply$7$$anonfun$apply$8$$anonfun$apply$9$$anonfun$apply$10$$anonfun$apply$11.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5$$anonfun$apply$6$$anonfun$apply$7$$anonfun$apply$8$$anonfun$apply$9$$anonfun$apply$10.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5$$anonfun$apply$6$$anonfun$apply$7$$anonfun$apply$8$$anonfun$apply$9.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5$$anonfun$apply$6$$anonfun$apply$7$$anonfun$apply$8.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5$$anonfun$apply$6$$anonfun$apply$7.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5$$anonfun$apply$6.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4$$anonfun$apply$5.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3$$anonfun$apply$4.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2$$anonfun$apply$3.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1$$anonfun$apply$2.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1$$anonfun$apply$1.class
│   ├── D1$$anonfun$veryLongFunctionNameIsHereAaaaaaaaa$1.class
│   ├── D1.class
│   └── D1$.class
└── META-INF
    └── MANIFEST.MF

The instrumented jars are similar in that they contain instrumented class files respectively.

And, most importantly, it's not the case, that the code from D1.scala is not executed. I tested that manually, it's executed, but it seems it's compiled in such a way, that coverage cannot detect that it was run.

[gergelyfabian]

Going further... I found this:

https://users.scala-lang.org/t/has-a-loop-recurs-comprehension-been-considered-for-scala/4787/21

"It was desugared this way in Scala 2.11 and earlier. Scala 2.12 uses low-level Java 8 constructs for creating lightweight lambdas, but they have the same semantics as anonymous inner classes shown above."

I think the issue is about the compilation of lightweight lambdas. The example in test/coverage/D1.scala is a big for comprehension, that is desugared in Scala to a combination of flatMap-withFilter-map with lambdas. I'm not sure how coverage is implemented in Bazel (or for Scala in Bazel), but we may need to adjust for an API change between Scala 2.11 and 2.12:

https://www.scala-lang.org/news/2.12.0/

"Scala and Java 8 interop is also improved for functional code, as methods that take functions can easily be called in both directions using lambda syntax. The FunctionN classes in Scala’s standard library are now Single Abstract Method (SAM) types, and all SAM types are treated uniformly – from type checking through code generation. No class file is generated for a lambda; invokedynamic is used instead." (highlighting by me)

[gergelyfabian]

Maybe there is an issue with Jacoco?

https://stackoverflow.com/questions/45674950/jacoco-need-special-handling-fro-lambdas
https://stackoverflow.com/questions/32284326/code-coverage-for-lambda-function

Also in Jacoco release notes a similar issue was mentioned:

https://www.jacoco.org/jacoco/trunk/doc/changes.html

Release 0.7.2 (2014/09/12)
Fixed Bugs
Do not ignore synthetic lambda methods to get code coverage for Java 8 lambda expressions (GitHub jacoco/jacoco#232).

However, I see, that most problably Bazel now uses Jacoco 0.8.3.

[gergelyfabian]

I believe this is a bug on Jacoco.

Looking at the implementation of the synthetic Lambda fix in Jacoco (https://github.com/jacoco/jacoco/pull/232/files) it seems there is a filter on lambda names, that they have to start with name.startsWith("lambda$"), otherwise they are filtered out from coverage results.

I've added a Java example (from jacoco/jacoco#885 (comment)) to my rules_scala repo, and coverage for the lambdas there is properly detected.

Then, I tried decompiling the code for D1.scala and Example.java:

---

For Scala: javap -c -p coverage.D1$:

Includes:
public static final scala.collection.immutable.List $anonfun$veryLongFunctionNameIsHereAaaaaaaaa$32(scala.collection.immutable.List, java.lang.String); (and a lot more, $32 counting down)

Clearly this method's name is not starting with "lambda$", but rather "$anonfun$".

---

For Java: javap -c -p coverage.Example:

Includes:
private static void lambda$example2$1();

This method's name starts with "lambda$".

---

So I'd claim, that Jacoco is prepared for how the Java compiler names the lambdas, but it isn't prepared for how the Scala 2.12 compiler names the lambdas, and then those get ignored in the coverage.

[gergelyfabian]

Current master of Jacoco seems to have support for Scala lambdas:

https://github.com/jacoco/jacoco/blob/6bcce6972b8939d7925c4b4d3df785d9a7b00007/org.jacoco.core/src/org/jacoco/core/internal/analysis/filter/SyntheticFilter.java

This was fixed in jacoco/jacoco#912 and then later improved in jacoco/jacoco#922.

Both changes are part of 0.8.5 release of Jacoco (Bazel still uses Jacoco 0.8.3).

[ittaiz]

Nice!
How do you think we should proceed?

[gergelyfabian]

I'm having hard time to try out this new Jacoco version, so it's simply theory I wrote about, before I'd have some hard proof that indeed this new Jacoco version works. I tried to replace the Jacoco version to 0.8.5 locally (in Bazel), but I haven't been successful (or the 0.8.5 version also does not work - but I doubt that).

[gergelyfabian]

I tried outside of Bazel...

Inspiration is coming from jacoco/jacoco#885 (comment).
I've downloaded http://search.maven.org/remotecontent?filepath=org/jacoco/jacoco/0.8.5/jacoco-0.8.5.zip and http://search.maven.org/remotecontent?filepath=org/jacoco/jacoco/0.8.3/jacoco-0.8.3.zip.

Modified test/coverage/D1.scala to add a main method to it:

  def main(args: Array[String]) {
    D1.veryLongFunctionNameIsHereAaaaaaaaa()
  }

Steps:

# Unpack jacoco archive (in this case 0.8.5).
unzip ~/Downloads/jacoco-0.8.5.zip
mkdir classes
# Added a main method to test/coverage/D1.scala and moved it to src/ folder.
scalac src/D1.scala -d classes
java -javaagent:lib/jacocoagent.jar -cp ~/opt/scala-2.12.10/lib/scala-library.jar:classes D1
java -jar lib/jacococli.jar report jacoco.exec --classfiles classes --sourcefiles src --html report
# Check the Jacoco report (a bit different graphical interface than for LCOV).
firefox report/index.html

1. When using Jacoco 0.8.3:
   a) Missed instructions: 3 of 32
   b) only the first line of veryLongFunctionNameIsHereAaaaaaaaa is covered.
2. When using Jacoco 0.8.5:
   a) Missed instructions: 3 of 295
   b) all lines of veryLongFunctionNameIsHereAaaaaaaaa are covered.

I'd suggest to create an issue on Bazel (or whereever else necessary) to upgrade to Jacoco 0.8.5 (at least to fix this bug for Scala).

[gergelyfabian]

Issue is vague because we're not really sure what happened.
When running our coverage test on 2.12 we found that the coverage file changed.
When we compared the files we saw significant changes which we can't explain for now.
This is an issue to track progress of understanding whether this is a bug or just different class generation between 2.11 and 2.12
See #1054 for more details.

I think now we know what happened.

Summary: Scala 2.12 changes class generation comparing to Scala 2.11, and it uses Java bytecode for native lambdas, but their names in the bytecode are different from Java conventional naming.
Jacoco before version 0.8.5 wasn't prepared for that (this bug was fixed in jacoco/jacoco#912).

We'd need to upgrade Jacoco version in Bazel to get this fixed.

[ittaiz]

Thanks!
Well done! 👍🏽
Can’t we control the jacoco version ourselves? AFK but that sounds strange

[gergelyfabian]

Thanks!
Well done! 👍🏽
Can’t we control the jacoco version ourselves? AFK but that sounds strange

In Bazel itself Jacoco version is in:

third_party/java/jacoco/BUILD seems to be the main definition. Jacoco jars are saved in third_party/java/jacoco/*.jar.

src/java_tools/junitrunner/java/com/google/testing/coverage/BUILD:

-        "//third_party/java/jacoco:blaze-agent-0.8.3",
-        "//third_party/java/jacoco:core-0.8.3",
-        "//third_party/java/jacoco:report-0.8.3",
+        "//third_party/java/jacoco:blaze-agent-0.8.5",
+        "//third_party/java/jacoco:core-0.8.5",
+        "//third_party/java/jacoco:report-0.8.5",

In src/main/java/com/google/devtools/build/lib/bazel/rules/java/jdk.WORKSPACE remote_java_tools_linux is included with http_archive, that also has Jacoco defined at 0.8.3 (in the package itself). The package includes for Jacoco a similar structure to third_party/java/jacoco.

tools/jdk/BUILD.java_tools also includes references to Jacoco version.

And surely that's not all, because changing the above did not suffice for me. I'm not sure we could override all of these from rules_scala. I believe Bazel should have a single setting for Jacoco, as many languages use it, e.g. not only Scala, but Java and Kotlin too.

[gergelyfabian]

CC: @iirina, what do you think, is there any other way to fix this issue, than upgrading Jacoco in Bazel to 0.8.5?

[gergelyfabian]

Opened an issue on Bazel to upgrade Jacoco to 0.8.5: bazelbuild/bazel#11674

[ittaiz]

Thanks, well done!

…

On Tue, Jun 30, 2020 at 9:03 AM Gergely Fábián ***@***.***> wrote: Opened an issue on Bazel to upgrade Jacoco to 0.8.5: bazelbuild/bazel#11674 <bazelbuild/bazel#11674> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1056 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF3JLI4ORR2K2BKJW2TRZF52JANCNFSM4N4DN2NQ> .

-- *Ittai Zeidman* 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>

[gergelyfabian]

I have the workaround for this issue. It can be fixed by using jacocorunner property of java_toolchain.

Example project: https://github.com/gergelyfabian/bazel-scala-example/tree/jacoco_coverage_fix

Steps so far:

1. Check out https://github.com/gergelyfabian/jacoco/tree/0.8.3-scala-lambda-fix (has the Scala fixes backported to 0.8.3) - this is to avoid API changes in Jacoco
2. Change in org.jacoco.build/pom.xml (Bazel seems to depend on this hard-coded jacoco pkgName):

-                pkgName = buildNumber.substring(buildNumber.length() - 7, buildNumber.length());
+                pkgName = "1f1cc91";

3. Build jacoco (mvn clean install).
4. Uncompress the built jacoco zip file, move and rename jacoco agent jar, org.jacoco.agent jar, org.jacoco.core jar, org.jacoco.report jar to third_party/java/jacoco/org.jacoco.core-0.8.3.jar in Bazel repo.
5. Run in Bazel repo: bazel build src/java_tools/junitrunner/java/com/google/testing/coverage:JacocoCoverage_jarjar_deploy.jar
6. Copy JacocoCoverage_jarjar_deploy.jar to your project (e.g. to tools/ in bazel-scala-example)
7. rules_scala needs a change, as the jacocorunner on scala_test is hardcoded. For now, use customized rules_scala from https://github.com/gergelyfabian/rules_scala/tree/custom_jacocorunner (to be able to override jacocorunner).
8. In bazel-scala-example: tools/coverage.sh and open the report with Firefox.

For me this workaround is ok until Bazel gets Jacoco upgraded officially.

There is a question though, how should we approach jacocorunner in scala_test?
Should it use the jacocorunner from the Java toolchain or should we change _jacocorunner to jacocorunner in scala_test properties to enable callers override the hardcoded path? (as I did in https://github.com/gergelyfabian/rules_scala/tree/custom_jacocorunner)

[gergelyfabian]

Discussing workarounds for the rules_scala part of the issue (in #1163 and #1166).

[gergelyfabian]

If anyone is interested, a loosely related topic...

I created a PR on Jacoco to improve Scala case class code coverage: jacoco/jacoco#1136.
Any such fix could be taken up in Bazel with the very same instructions from the above comments.

[gergelyfabian]

Workaround is using scala_toolchain with jacocorunner property (#1172).
You can use a script to build custom jacocorunner that includes the fix for Scala 2.12+ lambdas (#1173).

[gergelyfabian]

Discussing workarounds for the rules_scala part of the issue (in #1163 and #1166).

These workarounds were dropped and #1172 was chosen instead.