test: EXPOSED-233 Add additional SpringTransactionManager isolation test#1972
Merged
e5l merged 4 commits intoJetBrains:mainfrom Jan 23, 2024
Merged
Conversation
bog-walk
approved these changes
Jan 22, 2024
Member
bog-walk
left a comment
There was a problem hiding this comment.
Hi @FullOfOrange Thanks for putting these tests together!
Regarding your points above:
- Thanks for testing that the issue is actually resolved. I believe that the single connection tests, as well as the extra one, offer good coverage. At the moment I can't think up any additional cases to test, so unless you have some in mind, it's fair to set the isolation implementation as complete. If users come forward with new issues, we can then flesh out these test cases even further.
- & 3. Yes, unfortunately H2 only allows setting the entire database to read-only via
ACCESS_MODE_DATA=r;in the driver url. So we're not able to test setting the mode via the manager. Out of curiosity, I did run a test using a PostrgeSQL datasource and was able to set the read-only mode correctly with expected results. A case like this could warrant introducing a way to delegate such a test to a representatitve database. Maybe we need to consider an alternative toSpringTransactionTestBaseto run anything that can't be covered by H2.
e5l
approved these changes
Jan 23, 2024
Member
e5l
left a comment
There was a problem hiding this comment.
Hey @FullOfOrange, Thank you for the PR!
Good job, lgtm!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sorry for the late PR!
Added some tests to test the isolation level of SpringTransactionManager.
Write test code in
SpringTransactionSingleConnectionTestto check if the issue that when there is only one connection, the connection cannot be obtained unless the default isolation level is set is fixed in the latest version.Add test code to check if the isolation level is set well in the existing test
Discussion
I have confirmed that the problem encountered in this issue is resolved in the current version. I wonder if additional isolation tests are needed?
The
readOnlytest was tested with a spy test to make sure the value is set correctly. Unfortunately, H2 does not seem to support readOnly properly, so I was unable to test it.May spring test also need to test real database