Support for JaCoCo on Java 9 #1052
Conversation
|
Relevant CI job. |
| @@ -81,7 +78,6 @@ class JacocoPluginIntegrationTest extends AbstractIntegrationSpec { | |||
| then: output.contains "org.jacoco:org.jacoco.ant:0.6.0.201210061924" | |||
| } | |||
|
|
|||
| @Requires(FIX_TO_WORK_ON_JAVA9) | |||
There was a problem hiding this comment.
For all the tests below this one in this file, can we split them into a separate MultiVersionIntegrationSpec to test multiple versions of Jacoco? It looks like we don't have very many tests that use different versions.
| @@ -267,19 +257,17 @@ public class ThingTest { | |||
| ":test" in nonSkippedTasks | |||
| ":otherTests" in nonSkippedTasks | |||
| file("build/jacoco/jacocoMerge.exec").exists() | |||
| htmlReport("build/reports/jacoco/mergedReport/html").totalCoverage() == 65 | |||
| htmlReport("build/reports/jacoco/mergedReport/html").totalCoverage() == 64 | |||
There was a problem hiding this comment.
Why did this change? Just a different calculation on the jacoco side?
There was a problem hiding this comment.
I think something changes on the JaCoCo side on how they calculate the coverage number. I didn't look into their release notes though. Maybe they had a bug before or introduced a new one.
There was a problem hiding this comment.
Here's the reason: jacoco/jacoco#452
There was a problem hiding this comment.
Thanks, could we change our sample project to make this the same across versions?
| @@ -43,7 +43,7 @@ class JacocoVersionIntegTest extends MultiVersionIntegrationSpec { | |||
| then: | |||
| def report = htmlReport() | |||
| report.totalCoverage() == 100 | |||
| report.jacocoVersion() == version | |||
| report.jacocoVersion() == version || report.jacocoVersion().startsWith(version) | |||
There was a problem hiding this comment.
Why wouldn't these be the equal now?
There was a problem hiding this comment.
The latest JaCoCo library version has a non-timestamped version but they still use the timestamp when generating the report. Doh! I think it's simply an oversight on their end.
There was a problem hiding this comment.
Can we move that detail into the report fixture? e.g., so we can do something like:
report.assertJacocoVersion(version)
And assertJacocoVersion does the fuzzy match.
| @@ -43,7 +43,7 @@ class JacocoVersionIntegTest extends MultiVersionIntegrationSpec { | |||
| then: | |||
| def report = htmlReport() | |||
| report.totalCoverage() == 100 | |||
| report.jacocoVersion() == version | |||
| report.jacocoVersion() == version || report.jacocoVersion().startsWith(version) | |||
There was a problem hiding this comment.
Can this test be made to not require <=JDK7?
There was a problem hiding this comment.
Unfortunately older versions of JaCoCo don't work on JDK 8.
|
|
||
| @Requires(FIX_TO_WORK_ON_JAVA9) | ||
| class JacocoPluginCoverageVerificationIntegrationTest extends AbstractIntegrationSpec { |
There was a problem hiding this comment.
can this be made into a MultiVersionIntegrationSpec too?
|
The other thing I forgot to mention -- we should add something to the release notes about a default version change. |
|
@big-guy Yeah, I was planning to add something to the release notes after merging. |
|
@big-guy I addressed your comments for this PR. Can you please have another look? |
For more information see #1006.