Repository navigation
test(spanner): speed up unit test suite and eliminate idle retry sleeps - #18582
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors and optimizes various unit tests in the Google Cloud Spanner package. Key changes include replacing time-based sleeps with robust synchronization mechanisms like threading.Event, asyncio.Event, and threading.Barrier, mocking sleep calls to verify retry logic, reducing Argon2 parameters to speed up tests, and refactoring admin client instantiations to use mocked transports. The review feedback correctly identifies a parameter signature mismatch in the fake_delay helper function within test_batch.py and suggests aligning it with the original _delay_until_retry signature to prevent potential TypeError exceptions.
0df711c to
701269c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and optimizes various unit tests across the google-cloud-spanner package. Key changes include mocking sleep calls to prevent test delays, optimizing Argon2Id hashing parameters to speed up authentication tests, and replacing time-based polling with deterministic synchronization primitives like threading.Event, asyncio.Event, and threading.Barrier. Additionally, mock APIs are refactored to use mocked transports instead of anonymous credentials. Feedback on these changes suggests aligning parameter names in a mock delay function with the actual helper signature in test_batch.py, and catching BrokenBarrierError in test_metrics_concurrency.py to prevent secondary failures from obscuring the root cause of test errors.
701269c to
7b20ecc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes and stabilizes unit tests by mocking sleep delays, reducing Argon2 hashing parameters for faster execution, replacing arbitrary sleeps with robust thread synchronization mechanisms, and isolating admin API clients. The review feedback identifies a potential RuntimeError in Python 3.11+ caused by instantiating asyncio.Event outside of an active event loop, and recommends asserting the retry call count in test_batch.py to guarantee that retry logic is executed during the test.
7b20ecc to
b6ba3bc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and improves unit tests across several modules in google-cloud-spanner. Key changes include mocking sleep functions to prevent delays, lowering Argon2Id parameters to speed up cryptographic tests, utilizing mock transports for admin clients, and replacing polling-based assertions with thread joins, barriers, and events for more reliable synchronization. Feedback on these changes suggests making the fake_delay mock signature-agnostic in test_batch.py to prevent potential keyword argument failures, and adding a timeout to the entered_slow_build.wait() call in test_database_session_manager.py to avoid indefinite hangs during test execution.
b6ba3bc to
17ccee6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and improves various unit tests across the Spanner client library. Key changes include mocking sleep calls to speed up retry tests, reducing Argon2 hashing parameters in OPAQUE tests for faster execution, using a threading barrier to reliably test metrics concurrency, and improving database session manager tests by replacing custom polling loops with thread joins and explicit synchronization events. Additionally, mock admin APIs are introduced to avoid using anonymous credentials. The reviewer recommended combining several redundant import statements from the same admin modules in test_client.py and test_instance.py to clean up the code and improve readability.
17ccee6 to
8796976
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and optimizes various unit tests across the google-cloud-spanner package. Key changes include mocking sleep and delay functions to prevent actual delays, reducing Argon2 parameters to speed up authentication tests, utilizing synchronization primitives like threading.Event and threading.Barrier to replace flaky polling-based assertions, and introducing helper methods to cleanly mock admin clients. Feedback on these changes suggests increasing the barrier wait timeout in the concurrency metrics test from 5.0 to 10.0 seconds to safeguard against flakiness in resource-constrained CI environments.
|
/gemini review |
8796976 to
ca0fcd1
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors various unit tests across the Google Cloud Spanner package to improve test reliability, speed, and mock hygiene. Key changes include localizing time.sleep and asyncio.sleep mocks, reducing Argon2 parameters to speed up authentication tests, using threading.Barrier and asyncio.Event to replace fragile sleep-based synchronization in concurrency tests, and introducing helper methods to construct mock admin clients. The review feedback suggests localizing the time.time patch in test_session.py to target google.cloud.spanner_v1._helpers.time.time for consistency with the localized time.sleep mock.
Speed up the unit test suite by ~55% (reducing full suite runtime from ~66s to ~29s) by eliminating wall-clock sleep delays, avoiding live gRPC transport initialization, and reducing cryptographic CPU burn in protocol tests.
Key changes:
- Intercept retry backoff sleeps: Mock localized `time.sleep` and `asyncio.sleep` in transaction, snapshot, and batch commit retry tests. Each test now verifies retry counts and delay values without idling in real time.
- Deterministic concurrency synchronization: Replace arbitrary timing sleeps in session manager and metrics tests with explicit `asyncio.Event` and `threading.Barrier` coordination, eliminating CI flakiness under CPU contention.
- Eliminate teardown polling: Remove `_assert_true_with_timeout` and sleep polling from `test_database_session_manager.py` by relying on event-driven rotation callbacks and blocking thread joins.
- Isolate GAPIC admin transports: Use autospec mock transports in `test_client.py` and `test_instance.py` instead of default credentials, eliminating live gRPC channel creation and 60-second network timeout risks.
- Lower Omni Argon2id test parameters: Use minimal valid test parameters (1 iteration, 8 KB RAM) in protocol flow and state machine tests, reducing key derivation time by ~1,100x while leaving RFC golden vector tests untouched.
- Clean up legacy mocks: Convert legacy global `mock.patch("time.sleep")` calls in `test_session.py` to localized module paths adhering to repository mock hygiene rules.
ca0fcd1 to
842ee99
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the reliability, speed, and correctness of unit tests across the google-cloud-spanner package. Key changes include updating mock patches to target specific module-level imports of time.sleep, time.time, and asyncio.sleep rather than patching global namespaces, ensuring mock assertions are correctly triggered. Additionally, tests for OPAQUE authentication are optimized by reducing Argon2 parameter values to speed up execution. Concurrency and maintenance tests in DatabaseSessionsManager and metrics tracking are refactored to use synchronization primitives like threading.Event, asyncio.Event, and threading.Barrier instead of arbitrary sleeps, eliminating potential flakiness. Finally, admin client tests are updated to use mock transports instead of anonymous credentials. There are no review comments, so no further feedback is provided.
Speed up the unit test suite by ~55% (reducing full suite runtime from ~66s to ~29s) by eliminating wall-clock sleep delays, avoiding live gRPC transport initialization, and reducing cryptographic CPU burn in protocol tests.
Key changes:
time.sleepandasyncio.sleepin transaction, snapshot, and batch commit retry tests. Each test now verifies retry counts and delay values without idling in real time.asyncio.Eventandthreading.Barriercoordination, eliminating CI flakiness under CPU contention._assert_true_with_timeoutand sleep polling fromtest_database_session_manager.pyby relying on event-driven rotation callbacks and blocking thread joins.test_client.pyandtest_instance.pyinstead of default credentials, eliminating live gRPC channel creation and 60-second network timeout risks.mock.patch("time.sleep")calls intest_session.pyto localized module paths adhering to repository mock hygiene rules.