Skip to content

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