Skip to content

tests(bigtable): System/compatibility tests for testing shim compatiblity#17901

Open
daniel-sanche wants to merge 1 commit into
mainfrom
shim/01-compat-tests
Open

tests(bigtable): System/compatibility tests for testing shim compatiblity#17901
daniel-sanche wants to merge 1 commit into
mainfrom
shim/01-compat-tests

Conversation

@daniel-sanche

Copy link
Copy Markdown
Contributor

Migrating over @gkevinzheng PR from bigtable monorepo googleapis/python-bigtable#1218

Original description:

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.

Fixes googleapis/python-bigtable#1156

Note to reviewers: This PR has already been reviewed and merged to a staging branch, with the intention of doing a single merge to main. We are now planning to slowly rollout these changes back to the main branch. Minimal re-review should be necessary

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.

Fixes #1156

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
@daniel-sanche
daniel-sanche requested a review from mutianf July 24, 2026 23:19
@daniel-sanche
daniel-sanche requested a review from a team as a code owner July 24, 2026 23:19

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Bigtable row data handler to propagate the retry configuration when re-establishing a read stream on error. It also introduces comprehensive system tests to validate retry behavior, timeouts, and input error handling. The review feedback correctly identifies a missing COL_NAME2 variable definition in the test file that would lead to a NameError, and offers a memory-efficient optimization using itertools to avoid allocating a massive list for mock side effects.

Comment on lines +41 to +43
CELL_VAL_TRUE = b"true"
CELL_VAL_FALSE = b"false"
INT_COL_NAME = 67890

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The variable COL_NAME2 is used in test_table_append_row_input_errors (on lines 1194 and 1201) but is not defined anywhere in the module. This will cause a NameError when the tests are executed. Defining COL_NAME2 at the module level resolves this issue.

Suggested change
CELL_VAL_TRUE = b"true"
CELL_VAL_FALSE = b"false"
INT_COL_NAME = 67890
CELL_VAL_TRUE = b"true"
CELL_VAL_FALSE = b"false"
COL_NAME2 = b"col-name2"
INT_COL_NAME = 67890

Comment on lines +347 to +349
mutate_mock.side_effect = [initial_error_response] + [
followup_error_response
] * 1000000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating a list of 1,000,000 elements using [followup_error_response] * 1000000 is memory-inefficient and unnecessary. Using itertools.chain and itertools.repeat achieves the same result with O(1) memory complexity. Ensure itertools is imported at the module level rather than inside the function.

Suggested change
mutate_mock.side_effect = [initial_error_response] + [
followup_error_response
] * 1000000
mutate_mock.side_effect = itertools.chain(
[initial_error_response], itertools.repeat(followup_error_response)
)
References
  1. Do not import modules or classes inside functions if they are already imported at the module level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add new system tests

2 participants