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

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ Change the log4j setting in either `clients/src/test/resources/log4j.properties`

./gradlew -i -Dtest.single=RequestResponseSerializationTest core:test

### Generating test coverage reports ###
./gradlew reportCoverage

### Building a binary release gzipped tar ball ###
./gradlew clean
./gradlew releaseTarGz
Expand Down
60 changes: 60 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,18 @@ buildscript {
// For Apache Rat plugin to ignore non-Git files
classpath "org.ajoberstar:grgit:1.5.0"
classpath 'com.github.ben-manes:gradle-versions-plugin:0.12.0'
classpath 'org.scoverage:gradle-scoverage:2.0.1'
}
}

allprojects {
apply plugin: 'idea'
apply plugin: 'eclipse'
apply plugin: "jacoco"

repositories {
mavenCentral()
jcenter()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

apply plugin: 'com.github.ben-manes.versions'
Expand Down Expand Up @@ -249,8 +253,50 @@ subprojects {
configProperties = [importControlFile: "$rootDir/checkstyle/import-control.xml"]
}
test.dependsOn('checkstyleMain', 'checkstyleTest')

// Ignore core since its a scala project
if (it.path != ':core') {
// NOTE: Gradles Jacoco plugin does not support "offline instrumentation" this means that classes mocked by PowerMock
// may report 0 coverage, since the source was modified after initial instrumentation.
// See https://github.com/jacoco/jacoco/issues/51
jacocoTestReport {
dependsOn tasks.test
sourceSets sourceSets.main
reports {
html.enabled = true
xml.enabled = true
csv.enabled = false
}
}
}
}

// Aggregates all jacoco results into the root project directory
task jacocoRootReport(type: org.gradle.testing.jacoco.tasks.JacocoReport) {
def javaProjects = subprojects.findAll { it.path != ':core' }

description = 'Generates an aggregate report from all subprojects'
dependsOn(javaProjects.test)

additionalSourceDirs = files(javaProjects.sourceSets.main.allSource.srcDirs)
sourceDirectories = files(javaProjects.sourceSets.main.allSource.srcDirs)
classDirectories = files(javaProjects.sourceSets.main.output)
executionData = files(javaProjects.jacocoTestReport.executionData)

reports {
html.enabled = true
xml.enabled = true
}

// workaround to ignore projects that don't have any tests at all
onlyIf = { true }
doFirst {
executionData = files(executionData.findAll { it.exists() })
}
}

task reportCoverage(dependsOn: ['jacocoRootReport', 'core:reportScoverage'])

for ( sv in ['2_10', '2_11'] ) {
String svInDot = sv.replaceAll( "_", ".")

Expand Down Expand Up @@ -320,6 +366,7 @@ project(':core') {
println "Building project 'core' with Scala version ${versions.scala}"

apply plugin: 'scala'
apply plugin: "org.scoverage"
archivesBaseName = "kafka_${versions.baseScala}"

dependencies {
Expand All @@ -341,7 +388,20 @@ project(':core') {
testCompile libs.hadoopMiniKdc
testCompile libs.junit
testCompile libs.scalaTest

scoverage libs.scoveragePlugin
scoverage libs.scoverageRuntime
}

jacocoTestReport.enabled = false
scoverage {
reportDir = file("${rootProject.buildDir}/scoverage")
highlighting = false
}
checkScoverage {
minimumRate = 0.0
}
checkScoverage.shouldRunAfter('test')

configurations {
// manually excludes some unnecessary dependencies
Expand Down
11 changes: 6 additions & 5 deletions core/src/main/scala/kafka/utils/ZkUtils.scala
Original file line number Diff line number Diff line change
Expand Up @@ -452,12 +452,13 @@ class ZkUtils(val zkClient: ZkClient,
} catch {
case e1: ZkBadVersionException =>
optionalChecker match {
case Some(checker) => return checker(this, path, data)
case _ => debug("Checker method is not passed skipping zkData match")
case Some(checker) => checker(this, path, data)
case _ =>
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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}
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)
case e2: Exception =>
warn("Conditional update of path %s with data %s and expected version %d failed due to %s".format(path, data,
expectVersion, e2.getMessage))
Expand Down
3 changes: 3 additions & 0 deletions gradle/dependencies.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ versions += [
rocksDB: "4.1.0",
scalaTest: "2.2.6",
scalaParserCombinators: "1.0.4",
scoverage: "1.1.1",
slf4j: "1.7.18",
snappy: "1.1.2.1",
zkclient: "0.8",
Expand Down Expand Up @@ -86,6 +87,8 @@ libs += [
scalaCompiler: "org.scala-lang:scala-compiler:$versions.scala",
scalaTest: "org.scalatest:scalatest_$versions.baseScala:$versions.scalaTest",
scalaParserCombinators: "org.scala-lang.modules:scala-parser-combinators_$versions.baseScala:$versions.scalaParserCombinators",
scoveragePlugin: "org.scoverage:scalac-scoverage-plugin_$versions.baseScala:$versions.scoverage",
scoverageRuntime: "org.scoverage:scalac-scoverage-runtime_$versions.baseScala:$versions.scoverage",
slf4jApi: "org.slf4j:slf4j-api:$versions.slf4j",
slf4jlog4j: "org.slf4j:slf4j-log4j12:$versions.slf4j",
snappy: "org.xerial.snappy:snappy-java:$versions.snappy",
Expand Down