Rewrite integration tests to use a persistent bucket #364
Conversation
There was a problem hiding this comment.
I left some comments within code. But the main comment is not about a specific piece of code here. The goal of this task is to minimize bucket creation where it's not necessary for the test functionality. Now, test_raw_api does have two calls that create buckets. However, this is not the only or even main source of bucket creation: tests in test_download.py, test_file_version_attributes.py, test_upload.py create even more buckets.
|
|
||
| def raw_api_test_helper(raw_api, should_cleanup_old_buckets): |
There was a problem hiding this comment.
If I understand correctly, you're refactoring raw_api_test_helper() . It's one big linear function calling most raw_api endpoints in one go, you're splitting it into methods of a test class. I wouldn't necessarily mind this refactor in general, but it seems quite orthogonal to the issue at hand, which is creating less buckets in testing. I'd much prefer such refactor be done in a separate commit or even PR. Here, it would be enough to pass in a fixture that allows getting a dir in a bucket and cleans up afterwards.
There was a problem hiding this comment.
You're right that this refactor could be a different PR. I decided I wanted to have a clear split between bucket and non-bucket tests, and since this way of splitting makes more sense in the context of what is done in this PR (this refactor prob wouldn't have been done otherwise), I decided to do it here. At the end of this PR I'll rebase my commits and have a separate commit for this refactor.
b2sdk/_internal/raw_api.py
Outdated
| 'readBucketNotifications', | ||
| 'writeBucketNotifications', |
There was a problem hiding this comment.
I suspect I know why you removed those, this is just a reminder that they will need to be added back here at some point.
There was a problem hiding this comment.
I re-added these lines and marked test.integration.test_bucket.test_bucket_notification_rules to be skipped - since the API for these notifications is disabled this test should not be necessary and won't run (unless I'm missing something?)
jakub-cichalewski-reef
force-pushed
the
integration_test_reuse_buckets
branch
2 times, most recently
from
7d2f812 to
6ffefec
Compare
jakub-cichalewski-reef
force-pushed
the
integration_test_reuse_buckets
branch
from
6ffefec to
68b2af6
Compare
jakub-cichalewski-reef
force-pushed
the
integration_test_reuse_buckets
branch
3 times, most recently
from
1ca7c66 to
06ed8ae
Compare
jakub-cichalewski-reef
force-pushed
the
integration_test_reuse_buckets
branch
from
6c04318 to
2844b63
Compare
jakub-cichalewski-reef
changed the title
|
succesful Actions run here |
|
replaced by #379 |
No description provided.