-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-1132] add jacoco report on javaPreCommit #7699
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,5 +47,8 @@ builder.build { | |
} | ||
enabledForFailure(true) | ||
} | ||
jacocoCodeCoverage { | ||
execPattern('**/build/jacoco/*.exec') | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
A few ideas here:
:javaPreCommit
on their box. So that task is not a dedicated Jenkins task and we might not want this.-Pjenkins=true
?jacocoReport
depend ontest
? That way if you runjacocoReport
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.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.
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.
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.
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.
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.
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
asthis is required for local execution of jacocoTestReport
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 moreexplicit variant
gradlew test jacocoTestReport
here, because it is moreobvious to myself what i intend to do. So most likely i would add a
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.