Add preliminary support for code coverage #692

andyscott · 2019-02-16T02:15:48Z

This adds preliminary support for code coverage with Jacoco.

This will likely require some iteration before it's extremely robust. Ideally some of that iteration can happen in follow up work in separate PRs, since the code in this PR should mostly suffice for Scala centric projects.

I haven't tested this on any production codebases yet.

andyscott · 2019-02-16T02:24:35Z

This looks like it's just flat out failing with the older version of Bazel. Hopefully the 0.22.0 build passes.

johnynek · 2019-02-16T02:27:21Z

@softprops you did some work on this in the past, right? Do you have any time to weigh in on the PR?

andyscott · 2019-02-16T02:29:29Z

I should add that I chose to implement this slightly different compared to the Java rules:
A provider is used to "swap out" regular artifacts for instrumented artifacts when they're finally needed. The intention was to be as lazy as possible, and avoid any risk of recompilation of Scala artifacts.
Conversely, the Java rules simply emit an instrumented artifact when coverage is turned on.

johnynek · 2019-02-16T02:30:08Z

@andyscott can you give a quick overview of the approach? I did a quick scan, but I haven't been following the bazel coverage support. Can you help reviewers by providing some links for us to read on how that is supposed to work?

I'm very excited about this!

andyscott · 2019-02-16T02:32:59Z

As far as I can tell, there's no public documentation on how code coverage works internally in Bazel. This implementation was based off of information I gleaned from:

JacocoCoverageRunner, which wraps the test runner and handles emitting lcov files: https://github.com/bazelbuild/bazel/blob/0.22.0/src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoCoverageRunner.java
The java launcher template: https://github.com/bazelbuild/bazel/blob/0.22.0/src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt
Kotlin + Jacoco in Bazel: https://github.com/bazelbuild/rules_kotlin/pull/52/files
Lots of git grepping through the Bazel repo :).

It's currently EOD for me. I can circle back next week and hopefully provide more info.

ittaiz · 2019-02-16T08:24:06Z

@iirina you are the Bazel coverage go-to person, right? Any chance you can give us your 2c about why you chose your direction and what you think about using a provider? I think (not sure) that google are very cautious about adding a provider due to the memory footprint this adds

…

On Sat, 16 Feb 2019 at 4:33 Andy Scott ***@***.***> wrote: As far as I can tell, there's no public documentation on how code coverage works in Bazel. This implementation was based off of information I gleaned from: - JacocoCoverageRunner, which wraps the test runner and handles emitting lcov files: https://github.com/bazelbuild/bazel/blob/0.22.0/src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoCoverageRunner.java - The java launcher template: https://github.com/bazelbuild/bazel/blob/0.22.0/src/main/java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt - Kotlin + Jacoco in Bazel: https://github.com/bazelbuild/rules_kotlin/pull/52/files - Lots of git grepping through the Bazel repo :). It's currently EOD for me. I can circle back next week and hopefully provide more info. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#692 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF9RYODBFPYuilUPvAINeulP93Ab7ks5vN23cgaJpZM4a-uiz> .

ittaiz · 2019-02-16T08:30:45Z

I skimmed the code and I’m somewhat worried about the cost of the aspect. @dslomov is there a way to achieve something similar to the aspect declaration on the attribute from command line? I tried using the regular syntax but it’s not the same

ittaiz · 2019-02-16T08:40:46Z

One idea (which I’m fairly certain is possible) is to add a flag to the scala tool chain and only do work in the aspect if the flag is on. This limits the memory part and probably most of the analysis perf cost (not sure how much an aspect with a lookup and empty array return costs)

andyscott · 2019-02-16T17:57:26Z

It's pretty easy to remove the aspect and just conditionally emit an instrumented jar as the primary artifact.

johnynek · 2019-02-16T18:15:04Z

Here is a recent PR on coverage in the haskell rules: tweag/rules_haskell#699

dslomov · 2019-02-18T12:25:13Z

Neither the usage of aspects not providers in this PR should add much to the memory footprint (it is O(size of build graph) and should not be much)
What might be problematic is any flattening of depsets but I do not see it occurring here.

andyscott · 2019-02-26T23:28:56Z

Update: This PR works for us internally at Stripe. There also seems to be an issue with the reproducibility tests for the changes, which I'll look into.

Once that's taken care of, what steps do I need to take to get this to a mergeable state?

johnynek

This is very exciting and one of the biggest feature additions in quite some time. Very nice work.

That said, I'd like to suggest two core changes:

