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

test requires HTTP::Daemon 6.05+ and use 127.0.0.1 or [::1] according to server's sockdomain #280

Merged
merged 4 commits into from
Aug 23, 2019

Conversation

skaji
Copy link
Member

@skaji skaji commented Aug 3, 2019

We saw some test failures related IPv4/IPv6 (eg #278 #272 #101).

Now HTTP::Daemon 6.05 uses IO::Socket::IP, and IO::Socket::IP takes care of IPv4/IPv6,
so we should use "localhost" notations and let IO::Socket::IP select IPv4/IPv6 automatically.

Note:

  • dzil build will append "test requires HTTP::Daemon 6.05" to META.json, so I didn't add it in this PR.
  • t/TestServer.pm and t/cookies.t still use 127.0.0.1 notation. I hope we address them in another PR. ("localhost" is not a valid domain for cookies. So we cannot use "localhost" simply there)

@coveralls
Copy link

coveralls commented Aug 3, 2019

Pull Request Test Coverage Report for Build 273

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.711%

Totals Coverage Status
Change from base Build 265: 0.0%
Covered Lines: 725
Relevant Lines: 782

💛 - Coveralls

@@ -3,7 +3,7 @@ use strict;
use Test::More tests => 47;
use lib qw( t t/local );
use LocalServer;
use HTTP::Daemon;
use HTTP::Daemon 6.05;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, we could also add this to TestRequires in dist.ini: https://github.com/libwww-perl/WWW-Mechanize/blob/master/dist.ini#L26-L27

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the prereq version for HTTP::Daemon is 0, but has be set to 6.05, otherwise tests will fail if older HTTP::Daemon is installed. Or alternatively skip tests if HTTP::Daemon version is < 6.05.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the first comment, dzil build will bump HTTP::Daemon prereq.
I intentionally did not do it because it would also give us unrelated diff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually bumped HTTP::Daemon in 68af39e

@oalders
Copy link
Member

oalders commented Aug 22, 2019

@skaji is this ready to merge?

@skaji
Copy link
Member Author

skaji commented Aug 22, 2019

I have cherry-picked a commit from patch2 branch 7d796b8.
jkeenan confirmed that tests passed with the commit libwww-perl/HTTP-Daemon#33 (comment).

@oalders If travis ci passes, I think this PR is ready to merge!

@skaji skaji changed the title Use "localhost" and let IO::Socket::IP select IPv4/IPv6 automatically test requires HTTP::Daemon 6.05+ and use 127.0.0.1 or [::1] according to server's sockdomain Aug 22, 2019
@jkeenan
Copy link

jkeenan commented Aug 23, 2019

The CPANtester who originally reported the bad interaction between HTTP::Daemon and WWW::Mechanize (causing his rigs to hang indefinitely) has reported to me that skaji's branch patch2 corrects that problem. We are therefore eager to see a new WWW::Mechanize release to CPAN as soon as possible.

A new CPAN release is likely to resolve these 3 issues in your queue:
#253
#278
#281

Thank you very much.
Jim Keenan

@oalders
Copy link
Member

oalders commented Aug 23, 2019

Thanks @skaji, @eserte and @jkeenan!

@oalders oalders merged commit 4173118 into master Aug 23, 2019
@oalders oalders deleted the patch branch August 23, 2019 17:21
@Corion
Copy link

Corion commented Sep 10, 2019

Thank you all for fixing this! The log-server program at some time split off into its own CPAN module at Test::HTTP::LocalServer, where I added this fix as well!

I'll look into updating the program in WWW::Mechanize from Test::HTTP::LocalServer if I have features that the WWW::Mechanize distribution needs.

Corion pushed a commit to Corion/Test-HTTP-LocalServer that referenced this pull request Sep 10, 2019
This comes from

libwww-perl/WWW-Mechanize#280

Thanks to Shoichi Kaji for the code and Olaf Alders for the review
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.

None yet

6 participants