bazelbuild / rules_scala

Scala rules for Bazel
Apache License 2.0
364 stars 275 forks source link

code coverage support #184

Open johnynek opened 7 years ago

johnynek commented 7 years ago

This would look like using:

https://github.com/scoverage/scalac-scoverage-plugin

to get the scala test rules to emit code coverage reports, then aggregating them across the repo.

It is related to https://github.com/bazelbuild/bazel/issues/1118 but I don't think it is actually a blocker. We may just be able to produce coverage output of each test rule, then an aspect to walk all the rules and aggregate a total.

softprops commented 7 years ago

Has anyone started working on this? This is going to become a priority from my team soon. We're working on replacing our reliance on sbt in our jenkins pipeline with bazel. One part of that is figuring out the test report and codecov ( scoverage ) stories.

ittaiz commented 7 years ago

I don't know (and think) anyone started working on it but I'll add that there's been a lot of discussion on various coverage issues on bazel itself so I'd recommend anyone tackling this to read a bit beforehand On Fri, 5 May 2017 at 20:40 doug tangren notifications@github.com wrote:

Has anyone started working on this? This is going to become a priority from my team soon. We're working on replacing our reliance on sbt in our jenkins pipeline with bazel. One part of that is figuring out the test report and codecov ( scoverage ) stories.

ā€” You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/184#issuecomment-299528870, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUIF4kLM62g_5DrI10vlWYAwmLVb7Dfks5r217wgaJpZM4NG3kM .

softprops commented 7 years ago

šŸ‘

I was looking through the machinery that ties scoverage into sbt, and it looks like it may be possible to to just opt into code coverage for a regular scala_test by threading through a few additional arguments flags: namely: -Xplugin:path/to/plugin and -P:scoverage:dataDir:path/to/scoverage-data

softprops commented 7 years ago

opt in meaning maybe this is possible today without any additional changes and when the coverage story in bazel settles something like this could then just become a conveniences of being instrumented by default with running bazel cover

johnynek commented 7 years ago

The scala side should be easy. What I don't know is how to plug in to bazel's expectations (if they are even standard). Like where should we write the coverage files to? What should the format be? On Fri, May 5, 2017 at 08:41 doug tangren notifications@github.com wrote:

opt in meaning maybe this is possible today without any additional changes and when the coverage story in bazel settles something like this could then just become a conveniences of being instrumented by default with running bazel cover

ā€” 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/184#issuecomment-299544136, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEJdgmLCVqU5WVE95c7FE6dhkMXP38Tks5r221OgaJpZM4NG3kM .

softprops commented 7 years ago

My guess is it'll be the same as the test_filter flag story. When x is present in the env ( or args ) do y. My guess about where the conventional place to write reports would be customizable with rule dependent defaults, like junit test reports

ittaiz commented 7 years ago

I really suggest searching the Bazel github issues with the term coverage. There have been several discussions about it in the issues. On Fri, 5 May 2017 at 21:56 doug tangren notifications@github.com wrote:

My guess is it'll be the same as the test_filter flag story. When x is present in the env ( or args ) do y. My guess about where the conventional place to write reports would be customizable with rule dependent defaults, like junit test reports

ā€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/184#issuecomment-299547873, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUIFzVFzcvA0v4oPL3Z6UF9Srf6YWNZks5r23DSgaJpZM4NG3kM .

softprops commented 7 years ago

@johnynek do you know ( specifically for scalac ) if there needs to be anything special done for scalac plugins to set up a special kind of classpath separate form the classpath of the files being compiled?

I've now optimistically convinced myself this should be more straightforward that I originally thought so I couldn't help trying out a POC to close out my friday on a happy note.

my WORKSPACE

# other stuff ...

filegroup(
    name = "scoverage_plugin",
    srcs = ["sbt-plugins/scalac-scoverage-plugin_2.11-1.2.0.jar"],
    visibility = ["//visibility:public"],
)

java_import(
    name = "sbt-plugins",
    jars=glob([
    "sbt-plugins/scalac-scoverage*.jar"],
    exclude = [
            "sbt-plugins/scala-library*jar",
            "sbt-plugins/scala-xml*jar"
    ]),
    visibility = ["//visibility:public"],
)

my BUILD

scala_test(
    name = "foo",
    size = "small", 
    srcs = glob(
        [
            "src/test/scala/**/*.scala",
            "src/test/java/**/*.java",
        ],
    ),
    jvm_flags = [
        "-Dfile.encoding=UTF-8",
    ],
    plugins = ["@meetuplib//:scoverage_plugin"], # the scoverage scalac plugin jar
    scalacopts = [
      "-P:scoverage:dataDir:/path/to/target/scovtest/scoverage-data",
      "-Yrangepos"
    ],
   deps = [
      # other stuff....
        "@meetuplib//:sbt-plugins" # puts scoverage deps on my classpath
    ],
)

