Skip to content

Commit

Permalink
cleanup(storage): simplify integration test init (#14484)
Browse files Browse the repository at this point in the history 
This is the first step to clean up the integration test initialization.
It removes unused code in the common initialization function, avoids
using `StatusOr<>` when it makes no sense, and prefers using `Options{}`
instead of multiple overloads.

I stopped here because `MakeBucketIntegrationTestClient()` is not used
in as many places and I think it will make this change easier to grok.
  • Loading branch information
@coryan
coryan authored Jul 18, 2024
1 parent 22f92ce commit c1f97cd
Show file tree
Hide file tree
Showing 10 changed files with 99 additions and 142 deletions.
75 changes: 26 additions & 49 deletions google/cloud/storage/testing/storage_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,24 +53,36 @@ StorageIntegrationTest::~StorageIntegrationTest() {
// The client configured to create and delete buckets is good for our
// purposes.
auto client = MakeBucketIntegrationTestClient();
if (!client) return;
for (auto& o : objects_to_delete_) {
(void)client->DeleteObject(o.bucket(), o.name(),
Generation(o.generation()));
(void)client.DeleteObject(o.bucket(), o.name(), Generation(o.generation()));
}
for (auto& b : buckets_to_delete_) {
(void)RemoveBucketAndContents(*client, b);
(void)RemoveBucketAndContents(client, b);
}
}

google::cloud::storage::Client
StorageIntegrationTest::MakeIntegrationTestClient(google::cloud::Options opts) {
opts = google::cloud::internal::MergeOptions(
std::move(opts), Options{}
.set<RetryPolicyOption>(TestRetryPolicy())
.set<BackoffPolicyOption>(TestBackoffPolicy()));
#if GOOGLE_CLOUD_CPP_STORAGE_HAVE_GRPC
if (UseGrpcForMedia() || UseGrpcForMetadata()) {
return storage_experimental::DefaultGrpcClient(std::move(opts));
}
#endif // GOOGLE_CLOUD_CPP_STORAGE_HAVE_GRPC
return Client(std::move(opts));
}

google::cloud::StatusOr<google::cloud::storage::Client>
StorageIntegrationTest::MakeIntegrationTestClient() {
return MakeIntegrationTestClient(TestRetryPolicy());
return MakeIntegrationTestClient(Options{});
}

google::cloud::StatusOr<google::cloud::storage::Client>
google::cloud::storage::Client
StorageIntegrationTest::MakeBucketIntegrationTestClient() {
if (UsingEmulator()) return MakeIntegrationTestClient();
if (UsingEmulator()) return MakeIntegrationTestClient(Options{});

auto constexpr kInitialDelay = std::chrono::seconds(5);
auto constexpr kMaximumBackoffDelay = std::chrono::minutes(5);
Expand All @@ -79,48 +91,13 @@ StorageIntegrationTest::MakeBucketIntegrationTestClient() {
// little sense to wait any longer.
auto constexpr kMaximumRetryTime = std::chrono::minutes(10);
return MakeIntegrationTestClient(
LimitedTimeRetryPolicy(kMaximumRetryTime).clone(),
ExponentialBackoffPolicy(kInitialDelay, kMaximumBackoffDelay,
kBackoffScalingFactor)
.clone());
}

google::cloud::StatusOr<google::cloud::storage::Client>
StorageIntegrationTest::MakeIntegrationTestClient(
std::unique_ptr<RetryPolicy> retry_policy) {
return MakeIntegrationTestClient(std::move(retry_policy),
TestBackoffPolicy());
}

google::cloud::StatusOr<google::cloud::storage::Client>
StorageIntegrationTest::MakeIntegrationTestClient(
std::unique_ptr<RetryPolicy> retry_policy,
std::unique_ptr<BackoffPolicy> backoff_policy) {
auto client_options = ClientOptions::CreateDefaultClientOptions();
if (!client_options) return std::move(client_options).status();

auto opts = internal::MakeOptions(*std::move(client_options))
.set<RetryPolicyOption>(std::move(retry_policy))
.set<BackoffPolicyOption>(std::move(backoff_policy));

#if GOOGLE_CLOUD_CPP_STORAGE_HAVE_GRPC
if (UseGrpcForMedia() || UseGrpcForMetadata()) {
return storage_experimental::DefaultGrpcClient(std::move(opts));
}
#endif // GOOGLE_CLOUD_CPP_STORAGE_HAVE_GRPC

auto const idempotency =
google::cloud::internal::GetEnv("CLOUD_STORAGE_IDEMPOTENCY")
.value_or("always-retry");
if (idempotency == "strict") {
opts.set<IdempotencyPolicyOption>(StrictIdempotencyPolicy{}.clone());
} else if (idempotency != "always-retry") {
return Status(
StatusCode::kInvalidArgument,
"Invalid value for CLOUD_STORAGE_IDEMPOTENCY environment variable");
}
auto credentials = opts.get<Oauth2CredentialsOption>();
return Client(std::move(opts));
Options{}
.set<RetryPolicyOption>(
LimitedTimeRetryPolicy(kMaximumRetryTime).clone())
.set<BackoffPolicyOption>(
ExponentialBackoffPolicy(kInitialDelay, kMaximumBackoffDelay,
kBackoffScalingFactor)
.clone()));
}

std::unique_ptr<BackoffPolicy> StorageIntegrationTest::TestBackoffPolicy() {
Expand Down
15 changes: 4 additions & 11 deletions google/cloud/storage/testing/storage_integration_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class StorageIntegrationTest
static google::cloud::StatusOr<google::cloud::storage::Client>
MakeIntegrationTestClient();

static google::cloud::storage::Client MakeIntegrationTestClient(
google::cloud::Options opts);

/**
* Return a client with retry policies suitable for CreateBucket() class.
*
Expand All @@ -57,17 +60,7 @@ class StorageIntegrationTest
* one bucket every two seconds, suggesting that the default backoff should be
* at least that long.
*/
static google::cloud::StatusOr<google::cloud::storage::Client>
MakeBucketIntegrationTestClient();

/// Like MakeIntegrationTestClient() but with a custom retry policy
static google::cloud::StatusOr<google::cloud::storage::Client>
MakeIntegrationTestClient(std::unique_ptr<RetryPolicy> retry_policy);

/// Like MakeIntegrationTestClient() but with custom retry and bucket policies
static google::cloud::StatusOr<google::cloud::storage::Client>
MakeIntegrationTestClient(std::unique_ptr<RetryPolicy> retry_policy,
std::unique_ptr<BackoffPolicy> backoff_policy);
static google::cloud::storage::Client MakeBucketIntegrationTestClient();

static std::unique_ptr<BackoffPolicy> TestBackoffPolicy();
static std::unique_ptr<RetryPolicy> TestRetryPolicy();
Expand Down
45 changes: 22 additions & 23 deletions google/cloud/storage/tests/grpc_bucket_acl_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,10 @@ TEST_F(GrpcBucketAclIntegrationTest, AclCRUD) {

std::string bucket_name = MakeRandomBucketName();
auto client = MakeBucketIntegrationTestClient();
ASSERT_STATUS_OK(client);

// Create a new bucket to run the test, with the "private" PredefinedAcl so
// we know what the contents of the ACL will be.
auto metadata = client->CreateBucketForProject(
auto metadata = client.CreateBucketForProject(
bucket_name, project_id, BucketMetadata(), PredefinedAcl("private"),
Projection("full"));
ASSERT_STATUS_OK(metadata);
Expand All @@ -68,78 +67,78 @@ TEST_F(GrpcBucketAclIntegrationTest, AclCRUD) {
<< " created with a predefine ACL which should preclude this result.";

auto const existing_entity = metadata->acl().front();
auto current_acl = client->ListBucketAcl(bucket_name);
auto current_acl = client.ListBucketAcl(bucket_name);
ASSERT_STATUS_OK(current_acl);
EXPECT_THAT(AclEntityNames(*current_acl),
Contains(existing_entity.entity()).Times(1));

auto get_acl = client->GetBucketAcl(bucket_name, existing_entity.entity());
auto get_acl = client.GetBucketAcl(bucket_name, existing_entity.entity());
ASSERT_STATUS_OK(get_acl);
EXPECT_EQ(*get_acl, existing_entity);

auto not_found_acl = client->GetBucketAcl(bucket_name, "not-found-entity");
auto not_found_acl = client.GetBucketAcl(bucket_name, "not-found-entity");
EXPECT_THAT(not_found_acl, StatusIs(StatusCode::kNotFound));

auto create_acl = client->CreateBucketAcl(bucket_name, viewers,
BucketAccessControl::ROLE_READER());
auto create_acl = client.CreateBucketAcl(bucket_name, viewers,
BucketAccessControl::ROLE_READER());
ASSERT_STATUS_OK(create_acl);

current_acl = client->ListBucketAcl(bucket_name);
current_acl = client.ListBucketAcl(bucket_name);
ASSERT_STATUS_OK(current_acl);
EXPECT_THAT(AclEntityNames(*current_acl),
Contains(create_acl->entity()).Times(1));

auto c2 = client->CreateBucketAcl(bucket_name, viewers,
BucketAccessControl::ROLE_READER());
auto c2 = client.CreateBucketAcl(bucket_name, viewers,
BucketAccessControl::ROLE_READER());
ASSERT_STATUS_OK(c2);
// There is no guarantee that the ETag remains unchanged, even if the
// operation has no effect. Reset the one field that might change.
create_acl->set_etag(c2->etag());
EXPECT_EQ(*create_acl, *c2);

auto updated_acl = client->UpdateBucketAcl(
auto updated_acl = client.UpdateBucketAcl(
bucket_name, BucketAccessControl().set_entity(viewers).set_role(
BucketAccessControl::ROLE_OWNER()));
ASSERT_STATUS_OK(updated_acl);
EXPECT_EQ(updated_acl->entity(), create_acl->entity());
EXPECT_EQ(updated_acl->role(), BucketAccessControl::ROLE_OWNER());

// "Updating" an entity that does not exist should create the entity
auto delete_acl = client->DeleteBucketAcl(bucket_name, viewers);
auto delete_acl = client.DeleteBucketAcl(bucket_name, viewers);
ASSERT_STATUS_OK(delete_acl);
updated_acl = client->UpdateBucketAcl(
updated_acl = client.UpdateBucketAcl(
bucket_name, BucketAccessControl().set_entity(viewers).set_role(
BucketAccessControl::ROLE_OWNER()));
ASSERT_STATUS_OK(updated_acl);

auto patched_acl =
client->PatchBucketAcl(bucket_name, viewers,
BucketAccessControlPatchBuilder().set_role(
BucketAccessControl::ROLE_READER()));
client.PatchBucketAcl(bucket_name, viewers,
BucketAccessControlPatchBuilder().set_role(
BucketAccessControl::ROLE_READER()));
ASSERT_STATUS_OK(patched_acl);
EXPECT_EQ(patched_acl->entity(), create_acl->entity());
EXPECT_EQ(patched_acl->role(), BucketAccessControl::ROLE_READER());

// "Patching" an entity that does not exist should create the entity
delete_acl = client->DeleteBucketAcl(bucket_name, viewers);
delete_acl = client.DeleteBucketAcl(bucket_name, viewers);
ASSERT_STATUS_OK(delete_acl);
patched_acl =
client->PatchBucketAcl(bucket_name, viewers,
BucketAccessControlPatchBuilder().set_role(
BucketAccessControl::ROLE_READER()));
client.PatchBucketAcl(bucket_name, viewers,
BucketAccessControlPatchBuilder().set_role(
BucketAccessControl::ROLE_READER()));
ASSERT_STATUS_OK(patched_acl);
EXPECT_EQ(patched_acl->entity(), create_acl->entity());
EXPECT_EQ(patched_acl->role(), BucketAccessControl::ROLE_READER());

delete_acl = client->DeleteBucketAcl(bucket_name, viewers);
delete_acl = client.DeleteBucketAcl(bucket_name, viewers);
ASSERT_STATUS_OK(delete_acl);

current_acl = client->ListBucketAcl(bucket_name);
current_acl = client.ListBucketAcl(bucket_name);
ASSERT_STATUS_OK(current_acl);
EXPECT_THAT(AclEntityNames(*current_acl),
Not(Contains(create_acl->entity())));

auto status = client->DeleteBucket(bucket_name);
auto status = client.DeleteBucket(bucket_name);
ASSERT_STATUS_OK(status);
}

Expand Down
43 changes: 21 additions & 22 deletions google/cloud/storage/tests/grpc_default_object_acl_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,11 @@ TEST_F(GrpcDefaultObjectAclIntegrationTest, AclCRUD) {

std::string bucket_name = MakeRandomBucketName();
auto client = MakeBucketIntegrationTestClient();
ASSERT_STATUS_OK(client);

// Create a new bucket to run the test, with the "private"
// PredefinedDefaultObjectAcl, so we know what the contents of the ACL will
// be.
auto metadata = client->CreateBucketForProject(
auto metadata = client.CreateBucketForProject(
bucket_name, project_id, BucketMetadata(),
PredefinedDefaultObjectAcl("authenticatedRead"), Projection("full"));
ASSERT_STATUS_OK(metadata);
Expand All @@ -70,80 +69,80 @@ TEST_F(GrpcDefaultObjectAclIntegrationTest, AclCRUD) {
<< " this result.";

auto const existing_entity = metadata->default_acl().front();
auto current_acl = client->ListDefaultObjectAcl(bucket_name);
auto current_acl = client.ListDefaultObjectAcl(bucket_name);
ASSERT_STATUS_OK(current_acl);
EXPECT_THAT(AclEntityNames(*current_acl),
Contains(existing_entity.entity()).Times(1));

auto get_acl =
client->GetDefaultObjectAcl(bucket_name, existing_entity.entity());
client.GetDefaultObjectAcl(bucket_name, existing_entity.entity());
ASSERT_STATUS_OK(get_acl);
EXPECT_EQ(*get_acl, existing_entity);

auto not_found_acl =
client->GetDefaultObjectAcl(bucket_name, "not-found-entity");
client.GetDefaultObjectAcl(bucket_name, "not-found-entity");
EXPECT_THAT(not_found_acl, StatusIs(StatusCode::kNotFound));

auto create_acl = client->CreateDefaultObjectAcl(
auto create_acl = client.CreateDefaultObjectAcl(
bucket_name, viewers, ObjectAccessControl::ROLE_READER());
ASSERT_STATUS_OK(create_acl);

current_acl = client->ListDefaultObjectAcl(bucket_name);
current_acl = client.ListDefaultObjectAcl(bucket_name);
ASSERT_STATUS_OK(current_acl);
EXPECT_THAT(AclEntityNames(*current_acl),
Contains(create_acl->entity()).Times(1));

auto c2 = client->CreateDefaultObjectAcl(bucket_name, viewers,
ObjectAccessControl::ROLE_READER());
auto c2 = client.CreateDefaultObjectAcl(bucket_name, viewers,
ObjectAccessControl::ROLE_READER());
ASSERT_STATUS_OK(c2);
// There is no guarantee that the ETag remains unchanged, even if the
// operation has no effect. Reset the one field that might change.
create_acl->set_etag(c2->etag());
EXPECT_EQ(*create_acl, *c2);

auto updated_acl = client->UpdateDefaultObjectAcl(
auto updated_acl = client.UpdateDefaultObjectAcl(
bucket_name, ObjectAccessControl().set_entity(viewers).set_role(
ObjectAccessControl::ROLE_OWNER()));
ASSERT_STATUS_OK(updated_acl);
EXPECT_EQ(updated_acl->entity(), create_acl->entity());
EXPECT_EQ(updated_acl->role(), ObjectAccessControl::ROLE_OWNER());

// "Updating" an entity that does not exist should create the entity
auto delete_acl = client->DeleteDefaultObjectAcl(bucket_name, viewers);
auto delete_acl = client.DeleteDefaultObjectAcl(bucket_name, viewers);
ASSERT_STATUS_OK(delete_acl);
updated_acl = client->UpdateDefaultObjectAcl(
updated_acl = client.UpdateDefaultObjectAcl(
bucket_name, ObjectAccessControl().set_entity(viewers).set_role(
ObjectAccessControl::ROLE_OWNER()));
ASSERT_STATUS_OK(updated_acl);

auto patched_acl =
client->PatchDefaultObjectAcl(bucket_name, viewers,
ObjectAccessControlPatchBuilder().set_role(
ObjectAccessControl::ROLE_READER()));
client.PatchDefaultObjectAcl(bucket_name, viewers,
ObjectAccessControlPatchBuilder().set_role(
ObjectAccessControl::ROLE_READER()));
ASSERT_STATUS_OK(patched_acl);
EXPECT_EQ(patched_acl->entity(), create_acl->entity());
EXPECT_EQ(patched_acl->role(), ObjectAccessControl::ROLE_READER());

// "Patching" an entity that does not exist should create the entity
delete_acl = client->DeleteDefaultObjectAcl(bucket_name, viewers);
delete_acl = client.DeleteDefaultObjectAcl(bucket_name, viewers);
ASSERT_STATUS_OK(delete_acl);
patched_acl =
client->PatchDefaultObjectAcl(bucket_name, viewers,
ObjectAccessControlPatchBuilder().set_role(
ObjectAccessControl::ROLE_READER()));
client.PatchDefaultObjectAcl(bucket_name, viewers,
ObjectAccessControlPatchBuilder().set_role(
ObjectAccessControl::ROLE_READER()));
ASSERT_STATUS_OK(patched_acl);
EXPECT_EQ(patched_acl->entity(), create_acl->entity());
EXPECT_EQ(patched_acl->role(), ObjectAccessControl::ROLE_READER());

delete_acl = client->DeleteDefaultObjectAcl(bucket_name, viewers);
delete_acl = client.DeleteDefaultObjectAcl(bucket_name, viewers);
ASSERT_STATUS_OK(delete_acl);

current_acl = client->ListDefaultObjectAcl(bucket_name);
current_acl = client.ListDefaultObjectAcl(bucket_name);
ASSERT_STATUS_OK(current_acl);
EXPECT_THAT(AclEntityNames(*current_acl),
Not(Contains(create_acl->entity())));

auto status = client->DeleteBucket(bucket_name);
auto status = client.DeleteBucket(bucket_name);
ASSERT_STATUS_OK(status);
}

Expand Down
Loading

0 comments on commit c1f97cd

Please sign in to comment.