-
Notifications
You must be signed in to change notification settings - Fork 80
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
feat: Add concurrent integration test & utils #1596
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.
The rest of this is fine, but let's not commit 100MB of garbage to the project. Try generating that data in a temp dir, then using that in the 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.
See individual comments
IntegrationTests/Services/AWSS3IntegrationTests/S3ConcurrentTests.swift
Outdated
Show resolved
Hide resolved
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.
One question
…ift into feat/concurrent-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.
Tests still seem to take quite a while (10 sec per test on Linux+CRT, 30 per sec on Mac+URLSession) but I'm approving because the tests do what they are supposed to do.
If we want to adjust the number of concurrent runs or the test duration, we can do that at a later time.
* Add concurrent tests * Add missing 100x to a test's name * Remove 100MiB file and generate file instead. * See if using UUID for filename fixes this possibly-concurrency-issue. * Try temporaryDirectory property. * Fix concurrency issue * Comment fixes & remove STS getCallerIdentity concurrent calls. * Remove unused import * Remove concurrent test for event stream output * Refactor tests a bit * Use FileStream to write to file * Swap out filemanager with writing emtpy data approach * Tweak a bit * Simplify; no need to safe generated dummy data to a file, just use 50MB memory. * Change from 10x50MB to 100x5MB * Reduce payload from 5MB to 3MB * Reduce payload from 3MB to 1MB. * Add test for 1.5 MB to test aws chunked & flexible checksum flow also. --------- Co-authored-by: Sichan Yoo <chanyoo@amazon.com> Co-authored-by: Josh Elkins <jbelkins@users.noreply.github.com>
Issue #
#1561
Description of changes
count
number of times, idea taken from draft PR: feat: Add concurrent integration tests #1484New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.