when I run bazel test -s --progress_report_interval 1 --test_output streamed --strategy=Scalac=standalone //foo:* I get a class not found error on scala.xml.NamespaceBinding. Scala xml is a dependency of scoverage but I've already confirmed it's on my classpath. I checked my foo-test_worker_input file to be sure.

[info] Instrumentation completed [50790 statements]
error: java.lang.NoClassDefFoundError: scala/xml/NamespaceBinding
    at scoverage.ScoverageInstrumentationComponent$$anon$1.run(plugin.scala:121)
    at scala.tools.nsc.Global$Run.compileUnitsInternal(Global.scala:1501)
    at scala.tools.nsc.Global$Run.compileUnits(Global.scala:1486)
    at scala.tools.nsc.Global$Run.compileSources(Global.scala:1481)
    at scala.tools.nsc.Global$Run.compile(Global.scala:1582)
    at scala.tools.nsc.Driver.doCompile(Driver.scala:32)
    at scala.tools.nsc.MainClass.doCompile(Main.scala:23)
    at scala.tools.nsc.Driver.process(Driver.scala:51)
    at io.bazel.rulesscala.scalac.ScalacProcessor.compileScalaSources(ScalacProcessor.java:207)
    at io.bazel.rulesscala.scalac.ScalacProcessor.processRequest(ScalacProcessor.java:77)
    at io.bazel.rulesscala.worker.GenericWorker.run(GenericWorker.java:125)
    at io.bazel.rulesscala.scalac.ScalaCInvoker.main(ScalaCInvoker.java:42)
Caused by: java.lang.ClassNotFoundException: scala.xml.NamespaceBinding
    at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    ... 12 more
cat bazel-out/local-fastbuild/bin/foo/foo-test_worker_input
Classpath: # includes external/scala/lib/scala-xml_2.11-1.0.4.jar among otherthings
EnableIjar: False
Files: # bunch of sources
IjarCmdPath:
IjarOutput:
JarOutput: bazel-out/local-fastbuild/bin/foo/foo-test.jar
JavacOpts:
JavacPath: external/local_jdk/bin/javac
JavaFiles:  # bunch of sources
JvmFlags: -J-Dfile.encoding=UTF-8
Manifest: bazel-out/local-fastbuild/bin/foo/foo-test_MANIFEST.MF
Plugins: external/meetuplib/sbt-plugins/scalac-scoverage-plugin_2.11-1.2.0.jar
PrintCompileTime: False
ResourceDests: /log4j.properties,/malformed-manifest.json,/test-velocity.properties,/valid-manifest.json
ResourceJars:
ResourceSrcs: # bunch of sources
ResourceStripPrefix:
ScalacOpts: -P:scoverage:dataDir:/path/to/target/scovtest/scoverage-data,-Yrangepos
SourceJars:

The only thing that turned up in a lazy google search was this issue that popped on up the intellij mailing list. I didn't quite get the solution in that case or how it may be part of the problem in mine.

sdtwigg commented 7 years ago

I was actually speaking with ahirreddy@ about this the other day. I think the plugin pulls classes from the bootclasspath. Was eventually planning to verify this and fixup the rules to also setup the bootclasspath properly. Edit: You just need to add the dependencies as other (fake) -Xplugin. This makes a more structured deploy_jar construction tempting.

It might be easier just to build the plugin as a deploy_jar for now though.

softprops commented 7 years ago

@sdtwigg I'm more of a consumer than a producer in this case. I'm using the scoverage plugin and runtime jars from maven central and trying to provide them as a plugin value to a scala_test rule. Can you draw me a rough sketch of what you mean?

softprops commented 7 years ago

Now I'm thinking there may be something to do with the bootclasspath which may actually be related to an open bug with scala.

One thing I failed to mention above when setting up my test was that initially I didn't have the scalac-scoverage-runtime jar on my classpath which caused a similar issue. After adding that to my scala_tests' deps, that problem was resolved. The next issue I ran into was the xml.NameSpace error.

I tried the change mentioned in the issue by adding the -nobootcp to my scalacopts. Same issue.

I'm a little confused because the identity of this class technically didn't change between scala 2.10 and 2.11 so either way I expected it to be on the classpath one way or another.

softprops commented 7 years ago

I have a little bit more mental energy to this but I'm still stumped on the scala.xml.NameSpace issue. Though I was reminded that in order for scoverage to instrument code it needs to be applied to my scala_library targets ( not my scala_test ) targets which makes things a bit more awkward

johnynek commented 7 years ago

we should be able to use a target in the current repo as a plugin. If we can't now, that shouldn't be too much work. Then, we could just make a deploy jar ourselves by linking in the full path.

sdtwigg commented 7 years ago

Sorry, I never really described what deploy_jar is although seems like you found out but just in case: The deploy_jar is a fat-jar that contains the current compiled target and all its transitive dependencies (both runtime and compile time). It is analogous to the deploy_jar you can get from a java_binary (although not executable if from scala_library).

