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

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
3 participants
@openstrike
Contributor

openstrike commented Jul 27, 2017

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.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Jul 27, 2017

Member

@openstrike thanks very much for this! It looks like the builds are failing for anything less than Perl 5.18

Member

oalders commented Jul 27, 2017

@openstrike thanks very much for this! It looks like the builds are failing for anything less than Perl 5.18

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Jul 27, 2017

Contributor

Apparently so. Oddly, I've been testing on 5.16.3 without issue. I will see about applying a patch for the older versions.

Contributor

openstrike commented Jul 27, 2017

Apparently so. Oddly, I've been testing on 5.16.3 without issue. I will see about applying a patch for the older versions.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Jul 27, 2017

Contributor

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())

Contributor

openstrike commented Jul 27, 2017

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())

@skaji

This comment has been minimized.

Show comment
Hide comment
@skaji

skaji Jul 27, 2017

Member

Current master:

❯ prove -l t
t/common-req.t .............. ok
t/headers-auth.t ............ ok
t/headers-etag.t ............ ok
t/headers-util.t ............ ok
t/headers.t ................. ok
t/http-config.t ............. ok
t/message-charset.t ......... ok
t/message-decode-xml.t ...... ok
t/message-old.t ............. ok
t/message-parts.t ........... ok
t/message.t ................. ok
t/request.t ................. ok
t/request_type_with_data.t .. ok
t/response.t ................ ok
t/status-old.t .............. ok
t/status.t .................. ok
All tests successful.
Files=16, Tests=577,  1 wallclock secs ( 0.10 usr  0.04 sys +  1.00 cusr  0.13 csys =  1.27 CPU)
Result: PASS

This PR:

