impl: add integration tests for LRO Start and Await methods #14377
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14377 +/- ##
=======================================
Coverage 93.06% 93.06%
=======================================
Files 2191 2191
Lines 193203 193203
=======================================
+ Hits 179812 179814 +2
+ Misses 13391 13389 -2 ☔ View full report in Codecov by Sentry. |
| ASSERT_THAT(result, testing_util::IsOk()); | ||
| auto start_result = client.InsertDisk(ExperimentalTag{}, NoAwaitTag{}, | ||
| project_id_, zone_, disk); | ||
| ASSERT_THAT(start_result, testing_util::IsOk()); |
There was a problem hiding this comment.
nit (predates this PR): using ::google::cloud::testing_util::IsOk()
| std::string operation_string; | ||
| EXPECT_TRUE(start_result->SerializeToString(&operation_string)); | ||
|
|
||
| google::cloud::cpp::compute::v1::Operation operation; | ||
| EXPECT_TRUE(operation.ParseFromString(operation_string)); | ||
|
|
||
| auto await_result = client.InsertDisk(ExperimentalTag{}, operation).get(); |
There was a problem hiding this comment.
Why the string serializing/parsing?
auto operation = client.InsertDisk(ExperimentalTag{}, NoAwaitTag{},
project_id_, zone_, disk);
ASSERT_THAT(operation, IsOk());
auto result = client.InsertDisk(ExperimentalTag{}, *operation).get();| std::string instance_id = spanner_testing::RandomInstanceName(generator_); | ||
| Instance in(ProjectId(), instance_id); | ||
| ASSERT_FALSE(in.project_id().empty()); | ||
| ASSERT_FALSE(in.instance_id().empty()); |
There was a problem hiding this comment.
nit: remove?
I see that the above test does the same thing, but RandomInstanceName is never going to return an empty instance the way ProjectId() or InstanceId() might.
| auto instance_config = | ||
| client_.CreateInstanceConfig(ExperimentalTag{}, *operation).get(); | ||
| EXPECT_THAT(instance_config, StatusIs(StatusCode::kInvalidArgument)); |
There was a problem hiding this comment.
This confused me initially. Maybe a comment like:
// Verify that the client rejects LROs with the wrong metadata type.| EXPECT_THAT(instance->name(), Eq(in.FullName())); | ||
| EXPECT_EQ(instance->display_name(), "test-display-name"); |
There was a problem hiding this comment.
nit: local consistency of these?
This file seems to prefer EXPECT_EQ
| EXPECT_THAT(instance->name(), Eq(in.FullName())); | ||
| EXPECT_EQ(instance->display_name(), "test-display-name"); |
| std::string instance_id = spanner_testing::RandomInstanceName(generator_); | ||
| Instance in(ProjectId(), instance_id); | ||
| ASSERT_FALSE(in.project_id().empty()); | ||
| ASSERT_FALSE(in.instance_id().empty()); |
scotthart
force-pushed
the
generator_await_integeration_tests
branch
from
576ecbe to
d056181
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @dbolduc)
google/cloud/compute/integration_tests/compute_integration_test.cc line 94 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
nit (predates this PR):
using ::google::cloud::testing_util::IsOk()
Fixed throughout the file.
google/cloud/compute/integration_tests/compute_integration_test.cc line 102 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
Why the string serializing/parsing?
auto operation = client.InsertDisk(ExperimentalTag{}, NoAwaitTag{}, project_id_, zone_, disk); ASSERT_THAT(operation, IsOk()); auto result = client.InsertDisk(ExperimentalTag{}, *operation).get();
Added comment to explain.
google/cloud/spanner/admin/integration_tests/instance_admin_integration_test.cc line 227 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
nit: remove?
I see that the above test does the same thing, but
RandomInstanceNameis never going to return an empty instance the wayProjectId()orInstanceId()might.
Removed
google/cloud/spanner/admin/integration_tests/instance_admin_integration_test.cc line 246 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
This confused me initially. Maybe a comment like:
// Verify that the client rejects LROs with the wrong metadata type.
Comment added.
google/cloud/spanner/admin/integration_tests/instance_admin_integration_test.cc line 251 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
nit: local consistency of these?
This file seems to prefer
EXPECT_EQ
Fixed
google/cloud/spanner/admin/integration_tests/instance_admin_rest_integration_test.cc line 271 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
same nit about deleting.
Removed.
google/cloud/spanner/admin/integration_tests/instance_admin_rest_integration_test.cc line 295 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
same
Fixed
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @scotthart)
google/cloud/compute/integration_tests/compute_integration_test.cc line 102 at r1 (raw file):
Previously, scotthart (Scott Hart) wrote…
Added comment to explain.
Ah. It feels like we are testing protobuf, but that is fine.
part of the work for #7658
This change is