-
Notifications
You must be signed in to change notification settings - Fork 17
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
Run all tests against S3 and S3 Express #90
Conversation
@@ -79,11 +79,33 @@ jobs: | |||
python -m pip install -e "s3torchconnectorclient[test,e2e]" | |||
python -m pip install -e "s3torchconnector[test,e2e]" | |||
|
|||
- name: s3torchconnector integration tests | |||
run: pytest s3torchconnector/tst/e2e -n auto | |||
- name: s3torchconnector S3 integration tests |
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.
Should we add s3 express/standard to our matrix? That would decrease duplication here.
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.
Definitely, let me just get it to work!
storage_class = getenv("CI_STORAGE_CLASS") | ||
assert prefix == "" or prefix.endswith("/") | ||
|
||
nonce = random.randrange(2**64) |
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.
random.getrandbits
might be better here, or even just a UUID
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 is the current behaviour, I haven't changed it, just added extra logic.
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.
I am happy to update it if you could please detail your reasoning 😄
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.
I have no strong opinion here if you don't think it looks nicer :)
fc2dd71
to
2883276
Compare
@@ -29,10 +25,10 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
python-version: ["3.8", "3.9", "3.10", "3.11"] | |||
test-run: [ "S3", "S3 Express"] |
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.
Could we use paired values like described in this StackOverflow answer? https://stackoverflow.com/a/68940067
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.
Thanks a lot! That's awesome, I looked for something similar, but didn't manage to find it.
s3torchconnector/tst/e2e/conftest.py
Outdated
@@ -14,8 +13,9 @@ | |||
|
|||
def getenv(var: str) -> str: | |||
v = os.getenv(var) | |||
if v is None: | |||
raise Exception(f"required environment variable {var} is not set") | |||
optional = ["CI_STORAGE_CLASS"] |
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.
Why not have 'optional' be a default argument to the function?
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.
I did not intend on keeping this, but added.
@@ -29,7 +29,10 @@ def test_s3mapdataset_images_10_from_prefix(image_directory): | |||
|
|||
# Intentional usage to emphasize the accessor dataset[index] works. | |||
for index in range(len(dataset)): | |||
key = f"{image_directory.prefix}img{index:03d}.jpg" | |||
key = dataset[index].key | |||
if image_directory.storage_class is None: |
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.
Could we do a not equals express in case we want to try other storage classes?
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.
Added
next(client.get_object(f"{bucket}-{uuid.uuid4()}", key)) | ||
next( | ||
client.get_object( | ||
f"{sample_directory.bucket}-{uuid.uuid4()}", sample_directory.prefix |
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.
I think this is missing the key, but that shouldn't affect the outcome of the test
977806e
to
fe5fad8
Compare
fe5fad8
to
303f6b2
Compare
Enhance Github CI actions and variables to run all tests against both S3 and S3 Express. Address test flakiness by creating per test assets using BucketPrefixFixture, instead of static assets. Remove hypothesis setup from test_mountpoint_client_pickles() as it was flaky. We will add this back when we align on the limits of the parameters.
303f6b2
to
4685582
Compare
Currently, we are running just a subset of tests for S3 Express One Zone.
Enhance Github CI actions and variables to run all tests against both S3 and S3 Express One Zone. Address tests flakiness by creating per test assets using BucketPrefixFixture, instead of using static assets. Add storage class as part of test fixture.
These changes require the following environment variables for running the integration tests locally:
Description
Update automated test runs to cover S3 Express One Zone.
Testing
By submitting this pull request, I confirm that my contribution is made under the terms of BSD 3-Clause License and I agree to the terms of the LICENSE.