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

Add test for s3 back-end (read, write, listdir) using moto lib. #121

Merged
merged 2 commits into from
Feb 26, 2018

Conversation

michelorengo
Copy link

(sorry for the delay - just got back from vacation).
This is to address issue #117
The test coverage is admittedly very limited, and it is for 2 reasons:

  1. It looks like the s3.py was coded to handle use cases beyond what iorw.py requires. For instance: the ability to read from an archive/compressed file. Yet there are use cases I don't really understand. For example, while "bucket" is a concept defined by the S3 service, the class "Bucket" can be associated with other services (e.g. SQS or EC2). That makes little sense to me, but I'm not claiming I fully understand the original intent.
  2. As discussed before, I suggest that the s3.py be scrapped and replaced with a third-party module such as s3fs (which does a good job in providing a python file API on top of S3/boto3)

Therefore because a) I did not understand some of the complexity/ purposes of the s3.py functions and b) would like this module to be replaced, I wrote the tests for the functions iorw.py actually uses.
Hope this is fine.

@MSeal
Copy link
Member

MSeal commented Feb 20, 2018

That's perfect. You've stumbled upon some code that was (blindly?) copied over from an internal repository to handle some netflix specific use-cases. You intuition and suggestions on the topic sound right to me.

Looks like the specs need credentials, does it need anonymous creds passed to the client?

@michelorengo
Copy link
Author

Indeed you need the AWS credentials (from boto3). There are different ways to pass those credentials (see https://boto3.readthedocs.io/en/latest/guide/configuration.html#guide-configuration) but typically those are stored in the AWS credential file. You have a good point though that if you don't have those the tests on s3 will fail. I guess we could bypass the tests if boto3 cannot be properly initialized.

@MSeal
Copy link
Member

MSeal commented Feb 21, 2018

All the aws libraries have anonymous credentials you can set for testing purposes against mock s3's. I can look up the docs for boto3 in particular tomorrow to link.

@michelorengo
Copy link
Author

Oh! I did not know that. Thanks!

@MSeal
Copy link
Member

MSeal commented Feb 21, 2018

I think this should satisfy the ability: https://stackoverflow.com/questions/34865927/can-i-use-boto3-anonymously
If that has additional issues we can look at making any other credentials anonymous that it complains about (depends on the mock service).

@michelorengo
Copy link
Author

I see. I misunderstood what you wrote earlier. For some reason, I thought that "test" credentials were available. I understand, now, that you're talking about anonymous access to S3.

@codecov
Copy link

codecov bot commented Feb 25, 2018

Codecov Report

Merging #121 into master will increase coverage by 9.84%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   63.31%   73.16%   +9.84%     
==========================================
  Files           8        8              
  Lines         924      924              
==========================================
+ Hits          585      676      +91     
+ Misses        339      248      -91

@michelorengo
Copy link
Author

Rainy afternoon so I decided to tackle this.
Contrary to what I wrote, moto has no problem mocking the authentication, so the issue was not that "Indeed you need the AWS credentials".
It took me a long, long time (I was too focused on what I added) but I finally saw that the class S3 was instantiated in a different test ("test_s3_defaults"), and because the instances share a dictionary of AWS sessions, the S3() (no args) returned the non-authenticated session.
The fix was then to make sure that S3() was within a moto mock session.
And, of course, this issue is hidden if one runs the test with an AWS credential file. Ah well...

@MSeal
Copy link
Member

MSeal commented Feb 26, 2018

Awesome work. Thanks for getting that fix in. Did @rgbkrk invite you to the Nteract org yet? We like inviting people who make good contributions.

@MSeal MSeal merged commit 87ba942 into nteract:master Feb 26, 2018
@michelorengo
Copy link
Author

Thanks. And yes I was invited (and accepted the invitation). When I have a bit of time, I'll work on integrating s3fs package.

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.

4 participants