split the PR into three PRs:
a formatting only PR that improves the formatting, but has no other changes. I assume you used some tool to format it. Can we do that on master?
can we add this tools/bazel wrapper and a separate motivation for it.
just the changes relating to code coverage with a minimum amount of formatting change.
I request that you go ahead as part 3. include documentation in the README as to how generate code coverage reports, a brief explanation that there should be no additional cost to running tests in normal mode, and also give some indication of how much longer you should expect the tests to take with coverage enabled.

Thanks for taking this on.

PR: @ittaiz I think you will want to take a good look at this. I assume you all may be interested in providing code coverage reports. I love to use them to make sure I am hitting all the branches in my code in tests, without which I get nervous I have bugs lurking.

johnynek · 2019-03-01T02:41:00Z

scala/private/coverage_replacements_provider.bzl

+    "exports",
+]
+
+def _combine(*entriess, base = {}):


why entriess? Any meaning there?

It's just because it's a 2d array of entry items (or a 1d array of entries). In the comprehension I have:

[ entry[_CoverageReplacements].replacements for entries in entriess for entry in entries if _CoverageReplacements in entry ]

scala/scala.bzl

tools/bazel

scala/private/rule_impls.bzl

andyscott · 2019-03-01T14:42:05Z

Sure thing. `tools/bazel` was an accidental commit. I use it to easily pin to various bazel versions, and I think it got picked up in one of the recent commits. I'll submit a formatting PR and then a new code coverage PR. If you want the version pinning script, I can open a PR for that too (and configure CI to use it).

andyscott · 2019-03-04T18:39:26Z

This should be good to go-- it passes on 0.22.0. A fair amount of this functionality just won't work on older Bazel releases.

andyscott · 2019-03-06T00:05:56Z

All tests are passing now with the exception of the reproducibility tests, due to JMH.

johnynek

I think this looks good. @ittaiz do you have any concerns?

We have run this on our CI, and it worked fine for us.

johnynek · 2019-03-06T00:25:50Z

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

