-
Notifications
You must be signed in to change notification settings - Fork 81
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
build: DH-18379: Java Code Coverage #6576
Conversation
@@ -84,6 +85,35 @@ tasks.register('smoke') { | |||
it.dependsOn project(':Generators').tasks.findByName(LifecycleBasePlugin.CHECK_TASK_NAME) | |||
} | |||
|
|||
tasks.register("coverage") { | |||
System.setProperty "coverageEnabled", "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be configuring system properties based on when this task is executed... we should be using gradle properties for parts of the build system we want configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone.
if (Boolean.getBoolean("coverageEnabled")) { | ||
apply plugin: 'jacoco' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already having something like this as a buildSrc plugin. If the gradle property jacoco.enabled
is true
.
allprojects.findAll { p-> p.plugins.hasPlugin('java') }.each { | ||
it.apply(['from':rootProject.file("coverage/jacoco.gradle")]) | ||
} | ||
} | ||
|
||
tasks.register("coverage-merge", JacocoReport) { | ||
def jprojects = allprojects.findAll { p-> p.plugins.hasPlugin('java') } | ||
additionalSourceDirs = files(jprojects.sourceSets.main.allSource.srcDirs) | ||
sourceDirectories = files(jprojects.sourceSets.main.allSource.srcDirs) | ||
classDirectories = files(jprojects.sourceSets.main.output) | ||
reports { | ||
html.required = true | ||
csv.required = true | ||
xml.required = false | ||
} | ||
def projRootDir = project.rootDir.absolutePath | ||
executionData fileTree(projRootDir).include("**/build/jacoco/*.exec") | ||
doLast { | ||
def stdout = new StringBuilder(), stderr = new StringBuilder() | ||
def task = ('python ' + projRootDir + '/coverage/all-coverage.py ' + projRootDir).execute() | ||
task.consumeProcessOutput(stdout, stderr) | ||
task.waitFor() | ||
println(stdout) | ||
println(stderr) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be building in this logic at the root build.gradle level; we have buildSrc plugins to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about the configuration of the task's plugin or the doLast
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doLast
issue is another comment.
This comment is specifically that we should not typically be editing the root build.gradle.
We can create a subproject that contains all the logic necessary for coverage-merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been moved into it's own project.
println(stdout) | ||
println(stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone
} | ||
def projRootDir = project.rootDir.absolutePath | ||
executionData fileTree(projRootDir).include("**/build/jacoco/*.exec") | ||
doLast { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably bad to attach a doLast to an existing Task type; likely, you'll want a separate task to depend on the output of JacocoReport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been reworked and is in it's own project.
@@ -1,6 +1,7 @@ | |||
plugins { | |||
id 'base' | |||
id 'io.deephaven.project.register' | |||
id 'jacoco-report-aggregation' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused... is this plugin actually being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It is used for the "coverage-merge" task. It is only necessary for collecting the Java code coverage results into a top-level HTML file. Charles wanted this. It's not needed for any notion of automated coverage tracking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't JacocoReport
provided via the jacoco
plugin, https://docs.gradle.org/current/userguide/jacoco_plugin.html?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new coverage project, we use it as in the documentation.
Closing this without merging in favor of #6586, which deals with getting java coverage working and merging. |
check
This is the first step towards unified code coverage at the class level. It is not meant to provide fined-grained detail but instead a level that can point to areas of code that need test attention. Devs can then use IDE tools for more detail.
When this is added to a dispatch workflow, the Java HTML coverage hierarchy will be a collected artifact and will have a lot of detail like branch coverage.