sboorlagadda commented on code in PR #7935:
URL: https://github.com/apache/geode/pull/7935#discussion_r2384185944
##########
build-tools/geode-repeat-test/build.gradle:
##########
@@ -33,11 +33,15 @@ dependencies {
implementation(gradleApi())
implementation('com.google.guava:guava:31.1-jre')
+ // Gradle 7.6.6: Dual JUnit support needed because Gradle API internals use
JUnit 5 while our tests use JUnit 4
+ testImplementation 'junit:junit:4.13.2'
Review Comment:
Should we use JUnit Vintage and only call `useJUnitPlatform()`?
```
dependencies {
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'
testRuntimeOnly 'org.junit.vintage:junit-vintage-engine:5.8.1' // to run
JUnit 4
testImplementation 'junit:junit:4.13.2'
}
test { useJUnitPlatform() }
```
##########
build-tools/scripts/settings.gradle:
##########
@@ -15,25 +15,23 @@
* limitations under the License.
*/
-includeBuild("${rootDir}/../geode-dependency-management")
Review Comment:
You’ve removed includeBuild for several build tools and now depend on devs
first doing `publishToMavenLocal`. Consider using `includeBuild` with proper
dependency substitution or implement a task dependency that ensures plugins are
published before use.
##########
gradle/wrapper/gradle-wrapper.properties:
##########
@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-7.3.3-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-7.6.6-all.zip
Review Comment:
Can we also add distributionSha256Sum?
This is a Gradle security best practice (prevents tampered downloads). Also
ensure gradlew is executable in Git. Checksums are on the Gradle releases pages.
##########
build-tools/geode-repeat-test/src/main/java/org/apache/geode/gradle/testing/repeat/ExecutionTrackingTestResultProcessor.java:
##########
@@ -67,8 +68,9 @@ public void output(Object testId, TestOutputEvent event) {
processor.output(testId, event);
}
+ // Gradle 7.6.6 changed from failure(Object testId, Throwable result) to
failure(Object testId, TestFailure result)
@Override
- public void failure(Object testId, Throwable result) {
+ public void failure(Object testId, TestFailure result) {
Review Comment:
Good catch
##########
build-tools/geode-annotation-processor/build.gradle:
##########
@@ -18,12 +18,13 @@
plugins {
id 'java'
- id 'java-gradle-plugin'
Review Comment:
Why is this plugin removed? it is unsure if its intentional?
##########
build-tools/scripts/src/main/groovy/spotless.gradle:
##########
@@ -45,7 +45,9 @@ spotless {
// Spotless will not run on unchanged files unless this number changes.
bumpThisNumberIfACustomStepChanges(project.ext.'spotless-file-hash')
- removeUnusedImports()
+ // TODO: removeUnusedImports() disabled due to Java 17 module system
compatibility issue
+ // with Google Java Format accessing com.sun.tools.javac.util.Context
+ // removeUnusedImports()
Review Comment:
Should we add a JIRA and bring back spotless checks?
##########
build-tools/geode-annotation-processor/build.gradle:
##########
@@ -33,6 +34,8 @@ dependencies {
transitive(false)
}
implementation(gradleApi())
+ // JUnit 4 needed for compatibility with existing Gradle plugin test
infrastructure
+ implementation 'junit:junit:4.13.2'
Review Comment:
Should it be `testImplementation`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