❯ prove -l t
t/common-req.t .............. ok
t/headers-auth.t ............ ok
t/headers-etag.t ............ ok
t/headers-util.t ............ ok
t/headers.t ................. ok
t/http-config.t ............. ok
t/message-charset.t ......... ok
t/message-decode-xml.t ...... ok
t/message-old.t ............. ok
t/message-parts.t ........... ok
t/message.t ................. 1/191 Useless content call in void context at t/message.t line 574.
t/message.t ................. ok
t/request.t ................. 1/26 Use of uninitialized value $str in substitution (s///) at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Request.pm line 22.
Use of uninitialized value $request_line in split at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Request.pm line 31.
t/request.t ................. ok
t/request_type_with_data.t .. ok
t/response.t ................ 1/64 Use of uninitialized value $str in substitution (s///) at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Response.pm line 25.
Use of uninitialized value $status_line in pattern match (m//) at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Response.pm line 35.
Use of uninitialized value $status_line in split at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Response.pm line 39.
t/response.t ................ ok
t/status-old.t .............. ok
t/status.t .................. ok
All tests successful.
Files=16, Tests=739,  1 wallclock secs ( 0.10 usr  0.04 sys +  1.01 cusr  0.14 csys =  1.29 CPU)
Result: PASS

@openstrike Could you silence tests?

Member

skaji commented Jul 27, 2017

Current master:

❯ prove -l t
t/common-req.t .............. ok
t/headers-auth.t ............ ok
t/headers-etag.t ............ ok
t/headers-util.t ............ ok
t/headers.t ................. ok
t/http-config.t ............. ok
t/message-charset.t ......... ok
t/message-decode-xml.t ...... ok
t/message-old.t ............. ok
t/message-parts.t ........... ok
t/message.t ................. ok
t/request.t ................. ok
t/request_type_with_data.t .. ok
t/response.t ................ ok
t/status-old.t .............. ok
t/status.t .................. ok
All tests successful.
Files=16, Tests=577,  1 wallclock secs ( 0.10 usr  0.04 sys +  1.00 cusr  0.13 csys =  1.27 CPU)
Result: PASS

This PR:

❯ prove -l t
t/common-req.t .............. ok
t/headers-auth.t ............ ok
t/headers-etag.t ............ ok
t/headers-util.t ............ ok
t/headers.t ................. ok
t/http-config.t ............. ok
t/message-charset.t ......... ok
t/message-decode-xml.t ...... ok
t/message-old.t ............. ok
t/message-parts.t ........... ok
t/message.t ................. 1/191 Useless content call in void context at t/message.t line 574.
t/message.t ................. ok
t/request.t ................. 1/26 Use of uninitialized value $str in substitution (s///) at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Request.pm line 22.
Use of uninitialized value $request_line in split at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Request.pm line 31.
t/request.t ................. ok
t/request_type_with_data.t .. ok
t/response.t ................ 1/64 Use of uninitialized value $str in substitution (s///) at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Response.pm line 25.
Use of uninitialized value $status_line in pattern match (m//) at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Response.pm line 35.
Use of uninitialized value $status_line in split at /Users/skaji/src/github.com/libwww-perl/HTTP-Message/lib/HTTP/Response.pm line 39.
t/response.t ................ ok
t/status-old.t .............. ok
t/status.t .................. ok
All tests successful.
Files=16, Tests=739,  1 wallclock secs ( 0.10 usr  0.04 sys +  1.01 cusr  0.14 csys =  1.29 CPU)
Result: PASS

@openstrike Could you silence tests?

Avoid uninitialized warnings in Request.pm and Response.pm
Silence carp about void content call in t/message.t
@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Jul 28, 2017

Contributor

Commit a7c6ba1 silences the carp from t/message.t.
The other warnings were inherent in the module code so it seemed more sensible to fix them there.
The test suite now runs silently for me - let me know if you still see warnings.

Contributor

openstrike commented Jul 28, 2017

Commit a7c6ba1 silences the carp from t/message.t.
The other warnings were inherent in the module code so it seemed more sensible to fix them there.
The test suite now runs silently for me - let me know if you still see warnings.

@skaji

This comment has been minimized.

Show comment
Hide comment
@skaji

skaji Jul 29, 2017

Member

I would like to hear thoughts of other maintainers.

Member

skaji commented Jul 29, 2017

I would like to hear thoughts of other maintainers.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Jul 29, 2017

Member

@skaji's diff (and thoughts in general) look good to me.

Member

oalders commented Jul 29, 2017

@skaji's diff (and thoughts in general) look good to me.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Aug 1, 2017

Contributor

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.

Contributor

openstrike commented Aug 1, 2017

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.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Aug 7, 2017

Contributor

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.

Contributor

openstrike commented Aug 7, 2017

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.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Oct 12, 2017

Member

@skaji thoughts on the comment above?

Member

oalders commented Oct 12, 2017

@skaji thoughts on the comment above?

@skaji

This comment has been minimized.

Show comment
Hide comment
@skaji

skaji Oct 12, 2017

Member

I have no objection to the commit 8f94464.

BTW, I don't think we need to check $^W, because we usually don't execute perl with -w nowadays;
Once we decide on using carp(), call carp() always.
Sorry, I missed your comment.

should they not be using carp() just as the error message about void context does?

Carp::carp("Useless content call in void context") if $^W;

Member

skaji commented Oct 12, 2017

I have no objection to the commit 8f94464.

BTW, I don't think we need to check $^W, because we usually don't execute perl with -w nowadays;
Once we decide on using carp(), call carp() always.
Sorry, I missed your comment.

should they not be using carp() just as the error message about void context does?

Carp::carp("Useless content call in void context") if $^W;

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Oct 13, 2017

Member

We've also got a merge conflict here.

Member

oalders commented Oct 13, 2017

We've also got a merge conflict here.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Oct 13, 2017

Contributor

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.

Contributor

openstrike commented Oct 13, 2017

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.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Oct 13, 2017

Member

@openstrike thanks for this. It looks like we have a failing build now.

Member

oalders commented Oct 13, 2017

@openstrike thanks for this. It looks like we have a failing build now.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Oct 13, 2017

Contributor

All tests pass and there are no merge conficts as of now.

Contributor

openstrike commented Oct 13, 2017

All tests pass and there are no merge conficts as of now.

@oalders

This comment has been minimized.

Show comment
Hide comment
@oalders

oalders Oct 13, 2017

Member

Thanks for this! Closed via 74b9567

Member

oalders commented Oct 13, 2017

Thanks for this! Closed via 74b9567

@oalders oalders closed this Oct 13, 2017

oalders added a commit that referenced this pull request Dec 20, 2017

v6.14
    - 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment