Implement persistent bucket fixtures for integration tests #301
Conversation
…o-teardown, add error handling
| @@ -0,0 +1 @@ | |||
| Introduce PersistentBucketAggregate class to manage bucket name and subfolder. | |||
There was a problem hiding this comment.
users won't care for these details in CLI tool changelog
btw, what is your plan for SDK? you started with CLI, but won't this be harder to apply the same thing to SDK?
I can kinda see these changelog message make sense if they were part of public api of SDK that is used in here.
There was a problem hiding this comment.
Wasn't aware SDK was part of the same ticket, will proceed with it.
There was a problem hiding this comment.
yeah, I wouldn't rely on tickets' description much, more on what makes sense and fixing problem in one place while identical exists in another - doesn't
| @@ -0,0 +1 @@ | |||
| Update integration tests to use persistent buckets. | |||
There was a problem hiding this comment.
seems like duplicate
IMO as they are right now all of these would go under our "infrastracture" change category i.e. not something that typical users care about, since this is not what they get with the shipped version of CLI tool.
test/integration/cleanup_buckets.py
Outdated
| # of a persistent bucket, whose identity is shared across tests. | ||
| persistent_bucket = get_or_create_persistent_bucket(b2_api) | ||
| b2_api.clean_bucket(persistent_bucket) | ||
| b2_api.api.list_buckets() |
There was a problem hiding this comment.
overlooked, redundant
| # when tests tear down, as otherwise we'd lose the main benefit | ||
| # of a persistent bucket, whose identity is shared across tests. | ||
| persistent_bucket = get_or_create_persistent_bucket(b2_api) | ||
| b2_api.clean_bucket(persistent_bucket) |
There was a problem hiding this comment.
won't this break concurrently run tests?
There was a problem hiding this comment.
This is why lifecycle rules was suggested instead to cleanup in the first place.
There was a problem hiding this comment.
It cleans the buckets once, before the tests run.
There was a problem hiding this comment.
ok, so what will happen if I someone opens a second PR when one is being tested?
There was a problem hiding this comment.
the same thing that would happen if no changes were introduced—such a scenario disrupts the test bucket lifecycle, persistent or not. Question is, is it frequent enough to warrant addressing?
There was a problem hiding this comment.
The bucket cleanup process only removed stale buckets, not all of them, so before we did support concurrent GHA jobs, and this change breaks it, and for no reason AFAIK, since the solution is to simply to leave that bucket alone forever.
|
|
||
|
|
||
| @backoff.on_exception(backoff.expo, Exception, max_tries=3, max_time=10) | ||
| def delete_files(bucket: Bucket, subfolder: str): |
There was a problem hiding this comment.
| def delete_files(bucket: Bucket, subfolder: str): | |
| def delete_files(bucket: Bucket, subfolder: str | None = None): |
seems like these two methods should be combined
There was a problem hiding this comment.
Seems to me like these calls are going:
a) slow down tests (so +++ cost on development side & GHA)
b) incur extra cost on B2 API side - but I guess that doesn't matter much for us
While simply waiting for lifecycle rules to do their thing, also means some extra B2 storage costs, but is much faster since we have to do nothing.
There was a problem hiding this comment.
Even though the exec time and the number of buckets used dropped as compared to the state before, I was clearly overcorrecting by clearing the subfolders between cases, trying to fully mimic the behavior of recreated buckets. Clearly redundant.
| # CI environment | ||
| repo_id = os.environ.get("GITHUB_REPOSITORY_ID") | ||
| if not repo_id: | ||
| raise ValueError("GITHUB_REPOSITORY_ID is not set") | ||
| bucket_hash = hashlib.sha256(repo_id.encode()).hexdigest() |
There was a problem hiding this comment.
I kinda like the source of ID you used (especially the account id), but please note the tests are also run under Jenkins in case of staging environment.
Probably best to simply tests for GITHUB_REPOSITORY_ID presence and use account_id.
| @@ -0,0 +1 @@ | |||
| Improve internal testing infrastructure by updating integration tests to use persistent buckets. | |||
There was a problem hiding this comment.
as indicated in #301 (comment) this should go under infrastracture not changed category as it is not changing the exposed API of b2 CLI tool. The CI&tests are not even part of the releases.
Key changes:
Benefits: