-
Notifications
You must be signed in to change notification settings - Fork 60
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
More tests #85
More tests #85
Conversation
@openstrike thanks very much for this! It looks like the builds are failing for anything less than Perl 5.18 |
Apparently so. Oddly, I've been testing on 5.16.3 without issue. I will see about applying a patch for the older versions. |
The patch seems to have done the job. Note that all this does is skip the 2 problematic tests on those perls where they were failing. You might or might not consider the relevant code within the module to be a problem (line 173 of HTTP::Config which attempts to call can() on the argument to matching()) |
Current master:
This PR:
@openstrike Could you silence tests? |
Silence carp about void content call in t/message.t
Commit a7c6ba1 silences the carp from t/message.t. |
I would like to hear thoughts of other maintainers. |
@skaji's diff (and thoughts in general) look good to me. |
If the consensus is that the modules should issue warnings when called with unexpected arguments (such as undef) then should not the warnings issued be (a) clearer to the user and (b) refer to the offending line in the calling code? ie. should they not be using carp() just as the error message about void context does? If so, the test suite should then be extended to check that such warnings are correctly generated. |
There's a new branch at coverage-test which includes commit 8f94464 implementing my suggestion about carp above. If you are happy with this approach let me know and I'll merge it into this PR. Thanks. |
@skaji thoughts on the comment above? |
I have no objection to the commit 8f94464.
HTTP-Message/lib/HTTP/Message.pm Line 135 in b9f1c04
|
We've also got a merge conflict here. |
Thanks for the feedback. I've merged the changes from coverage-test into coverage (this PR) and have (hopefully) resolved the merge conflict. Travis is reporting failures in response.t now. I will check those and get back to you. |
@openstrike thanks for this. It looks like we have a failing build now. |
All tests pass and there are no merge conficts as of now. |
Thanks for this! Closed via 74b9567 |
- Add some useful examples in HTTP::Request (GH #92) (Chase Whitener). Batch requests are now explained. - PUT and PATCH docs updated (GH #84) (saturdaywalkers) - Trim trailing \r from status line so message() doesn't return it (GH #87) (Felipe Gasper) - Bring test coverage of HTTP::Config to 100% (GH #85) (Pete Houston) - Add 103 Early Hints to HTTP::Status (GH #94) (Tatsuhiko Miyagawa)
Here are some more tests to push up the coverage.
While adding these I noticed that the checks for encodings in HTTP::Message were not consistent (eg. decode checked for "identity" or "none" but encode only for "identity"). This seemed to be an oversight so I have added in the aliases. This is the only change made to anything other than the test suite.
This work has been done as part of the CPAN PR Challenge. Thanks for taking part.