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

Require network testing enabled for t/redirect.t #351

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Conversation

oalders
Copy link
Member

@oalders oalders commented Sep 23, 2020

Fixes #350

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #351 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #351   +/-   ##
=======================================
  Coverage   59.21%   59.21%           
=======================================
  Files          15       15           
  Lines        1378     1378           
  Branches      384      384           
=======================================
  Hits          816      816           
  Misses        380      380           
  Partials      182      182           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66e6639...826e0b7. Read the comment docs.

@coveralls
Copy link

coveralls commented Sep 23, 2020

Coverage Status

Coverage remained the same at 60.37% when pulling 826e0b7 on oalders/redirect into 66e6639 on master.

use LWP::UserAgent;
plan tests => 4;
use Test::More;
use Test::RequiresInternet;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be use Test::RequiresInternet '198.51.100.123'; ?

(Also, what is this IP? I know, from previous discussions in PRs, but it is not explained in the tests. If it has specific behaviour that the test expects, that should be explicitly stated in case we ever have to find an equivalent replacement.)

Copy link
Member Author

@oalders oalders Sep 23, 2020

Choose a reason for hiding this comment

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

0$ git diff
diff --git a/t/redirect.t b/t/redirect.t
index 8aa8c09b..004f29f5 100644
--- a/t/redirect.t
+++ b/t/redirect.t
@@ -2,7 +2,7 @@ use strict;
 use warnings;

 use Test::More;
-use Test::RequiresInternet;
+use Test::RequiresInternet '198.51.100.123';
 use LWP::UserAgent ();

 # This is a regression test for #171
$ prove -lv t/redirect.t
Must supply server and a port pairs. You supplied 198.51.100.123

With the incantation below, the test just hangs:

$ git diff
diff --git a/t/redirect.t b/t/redirect.t
index 8aa8c09b..a85a8673 100644
--- a/t/redirect.t
+++ b/t/redirect.t
@@ -2,7 +2,7 @@ use strict;
 use warnings;

 use Test::More;
-use Test::RequiresInternet;
+use Test::RequiresInternet ( '198.51.100.123' => 80 );
 use LWP::UserAgent ();

 # This is a regression test for #171

With the incantation in this patch:

$ prove -lv t/redirect.t

ok 1 - Timeout gives a client warning
ok 2 - ... and has tells us about the problem
ok 3 - Timeout with no redirects gives a client warning
ok 4 - ... and has tells us about the problem
ok
All tests successful.
Files=1, Tests=4,  2 wallclock secs ( 0.02 usr  0.00 sys +  0.09 cusr  0.02 csys =  0.13 CPU)
Result: PASS
$ NO_NETWORK_TESTING=1 prove -lv t/redirect.t
skipped: NO_NETWORK_TESTING
Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.05 cusr  0.01 csys =  0.07 CPU)
Result: NOTEST

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the IP, there's a comment in this commit: 63bbe49

https://tools.ietf.org/html/rfc5737

  1. Documentation Address Blocks

The blocks 192.0.2.0/24 (TEST-NET-1), 198.51.100.0/24 (TEST-NET-2),
and 203.0.113.0/24 (TEST-NET-3) are provided for use in
documentation.

Copy link
Member

Choose a reason for hiding this comment

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

correct me if I'm wrong: 198.51.100.123' is not in 198.51.100.0/24

Copy link
Member

Choose a reason for hiding this comment

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

198.51.100.0/24 should be 192.51.100.0 right through to 192.51.100.255 (/24 encompasses the entire bottom octet).

Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

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

LGTM!

@oalders oalders merged commit cd62154 into master Sep 24, 2020
@oalders oalders deleted the oalders/redirect branch September 24, 2020 00:27
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.

Redirect tests fail in environment with web gateway
4 participants