test(storage): add system tests for Bidi Read/Write (gRPC) - #18185
test(storage): add system tests for Bidi Read/Write (gRPC)#18185nidhiii-27 wants to merge 4 commits into
Conversation
Implement parameterized integration tests for Bidirectional Read and Write features, porting test specifications from the Java reference PR. Tests cover standard, RCU, and zonal buckets, and include checksum validation and stream closure scenarios. [Generated-by: AI]
There was a problem hiding this comment.
Code Review
This pull request introduces a new system test suite (test_bidi.py) to verify bidirectional streaming read and write operations using AsyncAppendableObjectWriter and AsyncMultiRangeDownloader. The feedback suggests several improvements to the test suite: explicitly catching asyncio.CancelledError (which inherits from BaseException rather than Exception), implementing a pytest fixture to handle temporary object cleanup robustly on test failures, and checking for exceptions returned by asyncio.gather to prevent masking failures during assertions.
| with pytest.raises(Exception) as excinfo: | ||
| await task | ||
|
|
||
| # The exception could be ServiceUnavailable or CancelledError | ||
| assert isinstance(excinfo.value, (exceptions.ServiceUnavailable, asyncio.CancelledError)) |
There was a problem hiding this comment.
In Python 3.8+, asyncio.CancelledError inherits from BaseException rather than Exception. As a result, pytest.raises(Exception) will not catch asyncio.CancelledError, which can cause the test to fail with an uncaught exception if the task is cancelled. To ensure both ServiceUnavailable and CancelledError are correctly caught, specify them explicitly in pytest.raises.
| with pytest.raises(Exception) as excinfo: | |
| await task | |
| # The exception could be ServiceUnavailable or CancelledError | |
| assert isinstance(excinfo.value, (exceptions.ServiceUnavailable, asyncio.CancelledError)) | |
| with pytest.raises((exceptions.ServiceUnavailable, asyncio.CancelledError)) as excinfo: | |
| await task |
| else: | ||
| pytest.fail(f"Unsupported location type: {bidi_location_type}") | ||
|
|
||
| # Helper to create objects using sync client if needed, |
There was a problem hiding this comment.
To prevent leaving orphaned objects in the preprod/prod bucket when assertions fail, consider using a pytest fixture to track and automatically clean up created objects. This ensures cleanup runs even if a test fails mid-execution.
| # Helper to create objects using sync client if needed, | |
| @pytest.fixture | |
| async def temp_objects(grpc_client, bidi_bucket): | |
| created_objects = [] | |
| yield created_objects | |
| for object_name in created_objects: | |
| try: | |
| await grpc_client.delete_object(bidi_bucket, object_name) | |
| except Exception: | |
| pass | |
| # Helper to create objects using sync client if needed, |
| # Verify valid one processed correctly | ||
| assert valid_buffer.getvalue() == data[:100] |
There was a problem hiding this comment.
When using asyncio.gather with return_exceptions=True, any exception raised by valid_task will be returned in results[0] instead of being raised. If valid_task fails, the assertion valid_buffer.getvalue() == data[:100] will fail, masking the actual exception and making debugging difficult. Checking and raising the exception from results[0] first ensures the real failure is reported with its stack trace.
| # Verify valid one processed correctly | |
| assert valid_buffer.getvalue() == data[:100] | |
| # Verify valid one processed correctly | |
| if isinstance(results[0], Exception): | |
| raise results[0] | |
| assert valid_buffer.getvalue() == data[:100] |
Split Bidi Read/Write tests into separate files: test_bidi_read.py and test_bidi_write.py. In test_bidi_read.py, add RCU ingest-on-read logic with 30 minutes sleep. In test_bidi_write.py, parameterize appendable upload with flush intervals, close actions, and sizes. [Generated-by: AI]
…raints - Monkey patch blob_to_proto to copy storage_class to proto. - Use from_blob and set RAPID storage class in tests. - Wrap all async tests in asyncio.wait_for with 60s timeout to prevent hangs. - Remove constraints from nox system test session to fix pip backtracking. [Generated-by: AI]
Pass None as client to Bucket constructors since it is required and we only use the instances for metadata conversion. [Generated-by: AI]
Implement parameterized integration tests for Bidirectional Read and Write
features, porting test specifications from the Java reference PR.
Tests cover standard, RCU, and zonal buckets, and include checksum
validation and stream closure scenarios.
[Generated-by: AI]