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

Increase Test Coverage #8

Merged
merged 10 commits into from Feb 6, 2017

Conversation

Projects
None yet
3 participants
@simbabque
Contributor

simbabque commented Feb 5, 2017

Increase the test coverage to a total of 93.5% if the live tests are not run, remove unused dependencies, clean up the tests and document a previously hidden feature.

This is my entry for the Pull Request Challenge 2017.

simbabque added some commits Feb 4, 2017

fix contradiction in headers type on default
There's a relatively new ->flatten in HTTP::Headers, but we now require at least 6.07. It lets us convert the whole Headers object to an array ref, so the contradiction goes away.
use the correct XML in unit tests
The response in the DATA section of s3.t was of the wrong kind, so it did not get parsed properly and we ended up with an empty bucket list. We now use the correct response ListAllMyBucketsResult. I've also prepared this to include more responses so we can test the error handling in ::ResponseParser->_parse_error. A possible pitfall was that the example response at http://docs.aws.amazon.com/AmazonS3/latest/API/SOAPListAllMyBuckets.html not only has syntax errors, but also the namespace does not have a trailing slash which we are expecting. Since no-one is complaining and it seems to work in production I believe the documentation is wrong.

After the preparation for the tests of _parse_error I suspect that the expect_nothing check in the trigger in ::ResponseParser is the wrong way around. The ListAllMyBuckets request has a 0, which indicates not to expect nothing. That makes sense as the response is a list of buckets. But if it's false we don't check for errors. So we only check for errors if the response has no body. But because the result of the error check has been ignored since the first commit anyway this never made a difference.
add tests for temporary redirect at adding bucket
We got in a few other useful tests as well. They don't really bring more coverage but they make sense.
add tests for ::FileIterator
There are a few things that are really hard to cover and I think they are not that pressing. The `$s->{prefix}` in `_fetch` is passed to as a paramter in the request to get a new page. It can be initialized in the constructor though Iterator::Paged, but is undocumented. It is used to limit bucket results. We should document it.
add `prefix` as documented arg including test case
Also prepare Changes for the next release because this is not an internal-only change.
don't mock `friendly_error` in the fake HTTP::Request
The `friendly_error` is part of AWS::S3::ResponseParser and there is no need to mock it the tests.
add tests for the error handling
With this the overall test coverage is now at 93.5% without the live tests.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 5, 2017

Coverage Status

Coverage increased (+8.8%) to 96.167% when pulling d3b2724 on simbabque:coverage into ea3c118 on leejo:master.

coveralls commented Feb 5, 2017

Coverage Status

Coverage increased (+8.8%) to 96.167% when pulling d3b2724 on simbabque:coverage into ea3c118 on leejo:master.

@leejo leejo merged commit 81d8bd8 into leejo:master Feb 6, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
coverage/coveralls Coverage increased (+8.8%) to 96.167%
Details
@leejo

This comment has been minimized.

Show comment
Hide comment
@leejo

leejo Feb 6, 2017

Owner

Thanks again! v0.13 will be on CPAN shortly.

Owner

leejo commented Feb 6, 2017

Thanks again! v0.13 will be on CPAN shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment