Skip to content

test(storage): add system tests for Bidi Read/Write (gRPC) - #18185

Draft
nidhiii-27 wants to merge 4 commits into
mainfrom
feat-bidi-read-write-system-tests
Draft

test(storage): add system tests for Bidi Read/Write (gRPC)#18185
nidhiii-27 wants to merge 4 commits into
mainfrom
feat-bidi-read-write-system-tests

Conversation

@nidhiii-27

Copy link
Copy Markdown
Contributor

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]

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]

@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 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.

Comment on lines +383 to +387
with pytest.raises(Exception) as excinfo:
await task

# The exception could be ServiceUnavailable or CancelledError
assert isinstance(excinfo.value, (exceptions.ServiceUnavailable, asyncio.CancelledError))

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

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.

Suggested change
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,

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

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.

Suggested change
# 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,

Comment on lines +481 to +482
# Verify valid one processed correctly
assert valid_buffer.getvalue() == data[:100]

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

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.

Suggested change
# 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant