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

Create an S3 client based on local storage #113

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

Conversation

hicder
Copy link

@hicder hicder commented Feb 14, 2021

Right now, the unit tests are not able to run because we use actual S3 storage for tests. We want to mock the S3 storage using local storage so that unit test run faster.

In this PR, I created a class TestS3ClientWrapper which extends AwsS3ClientWrapper. Also, update run_tests.sh to disable all networking for docker container to confirm that this test runs locally.

Before my changes, run_tests.sh never completes. With my changes, run_tests.sh completes all tests in 15 minutes. Breakdown as followed:

  • db_test2
[----------] Global test environment tear-down
[==========] 137 tests from 6 test cases ran. (55334 ms total)
[  PASSED  ] 137 tests.
  • db_basoc_test
[----------] Global test environment tear-down
[==========] 154 tests from 6 test cases ran. (207756 ms total)
[  PASSED  ] 154 tests.
  • env_basic_test
[----------] Global test environment tear-down
[==========] 15 tests from 4 test cases ran. (24 ms total)
[  PASSED  ] 15 tests.
  • cloud_manifest_test
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (7 ms total)
[  PASSED  ] 1 test.
  • db_cloud_test
[----------] Global test environment tear-down
[==========] 23 tests from 1 test case ran. (64610 ms total)
[  PASSED  ] 23 tests.
  • db_test
[----------] Global test environment tear-down
[==========] 165 tests from 3 test cases ran. (659752 ms total)
[  PASSED  ] 165 tests.

@hicder
Copy link
Author

hicder commented Feb 14, 2021

I'll remove Travis eventually. CircleCI seems to be much faster.

@hicder
Copy link
Author

hicder commented Feb 14, 2021

Once this PR is merged, we'll require CircleCI build to pass before merging. I think it will make merging much safer.

@@ -250,6 +309,366 @@ class AwsS3ClientWrapper {
std::shared_ptr<CloudRequestCallback> cloud_request_callback_;
};

namespace test {

Choose a reason for hiding this comment

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

Could we move this implementation to the test file, so that we don't clutter the production binary?


export AWS_ACCESS_KEY_ID
export AWS_SECRET_ACCESS_KEY

export SRC_ROOT=$(git rev-parse --show-toplevel)

Choose a reason for hiding this comment

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

Do we also need an integration test that actually uses S3?

@hicder
Copy link
Author

hicder commented Feb 16, 2021

@mrambacher is going to have a similar PR that mocks the StorageProvider, instead of mocking the S3Client in a week, so I'll put this PR on hold.

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