test(spanner): deflake multiplexed session mock tests and skip ITInstanceAdminTest in staging/devel#13020
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors tests in MultiplexedSessionDatabaseClientMockServerTest.java by consolidating test logic and adding request count assertions, and updates ITInstanceAdminTest.java to skip execution in development and staging environments. Feedback suggests that the refactored test is missing explicit multiplexed session flags in its configuration and lacks the necessary code to verify the expected DEADLINE_EXCEEDED exception.
| .setSessionPoolOption( | ||
| SessionPoolOptions.newBuilder().setFailOnSessionLeak().build()) |
There was a problem hiding this comment.
The SessionPoolOptions configuration is missing the flags to enable multiplexed sessions (e.g., setUseMultiplexedSession(true)). Since this test class is specifically designed to test multiplexed session behavior, these flags should be explicitly set to ensure the test is valid and consistent with the deleted testRetryWithNoSessionCreationWaitTime test.
.setSessionPoolOption(
SessionPoolOptions.newBuilder()
.setUseMultiplexedSession(true)
.setUseMultiplexedSessionForRW(true)
.setUseMultiplexedSessionPartitionedOps(true)
.setFailOnSessionLeak()
.build())
There was a problem hiding this comment.
Multiplexed sessions are enabled by default.
| // The first attempt should lead to a DEADLINE_EXCEEDED error being propagated from the | ||
| // CreateSession attempt. |
There was a problem hiding this comment.
The comment states that the first attempt should result in a DEADLINE_EXCEEDED error, but the code to perform this attempt and verify the exception (e.g., using assertThrows) is missing from the patch. This verification is crucial for testing the retry logic and was present in the deleted test testRetryWithNoSessionCreationWaitTime.
References
- Use Assertions.assertThrows if the exception is truly expected as part of the test scenario.
There was a problem hiding this comment.
This comment was not added in this test, and as such this review comment is a bit weird. And in addition: The assertThrows is one line below this comment....
| .setSessionPoolOption( | ||
| SessionPoolOptions.newBuilder().setFailOnSessionLeak().build()) |
There was a problem hiding this comment.
Multiplexed sessions are enabled by default.
| // The first attempt should lead to a DEADLINE_EXCEEDED error being propagated from the | ||
| // CreateSession attempt. |
There was a problem hiding this comment.
This comment was not added in this test, and as such this review comment is a bit weird. And in addition: The assertThrows is one line below this comment....
…anceAdminTest in staging/devel
sakthivelmanii
force-pushed
the
deflake-mock-test-and-skip-instance-admin-test
branch
from
d22899e to
6f2d550
Compare
3 participants
No description provided.