Update to Gradle 6.7 and use new toolchains feature for testing on Ja… #1627
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
mateuszrzeszutek,
trask and
tylerbenson
as code owners
gradle/java.gradle
Outdated
| } | ||
| } | ||
| } | ||
| if (rootProject.findProperty('testAdditionalJavaVersions') == 'true') { |
There was a problem hiding this comment.
I added this like the -java repo has since it doesn't hurt. Here, the tradeoff of test vs build time is so high that it probably doesn't make sense to take advantage of "build once, test many" that we do there, and instead continue to matrix in GitHub resulting in "build + test many" which is fine.
There was a problem hiding this comment.
I don't see where do we specify this property?
There was a problem hiding this comment.
Thought it could be useful from command line but on second thought guess not so much.
| @@ -48,10 +48,6 @@ repositories { | |||
|
|
|||
| description = 'OpenTelemetry instrumentations for Java' | |||
|
|
|||
| wrapper { | |||
| distributionType = Wrapper.DistributionType.ALL | |||
There was a problem hiding this comment.
We're using -bin in -java per recommendation from Gradle team I believe
gradle/java.gradle
Outdated
| } | ||
| } | ||
| } | ||
| if (rootProject.findProperty('testAdditionalJavaVersions') == 'true') { |
There was a problem hiding this comment.
I don't see where do we specify this property?
gradle/java.gradle
Outdated
| @@ -297,6 +224,12 @@ tasks.withType(Test).configureEach { | |||
| testLogging { | |||
| exceptionFormat = 'full' | |||
| } | |||
|
|
|||
| if (!isJavaVersionAllowed(JavaVersion.toVersion(11))) { | |||
There was a problem hiding this comment.
Can you add comment explaining this?
There was a problem hiding this comment.
Yeah - also made sure it only applies to test task which I guess is more appropriate.
gradle/java.gradle
Outdated
| } | ||
| } | ||
| } | ||
| if (rootProject.findProperty('testAdditionalJavaVersions') == 'true') { |
There was a problem hiding this comment.
Thought it could be useful from command line but on second thought guess not so much.
gradle/java.gradle
Outdated
| @@ -297,6 +224,12 @@ tasks.withType(Test).configureEach { | |||
| testLogging { | |||
| exceptionFormat = 'full' | |||
| } | |||
|
|
|||
| if (!isJavaVersionAllowed(JavaVersion.toVersion(11))) { | |||
There was a problem hiding this comment.
Yeah - also made sure it only applies to test task which I guess is more appropriate.
testing-common/testing-common.gradle
Outdated
| @@ -67,6 +67,8 @@ tasks.register("testDisabledFieldInjection", Test) { | |||
| includes = ["context/FieldBackedProviderFieldInjectionDisabledTest.class"] | |||
| } | |||
| test.dependsOn(testDisabledFieldInjection) | |||
| test.forkEvery 1 | |||
| tasks.withType(Test) { | |||
There was a problem hiding this comment.
Noticed that since this approach doesn't override test tasks like the previous one did, some configs get more complicated. I think it's rare so it might be ok but I can go back to the approach of overriding test instead of defining new tasks if it makes more sense.
There was a problem hiding this comment.
Found another file that was affected and realized it's probably hard to reason about right now, went back to the approach of using a rule.
| - name: Set JDK ${{ matrix.java }} home | ||
| run: echo JAVA_${{ matrix.java }}_HOME=${{ env.JAVA_HOME }} >> $GITHUB_ENV |
| `./gradlew testJava8` or `./gradlew testJava15`. If you don't have a JDK of these versions | ||
| installed, Gradle will automatically download it for you. |
| // We default to testing with Java 11 for most tests, but some tests don't support it, where we change | ||
| // the default test task's version so commands like `./gradlew check` can test all projects regardless | ||
| // of Java version. |
There was a problem hiding this comment.
@anuraaga Have you tested that this interplays correctly with test rule below? I think that when java version is requested explicitly by "testJava15", then we should not silently decrease it to satisfy "maxJavaVersion".
There was a problem hiding this comment.
Yeah that's the main reason I added it here, above the test rule. Though I don't think we have any situation to trigger ambiguity either because we have no case where minJavaVersionForTests is higher than 11. Even if we added it, it should work though.
…va versions.
Also fixes compatibility with codeNarc and Java 14+
Fixes #891