rules_scala icon indicating copy to clipboard operation
rules_scala copied to clipboard

Coverage on 2.12 might have issues

Open ittaiz opened this issue 4 years ago • 20 comments

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 https://github.com/bazelbuild/rules_scala/pull/1054 for more details.

ittaiz avatar Jun 12 '20 08:06 ittaiz

cc @gergelyfabian

ittaiz avatar Jun 12 '20 08:06 ittaiz

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 avatar Jun 12 '20 11:06 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 avatar Jun 12 '20 12:06 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 https://github.com/jacoco/jacoco/pull/232).

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

gergelyfabian avatar Jun 12 '20 12:06 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 https://github.com/jacoco/jacoco/issues/885#issuecomment-494388663) 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 avatar Jun 15 '20 08:06 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 https://github.com/jacoco/jacoco/pull/912 and then later improved in https://github.com/jacoco/jacoco/pull/922.

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

gergelyfabian avatar Jun 15 '20 09:06 gergelyfabian

Nice! How do you think we should proceed?

ittaiz avatar Jun 15 '20 09:06 ittaiz

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 avatar Jun 15 '20 12:06 gergelyfabian

I tried outside of Bazel...

Inspiration is coming from https://github.com/jacoco/jacoco/issues/885#issuecomment-494388663. 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 avatar Jun 15 '20 12:06 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 https://github.com/jacoco/jacoco/pull/912).

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

gergelyfabian avatar Jun 15 '20 12:06 gergelyfabian

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

ittaiz avatar Jun 15 '20 15:06 ittaiz

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 avatar Jun 16 '20 05:06 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 avatar Jun 16 '20 07:06 gergelyfabian

Opened an issue on Bazel to upgrade Jacoco to 0.8.5: https://github.com/bazelbuild/bazel/issues/11674

gergelyfabian avatar Jun 30 '20 06:06 gergelyfabian

Thanks, well done!

On Tue, Jun 30, 2020 at 9:03 AM Gergely Fábián [email protected] wrote:

Opened an issue on Bazel to upgrade Jacoco to 0.8.5: bazelbuild/bazel#11674 https://github.com/bazelbuild/bazel/issues/11674

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/1056#issuecomment-651561221, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQQF3JLI4ORR2K2BKJW2TRZF52JANCNFSM4N4DN2NQ .

--

Ittai Zeidman

40 Hanamal street, Tel Aviv, Israel

http://www.wix.com

ittaiz avatar Jun 30 '20 07:06 ittaiz

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";
  1. Build jacoco (mvn clean install).
  2. 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.
  3. Run in Bazel repo: bazel build src/java_tools/junitrunner/java/com/google/testing/coverage:JacocoCoverage_jarjar_deploy.jar
  4. Copy JacocoCoverage_jarjar_deploy.jar to your project (e.g. to tools/ in bazel-scala-example)
  5. 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).
  6. 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 avatar Dec 07 '20 07:12 gergelyfabian

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

gergelyfabian avatar Dec 15 '20 06:12 gergelyfabian

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

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

gergelyfabian avatar Dec 16 '20 13:12 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 avatar Jan 11 '21 07:01 gergelyfabian

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

These workarounds were dropped and #1172 was chosen instead.

gergelyfabian avatar Jan 11 '21 07:01 gergelyfabian