[
https://issues.apache.org/jira/browse/BEAM-1132?focusedWorklogId=193380&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-193380
]
ASF GitHub Bot logged work on BEAM-1132:
----------------------------------------
Author: ASF GitHub Bot
Created on: 01/Feb/19 14:15
Start Date: 01/Feb/19 14:15
Worklog Time Spent: 10m
Work Description: adude3141 commented on pull request #7699: [BEAM-1132]
add jacoco report on javaPreCommit
URL: https://github.com/apache/beam/pull/7699#discussion_r253059594
##########
File path:
buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy
##########
@@ -300,7 +300,7 @@ class BeamModulePlugin implements Plugin<Project> {
project.gradle.taskGraph.whenReady { graph ->
// Disable jacoco unless report requested such that task outputs can be
properly cached.
//
https://discuss.gradle.org/t/do-not-cache-if-condition-matched-jacoco-agent-configured-with-append-true-satisfied/23504
- def enabled = graph.allTasks.any { it instanceof JacocoReport }
+ def enabled = graph.allTasks.any { it instanceof JacocoReport ||
it.name.contains("javaPreCommit") }
Review comment:
Thanks @kennknowles for your feedback. You are actually
hitting a point here, which caused me some headaches i finally have
chosen to ignore for the time being.
__TL;DR__ This is not expected to survive upgrade to gradle 5 and further
subject to change on integration of more reporting
First let me recap my understanding of what is going on and why. Please
correct me, if i do get it wrong.
JacocoPlugin does collect coverage metrics on the given codebase. This
is done in a kind of two step process.
- jacoco plugin does instrument the test classes to enable the collection
of the metrics. This instrumentation is implicit, i.e. done on application
of the plugin. This means, there is no task to be executed. instead that
collection is done within the test task itself - *unless* explicitly
disabled. Which beam
[does](https://github.com/apache/beam/blob/master/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L303-L304),
as this instrumentation does not play well
with build cache and up-to-date checks. The collected metrics wre output
to some '*.exec' file within build/jacoco folder.
- Further the plugin provides the *jacocoTestReport* task, which transform
the '*.exec' file into human friendlier format as *xml* or *html*
The jenkins jacoco plugin does work on '*.exec' output. So even with the
proposed change, there is no need to execute the *jacocoReport* task at
all, we only need to ensure, that the first step is executed, i.e. we
enable the plugin. This change enables the first step on *javaPreCommit*
only. Of course how ot is done is not only extremely ugly but this also
does not scale.
> We want contributors to be able to reproduce :javaPreCommit on their
box. So that task is not a dedicated Jenkins task and we might not want this.
Definitely yes. It is very important and a *must have*. But the current
change does not prevent that. Unfortunately it does have an impact on
the local build, as it implicitly disables the build cache which might
hurt some devs. To be honest, i decided to ignore that, because i was hoping
to migrate to gradle 5 soon - currently waiting on
[gogradle](https://github.com/gogradle/gogradle/issues/276) fix. (You
might want to look into the latest comments, did not open a separate issue
here as it should be clear, that it is not yet fixed completely.)
With this migration i would have removed that workaround disabling jacocos
first step. But as far as i understand you now, you prefer to keep that
disabled, right? Which, of course, would require a more scalable solution
here.
> To always activate on Jenkins, how about the straightforward
-Pjenkins=true ?
Of course, this could be done. Either explicitly with gradle property or
implicitly by autodetecting that the build is runnning on jenkins by looking
for some system env var.
I decided against that, because this would obviously enable the first step
for any test task. Which by itself does not constitute a problem imho, but
on gradle 4 would also disable the build cache for all those builds. As i
did not look into the other builds (postcommit etc) i have no idea how
much impact that would have on overall jenkins performance.
And we still would need the first part `it instanceof JacocoReport` as
this is required for local execution of *jacocoTestReport*
> What does this look like if we alter the plugin to make jacocoReport
depend on test? That way if you run jacocoReport it always runs all the
tests anyhow, so we can just invoke it as the main target. There is some
funkiness around the needsRunnerTests and validatesRunnerTests that we
can solve later.
Yes. Thought about that too. Also a *test finalizedBy jacocoTestReport*
to not need to run jacocoTestReport where i intend to run test with coverage
information. It does not feel right to me to do a
`gradlew jacocoTestReport` in these cases. I d always prefer the more
explicit variant `gradlew test jacocoTestReport` here, because it is more
obvious to myself what i intend to do. So most likely i would add a
```
check dependsOn jacocoTestReport
jacocoTestReport mustRunAfter test
```
but did not look into that. And still all this would imply disabling the
build cache (also for local dev) as long we are tight to gradle 4, which
i did not want to do.
Main reason for ignoring this issue is the expectation to not stop here, but
add [PR
coverage](https://github.com/jenkinsci/github-pr-coverage-status-plugin)
to jenkins. Unfortunately this will require the generated reports
i.e. does not work on '*.exec' files but on the xml resp html output. So
we need to implement something along your suggestion anyway. But
i wanted to play around with this first on my private setup before getting
on the dev-list to ask how to do it and which branches to compare with
(currently i tend to use javaPreCommit_Cron, but honestly, i m lacking
knowledge her.
Of course, this is far away from the value added by that
[source code integration]( https://docs.codecov.io/docs/browser-extension).
So it might well be, we are kicking this jenkins/jacoco integration
altogether
in favour of some other tool/service.
We might still decide this is not good enough for now. So maybe the explicit
*javaPreCommit* together with a check if run on Jenkins would have the least
impact. Still, rather ugly though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 193380)
Time Spent: 1h 20m (was: 1h 10m)
> Jenkins JaCoCo plugin for Beam
> ------------------------------
>
> Key: BEAM-1132
> URL: https://issues.apache.org/jira/browse/BEAM-1132
> Project: Beam
> Issue Type: New Feature
> Components: testing
> Reporter: Kenneth Knowles
> Assignee: Michael Luckey
> Priority: Minor
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> Jenkins has a JaCoCo plugin and other Apache projects use it. We should try
> it too (and might as well disable coveralls, as I don't know anyone who looks
> at it).
> If this takes more than 10 minutes to set up, then another option is to just
> archive JaCoCo's HTML reports so we can browse them.
> Either of these should take just minutes and yield huge benefits in
> visibility of where we have really bad coverage.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
