[BEAM-1132] add jacoco report on javaPreCommit

[adude3141]

Add a post build action on javaPreCommit to collect and show Jacoco coverage report

---

Follow this checklist to help us incorporate your contribution quickly and easily:

• Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go		---	---	---	---	---	---
Java
Python		---			---	---	---

[adude3141]

Run Website PreCommit

[adude3141]

Run Website PreCommit

[adude3141]

R: @kennknowles

[kennknowles]

A few ideas here:

• 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.
• To always activate on Jenkins, how about the straightforward -Pjenkins=true ?
• 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.

[adude3141]

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, 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 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
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.
So it might well be, we are kicking this jenkins/jacoco integration altogether
in favour of some other tool/service.

We might 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.

[kennknowles]

Ah, sorry for the delay here. This is a major value-add and we should merge it.

[adude3141]

Run Seed Job

[adude3141]

@kennknowles Thanks for ur help here. Know ur busy, so no worries.

I hope I will be able to continue here soon. Lets see, how current setup works.