feat: Add system test for cross-region buckets#1760
Conversation
Adds a new system test, test_basic_wrd_x_region, to verify functionality with cross-region GCS buckets. Also updates the Cloud Build configuration to pass the necessary _CROSS_REGION_BUCKET environment variable to the test environment.
product-auto-label
Bot
added
size: l
|
/gcbrun(4f6bd14) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new system test for cross-region buckets, which is a valuable addition for verifying functionality across different bucket locations. The changes in the Cloud Build configuration and the new test in test_zonal.py are mostly correct. However, there is a critical issue with how the environment variable for the cross-region bucket is handled that will likely cause the new test to fail. Additionally, there is an opportunity to improve maintainability by refactoring the new test to reduce code duplication. Please see the detailed comments for suggestions.
| # TODO: replace this with a fixture once zonal bucket creation / deletion | ||
| # is supported in grpc client or json client client. | ||
| _ZONAL_BUCKET = os.getenv("ZONAL_BUCKET") | ||
| _CROSS_REGION_BUCKET = os.getenv("CROSS_REGION_BUCKET") |
There was a problem hiding this comment.
This will likely evaluate to None as the CROSS_REGION_BUCKET environment variable is not exported in the test execution environment. The cloudbuild.yaml configuration passes _CROSS_REGION_BUCKET, but the run_zonal_tests.sh script does not export it as CROSS_REGION_BUCKET for the pytest command (unlike ZONAL_BUCKET).
To fix this, you should add export CROSS_REGION_BUCKET=${_CROSS_REGION_BUCKET} to cloudbuild/run_zonal_tests.sh. This change is necessary for the new test to pass.
| def test_basic_wrd_x_region( | ||
| storage_client, | ||
| blobs_to_delete, | ||
| object_size, | ||
| event_loop, | ||
| grpc_client, | ||
| ): |
There was a problem hiding this comment.
This new test test_basic_wrd_x_region is nearly identical to the existing test_basic_wrd function. This significant code duplication can make the test suite harder to maintain.
To improve maintainability, consider refactoring this to avoid duplication. One approach would be to parameterize a single test function to handle both zonal and cross-region bucket cases, as well as the direct path option. For example, you could parameterize on a tuple containing (bucket_name, attempt_direct_path).
| event_loop, | ||
| grpc_client, | ||
| ): | ||
| object_name = f"test_basic_wrd-{str(uuid.uuid4())}" |
There was a problem hiding this comment.
For easier debugging and to avoid potential conflicts, it's good practice to make test object names specific to the test case. This helps identify which test created an object if cleanup fails.
| object_name = f"test_basic_wrd-{str(uuid.uuid4())}" | |
| object_name = f"test_basic_wrd_x_region-{str(uuid.uuid4())}" |
product-auto-label
Bot
added
size: s
|
/gcbrun(ad3387c) |
Renames open_retries to _open_retries in AsyncMultiRangeDownloader to encapsulate the retry counter. Updates test_zonal.py to verify the private attribute.
product-auto-label
Bot
added
size: m
feat: Add system test for cross-region buckets
- Adds a new system test, test_basic_wrd_x_region, to verify
functionality with cross-region GCS buckets.
- Also updates the Cloud Build configuration to pass the necessary
_CROSS_REGION_BUCKET environment variable to the test environment
This PR is a cherry-picked from
#1760 because that PR
is blocked by b/489420625
feat: Add system test for cross-region buckets
- Adds a new system test, test_basic_wrd_x_region, to verify
functionality with cross-region GCS buckets.
- Also updates the Cloud Build configuration to pass the necessary
_CROSS_REGION_BUCKET environment variable to the test environment
This PR is a cherry-picked from
googleapis/python-storage#1760 because that PR
is blocked by b/489420625
3 participants
feat: Add system test for cross-region buckets
Adds a new system test, test_basic_wrd_x_region, to verify functionality with cross-region GCS buckets.
Also updates the Cloud Build configuration to pass the necessary _CROSS_REGION_BUCKET environment variable to the test environment