tests: System/compatibility tests for testing shim compatiblity #1218

gkevinzheng · 2025-10-14T18:50:03Z

Additional system tests for testing shim compatibility with the classic client interface.

Also fixes a bug where encountering a retriable error after another retriable error mid-stream raised an exception instead of retrying.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

daniel-sanche

A couple comments for now, but I'll look at the tests in more detail soon

daniel-sanche · 2025-10-16T19:20:43Z

.kokoro/presubmit/system-3.12.cfg

+env_vars: {
+    key: "NOX_SESSION"
+    value: "system-3.12"
+}


I pulled in main, so this shouldn't be necessary anymore

daniel-sanche · 2025-10-16T19:21:18Z

google/cloud/bigtable/row_data.py

        self._row_merger = _RowMerger(self._row_merger.last_seen_row_key)
-        self.response_iterator = self.read_method(retry_request)
+        self.response_iterator = self.read_method(retry_request, retry=self.retry)



why was this change needed?

This change was needed because the read_rows method that gets passed in gets called with the GAPIC default retry instead of the retry that the user passed in or DEFAULT_READ_RETRY for this instance. If there's an error with this specific instance of self.read_method it doesn't get retried and errors out.

For example, if a retriable InternalError occurred mid-stream, then in this on_error handler the same error occurred, the error in the on_error handler would not be retried because the specific retry was not passed in for that RPC call.

I'm a bit torn, because this does feel like a very real bug. But the code here is also very complex, so I'm hesitant to make changes to the retry policy in case there are side-effects we're not considering

But I guess this is coming in the context of adding more system tests. Do you think the tests we have now are sufficient to catch any errors?

Actually, on second thought, this is a PR into the v3_staging branch, not main. We will be doing much more extensive changes here as part of the shim, before cutting a release. So I think adding this fix makes sense

daniel-sanche · 2025-10-16T19:22:43Z

tests/system/v2_client/test_data_api.py

+class SelectiveMethodsErrorInjector(UnaryStreamClientInterceptor):
+    def __init__(self):
+        # As long as there are more items on this list, the items on the list
+        # are as follows:


I assume after the list is exhausted, it always behaves as normal?

Yes, it should. I'll put a comment for that.

daniel-sanche · 2025-10-16T19:25:10Z

tests/system/v2_client/test_data_api.py

+        data_table, rows_to_delete, row_keys, columns, CELL_VAL_READ_ROWS_RETRY
+    )
+
+    yield data_table


Isn't this re-populating the table before each test function? Is that necessary?

Let me refactor the read_rows system tests to reconfigure the error injector instead of re-populating the table before each test function.

I'm a bit confused by the organization here. Why are some tests in a class, and others stand-alone? Do we need a separate fixture to populate the table? Do we really want to re-populate the table more than once per run?

Maybe take a look at the metrics tests, and see if you can structure things that way. Instead of building a new data_table_read_rows_with_error_injector for each function, you can build it once and clear its state between functions

daniel-sanche · 2025-10-16T22:02:54Z

tests/system/v2_client/test_data_api.py



-def _populate_table(data_table, rows_to_delete, row_keys):
+def _add_test_error_handler(retry):


a docstring could be helpful here

It looks like it overrides the on_error function to assert that backoff values are within expected bounds?

daniel-sanche · 2025-10-23T00:18:26Z

google/cloud/bigtable/row_data.py

        self._row_merger = _RowMerger(self._row_merger.last_seen_row_key)
-        self.response_iterator = self.read_method(retry_request)
+        self.response_iterator = self.read_method(retry_request, retry=self.retry)



I'm a bit torn, because this does feel like a very real bug. But the code here is also very complex, so I'm hesitant to make changes to the retry policy in case there are side-effects we're not considering

But I guess this is coming in the context of adding more system tests. Do you think the tests we have now are sufficient to catch any errors?

daniel-sanche · 2025-10-23T00:19:29Z

google/cloud/bigtable/row_data.py

        self._row_merger = _RowMerger(self._row_merger.last_seen_row_key)
-        self.response_iterator = self.read_method(retry_request)
+        self.response_iterator = self.read_method(retry_request, retry=self.retry)



Actually, on second thought, this is a PR into the v3_staging branch, not main. We will be doing much more extensive changes here as part of the shim, before cutting a release. So I think adding this fix makes sense

daniel-sanche · 2025-10-23T00:27:23Z

tests/system/v2_client/test_data_api.py

+        data_table, rows_to_delete, row_keys, columns, CELL_VAL_READ_ROWS_RETRY
+    )
+
+    yield data_table


I'm a bit confused by the organization here. Why are some tests in a class, and others stand-alone? Do we need a separate fixture to populate the table? Do we really want to re-populate the table more than once per run?

daniel-sanche · 2025-10-23T00:31:29Z

tests/system/v2_client/test_data_api.py

+    for row in rows_to_delete:
+        row.clear()
+        row.delete()
+        row.commit()


You should use a try...finally block to make sure resources are fully deleted, anywhere you spin up new resources. (Especially for clusters and tables)

daniel-sanche · 2025-10-23T00:33:34Z

tests/system/v2_client/test_data_api.py

+        data_table, rows_to_delete, row_keys, columns, CELL_VAL_READ_ROWS_RETRY
+    )
+
+    yield data_table


Maybe take a look at the metrics tests, and see if you can structure things that way. Instead of building a new data_table_read_rows_with_error_injector for each function, you can build it once and clear its state between functions

tests: System/compatibility tests for testing shim compatiblity

807ae39

gkevinzheng requested review from a team as code owners October 14, 2025 18:50

product-auto-label bot added the size: l Pull request size is large. label Oct 14, 2025

blunderbuss-gcf bot assigned sushanb Oct 14, 2025

product-auto-label bot added the api: bigtable Issues related to the googleapis/python-bigtable API. label Oct 14, 2025

gkevinzheng requested a review from daniel-sanche October 14, 2025 18:50

gcf-owl-bot bot and others added 5 commits October 14, 2025 18:52

🦉 Updates from OwlBot post-processor

e86899a

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Use 3.12 instead of 3.8 for system tests

0426ce2

linting

cca6ccd

🦉 Updates from OwlBot post-processor

1bd0230

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Skipped sample_row_keys test on emulator

b3ce1cd

gkevinzheng changed the base branch from main to v3_staging October 15, 2025 14:40

daniel-sanche reviewed Oct 16, 2025

View reviewed changes

gkevinzheng and others added 7 commits October 20, 2025 12:05

Merge branch 'v3_staging' into system-tests-legacy

e06550e

Use 3.12 instead of 3.8 for system tests

4499e4b

Addressed review feedback

a895bc2

Update test_data_api.py

a16360c

Delete test.out

533dba1

Delete .kokoro/presubmit/system-3.12.cfg

68a91be

Delete bigtable_delete.sh

72cc5bb

daniel-sanche reviewed Oct 23, 2025

View reviewed changes

Addressed review feedback

0327d4f



		def _populate_table(data_table, rows_to_delete, row_keys):
		def _add_test_error_handler(retry):

Uh oh!

tests: System/compatibility tests for testing shim compatiblity #1218

Are you sure you want to change the base?

tests: System/compatibility tests for testing shim compatiblity #1218

Conversation

gkevinzheng commented Oct 14, 2025

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkevinzheng Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gkevinzheng Oct 20, 2025 •

edited

Loading