Add spring7 tests #2262
Add spring7 tests #2262
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2262 +/- ##
=========================================
Coverage 82.07% 82.07%
+ Complexity 4754 4753 -1
=========================================
Files 465 465
Lines 14872 14872
Branches 1877 1877
=========================================
Hits 12206 12206
Misses 1978 1978
Partials 688 688 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,34 @@ | |||
| def springVersion = "7.0.0" | |||
There was a problem hiding this comment.
Wouldn't it be good to move the version into the libs.versions.toml, and use it there in a dependency, just do make it updateable via renovate, something like spring7Versionsimilar to junit?
| testImplementation projects.spockSpring | ||
| testImplementation libs.junit4 | ||
| testImplementation "org.springframework:spring-context" | ||
| testImplementation("org.springframework:spring-test") |
There was a problem hiding this comment.
To make is same as the other dependencies.
| testImplementation("org.springframework:spring-test") | |
| testImplementation "org.springframework:spring-test" |
|
|
||
| testImplementation projects.spockCore | ||
| testImplementation projects.spockSpring | ||
| testImplementation libs.junit4 |
There was a problem hiding this comment.
Why do you use Junit4 here?
Is it due to the SpringRunnerTest?
There was a problem hiding this comment.
I have no idea. -:-D
I just copied the spring6 to spring7 and the boot3 to boot4, changed the packages and made the adjustments to make them run.
So everything you said in both PRs is probably valid (didn't look in detail yet) but also for the existing projects.
|
There is also the new version missing in |
|
When I execute It seams that there is already a regression with Java 21 and spring 6: https://ge.spockframework.org/s/f5cvzjnfzd3da Note: I also needed to adapt the line in |
Vampire
force-pushed
the
spring7-test
branch
from
ba9cff6 to
c616136
Compare
leonard84
force-pushed
the
spring7-test
branch
from
c616136 to
1d7cf15
Compare
📝 WalkthroughWalkthroughThe changes introduce support for Spring 7 in the Spock framework by adding a new spring7-test module. Three existing Gradle build scripts receive syntax cleanup (removing parentheses from dependency declarations), and the new module includes its build configuration, test infrastructure classes, and multiple test cases validating Spring 7 compatibility with Spock tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
spock-spring/spring7-test/spring7-test.gradle (2)
1-1: Consider moving version tolibs.versions.tomlfor automated updates.Hardcoding the Spring version here means Renovate/Dependabot won't automatically detect and propose updates. Moving it to
libs.versions.toml(similar to other version declarations) would enable automated dependency management.
21-21: JUnit 4 dependency usage was questioned in prior review.A previous reviewer asked why JUnit 4 is used here. This may be intentional if testing
SpringRunnercompatibility, but worth confirming if JUnit 5 (Jupiter) would be more appropriate for Spring 7.
🧹 Nitpick comments (2)
spock-spring/spring7-test/spring7-test.gradle (1)
3-14: Redundant toolchain configuration.The
java.toolchainblock (lines 3-7) already configures Java 17. Thetasks.withType(JavaCompile)block (lines 9-14) redundantly sets the samelanguageVersion. The encoding configuration is valid, but the compiler version is duplicative.🔎 Suggested simplification
java { toolchain { languageVersion = JavaLanguageVersion.of(17) } } tasks.withType(JavaCompile).configureEach { - javaCompiler = javaToolchains.compilerFor { - languageVersion = JavaLanguageVersion.of(17) - } options.encoding = 'UTF-8' }spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy (1)
9-10: Consider adding@Configurationannotation.While Spring may detect the
@Beanmethods through component scanning or explicit@ContextConfigurationusage, adding the@Configurationannotation makes the intent explicit and is the conventional pattern.🔎 Suggested fix
import groovy.transform.CompileStatic +import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.Bean import java.util.concurrent.Callable import java.util.concurrent.ExecutorService @CompileStatic +@Configuration class NoMockConfig {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
settings.gradlespock-spring/spring3-test/spring3-test.gradlespock-spring/spring5-test/spring5-test.gradlespock-spring/spring6-test/spring6-test.gradlespock-spring/spring7-test/spring7-test.gradlespock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovyspock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/RuntimeCompatibilitySpec.groovyspock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringBeanTest.groovyspock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.javaspock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/TestConfig.groovy
🔇 Additional comments (10)
spock-spring/spring3-test/spring3-test.gradle (1)
8-8: LGTM!The syntax change aligns with the other dependency declarations in the file, improving consistency.
spock-spring/spring5-test/spring5-test.gradle (1)
10-10: LGTM!Consistent syntax style with other dependency declarations in this file.
spock-spring/spring6-test/spring6-test.gradle (1)
23-23: LGTM!Syntax consistency with surrounding dependency declarations.
settings.gradle (1)
107-107: LGTM!The new Spring 7 test module is correctly added in the non-2.5 variant block, maintaining logical version ordering after spring6-test.
Note: Based on PR comments, ensure that the new Spring version is also added to the set of versions to test in
spock-spring/spring.gradle.spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/TestConfig.groovy (1)
1-17: LGTM!Clean Spring configuration using
DetachedMockFactoryto provide a stubbedExecutorbean. The@CompileStaticannotation is a good practice for ensuring type safety.spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.java (1)
16-19: LGTM!The test serves its purpose of verifying that non-Spock tests using
SpringJUnit4ClassRunnerexecute correctly within the Spring 7 module.spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringBeanTest.groovy (1)
1-30: LGTM!Well-structured Spock specification demonstrating
@SpringBeanfunctionality with Spring 7. The test correctly:
- Uses
@ContextConfigurationwithNoMockConfigas the base configuration- Replaces the
ExecutorServicebean with a mock via@SpringBean- Verifies both the mock interaction and return value
spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/RuntimeCompatibilitySpec.groovy (1)
1-24: LGTM!The runtime compatibility spec is appropriately simple for its purpose—verifying that Spring 7 context loading and autowiring work correctly with Spock. The trivial
1 == 1assertion in the first test is intentional: if the Spring context fails to load, the test will fail before reaching that line.spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy (1)
25-35: LGTM with minor note on consistency.The
ServiceExecutorclass correctly models a service that depends onExecutorServicefor the mock replacement test scenario. Theexec()method usingdefreturn type is acceptable for test code, though an explicitStringreturn type would be slightly more self-documenting.spock-spring/spring7-test/spring7-test.gradle (1)
28-34: Spring 7 should be added to the test matrix inspock-spring/spring.gradle.The current
testSpringVersionsmap only includes Spring 4.3.5, 5.0.2, and 6.0.0. To ensure Spring 7 is tested alongside the other versions, add it to thetestSpringVersionsmap with the appropriate Java version constraints (e.g., Java 17+).
| @RunWith(SpringJUnit4ClassRunner.class) | ||
| @ContextConfiguration | ||
| public class SpringRunnerTest { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any context configuration files related to SpringRunnerTest
fd -t f "SpringRunnerTest" spock-spring/spring7-test/
echo "---"
# Also search for any -context.xml files in spring7-test
fd -t f "\-context\.xml$" spock-spring/spring7-test/
echo "---"
# Check what files exist in the test directory structure
fd -t f "\.xml$|\.java$|\.groovy$" spock-spring/spring7-test/src/test | head -20Repository: spockframework/spock
Length of output: 621
🏁 Script executed:
#!/bin/bash
# Read the SpringRunnerTest.java file to see full class content
cat -n spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.javaRepository: spockframework/spock
Length of output: 770
🏁 Script executed:
#!/bin/bash
# Check how other tests configure their Spring context
rg -A 3 "@ContextConfiguration|@SpringBootTest" spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/ -t groovy -t javaRepository: spockframework/spock
Length of output: 1482
🏁 Script executed:
#!/bin/bash
# Check if there's any base test class or configuration that SpringRunnerTest might inherit from
rg "class SpringRunnerTest|extends|@Configuration" spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.java -A 5 -B 5Repository: spockframework/spock
Length of output: 367
Specify context configuration explicitly.
The @ContextConfiguration annotation has no classes or locations parameter, which is inconsistent with other test classes in the same directory and relies on Spring's default context discovery. Since this test doesn't use any Spring beans, either:
- Remove
@ContextConfigurationif no context is needed, or - Specify
@ContextConfiguration(classes = {})to explicitly define an empty context
This prevents reliance on Spring's implicit behavior which may vary across versions or configurations.
🤖 Prompt for AI Agents
In
spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.java
around lines 12 to 14, the @ContextConfiguration annotation is empty which
relies on Spring's default context discovery; since this test does not use any
Spring beans, either remove the @ContextConfiguration annotation entirely, or
make the configuration explicit by changing it to @ContextConfiguration(classes
= {}) so the test does not depend on implicit/version-dependent context
discovery.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.