+    }
+
+    @Override
+    public void processRequest(List < String > args) {


weird spacing on the List<String> here.

I can fix it. Not sure how it wound up this way (perhaps my editor's Java formatter got carried away?).

ittaiz

Any chance we can add a flag for this in the toolchain?
I have a bad feeling on the overhead of these loops in the aspect and I'd love a way to turn it off if I'm right. Should be simple to add. WDYT?

ittaiz · 2019-02-27T05:05:53Z

src/java/io/bazel/rulesscala/coverage/instrumenter/BUILD

+)
+
+filegroup(
+    name = "instrumenter_files",


Is this indirection needed?

No, it's just a relic from the original work to develop this code. I can remove it.

should you remove it?

tools/bazel

ittaiz · 2019-02-27T05:10:38Z

scala/private/rule_impls.bzl

+
+    rjars = out.transitive_rjars
+
+    coverage_runfiles = []


This will be needed for junior test as well, right? Is there commonality? Can/should ewe reuse?

What do you mean by junior test?

Junit (damn autocorrect)

Do I need to implement them for junit to have coverage?

johnynek · 2019-03-06T20:24:16Z

can you merge master? I was trying to get this green by rerunning the flakey test. hasn't happened yet.

johnynek · 2019-03-20T21:16:18Z

@andyscott my understanding is that this does not change runtime of build, is that accurate?

I'm in favor of merge. @ittaiz do you have any objections?

andyscott · 2019-03-20T22:22:12Z

It doesn't change the runtime.

For what it's worth, I'm part of the way through feature flagging the dictionary merge loops. I should have that commit ready soon.

andyscott · 2019-03-25T15:13:33Z

@johnynek @ittaiz 👋 just a friendly bump on this one

beala-stripe · 2019-03-25T21:41:27Z

Eager to see this one merged. Thanks everyone!

johnynek · 2019-03-26T03:19:03Z

This looks great. Good work.

I'd like to merge. I'll give @ittaiz 12 more hours to speak up if he or Wix have any concerns.

Thanks for making it configurable (although I do have concerns we have underdocumented how to use all our different options here, and that is hurting adoption).

ittaiz

Thank you for this work and especially for adding the configurability! I appreciate it.
Do y'all have some metrics on how this impacted (if any) your build performance? (analysis time, execution time, etc).
Also I replied to two small existing comments you didn't resolve but since I've been blocking this for a while we can merge as is and either ignore them or do a followup for them.
Your decision.

ittaiz · 2019-03-26T04:30:33Z

src/java/io/bazel/rulesscala/coverage/instrumenter/BUILD

+)
+
+filegroup(
+    name = "instrumenter_files",


should you remove it?

ittaiz · 2019-03-26T04:32:24Z

scala/private/rule_impls.bzl

+
+    rjars = out.transitive_rjars
+
+    coverage_runfiles = []


Do I need to implement them for junit to have coverage?

johnynek · 2019-03-26T05:07:29Z

Thanks @ittaiz I’ll let @andyscott follow up tomorrow but merge so we can iterate.

Also Andy, it would be nice to make sure the README has good docs on how to use this. And also a blog post would be nice to consider Andy.

ittaiz · 2019-03-26T06:34:33Z

+💯 (although I would really appreciate the clarification if junit tests indeed need more work like I think)

…

On Tue, 26 Mar 2019 at 7:07 P. Oscar Boykin ***@***.***> wrote: Thanks @ittaiz <https://github.com/ittaiz> I’ll let @andyscott <https://github.com/andyscott> follow up tomorrow but merge so we can iterate. Also Andy, it would be nice to make sure the README has good docs on how to use this. And also a blog post would be nice to consider Andy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#692 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFxkoA-oxgsQvPlV5tQdfhOntC5l3ks5vaasSgaJpZM4a-uiz> .

andyscott · 2019-03-31T05:38:52Z

Just saw these comments. I'll follow up this work week!

softprops · 2019-05-13T22:26:26Z

Is there any useful README documentation for getting started with rules_scala and code coverage or is the best place to discover that to look for coverage interfaces in the upstream bazel docs?

andyscott requested a review from johnynek as a code owner February 16, 2019 02:15

googlebot added the cla: yes label Feb 16, 2019

Add preliminary support for code coverage

bab69b3

andyscott force-pushed the code-coverage branch from c9f5f2f to bab69b3 Compare February 25, 2019 22:06

Fix conditional bug

6810070

johnynek reviewed Mar 1, 2019

View reviewed changes

andyscott mentioned this pull request Mar 1, 2019

Reformat core files [split from code coverage PR] #707

Merged

andyscott added 3 commits March 1, 2019 07:16

Remove accidental tools/bazel launcher

c2ed1e7

Get rid of default argument

721b7ec

Merge branch 'master' into code-coverage

0fd3981

andyscott added 4 commits March 5, 2019 15:00

Bump bazel versions to 0.18.x+

1cbe4b0

Add back 0.16.x hash

f6dc48e

Revert launcher template version

e291ad8

18 -> 19

14b9593

johnynek approved these changes Mar 6, 2019

View reviewed changes

beala-stripe mentioned this pull request Mar 6, 2019

Prepare for --incompatible_use_toolchain_providers_in_java_common #711

Merged

ittaiz reviewed Mar 6, 2019

View reviewed changes

Merge branch 'master' into code-coverage

738b1b4

andyscott added 3 commits March 20, 2019 16:17

Feature flag coverage logic

9f0faf4

Fix test script

d2761bf

Remove negative case

d508a76

ittaiz approved these changes Mar 26, 2019

View reviewed changes

johnynek merged commit 379c65e into bazelbuild:master Mar 26, 2019

andyscott deleted the code-coverage branch March 31, 2019 05:38

beala-stripe mentioned this pull request Apr 3, 2019

code coverage support #184

Open

gergelyfabian mentioned this pull request Dec 20, 2019

bazel coverage support for Scala bazelbuild/bazel#7109

Closed

Add preliminary support for code coverage #692

Add preliminary support for code coverage #692

Conversation

andyscott commented Feb 16, 2019 • edited Loading

andyscott commented Feb 16, 2019

johnynek commented Feb 16, 2019

andyscott commented Feb 16, 2019 • edited Loading

johnynek commented Feb 16, 2019

andyscott commented Feb 16, 2019 • edited Loading

ittaiz commented Feb 16, 2019 via email

ittaiz commented Feb 16, 2019

ittaiz commented Feb 16, 2019

andyscott commented Feb 16, 2019

johnynek commented Feb 16, 2019

dslomov commented Feb 18, 2019

andyscott commented Feb 26, 2019 • edited Loading

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyscott commented Mar 1, 2019 via email • edited Loading

andyscott commented Mar 4, 2019

andyscott commented Mar 6, 2019

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Mar 6, 2019

johnynek commented Mar 20, 2019

andyscott commented Mar 20, 2019

andyscott commented Mar 25, 2019

beala-stripe commented Mar 25, 2019

johnynek commented Mar 26, 2019

ittaiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Mar 26, 2019

ittaiz commented Mar 26, 2019 via email

andyscott commented Mar 31, 2019

softprops commented May 13, 2019

andyscott commented Feb 16, 2019 •

edited

Loading

andyscott commented Feb 16, 2019 •

edited

Loading

andyscott commented Feb 16, 2019 •

edited

Loading

andyscott commented Feb 26, 2019 •

edited

Loading

andyscott commented Mar 1, 2019 via email •

edited

Loading