Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2261 +/- ##
============================================
- Coverage 82.16% 82.16% -0.01%
- Complexity 4822 4831 +9
============================================
Files 472 477 +5
Lines 15036 15054 +18
Branches 1905 1905
============================================
+ Hits 12355 12369 +14
- Misses 1989 1993 +4
Partials 692 692
🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR adds a new boot4-test module to spock-spring, introducing a Spring Boot 4 test application with JPA entities, services, a web controller, configuration properties, and comprehensive Spock-based integration tests covering DataJpaTest, WebMvcTest, and scoped bean mocking patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Greptile SummaryThis PR adds a new Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
SG[settings.gradle - non-Groovy-2.5 block] --> B3[spock-spring:boot3-test]
SG --> B4[spock-spring:boot4-test]
SG --> S6[spock-spring:spring6-test]
B4 --> GRADLE[boot4-test.gradle - Spring Boot 4.0.3]
GRADLE --> DEPS[New test starters - data-jpa-test and webmvc-test]
B4 --> MAIN[Main sources]
MAIN --> M1[SimpleBootApp]
MAIN --> M2[HelloWorldService]
MAIN --> M3[ScopedHelloWorldService]
MAIN --> M4[HelloWorldController]
MAIN --> M5[Book and BookRepository]
B4 --> TESTS[Test specs]
TESTS --> T1[SimpleBootAppIntegrationSpec]
TESTS --> T2[SpringBootTestAnnotationIntegrationSpec]
TESTS --> T3[SpringBeanIntegrationSpec]
TESTS --> T4[WebMvcTestIntegrationSpec]
TESTS --> T5[DataJpaTestIntegrationSpec]
TESTS --> T6[SpringBootTestAnnotationScopedMockSpec]
TESTS --> T7[SpringBootTestAnnotationScopedProxyMockSpec]
Last reviewed commit: aa7c7ed |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import org.springframework.boot.data.jpa.test.autoconfigure.DataJpaTest | ||
| import org.springframework.boot.jpa.test.autoconfigure.TestEntityManager |
There was a problem hiding this comment.
Inconsistent package roots for JPA test imports
DataJpaTest is imported from org.springframework.boot.data.jpa.test.autoconfigure while TestEntityManager is imported from org.springframework.boot.jpa.test.autoconfigure — note the difference: data.jpa vs jpa. This inconsistency in the package root is worth verifying against the actual Spring Boot 4 module layout. If both classes ship in the same artifact (spring-boot-starter-data-jpa-test), they are likely in the same or a closely related package tree, and one of these imports may be wrong. The boot3 analogue imported both from org.springframework.boot.test.autoconfigure.orm.jpa.* with a single wildcard import, which naturally avoided this kind of mismatch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
spock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedProxyMockSpec.groovy (1)
52-55: Use Spring's proxy detection API instead of hard-coded internal bean names and generated class prefixes.The
scopedTarget.helloWorldServicelookup and$$SpringCGLIB$$class name prefix check depend on Spring Framework internals and generated class naming conventions. Spring provides standard testing utilities for this:AopUtils.isCglibProxy()to check proxy type semantically, andAopTestUtils.getTargetObject()to safely unwrap the target from the proxy. This pattern appears across the boot2, boot3, and boot4 test suites and should use the standard APIs instead.Suggested change
- def helloWorldServiceMock = context.getBean("scopedTarget.helloWorldService") as HelloWorldService + def helloWorldServiceMock = AopTestUtils.getTargetObject(helloWorldService) expect: - helloWorldService.class.simpleName.startsWith('HelloWorldService$$SpringCGLIB$$') + AopUtils.isCglibProxy(helloWorldService)Add imports:
import org.springframework.aop.support.AopUtils import org.springframework.test.util.AopTestUtils🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedProxyMockSpec.groovy` around lines 52 - 55, Replace the brittle internal lookup and name-prefix assertion by using Spring's proxy utilities: stop fetching the bean via context.getBean("scopedTarget.helloWorldService") and stop asserting on helloWorldService.class.simpleName; instead import and use AopUtils.isCglibProxy(helloWorldService) to assert it's a CGLIB proxy and use AopTestUtils.getTargetObject(helloWorldService) to obtain/unwrap the target for any further assertions (update references to helloWorldServiceMock/helloWorldService accordingly).
🤖 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/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationIntegrationSpec.groovy`:
- Around line 31-32: The test enables bean definition overriding in the
`@SpringBootTest` annotation (in SpringBootTestAnnotationIntegrationSpec) by
setting the property 'spring.main.allow-bean-definition-overriding=true'; remove
that property from the annotation so the context fails fast on accidental
duplicate beans (i.e., edit the `@SpringBootTest`(...) on the class to drop the
properties array or remove that entry), then re-run the spec to ensure startup
still succeeds.
In
`@spock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedMockSpec.groovy`:
- Around line 32-33: The test currently uses `@ScanScopedBeans` but tests a
singleton replacement bean (MockConfig.helloWorldService()), so it doesn't
actually verify scoped scanning; either change the replacement bean to a
non-singleton scope (e.g., annotate MockConfig.helloWorldService() as
`@Scope`("prototype") or use a scoped annotation used by
SpringMockTestExecutionListener) and update assertions to target that scoped
bean, or remove `@ScanScopedBeans` from the spec and rename the test class/methods
to indicate it's validating singleton/mock replacement behavior (adjust any
other occurrences at the same locations noted around lines 62-68 accordingly).
---
Nitpick comments:
In
`@spock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedProxyMockSpec.groovy`:
- Around line 52-55: Replace the brittle internal lookup and name-prefix
assertion by using Spring's proxy utilities: stop fetching the bean via
context.getBean("scopedTarget.helloWorldService") and stop asserting on
helloWorldService.class.simpleName; instead import and use
AopUtils.isCglibProxy(helloWorldService) to assert it's a CGLIB proxy and use
AopTestUtils.getTargetObject(helloWorldService) to obtain/unwrap the target for
any further assertions (update references to
helloWorldServiceMock/helloWorldService accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a080b41-a247-4a01-aaae-43aa1a5f18bf
📒 Files selected for processing (16)
settings.gradlespock-spring/boot4-test/boot4-test.gradlespock-spring/boot4-test/src/main/java/org/spockframework/boot4/SimpleBootApp.javaspock-spring/boot4-test/src/main/java/org/spockframework/boot4/jpa/Book.javaspock-spring/boot4-test/src/main/java/org/spockframework/boot4/jpa/BookRepository.javaspock-spring/boot4-test/src/main/java/org/spockframework/boot4/service/HelloWorldService.javaspock-spring/boot4-test/src/main/java/org/spockframework/boot4/service/ScopedHelloWorldService.javaspock-spring/boot4-test/src/main/java/org/spockframework/boot4/web/HelloWorldController.javaspock-spring/boot4-test/src/main/resources/application.propertiesspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/DataJpaTestIntegrationSpec.groovyspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SimpleBootAppIntegrationSpec.groovyspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBeanIntegrationSpec.groovyspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationIntegrationSpec.groovyspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedMockSpec.groovyspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedProxyMockSpec.groovyspock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/WebMvcTestIntegrationSpec.groovy
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE, | ||
| properties = ['spring.main.allow-bean-definition-overriding=true']) |
There was a problem hiding this comment.
Drop bean overriding from this context-load spec.
Nothing in this spec replaces application beans, so enabling spring.main.allow-bean-definition-overriding only weakens the test: an accidental duplicate bean would be silently accepted instead of failing startup.
Suggested cleanup
-@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE,
- properties = ['spring.main.allow-bean-definition-overriding=true'])
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE, | |
| properties = ['spring.main.allow-bean-definition-overriding=true']) | |
| `@SpringBootTest`(webEnvironment = SpringBootTest.WebEnvironment.NONE) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@spock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationIntegrationSpec.groovy`
around lines 31 - 32, The test enables bean definition overriding in the
`@SpringBootTest` annotation (in SpringBootTestAnnotationIntegrationSpec) by
setting the property 'spring.main.allow-bean-definition-overriding=true'; remove
that property from the annotation so the context fails fast on accidental
duplicate beans (i.e., edit the `@SpringBootTest`(...) on the class to drop the
properties array or remove that entry), then re-run the spec to ensure startup
still succeeds.
| @ScanScopedBeans | ||
| @SpringBootTest(properties = ['spring.main.allow-bean-definition-overriding=true']) |
There was a problem hiding this comment.
@ScanScopedBeans isn't actually under test here.
Per spock-spring/src/main/java/org/spockframework/spring/SpringMockTestExecutionListener.java:94-103, singleton bean definitions are attached even without @ScanScopedBeans; only non-singletons take the scoped-scan path. Since MockConfig.helloWorldService() is a plain singleton @Bean, this spec would still pass if scoped-bean scanning regressed.
Either make the replacement bean scoped or point the assertions at an actual scoped mock; otherwise this should drop @ScanScopedBeans and be renamed to reflect the non-scoped case.
Also applies to: 62-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@spock-spring/boot4-test/src/test/groovy/org/spockframework/boot4/SpringBootTestAnnotationScopedMockSpec.groovy`
around lines 32 - 33, The test currently uses `@ScanScopedBeans` but tests a
singleton replacement bean (MockConfig.helloWorldService()), so it doesn't
actually verify scoped scanning; either change the replacement bean to a
non-singleton scope (e.g., annotate MockConfig.helloWorldService() as
`@Scope`("prototype") or use a scoped annotation used by
SpringMockTestExecutionListener) and update assertions to target that scoped
bean, or remove `@ScanScopedBeans` from the spec and rename the test class/methods
to indicate it's validating singleton/mock replacement behavior (adjust any
other occurrences at the same locations noted around lines 62-68 accordingly).
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests