Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Coverage in Kotlin #429

Closed
wants to merge 6 commits into from
Closed

Conversation

sgammon
Copy link

@sgammon sgammon commented Dec 16, 2020

Builds on previous work (#52) to add coverage support to Kotlin targets in Bazel.

So far, it's working (in basic form), but seems to omit major swaths of our code when I test it on a live codebase. I'm not sure why that happens yet.

@sgammon
Copy link
Author

sgammon commented Dec 16, 2020

@hchauvin this is basically a clone of your work, but placed atop HEAD. cc / @cgruber

@sgammon
Copy link
Author

sgammon commented Dec 16, 2020

so, coverage works for me on this branch, at least in the environment I'm using (JUnit 5, Java 11 toolchain). however, coverage does not show for targets that touch Java plugins.

in my case, whether the targets are Kotlin or Java, if there is a plugin involved with building them, there is no coverage. otherwise, kotlin sources show up in coverage report as expected. the issue w/plugins is likely blocked on the discussion/fix in bazelbuild/bazel#5426.

@hchauvin
Copy link

@sgammon Thank you for resuscitating this work!

@sgammon
Copy link
Author

sgammon commented Dec 17, 2020

@hchauvin it is a poor imitation of your original PR but it works!! lol. also it will need tests, which i will copy in soon once i isolate this plugin issue.

Sam Gammon added 6 commits January 12, 2021 21:20
Builds on previous work (bazelbuild#52) to add
coverage support to Kotlin targets in Bazel.

So far, it's working (in basic form), but seems to omit major
swaths of our code when I test it on a live codebase. I'm not
sure why that happens yet.
@sgammon
Copy link
Author

sgammon commented Jan 13, 2021

i have discovered a few issues with this work, which should block it before merge:

  1. there is a path issue, where java/ is trimmed from the beginning of paths emitted to .instrumented_files and coverage.dat. moving Kotlin sources outside of java/ fixes this. see (sgammon/coverage-repro)[https://github.com/sgammon/coverage-repro] for a reproduction of this and other bugs.

  2. there is a bug related to Kotlin sources and coverage, which only seems to surface if Java plugins are involved with building the target. in this case, the behavior seems like the file is never instrumented, although it does show up in .instrumented_files (just not in coverage.dat - or rather, the file is there, but it's empty).

  3. i can't get coverage to work properly for regular Java targets if they are located somewhere other than java/. this is why the repro has app/ (for Kotlin sources, which works, so long as plugins aren't involved), and java/app/, for the Java sources. this seems unrelated to rules_kotlin.

any ideas, @hsyed / @cgruber? first issue above feels like some hard-coded path. second seems like a subtle bug which was discussed in bazelbuild/bazel#5426 but may end up being Kotlin-specific.

the repro can be run with make, which produces a coverage report that includes all "source" files except app/KotlinController.kt, which demonstrates issue 2 above (it should show up, but doesn't). to repro issue 3, move the java/app/ sources into app/, and they disappear from coverage.

tested against bazel 3.7.0, 3.7.2, HEAD (@...b46fc9d8).

@jameslan
Copy link

Hi @sgammon, thank you for your effort! I am really looking for it!

When I try using your branch to test in my project with coverage, it fails with error:

Error: Could not find or load main class com.google.testing.coverage.JacocoCoverageRunner

I checked what the command bazel is running and found that it didn't set the environment variables JAVA_RUNTIME_CLASSPATH_FOR_COVERAGE and SINGLE_JAR_TOOL comparing to the java targets.

So my classpath doesn't contain ../remote_java_tools_darwin/java_tools/JacocoCoverage_jarjar_deploy.jar

Are there configurations needed to make it work?

@jameslan
Copy link

I found that I have to use the following patch to add _jacocorunner to the runtime_deps, to make the JacocoCoverageRunner work.

diff --git a/kotlin/internal/jvm/impl.bzl b/kotlin/internal/jvm/impl.bzl
index 9771bbf..5ed52fa 100644
--- a/kotlin/internal/jvm/impl.bzl
+++ b/kotlin/internal/jvm/impl.bzl
@@ -235,8 +235,13 @@ _SPLIT_STRINGS = [
 ]
 
 def kt_jvm_junit_test_impl(ctx):
+    if ctx.configuration.coverage_enabled:
+        runner = ctx.files._bazel_test_runner + ctx.files._jacocorunner
+    else:
+        runner = ctx.files._bazel_test_runner
+
     providers = _kt_jvm_produce_jar_actions(ctx, "kt_jvm_test")
-    runtime_jars = depset(ctx.files._bazel_test_runner, transitive = [providers.java.transitive_runtime_jars])
+    runtime_jars = depset(runner, transitive = [providers.java.transitive_runtime_jars])
 
     test_class = ctx.attr.test_class

context.execute("instrument class files") {
jacocoProcessor.instrument(this)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There may have an issue with the inline function across modules.

If the compiled jar has only instrumented byte code, calling the inline function when running the test will result in an error like

java.lang.IllegalAccessError: class com.package.Foo tried to access private method 'boolean[] com.package.BarKt.$jacocoInit()'

The reason is that when compiling Bar.kt, it has only the instrumented version of Foo. And the compiler embeds the bytecode of the instrumented inline function.

I think the solution is to create two jars, the uninstrumented one for compilation and the instrumented one for runtime.

@sgammon
Copy link
Author

sgammon commented Jul 7, 2021

supersedes #52, superseded by #505 / #508

@sgammon sgammon closed this Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants