test(storage): add GCS RCU & Bidi Read/Write integration tests - #16365
test(storage): add GCS RCU & Bidi Read/Write integration tests#16365nidhiii-27 wants to merge 1 commit into
Conversation
[Generated-by: AI]
product-auto-label
Bot
added
the
api: storage
![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 introduces new integration tests for RCU bidirectional read and write operations (rcu_bidi_read_integration_test.cc and rcu_bidi_write_integration_test.cc) and registers them in the build configuration files. The reviewer feedback highlights a critical issue in the read integration test where a 30-minute sleep is used to wait for background uptiering, which would cause CI timeouts; it is recommended to pre-create these objects out-of-band instead. Additionally, several style guide violations regarding the use of auto were identified, specifically where auto obscures StatusOr, domain objects, and protobuf messages, is used for primitives in loops, or lacks explicit qualifiers.
| std::cout | ||
| << "Sleeping for 30 minutes to allow background uptiering to RZ..." | ||
| << std::endl; | ||
| std::this_thread::sleep_for(std::chrono::minutes(30)); | ||
| std::cout << "Wake up! Continuing with integration test execution." | ||
| << std::endl; |
There was a problem hiding this comment.
Sleeping for 30 minutes in an integration test is a major bottleneck and will likely cause CI timeouts. Since the bucket and object names are static, these objects should be pre-created and uptiered out-of-band. The test should assume they already exist and skip the creation and sleep steps.
| auto client = MakeIntegrationTestClient(options_); | ||
|
|
||
| if (params.location_type == LocationType::kRegionalRapid) { | ||
| auto metadata = client.GetBucketMetadata(bucket_name_); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used when it hides StatusOr<T> or domain objects. Please use the explicit type StatusOr<BucketMetadata> instead.
StatusOr<BucketMetadata> metadata = client.GetBucketMetadata(bucket_name_);References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
| } | ||
|
|
||
| void TriggerIngestOnRead(AsyncClient& async, std::string const& object_name) { | ||
| auto r = async.ReadObject(BucketName(bucket_name_), object_name).get(); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used when it hides StatusOr<T> or domain objects. Please use the explicit type StatusOr<std::pair<AsyncReader, AsyncToken>> instead.
StatusOr<std::pair<AsyncReader, AsyncToken>> r = async.ReadObject(BucketName(bucket_name_), object_name).get();References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
| auto views1 = res1->first.contents(); | ||
| ASSERT_FALSE(views1.empty()); | ||
| std::string content1; | ||
| for (auto v : views1) content1 += std::string(v); |
There was a problem hiding this comment.
According to the repository style guide, auto must be explicitly qualified with const, reference (&), or pointer (*) to prevent unintended copies or ambiguous mutability. Since views1 contains std::string_view, we can use std::string_view explicitly.
for (std::string_view v : views1) content1 += std::string(v);References
- Enforce Explicit Qualifiers: Ensure
autois explicitly qualified withconst, reference (&), or pointer (*) (e.g.,auto const&,auto&,auto*) to prevent unintended copies or ambiguous mutability. (link)
| if (params.location_type == LocationType::kRegionalRapid || | ||
| params.location_type == LocationType::kZonalRapid) { | ||
| google::storage::v2::BidiWriteObjectRequest request; | ||
| auto* spec = request.mutable_write_object_spec(); |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used when it hides protobuf messages/fields. Please use the explicit type google::storage::v2::WriteObjectSpec* instead.
google::storage::v2::WriteObjectSpec* spec = request.mutable_write_object_spec();References
- Reject Obscured Domain & Return Types: Flag and reject
autowhen it hidesStatusOr<T>, domain objects, protobuf messages/fields, or function return types. (link)
| for (auto loc : locations) { | ||
| for (auto size : sizes) { | ||
| for (auto close : close_actions) { | ||
| for (auto flush : flush_thresholds) { |
There was a problem hiding this comment.
According to the repository style guide, auto should not be used for primitives or simple loop variables where explicit types are clearer and prevent unintended conversions. Please use explicit types for the loop variables.
for (LocationType loc : locations) {
for (std::size_t size : sizes) {
for (bool close : close_actions) {
for (std::size_t flush : flush_thresholds) {References
- Disallow
autofor Primitives: Require explicit numeric and scalar types (std::size_t,std::int64_t,bool, etc.) rather than deducing them from literals. (link)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16365 +/- ##
==========================================
- Coverage 92.26% 92.25% -0.02%
==========================================
Files 2237 2237
Lines 210434 210434
==========================================
- Hits 194166 194144 -22
- Misses 16268 16290 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
PR generated by AI to add GCS RCU and Bidirectional Read/Write integration tests.