I realized over the weekend that just directly using a deploy_jar from one of the rules_scala rules probably won't work because I don't think it will put the plugin.xml in the right place. So, would need to take the deploy_jar from the rules and have another action/genrule put the plugin.xml inside that fat jar.

I evntually wanted to demonstrate this by replacing the linter plugin in the tests with a custom rolled one (both to demonstrate how to make plugins and drop a rules_scala dependency). Unfortunately, I am really reluctant to say I have the time to do this :( . Had spent my 'free-work' time over weekend pushing through the bazel changes outlined in: https://github.com/bazelbuild/rules_scala/issues/57#issuecomment-299323413

PS: Do you think it is a problem that using two plugins might have redundant classes in their fat-jars? (Assuming that the classes are identical duplicates then it seems like it would just be like a 'stylistic' issue versus a functional one.)

softprops commented 7 years ago

How do you make a deploy_jar without a main class, do scalac plugins run with a java main?

Also, I can kind of half confirm the deploy_jar thing works. I cloned the scoverage repo and used sbt-assembly to bake me a fat jar. I referenced that in a java_import and that removed the scala.xml.NamespaceBinding issue.

Next issue is related to sandboxing which is probably just something Im doing wrong. Given the scalacopt for the plugin's data dir ( -P:scoverage:dataDir ), let's call that /path/to/scovtest/scoverage-data/

[info] Instrumentation completed [852 statements]
[info] Wrote instrumentation file [/path/to/scovtest/scoverage-data/scoverage.coverage.xml]
[info] Will write measurement data to [/path/to/scovtest/scoverage-data]
Discovery starting.
*** RUN ABORTED *** (153 milliseconds)
  java.lang.RuntimeException: Unable to load a Suite class that was discovered in the runpath: com.meetup.base.util.StatusesTest
  at org.scalatest.tools.DiscoverySuite$.getSuiteInstance(DiscoverySuite.scala:84)
  at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:38)
  at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:37)
  at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
  at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
  at scala.collection.Iterator$class.foreach(Iterator.scala:893)
  at scala.collection.AbstractIterator.foreach(Iterator.scala:1336)
  at scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
  at scala.collection.AbstractIterable.foreach(Iterable.scala:54)
  at scala.collection.TraversableLike$class.map(TraversableLike.scala:234)
  ...
  Cause: java.io.FileNotFoundException: /path/to/scovtest/scoverage-data/scoverage.measurements.1 (Read-only file system)
  at java.io.FileOutputStream.open0(Native Method)
  at java.io.FileOutputStream.open(FileOutputStream.java:270)
  at java.io.FileOutputStream.<init>(FileOutputStream.java:213)
  at java.io.FileWriter.<init>(FileWriter.java:107)
  at scoverage.Invoker$$anonfun$1.apply(Invoker.scala:42)
  at scoverage.Invoker$$anonfun$1.apply(Invoker.scala:42)
  at scala.collection.concurrent.TrieMap.getOrElseUpdate(TrieMap.scala:901)
  at scoverage.Invoker$.invoked(Invoker.scala:42)
  at com.meetup.base.util.StatusesTest.<init>(StatusesTest.scala:6)
  at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)

This kind of makes sense because sandboxing shouldn't let me write outside. To continue the POC I added --spawn_strategy=standalone

bazel test --spawn_strategy=standalone --verbose_failures  --progress_report_interval 1 --test_output streamed --strategy=Scalac=standalone //base:*

that allows the scalac plugin to generate two files

scoverage.coverage.xml  scoverage.measurements.1

So thats the recording process in a nutshell. Report generation... that's a whole other beast but I just need to confirm that the coverage recordings are all that my jenkins plugin requires. Reports may not be a need to have for me atm.

softprops commented 7 years ago

Unfortunately, I am really reluctant to say I have the time to do this

I'm happy to help but will probably need more direction, I have a decent sense for bazel's architectural model still have some blindspots.

I'm very much in a POC phase right now where Im testing to see if it can work in a timeboxed period of time. The next to think about is probably more relevant to the discussion on bazel core's issue tracker. How should the library rules detect the conditional case for appending the scalac plugin. You typically will only want to do that in some testing mode. What you ship shouldn't have the scoverage instrumentation.

softprops commented 7 years ago

circling back to this. I had a few questions that I think could be answered faster here but perhaps should be asked on the bazel-discuss list.

I think I found some examples of where I can create the scoverage binary jar here. I think that was suggested above. I'm just double check to make sure that's the right pattern to follow.

So the way to know the coverage should be enabled or not is documented here. The problem I'm running into is that I need access to the scoverage jar to conditionally append it to this list. What I haven't seem examples of yet in bazel rules is how to dynamically access a dependency from within an implementation. My guess is that there's something fundamentally wrong with me asking how to do that because bazel needs to run though the analysis phase first before I have the context. That said, pointers are welcome!

This is related to the above. I'll need to provide the scoverage enabled compile run -P:scoverage:dataDir:path/to/scoverage-data scalac plugin arg via the scalacopts list. This should be more straight forward but I think I'll need some flavor of make var eval for getting the location of the test-logs directory for the scala_library. I'm not yet sure where the conventional location to put these files but test-logs somehow seems appropriate.

softprops commented 7 years ago

The answer came to me on the train ride home. I can just declare these as private ( _ ) attr dependencies for rules that depend on _compile. The intuition being that rules can have dependencies that they do not always use

softprops commented 7 years ago

I put together an initial prototype that was intended as a pt 1 of the process to have scoverage record starting measurements here One part I'm getting stuck on is the directory that scoverage should write data to

I'm currently using a subdirectory of testlogs

scoverage_data_dir = "bazel-testlogs/{pkg}/{name}/scoverage-data".format(
   pkg=ctx.label.package,
   name=ctx.label.name
)

This doesn't quite work and I'll allude to why

bazel coverage --test_output streamed //test:CalculatorTest
WARNING: Streamed test output requested. All tests will be run locally, without sharding, one at a time.
INFO: Using default value for --instrumentation_filter: "//test".
INFO: Override the above default with --instrumentation_filter
INFO: Found 1 test target...
INFO: From scala //test:Calculator:
[info] Cleaning datadir [bazel-testlogs/test/Calculator/scoverage-data]
[info] Beginning coverage instrumentation
[info] Instrumentation completed [1 statements]
[info] Wrote instrumentation file [bazel-testlogs/test/Calculator/scoverage-data/scoverage.coverage.xml]
[info] Will write measurement data to [bazel-testlogs/test/Calculator/scoverage-data]
INFO: From scala //scala/support:test_reporter:
[info] Cleaning datadir [bazel-testlogs/scala/support/test_reporter/scoverage-data]
[info] Beginning coverage instrumentation
[warn] Could not instrument [EmptyTree$/null]. No pos.
[info] Instrumentation completed [458 statements]
[info] Wrote instrumentation file [bazel-testlogs/scala/support/test_reporter/scoverage-data/scoverage.coverage.xml]
[info] Will write measurement data to [bazel-testlogs/scala/support/test_reporter/scoverage-data]
An exception or error caused a run to abort. This may have been caused by a problematic custom reporter.
java.io.FileNotFoundException: bazel-testlogs/scala/support/test_reporter/scoverage-data/scoverage.measurements.1 (No such file or directory)
    at java.io.FileOutputStream.open0(Native Method)
    at java.io.FileOutputStream.open(FileOutputStream.java:270)
    at java.io.FileOutputStream.<init>(FileOutputStream.java:213)
    at java.io.FileWriter.<init>(FileWriter.java:107)
    at scoverage.Invoker$$anonfun$1.apply(Invoker.scala:42)
    at scoverage.Invoker$$anonfun$1.apply(Invoker.scala:42)
    at scala.collection.concurrent.TrieMap.getOrElseUpdate(TrieMap.scala:901)
    at scoverage.Invoker$.invoked(Invoker.scala:42)
    at io.bazel.rules.scala.JUnitXmlReporter.<init>(JUnitXmlReporter.scala:45)

note: dispite the fact that scoverage says it wrote the initial coverage.xml file it doesn't actually exist

[info] Wrote instrumentation file [bazel-testlogs/test/Calculator/scoverage-data/scoverage.coverage.xml]

An the reason is test logs are attributed to test packages not library packages. Looking for pointers on an alternative approach for picking this directory.

I did notice that the JUnit xml reporters using and env var to know where to record data. I'm not sure if there's a similar feature for coverage writers. I don't think that skylark rules can access env vars if there was.

softprops commented 7 years ago

status update. I updated my branch with a change that partially addresses the coverage dir problem. I realized that the action that writes the coverage data needs to own the process of creating the directory so I ended up threading the coverage dir as a new CompileOption to let the ScalacProcessor create the path. That gets me the ability to create the instrumentation data

ls -al bazel-out/local-fastbuild/bin/test/Calculator-coverage/scoverage.coverage.xml
-rw-r--r-- 1 doug users 523 May 24 18:57 bazel-out/local-fastbuild/bin/test/Calculator-coverage/scoverage.coverage.xml

But I am still unsure how to provide that directory to the test runner which I think I need to write the measurement data to. Let me know what you folks think of my current direction. I'd like to pivot early in the process of I'm going down the wrong direction.

[info] Cleaning datadir [bazel-out/local-fastbuild/bin/test/Calculator-coverage]
[info] Beginning coverage instrumentation
[info] Instrumentation completed [1 statements]
[info] Wrote instrumentation file [bazel-out/local-fastbuild/bin/test/Calculator-coverage/scoverage.coverage.xml]
[info] Will write measurement data to [bazel-out/local-fastbuild/bin/test/Calculator-coverage]
INFO: From scala //scala/support:test_reporter:
[info] Cleaning datadir [bazel-out/local-fastbuild/bin/scala/support/test_reporter-coverage]
[info] Beginning coverage instrumentation
[warn] Could not instrument [EmptyTree$/null]. No pos.
[info] Instrumentation completed [458 statements]
[info] Wrote instrumentation file [bazel-out/local-fastbuild/bin/scala/support/test_reporter-coverage/scoverage.coverage.xml]
[info] Will write measurement data to [bazel-out/local-fastbuild/bin/scala/support/test_reporter-coverage]
An exception or error caused a run to abort. This may have been caused by a problematic custom reporter.
java.io.FileNotFoundException: bazel-out/local-fastbuild/bin/scala/support/test_reporter-coverage/scoverage.measurements.1 (No such file or directory)
    at java.io.FileOutputStream.open0(Native Method)
    at java.io.FileOutputStream.open(FileOutputStream.java:270)
    at java.io.FileOutputStream.<init>(FileOutputStream.java:213)
    at java.io.FileWriter.<init>(FileWriter.java:107)
    at scoverage.Invoker$$anonfun$1.apply(Invoker.scala:42)
    at scoverage.Invoker$$anonfun$1.apply(Invoker.scala:42)
    at scala.collection.concurrent.TrieMap.getOrElseUpdate(TrieMap.scala:901)
    at scoverage.Invoker$.invoked(Invoker.scala:42)
johnynek commented 7 years ago

I assume there is some env-var similar to test where we are supposed to write the file, and in a particular format, but I really haven't looked. I would google the bazel google group and see the story there.

softprops commented 7 years ago

Following up but still blocked. I posted the bazel discuss list to ask for some direction/input but haven't gotten as much feed back as I've gotten here. Consider this a repost of my current state.

I'm trying to follow along with changes that were brought into 5.0 that may help me get code coverage working for scala.

My current blocker is knowing where to write coverage data to.

If I'm reading this right, this should be the block of code that exposes set of COVERAGE_* environment variables to test runs. I can see that coverage is enabled by inspecting ctx.configuration.coverage_enabled when I run bazel coverage and I believe that I'm setting the right instrumented_files field in the struct scala_library returns, but it's not immediately clear why coverage data may be null, which may be why I'm not seeing those env variables with I run bazel coverage -s

If anyone here has time to take a quick look at what I have so far, below is our diff of rules_scala

https://github.com/meetup/rules_scala/compare/master...scoverage-support

ittaiz commented 7 years ago

Not that familiar with coverage but from my experience the Bazel people monitor the Bazel tag in SO more than the discuss group. Maybe posting there will yield faster results. On Tue, 30 May 2017 at 21:20 doug tangren notifications@github.com wrote:

Following up but still blocked. I posted the bazel discuss list https://groups.google.com/d/msg/bazel-discuss/pJQBD-vWaVo/1K5txXhWCAAJ to ask for some direction/input but haven't gotten as much feed back as I've gotten here. Consider this a repost of my current state.

I'm trying to follow along with changes that were brought into 5.0 that may help me get code coverage working for scala.

My current blocker is knowing where to write coverage data to.

If I'm reading this right, this https://github.com/bazelbuild/bazel/blob/1fd27bc4d97533031401c54aa05eb75b13c6874b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java#L427-L435 should be the block of code that exposes set of COVERAGE_* environment variables to test runs. I can see that coverage is enabled by inspecting ctx.configuration.coverage_enabled when I run bazel coverage and I believe that I'm setting the right instrumented_files field in the struct scala_library returns, but it's not immediately clear why coverage data may be null https://github.com/bazelbuild/bazel/blob/1fd27bc4d97533031401c54aa05eb75b13c6874b/src/main/java/com/google/devtools/build/lib/rules/test/TestRunnerAction.java#L559-L561, which may be why I'm not seeing those env variables with I run bazel coverage -s

If anyone here has time to take a quick look at what I have so far, below is our diff of rules_scala

meetup/rules_scala@master...scoverage-support https://github.com/meetup/rules_scala/compare/master...scoverage-support

ā€” You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/184#issuecomment-304964598, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUIF-iz-48pNg58JGjWg7abgHH2I_ubks5r_F4IgaJpZM4NG3kM .

softprops commented 7 years ago

Thanks @ittaiz linking here for reference

dsilvasc commented 6 years ago

FYI, here's how pants integrates with jacoco for code coverage: https://github.com/pantsbuild/pants/commit/a6437101d9758172307df4084b8512faa09d483f

johnynek commented 6 years ago

http://www.jacoco.org/jacoco/trunk/doc/agent.html

documentation on using the java agent. Seems like it might not be that much work.

softprops commented 6 years ago

Does jacoco work for scala as well? At Meetup I know some engineers use jacoco specifically for java and scoverage for everything else.

Any new news on this enabling this would be great! I kind of dropped my initial effort of getting scoverage to work with bazel in favor of letting a jenkins job using sbt to generated reports be our answer for coverage as we moved our primary building over to bazel. My original scoverage branch for this scala_rules is likely horrible unmergable with this master branch by now but I still have it here for reference.

johnynek commented 6 years ago

It looks like Jacoco works on byte code unless Iā€™m wrong. So it may work for both java and scala.

johnynek commented 6 years ago

I think maybe the only open issue is converting to lcov format, which I think is what bazel is standardizing on.

hchauvin commented 6 years ago

I have a workable draft for code coverage for Kotlin which seamlessly integrates with how Bazel collects code coverage for Java (https://github.com/bazelbuild/rules_kotlin/pull/52/commits/fd27f16b2e2c76a1eeb83f0c7730b827d4b23f42), I think a similar approach could be used here.

These steps work for Kotlin. I don't know much about the Scala toolchain, but I think these steps should work here as well with only a few changes.

softprops commented 6 years ago

I think the lcov format was one of the blockers for me when looking at scoverage.

I think because Jacoco works on byte code it introduces some nuances with scala. In particular it may be less correct with mappings to source code because it may be less aware of the many methods scala synthesizes for types. Here's an old post on the problem. This may have changed. I'd be interested in experimenting though.

hchauvin commented 6 years ago

@softprops The JacocoRunner converts the Jacoco format to LCOV, so this is eventually I suppose what you would have to do.

Concerning using Jacoco with Kotlin, actually I don't know of any better solution with Kotlin, so that's why I used that. You'll get line coverage right, but you'll get missing method coverage for getters/setters that you don't use and missing branch coverage when using the safe call operator (?.). I've learned to trust the line coverage, but I'm less ready to draw conclusions when looking at method/branch coverage.

According to the post you shared, the problem might be more severe with Scala because of traits and overall because the manipulation is much more involved.

Jacoco definitely does not address these concerns yet, and they are on the FilterOptions todo list (https://github.com/jacoco/jacoco/wiki/FilteringOptions).

Maybe in your case it would be best to fork the JacocoRunner so that it not only collects coverage data with the Jacoco agent but also from scoverage?

One remark on your patch: there are two reasons you don't see anything collected:

  1. LcovMerger should be explicitly given as a dependency of your test and binary rules (see the native Java rules, that's what they do), otherwise you revert to a legacy collection of gcno/gcda files for clang coverage:
    "_lcov_merger": attr.label(
    default = Label("@bazel_tools//tools/test:LcovMerger"),
    executable = True,
    cfg = "target",
    ),

The relevant shenanigans are in https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh

  1. For LcovMerger to work, you need to put somewhere within the folder given by the COVERAGE_DIR environment variable a .dat file (it can contain LCOV lines or anything else such as XML, LcovMerger currently does not care). Eventually, LcovMerger should be able, as its name indicates, to merge all the .dat files in the COVERAGE_DIR, but right now it is an error to have either zero or more than one *.dat file.
dsilvasc commented 6 years ago

Back in 2013 someone implemented jacoco filters to ignore some of the generated code for Scala:

http://timezra.blogspot.co.uk/2013/10/jacoco-and-scala.html https://github.com/timezra/jacoco-scala-maven-plugin

hchauvin commented 6 years ago

@dsilvasc This is an interesting option, looking at the maven plugin the filtering mechanism does not seem that involved. I didn't see any PR on Jacoco dealing with Scala using the new filtering mechanism available since January 3rd in 0.8.0, but looking at the pace other filters were incorporated into Jacoco, it might actually be a viable solution.

softprops commented 6 years ago

@hchauvin thanks for posting. To add to the notes about reporting formats, the scalac native scoverage tool can be made to export cobertura xml so consider that a +1 from the scala camp.

hchauvin commented 6 years ago

@softprops You mean that in reference to the fact that Bazel will probably converge as a whole to cobertura xml? https://github.com/bazelbuild/bazel/issues/5246

softprops commented 6 years ago

It will make help make the case for supporting a scala baze code coverage integration simpler. I don't know there is a way to make scoverage emit lcov formatted output.

gdeest commented 6 years ago

Is there any progress on this ? It looks like we'll need it quite soon for one of our projects. cc @aherrmann

greggdonovan commented 5 years ago

@iirina Any advice on how to proceed with Scala coverage now that --experimental_java_coverage is out and lcov has been standardized on? Does it make sense to wait for the coverage_toolchain work before building Scala coverage or is the experimental_java_coverage flag a sufficient foundation? Thanks!

iirina commented 5 years ago

For Scala the --experimental_java_coverage is enough to add coverage. I am working on a document with instructions on how to add coverage support for JVM languages, it should be ready this week.

iirina commented 5 years ago

@greggdonovan I put together this document with the instructions. I want to write a blog post, but to speed things up a doc should work for now.

prebeta commented 5 years ago

Is anyone actively working on bazel scala coverage right now?

It seems like @softprops had a partially working solution that would instrument the classes using scoverage, but there are a few blockers regarding collecting the coverage data and the output format for the coverage data.

I'd be interested in picking things up, but it looks like there are a couple options moving forward. We can implement the coverage using Jacoco and potentially plug into Bazel's JacocoTestRunner or we can continue with the Scoverage changes which may require some custom tooling to handle coverage formats.

dsilvasc commented 5 years ago

@prebeta Which one provides more accurate coverage reports for Scala code?

prebeta commented 5 years ago

@dsilvasc from my initial investigations, the branch/class coverage numbers are pretty much the same between the two. The issue with mixins skewing coverage results in Jacoco seems has been resolved in 8.0+. However, scoverage is able to give more precise statement coverage in the case of single line conditionals, but I'm not sure if this is even translatable into lcov format. Here's an example of scoverage vs jacoco reports.

Scoverage report: https://i.imgur.com/ShYX29f.png Detected 7/10 statements covered: https://i.imgur.com/Z4Epyum.png

Jacoco report: https://i.imgur.com/0YnDnxF.png Detected 7/8 lines covered : https://i.imgur.com/btAdKY5.png

beala-stripe commented 5 years ago

@andyscott added preliminary code coverage support in https://github.com/bazelbuild/rules_scala/pull/692

It's 'preliminary' because it produces lcov coverage traces, but iiuc there were some bazel bugs blocking bazel's built in support for generating coverage reports. You have to run genhtml over the coverage traces yourself. @andyscott could elaborate more.

I did however have some success uploading the coverage traces to codecov.io for a small test project.

prebeta commented 5 years ago

@beala-stripe Thanks for the pointer! I was doing some testing and it seems to work well. :)

However, I'm hitting an issue when generating coverage using genhtml, the sources listed in coverage.dat files are relative to a scala root directory instead of the project root.

For example, I have this project structure:

ā”œā”€ā”€ BUILD
ā””ā”€ā”€ src
    ā”œā”€ā”€ main
    ā”‚Ā Ā  ā””ā”€ā”€ scala
    ā”‚Ā Ā      ā”œā”€ā”€ com
    ā”‚Ā Ā      ā”‚Ā Ā  ā””ā”€ā”€ scalasample
    ā”‚Ā Ā      ā”‚Ā Ā      ā”œā”€ā”€ Calculator.scala
    ā”‚Ā Ā      ā”‚Ā Ā      ā””ā”€ā”€ CalculatorTrait.scala
    ā””ā”€ā”€ test
        ā””ā”€ā”€ scala
            ā””ā”€ā”€ com
                ā””ā”€ā”€ scalasample
                    ā””ā”€ā”€ CalculatorTest.scala

But when I run bazel coverage :calculator_test --extra_toolchains="@io_bazel_rules_scala//test/coverage:enable_code_coverage_aspect", the source files in coverage.dat look like:

SF:com/scalasample/Calculator.scala
SF:com/scalasample/CalculatorTest.scala
SF:com/scalasample/CalculatorTrait.scala

instead of:

src/main/scala/com/scalasample/Calculator.scala
src/main/scala/com/scalasample/CalculatorTrait.scala
src/test/scala/com/scalasample/CalculatorTest.scala

and it makes genhtml confused when generating the coverage report with sources.

The issue looks very similar to: https://github.com/bazelbuild/bazel/issues/2528, and the solution was to pass in --experimental_java_coverage in order to get coverage to list java source files relative to the workspace directory.

I'm currently on bazel version 0.23.1, and the --experimental_java_coverage still exists. However, I get an error at test runtime when I pass the flag in:

Exception in thread "main" java.lang.IllegalStateException: JACOCO_METADATA_JAR environment variable is not set, and no META-INF/MANIFEST.MF on the classpath has a Coverage-Main-Class attribute.  Cannot determine the name of the main class for the code under test.
    at com.google.testing.coverage.JacocoCoverageRunner.getMainClass(JacocoCoverageRunner.java:267)
    at com.google.testing.coverage.JacocoCoverageRunner.main(JacocoCoverageRunner.java:398)

It looks like the flag has been enabled by default in 0.24.1: https://github.com/bazelbuild/bazel/commit/e64066d9eb1ebfc3f52ec1e95459849921a19e71 and when I run a coverage on that version the build, the build completes and the tests run, but I get a different error in the test.log afterwards:

...
Discovery starting.
Discovery completed in 42 milliseconds.
Run starting. Expected test count is: 2
CalculatorTest:
- testAddOrSubtract
- testMultiply
Run completed in 119 milliseconds.
Total number of tests run: 2
Suites: completed 2, aborted 0
Tests: succeeded 2, failed 0, canceled 0, ignored 0, pending 0
All tests passed.
java.util.zip.ZipException: error in opening zip file
    at java.util.zip.ZipFile.open(Native Method)
    at java.util.zip.ZipFile.<init>(ZipFile.java:225)
    at java.util.zip.ZipFile.<init>(ZipFile.java:155)
    at java.util.jar.JarFile.<init>(JarFile.java:166)
    at java.util.jar.JarFile.<init>(JarFile.java:130)
    at com.google.testing.coverage.JacocoCoverageRunner.addEntriesToExecPathsSet(JacocoCoverageRunner.java:285)
    at com.google.testing.coverage.JacocoCoverageRunner.createPathsSet(JacocoCoverageRunner.java:270)
    at com.google.testing.coverage.JacocoCoverageRunner.createReport(JacocoCoverageRunner.java:149)
    at com.google.testing.coverage.JacocoCoverageRunner.create(JacocoCoverageRunner.java:142)
    at com.google.testing.coverage.JacocoCoverageRunner$2.run(JacocoCoverageRunner.java:518)
+ TEST_STATUS=1
+ touch /home/tony/.cache/bazel/_bazel_tony/46a620847bc368abb6ab2ca398c067f0/sandbox/linux-sandbox/187/execroot/__main__/bazel-out/k8-fastbuild/testlogs/sandbox/calculator_test/coverage.dat
+ [[ 1 -ne 0 ]]
+ echo --
--
+ echo Coverage runner: Not collecting coverage for failed test.
Coverage runner: Not collecting coverage for failed test.
+ echo The following commands failed with status 1
The following commands failed with status 1
+ echo /home/tony/.cache/bazel/_bazel_tony/46a620847bc368abb6ab2ca398c067f0/sandbox/linux-sandbox/187/execroot/__main__/bazel-out/k8-fastbuild/bin/sandbox/calculator_test.runfiles/__main__/sandbox/calculator_test
/home/tony/.cache/bazel/_bazel_tony/46a620847bc368abb6ab2ca398c067f0/sandbox/linux-sandbox/187/execroot/__main__/bazel-out/k8-fastbuild/bin/sandbox/calculator_test.runfiles/__main__/sandbox/calculator_test
+ exit 1
andyscott commented 5 years ago

@prebeta-- We have the same issue internally at Stripe!

To work around this I've pieced together a Python script that rebuilds .dat files with resolved file paths. At the moment it's got some internal paths hard coded, but I could clean it up and share it publicly if it'd be useful.

prebeta commented 5 years ago

@prebeta-- We have the same issue internally at Stripe!

To work around this I've pieced together a Python script that rebuilds .dat files with resolved file paths. At the moment it's got some internal paths hard coded, but I could clean it up and share it publicly if it'd be useful.

@andyscott thanks for clarifying :)

I've worked around the pathing issues as well with a similar script, I just wanted to bring attention to the potential issues with newer versions of bazel and having --experimental_java_coverage set by default.

softprops commented 5 years ago

What's the current story on this and are they examples to get started, perhaps for folks coming from the land of sbt?

gergelyfabian commented 4 years ago

@prebeta-- We have the same issue internally at Stripe! To work around this I've pieced together a Python script that rebuilds .dat files with resolved file paths. At the moment it's got some internal paths hard coded, but I could clean it up and share it publicly if it'd be useful.

@andyscott thanks for clarifying :)

I've worked around the pathing issues as well with a similar script, I just wanted to bring attention to the potential issues with newer versions of bazel and having --experimental_java_coverage set by default.

Let me summarize the state (as I see it) for December 2019 (Bazel 1.2.0 and rules_scala 26cf9b74fc46f1e9a970c97837447549ed7257b6).

In the latest versions of Bazel --experimental_java_coverage is removed. You should install lcov package to have access to genhtml (e.g. on Ubuntu: sudo apt install lcov).

To run code coverage with Jacoco for Scala code you can run (you need the extra toolchain as the scala support wasn't enabled by default - please see the comments in https://github.com/bazelbuild/rules_scala/pull/692):

bazel coverage --extra_toolchains="@io_bazel_rules_scala//test/coverage:enable_code_coverage_aspect" //...

It will produce several .dat files for your modules.

Then you've got two issues to solve:

  1. bazel does not summarize the coverage statistics for you
  2. when you try running genhtml for the .dat files it won't have access to the source files

Both can be solved by using an approach taken e.g. by Gerrit project. See https://github.com/bazelbuild/bazel/issues/2528 for more information, this also seems to be an issue for Java source code. A possible solution is running a script that moves all source files to a new folder and runs genhtml from that place so that genhtml can have access to them. You can check https://gerrit-review.googlesource.com/c/gerrit/+/106471/6/tools/coverage.sh for inspiration. Current version of this script is also in Gerrit source code: https://gerrit.googlesource.com/gerrit/+/master/tools/coverage.sh.

Using a modified version of this script solved the html report problem for me.