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

Mock S3 tests instead of using s3-server #131

Closed
brainstorm opened this issue Jan 8, 2023 · 9 comments · Fixed by #150
Closed

Mock S3 tests instead of using s3-server #131

brainstorm opened this issue Jan 8, 2023 · 9 comments · Fixed by #150
Assignees
Labels
enhancement New feature or request

Comments

@brainstorm
Copy link
Member

See issues:

This only affects testing, but it pulls in vulnerable Rust deps and also OpenSSL which we really don't need nor want.

@mmalenic
Copy link
Member

mmalenic commented Jan 8, 2023

Yeah, I think this one is important, as we would like to use rustls for everything if possible.

@mmalenic
Copy link
Member

mmalenic commented Jan 8, 2023

Currently the only place this comes up is here:

// Setup s3-server.
let fs = FileSystem::new(server_base_path).unwrap();
let mut auth = SimpleAuth::new();
auth.register(String::from("access_key"), String::from("secret_key"));
let mut service = S3Service::new(fs);
service.set_auth(auth);

If we wanted to remove this without any additional dependencies, we could refactor calls to the aws sdk into a trait, and mock a test implementation to only test the logic of our code, as suggested here.

@brainstorm brainstorm changed the title Remove Rusoto dependency by fixing s3-server Mock S3 tests instead of using s3-server Jan 9, 2023
@brainstorm
Copy link
Member Author

brainstorm commented Jan 9, 2023

Indeed, we should just mock instead of running a server in the background, although isn't mocking per-method a bit tedious? We'll need to rejig those tests:

failures:
    storage::aws::tests::existing_key
    storage::aws::tests::file_size
    storage::aws::tests::non_existing_key
    storage::aws::tests::retrieval_type
    storage::aws::tests::url_of_existing_key
    storage::aws::tests::url_of_non_existing_key
    storage::aws::tests::url_with_specified_open_ended_range
    storage::aws::tests::url_with_specified_range

This issue outlines how to mock at a full ::Client level, perhaps easier?:

awslabs/aws-sdk-rust#199

@brainstorm brainstorm self-assigned this Jan 16, 2023
@brainstorm brainstorm added the enhancement New feature or request label Jan 18, 2023
@Nugine
Copy link

Nugine commented Jan 18, 2023

s3-server has ended its maintenance as I announced recently in the tracking issue. datenlord/s3-server#108 (comment)

s3s has fixed the vulnerable deps problem because it uses custom codegen instead of depending on the unmaintained Rusoto.
Here is the example if you have interest in migrating to s3s-fs. It would be easy as the interface is almost the same.

I'm also thinking how to provide a simple method to mock a full S3 client. The auto-generated S3 trait may give you a kind of inspiration. Here is an example of how to implement the trait.

@brainstorm
Copy link
Member Author

Thanks for the detailed response, @Nugine! I'd be very interested in full S3 client mocking since I'd like to avoid having s3 servers running when running the testsuite (less complexity and less problems with deployment, operations, ports, etc..).

@brainstorm
Copy link
Member Author

brainstorm commented Jan 26, 2023

@Nugine, I'm wondering if https://crates.io/crates/s3s-aws is your ongoing effort to "mock a full S3 client", I'm a bit on a holding pattern to see what to use that doesn't require a running s3 server with open ports in the background :)

@Nugine
Copy link

Nugine commented Jan 27, 2023

s3s-aws provides two different types: Connector and Proxy.

s3s_aws::Connector implments SmithyConnector. It can be used to turn an S3Service into the http connector in SdkConfig. So you just need to change the sdk config in mocking tests.

Here is an example combining aws-sdk-s3, s3s-aws and s3s-fs.

The logical call stack is like this:

aws_sdk_s3::Client::get_object
    s3s_aws::Connector::call
            s3s::S3Service::call
                <s3s_fs::FileSystem as s3s::S3>::get_object

It does not bind any port. The whole action happens in the test process.

Note that s3s_fs::FileSystem changes the data directory. You may need to serialize the tests to avoid race cases.

However, s3s_fs::FileSystem is not a full-featured and well-tested implementation. It may be buggy. If you want to avoid the possible bugs inside s3s_fs::FileSystem, I would recommand testing against minio, an S3-compatible object storage server.

@brainstorm
Copy link
Member Author

Thanks a ton @Nugine, I just migrated the tests and they seem to work... could you please publish your latest 0.2.1-dev changes from your master so that I can pin it to a version from crates.io? I'm pointing it to your git repo for the moment on PR #150 but it'll probably break soon if it's not pinned.

@Nugine
Copy link

Nugine commented Feb 2, 2023

I released v0.3.0 just now. It contains several bug fixes and a small break change. Feel free to open issues if you find any problem.

brainstorm added a commit that referenced this issue Feb 6, 2023
* fix(search) Bump up s3s-* crates as suggested in #131 (comment)
* test(search): fix tests related to improperly formatted `folder` in URLs for S3 mock filesystem
* fix(search): check that a key exists before formatting a URL, ensuring that pre-signed URLs are not generated for non-existent keys
* fix(search): use KeyNotFound error when a key is not found
* fix(search): use KeyNotFound for head and get object in AwsS3Storage
* test(search): add set of tests targeting non-existent keys
* build(search): remove unused dependencies

---------

Co-authored-by: Marko Malenic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants