test(spanner): add pytest-xdist parallel execution with state isolation#17344
test(spanner): add pytest-xdist parallel execution with state isolation#17344chalmerlowe wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request parallelizes unit tests by installing pytest-xdist and running pytest with -n auto, while addressing test isolation issues by resetting metrics singletons, cleaning up asyncio event loops, and updating subtests to use string identifiers. Feedback on these changes suggests keeping the mypy session skipped as it is out of scope, using pytest's monkeypatch fixture for environment variable management, and declaring pytest-xdist in the package's primary dependency file.
22b979b to
cc28cd2
Compare
|
|
||
| return mock.create_autospec(SpannerClient, instance=True) | ||
|
|
||
| def _assert_concurrent_transaction_invariants(self, call_args_list, expected_count=2): |
There was a problem hiding this comment.
For the reviewer:
This new function is a helper that enables us to confirm test results with more stability in concurrently run systems by checking for elements that won't change during a concurrent transation rather than elements that might change depending on the order that a concurrent set of tests finishes.
There are four locations where we strip out some long test bodies that were very specific (e.g. did test 1.1 come before 2.1, etc) and replace them with:
Is there a single "begin" transaction.
Do other transactions reuse the same transaction ID throughout.
|
|
||
| self._batch_update_helper(transaction=transaction, database=database, api=api) | ||
|
|
||
| api.execute_sql.assert_any_call( |
There was a problem hiding this comment.
As noted above, here we are replacing the test body that was looking for specific id numbers (and became brittle when run in a concurrent transaction).
Replaced it with the more invariant test body from the helper function: _assert_concurrent_transaction_invariants
| self._execute_update_helper(transaction=transaction, api=api) | ||
| self.assertEqual(api.execute_sql.call_count, 1) | ||
|
|
||
| api.execute_sql.assert_any_call( |
There was a problem hiding this comment.
Replaced the test body with the more invariant test body: _assert_concurrent_transaction_invariants
| ) | ||
| self._assert_concurrent_transaction_invariants(api.execute_batch_dml.call_args_list, 2) | ||
|
|
||
| self.assertEqual(api.execute_batch_dml.call_count, 2) |
There was a problem hiding this comment.
Replaced the test body with the more invariant test body: _assert_concurrent_transaction_invariants
|
|
||
| self._execute_update_helper(transaction=transaction, api=api) | ||
|
|
||
| api.execute_sql.assert_any_call( |
There was a problem hiding this comment.
Replaced the test body with the more invariant test body: _assert_concurrent_transaction_invariants
|
|
||
| self._execute_update_helper(transaction=transaction, api=api) | ||
|
|
||
| begin_read_write_count = sum( |
There was a problem hiding this comment.
Replaced the test body with the more invariant test body: _assert_concurrent_transaction_invariants
7821620 to
ab88581
Compare
ab88581 to
72804c4
Compare
daniel-sanche
left a comment
There was a problem hiding this comment.
couple small comments, otherwise LGTM
| called_with.append((txn, args, kw)) | ||
| txn.insert(TABLE_NAME, COLUMNS, VALUES) | ||
|
|
||
| import threading |
There was a problem hiding this comment.
can this be moved to to top of the file?
| called_with.append((txn, args, kw)) | ||
| txn.insert(TABLE_NAME, COLUMNS, VALUES) | ||
|
|
||
| import threading |
There was a problem hiding this comment.
if we import threading globally, this shouldnt be needed
| called_with.append((txn, args, kw)) | ||
| txn.insert(TABLE_NAME, COLUMNS, VALUES) | ||
|
|
||
| import threading |
There was a problem hiding this comment.
same comments about imports
| CURRENT_DIRECTORY / "testing" / f"constraints-{session.python}.txt" | ||
| ) | ||
| install_unittest_dependencies(session, "-c", constraints_path) | ||
| session.install("pytest-xdist") |
There was a problem hiding this comment.
Can this be added to UNIT_TEST_STANDARD_DEPENDENCIES?
This PR adds
pytest-xdistto parallelize unit tests and thecore_deps_from_sourcenox sessions for thegoogle-cloud-spannerpackage.By running tests in parallel using
-n auto, the execution time of the Spanner unit tests are reduced from ~8 minutes to 4 minutes.State isolation and test reliability are achieved by:
subTest()instead of complex objects. This keeps subtests lightweight and prevents serialization errors.monkeypatchfixture). This ensures metric counters don't leak from one test into another.side_effectinstead ofreturn_value). This prevents one thread from exhausting a mock's results before another thread can read them._assert_concurrent_transaction_invariants) that verifies the behavior of concurrent threads (ensuring exactly one thread starts the transaction while others wait/reuse it), rather than checking fragile call logs or counting sequential request IDs. This allowed us to safely run four concurrent tests.Note
The long pole in the tent is still the
systemtests, which require about 30 minutes. It is not as simple as just adding xdist because there are other factors that limit velocity including the fact that system tests actually interact with live systems.