Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2262 +/- ##
============================================
- Coverage 82.16% 82.15% -0.01%
- Complexity 4822 4823 +1
============================================
Files 472 472
Lines 15036 15038 +2
Branches 1905 1907 +2
============================================
+ Hits 12354 12355 +1
- Misses 1989 1991 +2
+ Partials 693 692 -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
📝 WalkthroughWalkthroughAdds a Spring 7 test module with tests and configs, updates Spring-version matrix and Gradle/groovy settings, extends injection handling to Jakarta annotations, includes the new module in non-2.5 settings, and performs small Gradle DSL syntax tweaks in existing test modules. 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 Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
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 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Turns out it was not a Spring regression, but it switched from using debug information to get parameter names for bean names to using the Now the test should become green. |
|
Indeed this would have stopped working with Spring 6.1 already, but we still test against 6.0.0 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy (1)
32-34: Add timeout to unbounded.get()call inServiceExecutor.exec().Line 33 calls
.get()without a timeout, which can block indefinitely if the executor is miswired or stalled. Use a bounded wait with an explicit timeout duration.Suggested refactor
+import java.util.concurrent.TimeUnit import java.util.concurrent.Callable import java.util.concurrent.ExecutorService @@ - def exec() { - executorService.submit({"done"} as Callable).get() + String exec() { + executorService.submit({ "done" } as Callable<String>).get(5, TimeUnit.SECONDS) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy` around lines 32 - 34, The exec() method currently calls Future.get() with no timeout (executorService.submit({"done"} as Callable).get()), which can block indefinitely; change this to use Future.get(long, TimeUnit) with a reasonable bounded timeout (e.g. 5 seconds) and handle TimeoutException, InterruptedException and ExecutionException appropriately (log/propagate/fail the test). Update ServiceExecutor.exec()/NoMockConfig.exec() to call get(<timeout>, TimeUnit.SECONDS), add the TimeUnit import, and ensure you catch TimeoutException to cancel the Future and surface a clear error.
🤖 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/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy`:
- Around line 32-34: The exec() method currently calls Future.get() with no
timeout (executorService.submit({"done"} as Callable).get()), which can block
indefinitely; change this to use Future.get(long, TimeUnit) with a reasonable
bounded timeout (e.g. 5 seconds) and handle TimeoutException,
InterruptedException and ExecutionException appropriately (log/propagate/fail
the test). Update ServiceExecutor.exec()/NoMockConfig.exec() to call
get(<timeout>, TimeUnit.SECONDS), add the TimeUnit import, and ensure you catch
TimeoutException to cancel the Future and surface a clear error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8008ceac-0658-4400-a163-13a66722ebf4
📒 Files selected for processing (19)
settings.gradlespock-spring/spring.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.groovyspock-spring/src/main/java/org/spockframework/spring/SpringExtension.javaspock-spring/src/test/groovy/org/spockframework/spring/ContextHierarchyExample.groovyspock-spring/src/test/groovy/org/spockframework/spring/InjectionExamples.groovyspock-spring/src/test/groovy/org/spockframework/spring/MockInjectionExample.groovyspock-spring/src/test/groovy/org/spockframework/spring/SharedFieldsInjection.groovyspock-spring/src/test/groovy/org/spockframework/spring/docs/MockExamples.groovyspock-spring/src/test/groovy/org/spockframework/spring/mock/ComplexUsageSpec.groovyspock-spring/src/test/groovy/org/spockframework/spring/mock/imported/SpringSpyWithAopProxyTests.groovy
💤 Files with no reviewable changes (1)
- spock-spring/src/test/groovy/org/spockframework/spring/mock/ComplexUsageSpec.groovy
✅ Files skipped from review due to trivial changes (4)
- spock-spring/spring3-test/spring3-test.gradle
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/TestConfig.groovy
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/RuntimeCompatibilitySpec.groovy
- spock-spring/spring5-test/spring5-test.gradle
🚧 Files skipped from review as they are similar to previous changes (6)
- spock-spring/spring6-test/spring6-test.gradle
- settings.gradle
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.java
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringBeanTest.groovy
- spock-spring/spring7-test/spring7-test.gradle
- spock-spring/spring.gradle
🔎 No tests executed 🔎🏷️ Commit: 75ddaa1 Learn more about TestLens at testlens.app. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spock-spring/src/test/groovy/org/spockframework/spring/docs/MockExamples.groovy (1)
30-50: Prefer isolated qualifier examples instead of dual-annotating each field.Adding both
@JavaxNamedand@jakarta.inject.Namedto the same injection point makes failures harder to attribute and weakens the example’s explanatory value. Consider separate examples/tests per qualifier flavor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-spring/src/test/groovy/org/spockframework/spring/docs/MockExamples.groovy` around lines 30 - 50, The fields GreeterService serviceMock, serviceStub, serviceSpy and alternativeMock are dual-annotated with `@JavaxNamed` and `@Autowired` `@Named` which confuses failures; split the examples so each injection point shows only one qualifier flavor: either use `@JavaxNamed` on its own for a dedicated test/example (e.g. serviceMock/serviceStub/serviceSpy with `@JavaxNamed`) and create separate examples/tests that use `@Autowired` `@Named` (or jakarta variant) for the same logical cases, removing the dual annotations from each field and keeping one qualifier per example to make failures and intent clear.spock-spring/src/test/groovy/org/spockframework/spring/SharedFieldsInjection.groovy (1)
60-63: Separate javax/jakarta injection scenarios to avoid masked regressions.Using both
@javax.inject.Injectand@jakarta.inject.Injecton the same field makes the test pass even if one path breaks. Consider parameterizing these specs so each run uses exactly one annotation variant.Also applies to: 85-88, 116-119, 206-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-spring/src/test/groovy/org/spockframework/spring/SharedFieldsInjection.groovy` around lines 60 - 63, The test uses both javax and jakarta injection annotations on the same field (e.g., the SharedFieldsInjection.groovy fields like sharedService annotated with `@javax.inject.Inject` and `@Inject`), which masks regressions; update the spec to run separate scenarios for each injection API by parameterizing the tests (or duplicating the field cases) so each execution uses exactly one annotation variant per field (apply same change for the other occurrences around lines with the other shared fields referenced in the review); ensure the parameterization/variants exercise only `@javax.inject.Inject` in one run and only `@jakarta.inject.Inject` in the other so failures aren’t hidden.
🤖 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/src/test/groovy/org/spockframework/spring/docs/MockExamples.groovy`:
- Around line 30-50: The fields GreeterService serviceMock, serviceStub,
serviceSpy and alternativeMock are dual-annotated with `@JavaxNamed` and
`@Autowired` `@Named` which confuses failures; split the examples so each injection
point shows only one qualifier flavor: either use `@JavaxNamed` on its own for a
dedicated test/example (e.g. serviceMock/serviceStub/serviceSpy with
`@JavaxNamed`) and create separate examples/tests that use `@Autowired` `@Named` (or
jakarta variant) for the same logical cases, removing the dual annotations from
each field and keeping one qualifier per example to make failures and intent
clear.
In
`@spock-spring/src/test/groovy/org/spockframework/spring/SharedFieldsInjection.groovy`:
- Around line 60-63: The test uses both javax and jakarta injection annotations
on the same field (e.g., the SharedFieldsInjection.groovy fields like
sharedService annotated with `@javax.inject.Inject` and `@Inject`), which masks
regressions; update the spec to run separate scenarios for each injection API by
parameterizing the tests (or duplicating the field cases) so each execution uses
exactly one annotation variant per field (apply same change for the other
occurrences around lines with the other shared fields referenced in the review);
ensure the parameterization/variants exercise only `@javax.inject.Inject` in one
run and only `@jakarta.inject.Inject` in the other so failures aren’t hidden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2660bd22-0a50-438d-8c92-14388bb561cb
📒 Files selected for processing (19)
settings.gradlespock-spring/spring.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.groovyspock-spring/src/main/java/org/spockframework/spring/SpringExtension.javaspock-spring/src/test/groovy/org/spockframework/spring/ContextHierarchyExample.groovyspock-spring/src/test/groovy/org/spockframework/spring/InjectionExamples.groovyspock-spring/src/test/groovy/org/spockframework/spring/MockInjectionExample.groovyspock-spring/src/test/groovy/org/spockframework/spring/SharedFieldsInjection.groovyspock-spring/src/test/groovy/org/spockframework/spring/docs/MockExamples.groovyspock-spring/src/test/groovy/org/spockframework/spring/mock/ComplexUsageSpec.groovyspock-spring/src/test/groovy/org/spockframework/spring/mock/imported/SpringSpyWithAopProxyTests.groovy
💤 Files with no reviewable changes (1)
- spock-spring/src/test/groovy/org/spockframework/spring/mock/ComplexUsageSpec.groovy
✅ Files skipped from review due to trivial changes (5)
- spock-spring/spring3-test/spring3-test.gradle
- settings.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/TestConfig.groovy
🚧 Files skipped from review as they are similar to previous changes (11)
- spock-spring/src/main/java/org/spockframework/spring/SpringExtension.java
- spock-spring/src/test/groovy/org/spockframework/spring/ContextHierarchyExample.groovy
- spock-spring/src/test/groovy/org/spockframework/spring/mock/imported/SpringSpyWithAopProxyTests.groovy
- spock-spring/src/test/groovy/org/spockframework/spring/MockInjectionExample.groovy
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringRunnerTest.java
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/SpringBeanTest.groovy
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/RuntimeCompatibilitySpec.groovy
- spock-spring/spring.gradle
- spock-spring/spring7-test/src/test/groovy/org/spockframework/spring7/NoMockConfig.groovy
- spock-spring/spring7-test/spring7-test.gradle
- spock-spring/src/test/groovy/org/spockframework/spring/InjectionExamples.groovy
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores