Skip to content
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

Add integration tests for S3 storage driver #1580

Merged
merged 4 commits into from
May 13, 2021
Merged

Conversation

c-w
Copy link
Member

@c-w c-w commented May 7, 2021

Add integration tests for S3 storage driver

Description

This pull request adds integration tests for the S3 storage driver.

Given that there are existing buckets in the test AWS account, this pull request also updates the base tests to ignore any containers that weren't created as part of the test.

Unlike for Azure and MinIO, on S3, create_container for an already existing container seems to not throw ContainerAlreadyExistsError so the assertion has been disabled in this driver's test suite. In the future we may want to consider fixing all these minor inconsistencies that are brought to light by the tests.

Status

  • done, ready for review

Checklist (tick everything that applies)

@codecov-commenter
Copy link

codecov-commenter commented May 7, 2021

Codecov Report

Merging #1580 (57f794f) into trunk (fcab0c6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            trunk    #1580   +/-   ##
=======================================
  Coverage   83.01%   83.01%           
=======================================
  Files         394      394           
  Lines       84999    84999           
  Branches     9040     9040           
=======================================
  Hits        70558    70558           
  Misses      11374    11374           
  Partials     3067     3067           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcab0c6...57f794f. Read the comment docs.

@c-w c-w force-pushed the storage-s3-integration-tests branch from 305d680 to 9311c7e Compare May 7, 2021 02:10
@c-w c-w force-pushed the storage-s3-integration-tests branch from 9311c7e to 57f794f Compare May 7, 2021 02:56
@c-w c-w force-pushed the storage-s3-integration-tests branch from 46daf0e to 7ea8b2b Compare May 7, 2021 03:18
@classmethod
def _random_container_name(cls):
suffix = random_string(cls.container_name_max_length)
name = cls.container_name_prefix + suffix
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think including a special prefix on which we can filter on in the "clean up phase" is a good idea.


@classmethod
def tearDownClass(cls):
client = boto3.Session(
Copy link
Member

Choose a reason for hiding this comment

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

Just curious why we use boto3 for the clean up phase and not the Libcloud itself?

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 figured that if we have a bug/regression in libcloud, we don't want to keep stray resources around so might as well use the official library to perform the cleanup.

@Kami
Copy link
Member

Kami commented May 7, 2021

I see the overall integration tests duration is ~14 minutes and test_objects_stream_iterable takes ~ 90 seconds.

Is there any way we could speed that up a bit? E.g. use a smaller object for that test or similar. Or we are intentionally testing large object size and we can't avoid slower duration?

@Kami
Copy link
Member

Kami commented May 7, 2021

Unlike for Azure and MinIO, on S3, create_container for an already existing container seems to not throw ContainerAlreadyExistsError so the assertion has been disabled in this driver's test suite. In the future we may want to consider fixing all these minor inconsistencies that are brought to light by the tests.

Yeah I agree, this seems like something we should eventually fix for consistency across drivers. Please feel free to open an issue for it if you get a chance.

On that note, what is the current behavior? It just returns an existing container with that name?

@c-w
Copy link
Member Author

c-w commented May 13, 2021

On that note, what is the current behavior? It just returns an existing container with that name?

Yes, create_container on an existing container returns a container with the same properties as the existing one. Here's a repro:

from libcloud.storage.types import Provider
from libcloud.storage.providers import get_driver

cls = get_driver(Provider.S3)
driver = cls(AWS_ACCESS_KEY_ID, AWS_ACCESS_KEY_SECRET)

containers = driver.list_containers()
print(driver.create_container(containers[0].name).name == containers[0].name)

@c-w c-w merged commit 72b266c into trunk May 13, 2021
@c-w c-w deleted the storage-s3-integration-tests branch May 13, 2021 23:44
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 15, 2021
* Enable tests to work with existing containers

* Add integration tests for S3

* Pass secrets to integration tests

* Fix error due to missing addClassCleanup
d-mo pushed a commit to mistio/libcloud that referenced this pull request Nov 19, 2021
* Enable tests to work with existing containers

* Add integration tests for S3

* Pass secrets to integration tests

* Fix error due to missing addClassCleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants