-
Notifications
You must be signed in to change notification settings - Fork 10
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
Refactor integration tests to use persistent buckets #379
Refactor integration tests to use persistent buckets #379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gj on refactoring the old test code related to this change.
Out of the major comments I only have one: Some of the code seem to duplicate stuff we need in CLI tests - whats the plan here? Copy pasting it or exposing it as part of SDK or what?
test/integration/helpers.py
Outdated
|
||
|
||
def _clean_and_delete_bucket(raw_api, api_url, account_auth_token, account_id, bucket_id): | ||
# Delete the files. This test never creates more than a few files, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test? which test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is still tightly tied with raw-api tests - not sure why it was moved to general helpers if its only useful in raw-api tests.
You seem to have created a test_bucket
fixture - wouldn it make better sense for it to do a cleanup of the bucket in there i.e.
def test_bucket(...):
...
yield bucket
_clean_and_delete_bucket(..., bucket)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test? which test?
Context got lost when I moved it from test_raw_api.py
. Will fix.
It's also called, albeit indirectly, by cleanup_old_buckets
nox session to remove stale buckets. This is what warranted the move in the first place.
test/integration/conftest.py
Outdated
application_key_id = os.environ.get('B2_TEST_APPLICATION_KEY_ID') | ||
application_key = os.environ.get('B2_TEST_APPLICATION_KEY') | ||
if application_key_id is None or application_key is None: | ||
pytest.fail('B2_TEST_APPLICATION_KEY_ID or B2_TEST_APPLICATION_KEY is not set.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use b2_auth_data
fixture instead
I understand you're referring to the persistent bucket retrieval code. Currently, this code differs slightly between the two repositories. In the SDK, the code for deriving the persistent bucket name takes into account the arguments used to create a bucket. However, I haven't observed a similar need in the CLI. |
Even if the one here is slightly different, it is duplicated. But I guess it is a more general problem with entire integration test suite case and perhaps best to dealt with as separate task. edited: I see #379 (comment) was not yet address, so please create PR after doing that |
test/integration/test_raw_api.py
to use persistent bucket where applicable