KAFKA-3451: Add basic HTML coverage report generation to gradle #1121

granthenke · 2016-03-23T17:07:10Z

No description provided.

granthenke · 2016-03-23T17:08:10Z

core/src/main/scala/kafka/utils/ZkUtils.scala

+            debug("Checker method is not passed skipping zkData match")
+            warn("Conditional update of path %s with data %s and expected version %d failed due to %s"
+              .format(path, data,expectVersion, e1.getMessage))
+            (false, -1)


Note: This minimal change was made because the old "style" broke the Scoverage code instrumentation.

It's a bit concerning that valid Scala code breaks scoverage.

Yeah, its a bit unfortunate. It looks like its a Scala issue tracked by SI-8979. More discussion on the Scoverage issue can be seen here.

On the bright side, it encourages better coding practice. ;)

granthenke · 2016-03-23T20:10:41Z

cc @ijuma @SinghAsDev @ewencp @gwenshap

granthenke · 2016-03-29T15:45:08Z

@gwenshap @ewencp Ping for review

ijuma · 2016-03-29T16:04:19Z

Thanks for the PR @granthenke. I think it would be good to explain a bit more how this hooks into the build. Is it just a command that can be run explicitly, but it doesn't change anything else otherwise (i.e. no slowdown in compilation or test execution speed)? Or does it instrument things by default?

granthenke · 2016-03-29T16:15:40Z

@ijuma This just adds optional tasks to the build to generate the report output. It should not affect the existing build workflow.

I didn't make it part of the default build for a few reasons:

this is just an initial patch to have basic coverage reports, more polish may be needed
instrumentation and testing can add a lot of time to the build
the report is generally not needed on every build

Eventually we may want to incorporate it and coverage checks into the standard build, but for now I wanted the basic ability to report coverage.

SinghAsDev · 2016-03-29T16:49:08Z

Grant, thanks for restarting this. Code coverage is a useful tool to make sure new patches are coming with proper unit tests. However, it will be hard to say so without some sort of a baseline number. Having a consolidated report, for scala and java code, can give us a reference number. In parent JIRA that is what I tried to achieve. Many projects are already taking advantage of sonarqube, https://analysis.apache.org/. It will be nice to have Kafka take advantage of it as well.

granthenke · 2016-03-29T16:56:39Z

@SinghAsDev Yes, those features could definitely be useful. I left that out of the scope of this patch (KAFKA-3451) and it still can be done under the parent patch KAFKA-1722. Since that looked to be a longer discussion, this gets some functionality now.

Please feel free to drive that addition after this patch.

SinghAsDev · 2016-03-29T17:14:11Z

Yea, that is what I meant, just wanted to provide the bigger picture here. @ijuma I think having a separate task will make sure people don't have to unnecessarily wait for coverage reports and coverage reports builds can run the required task.

ijuma · 2016-03-29T17:29:05Z

Thanks for clarifying Grant. My preference is definitely for a separate task, so that's good.

gwenshap · 2016-03-31T05:12:28Z

It looks like checking coverage actually runs the tests. Is this expected?

ijuma · 2016-03-31T06:38:17Z

I think so. The reason is that the tests must run after the code is instrumented so that coverage can be recorded.

ewencp · 2016-03-31T16:25:36Z

build.gradle

  repositories {
    mavenCentral()
+    jcenter()


This doesn't actually seem necessary based on the couple of targets I built? I'm not even sure what dependencies needed this? Same is true of the jcenter() reference at buildscript level, although that wasn't introduced in this patch.

I will double check and remove it if its not needed.

ewencp · 2016-03-31T16:26:46Z

@gwenshap Yeah, I think this is expected. It doesn't, however, seem to obey the same parallelism as normal tests. Unfortunate, but hopefully fixable in a follow up. I previously started to review this and then got sidetracked as the test run took about 40 minutes to execute :)

Overall, I'm +1 on this pretty non-risky patch that makes an awesome incremental step towards Kafka actually paying attention to test coverage. Left one question, but otherwise LGTM

gwenshap · 2016-03-31T16:29:04Z

+1 as well. I like the report :)

I was a bit concerned by the time it took, but looks like this is inevitable.

granthenke · 2016-03-31T16:32:01Z

@ewencp @gwenshap Yeah the java runs aren't as bad, and if you like you can run a report on just the modules you care about. The Scala coverage report definitely takes a while. Thats why I have made this an optional task to start. Hopefully we can speed this up in the future. Speeding up core tests in general would be nice too.

granthenke · 2016-03-31T16:55:22Z

@ewencp @gwenshap Removed the extraneous jcenter() line

Author: Grant Henke <[email protected]> Reviewers: Gwen Shapira, Ismael Juma, Ewen Cheslack-Postava Closes #1121 from granthenke/coverage (cherry picked from commit 623ab1e) Signed-off-by: Gwen Shapira <[email protected]>

