Skip to content
This repository was archived by the owner on Apr 1, 2026. It is now read-only.

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

Merged
gkevinzheng merged 14 commits into from
Nov 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
807ae39
tests: System/compatibility tests for testing shim compatiblity
gkevinzheng Sep 30, 2025
e86899a
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 14, 2025
0426ce2
Use 3.12 instead of 3.8 for system tests
gkevinzheng Oct 14, 2025
cca6ccd
linting
gkevinzheng Oct 14, 2025
1bd0230
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Oct 14, 2025
b3ce1cd
Skipped sample_row_keys test on emulator
gkevinzheng Oct 15, 2025
e06550e
Merge branch 'v3_staging' into system-tests-legacy
gkevinzheng Oct 20, 2025
4499e4b
Use 3.12 instead of 3.8 for system tests
gkevinzheng Oct 14, 2025
a895bc2
Addressed review feedback
gkevinzheng Oct 21, 2025
a16360c
Update test_data_api.py
gkevinzheng Oct 21, 2025
533dba1
Delete test.out
gkevinzheng Oct 21, 2025
68a91be
Delete .kokoro/presubmit/system-3.12.cfg
gkevinzheng Oct 21, 2025
72cc5bb
Delete bigtable_delete.sh
gkevinzheng Oct 21, 2025
0327d4f
Addressed review feedback
gkevinzheng Oct 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .kokoro/presubmit/system-3.12.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Format: //devtools/kokoro/config/proto/build.proto

# Only run this nox session.
env_vars: {
key: "NOX_SESSION"
value: "system-3.12"
}

@daniel-sanche daniel-sanche Oct 16, 2025

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.

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

2 changes: 1 addition & 1 deletion google/cloud/bigtable/row_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ def _on_error(self, exc):
retry_request = self._create_retry_request()

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)

@daniel-sanche daniel-sanche Oct 16, 2025

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.

why was this change needed?

@gkevinzheng gkevinzheng Oct 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@daniel-sanche daniel-sanche Oct 23, 2025

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.

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 daniel-sanche Oct 23, 2025

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.

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

def _read_next(self):
"""Helper for :meth:`__iter__`."""
Expand Down
3 changes: 1 addition & 2 deletions noxfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
UNIT_TEST_EXTRAS: List[str] = []
UNIT_TEST_EXTRAS_BY_PYTHON: Dict[str, List[str]] = {}

SYSTEM_TEST_PYTHON_VERSIONS: List[str] = ["3.8", "3.12"]
SYSTEM_TEST_PYTHON_VERSIONS: List[str] = ["3.12"]
SYSTEM_TEST_STANDARD_DEPENDENCIES: List[str] = [
"mock",
"pytest",
Expand Down Expand Up @@ -206,7 +206,6 @@ def install_unittest_dependencies(session, *constraints):
)
def unit(session, protobuf_implementation):
# Install all test dependencies, then install this package in-place.

if protobuf_implementation == "cpp" and session.python in ("3.11", "3.12", "3.13"):
session.skip("cpp implementation is not supported in python 3.11+")

Expand Down
Loading
Loading