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

Update AWS SDK to V2 #43

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

alexwhiteoval
Copy link

  • Merged in master branch.
  • Test has been re-written so the AWS SDK is now mocked and has the expected methods validated.
  • Updated StreamPart to use BinaryUtils instead of old Base64 class for encoding.

@alexmojaki
Copy link
Owner

It looks like integrity checking is no longer being tested, and that mocking makes it difficult to do so. That seems like a significant problem.

@alexwhiteoval
Copy link
Author

OK, I can have a look at this but it won't be today.

@alexwhiteoval
Copy link
Author

I have updated the tests to include integrity checking

@alexmojaki
Copy link
Owner

The test doesn't actually check the uploaded content. The list of StringBuilders is never examined. It looks like mocking has been taken too far. If setting up some local equivalent to S3 like s3proxy or localstack is too much trouble, then I'd prefer a test against the real S3.

@alexwhiteoval
Copy link
Author

That is correct I have written it as a unit test and didn't intend on testing the AWS SDK, only the code in the function. The mocking allows for testing that each part upload matches and that the upload ordering returns the correct hash at the end of the upload. The hashes are coming from the uploaded content as they do from AWS.

If you would like an integration test that includes the AWS SDK then that is fine, do you have a preference for using an s3proxy or localstack?

@alexmojaki
Copy link
Owner

I don't have any preference, I imagine there's many more similar S3 'emulators' out there that could work. The original s3proxy test was full of boilerplate that I didn't really understand, which I'd rather avoid, but using s3proxy nowadays may or may not require that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants