Skip to content

test(storage): add CRD tests #1952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 29, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 56 additions & 1 deletion src/integration-tests/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
use crate::Error;
use crate::Result;
use gax::paginator::{ItemPaginator, Paginator};
use rand::Rng;
use storage::model::Bucket;

pub const BUCKET_ID_LENGTH: usize = 32;
pub const BUCKET_ID_LENGTH: usize = 63;

pub async fn buckets(builder: storage::client::ClientBuilder) -> Result<()> {
// Enable a basic subscriber. Useful to troubleshoot problems and visually
Expand All @@ -38,6 +40,48 @@ pub async fn buckets(builder: storage::client::ClientBuilder) -> Result<()> {

cleanup_stale_buckets(&client, &project_id).await?;

let bucket_id = random_bucket_id();
let bucket_name = format!("projects/_/buckets/{bucket_id}");

println!("\nTesting create_bucket()");
let create = client
.create_bucket("projects/_", bucket_id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: As you may recall, the rate limit for this call is 0.5Hz. We should to adjust the backoff to start a 2s, at least for this call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rate limit for this call is 0.5Hz.

I did read that. I saw we ran CRUD tests for buckets in Cloud C++ so figured it couldn't be that bad to run them here.

adjust the backoff to start a 2s

Good idea. Done.

.set_bucket(
Bucket::new()
.set_project(format!("projects/{project_id}"))
.set_labels([("integration-test", "true")]),
)
.send()
.await?;
println!("SUCCESS on create_bucket: {create:?}");
assert_eq!(create.name, bucket_name);

println!("\nTesting get_bucket()");
let get = client.get_bucket(&bucket_name).send().await?;
println!("SUCCESS on get_bucket: {get:?}");
assert_eq!(get.name, bucket_name);

println!("\nTesting list_buckets()");
let mut paginator = client
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I have taken to calling these "items"... they certainly are not a paginator after the items() call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, I am going to call them bucket(s). Seems more specific.

.list_buckets(format!("projects/{project_id}"))
.paginator()
.await
.items();
let mut bucket_names = Vec::new();
while let Some(bucket) = paginator.next().await {
bucket_names.push(bucket?.name);
}
println!("SUCCESS on list_buckets");
assert!(
bucket_names.iter().any(|name| name == &bucket_name),
"missing bucket name {} in {bucket_names:?}",
&bucket_name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
"missing bucket name {} in {bucket_names:?}",
&bucket_name
"missing bucket name {bucket_name} in {bucket_names:?}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

);

println!("\nTesting delete_bucket()");
client.delete_bucket(bucket_name).send().await?;
println!("SUCCESS on delete_bucket");

Ok(())
}

Expand Down Expand Up @@ -102,3 +146,14 @@ async fn cleanup_bucket(client: storage::client::Storage, name: String) -> Resul
let _ = futures::future::join_all(pending).await;
client.delete_bucket(&name).send().await
}

pub(crate) fn random_bucket_id() -> String {
const CHARSET: &[u8] = b"abcdefghijklmnopqrstuvwxyz0123456789";
const PREFIX: &str = "rust-sdk-testing-";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the integration-tests label we do not need a long prefix like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the prefix though. It makes it more obvious what these things are if they leak (especially in a testing project). I don't mind sacrificing some bits of randomness.

let mut bucket_id = String::new();
for _ in 0..(BUCKET_ID_LENGTH - PREFIX.len()) {
let idx = rand::rng().random_range(0..CHARSET.len());
bucket_id.push(CHARSET[idx] as char);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

Suggested change
let mut bucket_id = String::new();
for _ in 0..(BUCKET_ID_LENGTH - PREFIX.len()) {
let idx = rand::rng().random_range(0..CHARSET.len());
bucket_id.push(CHARSET[idx] as char);
}
let id = std::iter::from_fn(|| move { rand::rng().random_range(0..CHARSET.len()) })
.take(BUCKET_ID_LENGTH - PREFIX.len())
.map(|idx| char::from(CHARSET[idx])
.collect();

Or if you want to be nice, add an analog to Alphanumeric for lowercase characters to lib.rs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are now the proud owner of RandomChars which impls rand::distr::Distribution<u8> for any set of characters we may care about.

format!("{PREFIX}{bucket_id}")
}
Loading