ijuma · 2016-03-31T23:37:40Z

This PR seems to cause issues to the Gradle import of IntelliJ 2016.1.1. References to test classes are marked as red. If I revert to the commit before this and I re-import then things work fine. Anyone one else tested this?

granthenke · 2016-04-01T02:35:05Z

@ijuma I tested it with Intellij 15.0.3 and had no issues. I will update my Intellij and report back.

granthenke · 2016-04-01T05:12:10Z

@ijuma I didn't have any issues. You could try refreshing the gradle project, file -> synchronize, or file -> invalidate caches.

ijuma · 2016-04-01T06:03:26Z

@granthenke I did both before asking the question. I'll try a clean checkout as well. You checked test files, yes?

granthenke · 2016-04-01T14:17:44Z

@ijuma It looks like this is a regression in Intellij that is exposed when 2 modules point to the same source. It is tracked by IDEA-154014, IDEA-153264, and IDEA-153231.

It can be worked around by going to File -> Project Structure -> Modules and removing the core_scoverage and core_testScoverage modules.

ijuma · 2016-04-01T16:30:22Z

Oh, that's really annoying because it will probably break every time one reloads the gradle build (ie when changing branches). Thanks for finding the issues in the IntelliJ tracker.

ijuma · 2016-04-05T22:00:44Z

The latest IntelliJ 2016.1 EAP fixes this issue: https://confluence.jetbrains.com/display/IDEADEV/IntelliJ+IDEA+2016.1+145.844.1+Release+Notes

…1121)

KAFKA-3451: Add basic HTML coverage report generation to gradle

3ccf9fc

granthenke reviewed Mar 23, 2016
View reviewed changes

ewencp reviewed Mar 31, 2016
View reviewed changes

Remove jcenter

41b9354

asfgit closed this in 623ab1e Mar 31, 2016

efeg added a commit to efeg/kafka that referenced this pull request May 29, 2024

Update stale dependencies and the access to deprecated APIs. (apache#…

788f296

…1121)

mumrah pushed a commit to mumrah/kafka that referenced this pull request Aug 14, 2024

MINOR: Fixing build (apache#1121)

c693306

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-3451: Add basic HTML coverage report generation to gradle #1121

KAFKA-3451: Add basic HTML coverage report generation to gradle #1121

granthenke commented Mar 23, 2016

granthenke Mar 23, 2016

ijuma Mar 29, 2016

granthenke Mar 29, 2016

ijuma Mar 29, 2016

granthenke commented Mar 23, 2016

granthenke commented Mar 29, 2016

ijuma commented Mar 29, 2016

granthenke commented Mar 29, 2016

SinghAsDev commented Mar 29, 2016

granthenke commented Mar 29, 2016

SinghAsDev commented Mar 29, 2016

ijuma commented Mar 29, 2016

gwenshap commented Mar 31, 2016

ijuma commented Mar 31, 2016

ewencp Mar 31, 2016

granthenke Mar 31, 2016

ewencp commented Mar 31, 2016

gwenshap commented Mar 31, 2016

granthenke commented Mar 31, 2016

granthenke commented Mar 31, 2016

ijuma commented Mar 31, 2016

granthenke commented Apr 1, 2016

granthenke commented Apr 1, 2016

ijuma commented Apr 1, 2016

granthenke commented Apr 1, 2016

ijuma commented Apr 1, 2016

ijuma commented Apr 5, 2016

KAFKA-3451: Add basic HTML coverage report generation to gradle #1121

KAFKA-3451: Add basic HTML coverage report generation to gradle #1121

Conversation

granthenke commented Mar 23, 2016

granthenke Mar 23, 2016

Choose a reason for hiding this comment

ijuma Mar 29, 2016

Choose a reason for hiding this comment

granthenke Mar 29, 2016

Choose a reason for hiding this comment

ijuma Mar 29, 2016

Choose a reason for hiding this comment

granthenke commented Mar 23, 2016

granthenke commented Mar 29, 2016

ijuma commented Mar 29, 2016

granthenke commented Mar 29, 2016

SinghAsDev commented Mar 29, 2016

granthenke commented Mar 29, 2016

SinghAsDev commented Mar 29, 2016

ijuma commented Mar 29, 2016

gwenshap commented Mar 31, 2016

ijuma commented Mar 31, 2016

ewencp Mar 31, 2016

Choose a reason for hiding this comment

granthenke Mar 31, 2016

Choose a reason for hiding this comment

ewencp commented Mar 31, 2016

gwenshap commented Mar 31, 2016

granthenke commented Mar 31, 2016

granthenke commented Mar 31, 2016

ijuma commented Mar 31, 2016

granthenke commented Apr 1, 2016

granthenke commented Apr 1, 2016

ijuma commented Apr 1, 2016

granthenke commented Apr 1, 2016

ijuma commented Apr 1, 2016

ijuma commented Apr 5, 2016