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

Fog mock accuracy, fixes #341 #344

Merged
merged 4 commits into from
Feb 15, 2017
Merged

Fog mock accuracy, fixes #341 #344

merged 4 commits into from
Feb 15, 2017

Conversation

easkay
Copy link
Contributor

@easkay easkay commented Feb 12, 2017

Intro

Opening this PR for some early feedback and discussion. Originally I raised the issue (#341) under my other account (@ac-astuartkregor).

Important considerations

It occurred to me that some folks might be using the existing functionality in their tests. Therefore this PR could cause breakages if they are already reliant on the response status being set, rather than having to rescue the exception in their code.

What's been addressed?

I have looked at the storage requests and found any instances where a mock was setting a status code but not raising an Excon::Error of the appropriate flavour. The changes made have been verified against the actual result received when Fog is not in mock mode. Extra tests have been added to verify the success and failure cases of the various requests, in particular the get_object and head_object requests.

What's left to discuss?

I'd like to know what people feel the scope of this PR should be. For example, should other AWS mocks be examined for accuracy as well? Should mock accuracy outside of exceptions vs response status be considered?

@geemus
Copy link
Member

geemus commented Feb 15, 2017

High level response to your comments follows, will dig into code specifics next.

I wouldn't worry too much about breaking existing mock usage for people, as it would be a break to tests rather than running code (and it should help them improve their systems).

In terms of scope, I would say it's worth revisiting if/when it becomes an issue for someone who is willing to help out (like this case), but otherwise we are probably broadly in the "good enough" bucket for most mocks.

@geemus
Copy link
Member

geemus commented Feb 15, 2017

@easkay Given our discussion earlier, this looks good to me. I'd be happy to merge, but just wanted to double check with you if you have further concerns or things you want to do before we pull it in. Thanks!

@easkay
Copy link
Contributor Author

easkay commented Feb 15, 2017

Hi @geemus
Thanks for the quick reply. Fair enough about breaking test code. I just wanted to make sure that was considered so that it could be handled if necessary.

As far as I'm concerned, if you're happy to merge it, then ship away! Thanks very much :)

@geemus
Copy link
Member

geemus commented Feb 15, 2017

I certainly appreciate your care and concern on breakage, it was a great question to ask and discuss.

Thanks!

@geemus geemus merged commit d828888 into fog:master Feb 15, 2017
@easkay easkay deleted the fog_mock_accuracy branch February 15, 2017 18:47
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