-
-
Notifications
You must be signed in to change notification settings - Fork 962
Fix/hibernate session integration tests 14920 #15002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 7.0.x
Are you sure you want to change the base?
Fix/hibernate session integration tests 14920 #15002
Conversation
@RAJ-debug858 Thank you. Can you resubmit with the a target branch of 7.0.x? When I try to make this change I get "There are no new commits between base branch '7.0.x' and head branch 'fix/hibernate-session-integration-tests-14920'", so I am guessing some of your new commits are not present on your forked branch. |
FYI: you can add your branch as a remote, fetch, cherry-pick your change, and then adjust for where its supposed to be. GitHub is showing 10,000+ commits so my guess is this change was made against an earlier branch before we created the mono repo. |
@jamesfredley Thank you! I’ve rebased my changes onto the 7.0.x branch and force-pushed the updated branch. The PR now targets 7.0.x as requested. |
|
@@ -47,7 +47,8 @@ dependencies { | |||
|
|||
api project(':grails-databinding') | |||
api project(':grails-datamapping-core') | |||
api project(':grails-test-core') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test-core is meant to be the parent to this dependency. Why are you removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored grails-test-core dependency in grails-testing-support-core
grails-test-core/build.gradle
Outdated
@@ -31,6 +31,7 @@ dependencies { | |||
implementation platform(project(':grails-bom')) | |||
|
|||
api 'org.springframework:spring-tx' | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra newline should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hibernate considers OSIV an anti pattern. I understand that previous Grails applications have used it and I fully support adding test support for the feature, but we should not make this behavior the default given that it's considered bad practice upstream.
@@ -48,6 +48,7 @@ dependencies { | |||
api project(':grails-databinding') | |||
api project(':grails-datamapping-core') | |||
api project(':grails-test-core') | |||
api('org.spockframework:spock-core') { transitive = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incudes by test-core so it should be unnecessary
@@ -29,7 +29,7 @@ | |||
* @author Graeme Rocher | |||
* @since 3.0 | |||
*/ | |||
@ContextConfiguration(loader = GrailsApplicationContextLoader.class) | |||
@ContextConfiguration(loader = SpringBootContextLoader.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't we using the Grails Context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay we should use the Grails-specific context loader since it provides Grails
application context setup rather than just generic Spring Boot context. The GrailsApplicationContextLoader
extends SpringBootContextLoader but adds Grails-specific initialization through GrailsApp.
Can't the test behavior be driven by whether the application uses OSIV? |
@@ -60,7 +60,10 @@ class IntegrationSpecConfigurerExtension implements IAnnotationDrivenExtension<A | |||
|
|||
IntegrationSpecMethodInterceptor(ApplicationContext applicationContext) { | |||
this.applicationContext = applicationContext | |||
this.mode = new GrailsTestMode(autowire: true, wrapInTransaction: true, wrapInRequestEnvironment: true) | |||
// By default, bind session for integration tests to match application behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The application behavior depends if OSIV is configured and if hibernate is used. We should have our tests default to best behaviors, or even better, detect the configuration and then only apply it based on the application configuration.
@@ -1 +1 @@ | |||
org.grails.testing.spock.TestingSupportExtension | |||
org.grails.test.spock.WithSessionSpecExtension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the original extension? Why are we replacing the old behavior instead of augmenting it?
@CompileStatic | ||
class GrailsTestSessionInterceptor { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(GrailsTestSessionInterceptor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a groovy class, is there a reason you can't use @Slf4j
?
@@ -30,6 +30,7 @@ class GrailsTestInterceptor { | |||
private String[] testClassSuffixes | |||
|
|||
private GrailsTestTransactionInterceptor transactionInterceptor | |||
private GrailsTestSessionInterceptor sessionInterceptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not compiling.
I've successfully implemented a solution to address the Hibernate session and transaction issue in Grails
integration tests. The solution provides a way for integration tests to run with the same transactional context
as the application.
What was implemented:
(grails-testing-support-core/src/main/groovy/grails/testing/mixin/integration/WithSession.groovy):
- New annotation that binds a Hibernate session without starting a transaction
- Can be applied at class or method level
- Supports specifying specific datasources
- GrailsTestSessionInterceptor - Manages session binding without transactions
- WithSessionSpecExtension - Spock extension for handling @WithSession
- WithSessionTransformation - AST transformation for the annotation
- Updated GrailsTestMode to support bindSession flag
- Modified GrailsTestInterceptor to handle session-only binding
- Updated IntegrationSpecConfigurerExtension to enable session binding by default
- WithSessionIntegrationSpec - Demonstrates usage of @WithSession
- SessionBindingComparisonSpec - Shows differences between test modes
- Added comprehensive documentation in the integration testing guide
How it solves the problem:
The issue described three scenarios:
The new @WithSession annotation provides a session binding that matches the OISV (Open Session In View) pattern
used in running applications, where:
This ensures integration tests accurately reflect the application's runtime behavior, preventing false positives
and false negatives in test results.