Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2262 +/- ##
============================================
- Coverage 82.16% 82.01% -0.16%
+ Complexity 4822 4815 -7
============================================
Files 472 472
Lines 15036 15038 +2
Branches 1905 1907 +2
============================================
- Hits 12355 12334 -21
- Misses 1989 2011 +22
- Partials 692 693 +1
🚀 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?
There was a problem hiding this comment.
Maybe?
Here I just copied the spring6 tests as they are and made them be spring7 tests.
I'd say such a polishing could be a separate PR?
|
|
||
| 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Spring 7 test module and tests, updates the Spring test-version matrix, includes Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (Spock / JUnit)
participant Spring as Spring Context
participant Service as ServiceExecutor
participant Exec as ExecutorService
participant Future as Future
Test->>Spring: load `@ContextConfiguration` (NoMockConfig/TestConfig)
Spring->>Service: instantiate ServiceExecutor (inject ExecutorService)
Test->>Service: invoke exec()
Service->>Exec: submit Callable
Exec->>Future: return Future
Future->>Service: get() -> result
Service-->>Test: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
settings.gradle (1)
107-107: LGTM! Module inclusion follows the established pattern.The
spring7-testmodule is correctly placed alongsidespring6-testandboot3-testin the non-2.5 variant block, which appropriately requires Java 17+.Note: Per PR comments from AndreasTu, ensure that
spock-spring/spring.gradleis also updated to include Spring 7 in the version mapping. Add"7.0.0": (17..17)to thetestSpringVersionsmap (currently missing Spring 7).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@settings.gradle` at line 107, Update the Spring version-to-Java mapping in spock-spring/spring.gradle by adding an entry for Spring 7: locate the testSpringVersions map and insert the key "7.0.0" with the value (17..17) so Spring 7 is associated with Java 17 (e.g., add "7.0.0": (17..17) to the testSpringVersions map).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@settings.gradle`:
- Line 107: Update the Spring version-to-Java mapping in
spock-spring/spring.gradle by adding an entry for Spring 7: locate the
testSpringVersions map and insert the key "7.0.0" with the value (17..17) so
Spring 7 is associated with Java 17 (e.g., add "7.0.0": (17..17) to the
testSpringVersions map).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3188ea99-7f97-463f-b647-4faa8380e0d9
📒 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
🚧 Files skipped from review as they are similar to previous changes (7)
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy
- spock-spring/spring7-test/spring7-test.gradle
- spock-spring/spring5-test/spring5-test.gradle
- spock-spring/spring6-test/spring6-test.gradle
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.java
- spock-spring/spring3-test/spring3-test.gradle
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/TestConfig.groovy
Added
Also here, I'd say this could / should be a separate PR. Besides that I did as you said, adding 21 to the upper range and executing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spock-spring/spring.gradle`:
- Around line 16-17: The Spring Java-version map currently pins "6.0.0" and
"7.0.0" to (17..17), which causes the .findAll filter to drop these entries on
JDK 21 and prevents spring6Test/spring7Test variants from being generated;
update the map entries for the keys "6.0.0" and "7.0.0" (in spring.gradle) to a
broader range such as (17..21) so they include Java 21 (or use a dynamic upper
bound that includes the running JDK), ensuring the .findAll on line 18 will keep
those versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11570e0f-c92d-49b0-b8b9-3fa82dc5571f
📒 Files selected for processing (1)
spock-spring/spring.gradle
Vampire
force-pushed
the
spring7-test
branch
2 times, most recently
from
bbb9a75 to
44cffd5
Compare
Ah, old brain. Actually, with 6.x it works for me for both, 17 and 21. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spock-spring/spring.gradle (1)
16-17: Cap Spring Framework 6.0.x test coverage to Java 21.Line 16 currently expands automatically to the repo's highest configured JDK (currently 25). Spring Framework 6.0.x officially supports only Java 17–21, so tests may run on JDK 25 where compatibility is not guaranteed by the framework.
Line 17 (Spring 7.0.0) can safely remain uncapped since Spring 7.0.0 officially targets Java 17–25+.
Proposed patch
+def maxTestedSpring6Java = 21 + def testSpringVersions = [ (springVersion): (8..11), "5.0.2.RELEASE": (8..11), - "6.0.0" : (17..javaVersions.max()), + "6.0.0" : (17..Math.min(javaVersions.max(), maxTestedSpring6Java)), "7.0.0" : (17..javaVersions.max()) ].findAll { javaVersion in it.value }.keySet()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-spring/spring.gradle` around lines 16 - 17, The Spring 6.0.x test matrix currently uses (17..javaVersions.max()), which expands to the repo's highest JDK; change the map entry for the key "6.0.0" to use a capped range (17..21) instead of javaVersions.max() so tests for "6.0.0" only run on Java 17–21; leave the "7.0.0" entry as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spock-spring/spring.gradle`:
- Around line 16-17: The Spring 6.0.x test matrix currently uses
(17..javaVersions.max()), which expands to the repo's highest JDK; change the
map entry for the key "6.0.0" to use a capped range (17..21) instead of
javaVersions.max() so tests for "6.0.0" only run on Java 17–21; leave the
"7.0.0" entry as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61b7aa5e-edab-45c0-ac9f-9b05d43a8555
📒 Files selected for processing (1)
spock-spring/spring.gradle
|
Ah, it was mostly that Spring 7 now only supports There is only one test left that does not work with Spring 7. |
|
Hoping this is a Spring regression, I opened spring-projects/spring-framework#36463 |
3 participants
Summary by CodeRabbit
New Features
Chores