bpo-36019: Use pythontest.net instead of example.com #11941

matrixise · 2019-02-19T16:29:30Z

https://bugs.python.org/issue36019

vstinner · 2019-02-19T16:38:57Z

Can you add a NEWS entry?

matrixise · 2019-02-19T16:43:11Z

Sure but only when my PR won’t be in WIP. Thanks 🙏

eamanu

Great!. It would be great use https on the future :-)

vstinner · 2019-02-20T11:15:31Z

It would be great use https on the future :-)

Wait, some tests must not use HTTPS but HTTP. Some tests are written to explicitly test cleartext HTTP (tcp/80). Right now, it seems like: pythontest.net does't redirect to HTTP.

Maybe we should use a constant to test.support for HTTP-only URL.

matrixise · 2019-02-20T11:23:58Z

@vstinner like that?

from test.support import TEST_HTTP_URL
from test.support import TEST_HTTPS_URL

and where

TEST_HTTP_URL='http://www.pythontest.net'
TEST_HTTP_URL='https://www.pythontest.net'

vstinner · 2019-02-20T14:18:03Z

There is no need for HTTPS yet.

vstinner · 2019-02-20T14:26:09Z

"`from test.support import TEST_HTTP_URL" LGTM. Maybe just add a short comment where you define the constant to explain the purpose of this URL. I understood that it must be a public HTTP server, it must use HTTP (not HTTPS), and it must support at least GET verb.

matrixise · 2019-02-21T12:48:18Z

ok, I am going to add this "constant" in test/support/__init__.py

vstinner · 2019-02-21T14:24:43Z

Lib/test/test_codecmaps_cn.py

 import unittest

 class TestGB2312Map(multibytecodec_support.TestBase_Mapping,
                   unittest.TestCase):
    encoding = 'gb2312'
-    mapfileurl = 'http://www.pythontest.net/unicode/EUC-CN.TXT'
+    mapfileurl = f'{support.TEST_HTTP_URL}/unicode/EUC-CN.TXT'


Please revert this change. I would like to be able to modify TEST_HTTP_URL without breaking this URL.

just this one? or the complete commit?

bedevere-bot · 2019-02-21T14:25:00Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner

It seems like you misunderstood what I proposed. The idea of a constant is to adding a constant is to allow to use a different URL without having to update tests. Without your current change, if I modify TEST_HTTP_URL to point to google.com, example.com, etc. A lot of tests fail.

Please restrict the change example.com, and only example.com : https://bugs.python.org/issue36019

vstinner · 2019-02-21T15:32:15Z

Lib/test/test_hashlib.py

@@ -50,7 +50,7 @@ def hexstr(s):
    return r


-URL = "http://www.pythontest.net/hashlib/{}.txt"
+URL = f"{TEST_HTTP_URL}/hashlib/{{}}.txt"


Don't do that. It will break the test if TEST_HTTP_URL is changed to example.com or anything else.

vstinner · 2019-02-21T15:32:50Z

Lib/test/test_urllib.py

@@ -712,7 +712,7 @@ def _reporthook(par1, par2, par3):

        with self.assertRaises(urllib.error.ContentTooShortError):
            try:
-                urllib.request.urlretrieve('http://example.com/',
+                urllib.request.urlretrieve(f'{support.TEST_HTTP_URL}/',


What's the point of adding a slash? Why not using TEST_HTTP_URL directly?

Suggested change

urllib.request.urlretrieve(f'{support.TEST_HTTP_URL}/',

urllib.request.urlretrieve(support.TEST_HTTP_URL,

vstinner · 2019-02-21T15:34:08Z

Lib/test/test_urllib2net.py


    def test_redirect_url_withfrag(self):
-        redirect_url_with_frag = "http://www.pythontest.net/redir/with_frag/"
+        redirect_url_with_frag = f"{support.TEST_HTTP_URL}/redir/with_frag/"


Ditto. Revert such change. The URL becomes wrong if TEST_HTTP_URL becomes example.org, python.org, google.com or whatever else.

vstinner · 2019-02-21T15:36:00Z

Doc/library/test.rst

+.. data:: TEST_HTTP_URL
+
+   Define the URL of a dedicated HTTP server for the network tests
+   (http://www.pythontest.net).


Please remove the value of the constant, it must not be documented. The value doesn't matter, and it reduces the maintenance burden to not have to update the doc, each time the URL changes.

Lib/test/support/__init__.py

…lt maybe I am tired :/

vstinner

Another request for testURLread().

I blocked connections to example.com using:

# I added example.com to localhost
$ sudo head /etc/hosts
127.0.0.1   apu localhost localhost.localdomain localhost4 localhost4.localdomain4 example.com
::1         apu localhost localhost.localdomain localhost6 localhost6.localdomain6 example.com

$ host example.com
example.com has address 93.184.216.34
example.com has IPv6 address 2606:2800:220:1:248:1893:25c8:1946

# Reject all connections to example.com
$ sudo iptables -A OUTPUT -d 93.184.216.34 -j REJECT
$ sudo ip6tables -A OUTPUT -d 2606:2800:220:1:248:1893:25c8:1946 -j REJECT

And I modified transient_internet() to get an error:

diff --git a/Lib/test/support/__init__.py b/Lib/test/support/__init__.py
index b442070b35..674195e442 100644
--- a/Lib/test/support/__init__.py
+++ b/Lib/test/support/__init__.py
@@ -1483,22 +1483,8 @@ def transient_internet(resource_name, *, timeout=30.0, errnos=()):
     """Return a context manager that raises ResourceDenied when various issues
     with the Internet connection manifest themselves as exceptions."""
     default_errnos = [
-        ('ECONNREFUSED', 111),
-        ('ECONNRESET', 104),
-        ('EHOSTUNREACH', 113),
-        ('ENETUNREACH', 101),
-        ('ETIMEDOUT', 110),
-        # socket.create_connection() fails randomly with
-        # EADDRNOTAVAIL on Travis CI.
-        ('EADDRNOTAVAIL', 99),
     ]
     default_gai_errnos = [
-        ('EAI_AGAIN', -3),
-        ('EAI_FAIL', -4),
-        ('EAI_NONAME', -2),
-        ('EAI_NODATA', -5),
-        # Encountered when trying to resolve IPv6-only hostnames
-        ('WSANO_DATA', 11004),
     ]
 
     denied = ResourceDenied("Resource %r is not available" % resource_name)

Only test_timeout failed because of my patch. So it's good.

vstinner · 2019-02-21T17:27:49Z

Lib/test/test_urllibnet.py

@@ -24,8 +24,8 @@ def tearDown(self):
        socket.setdefaulttimeout(None)

    def testURLread(self):
-        with support.transient_internet("www.example.com"):
-            f = urllib.request.urlopen("http://www.example.com/")
+        with support.transient_internet("www.pythontest.net"):


Why not using support.TEST_HTTP_URL here?

because support.transient_internet uses a hostname and not scheme+hostname.

for that, I have introduced support.TEST_HOSTNAME

vstinner · 2019-02-21T17:28:14Z

Misc/NEWS.d/next/Tests/2019-02-19-18-30-40.bpo-36019._hBHx7.rst

@@ -0,0 +1,2 @@
+Replace http://example.com by http://pythontest.net for some tests.
+Contributed by Stéphane Wirtel


One NEWS entry is enough. Please merge both NEWS entries.

…urllibnet

vstinner · 2019-02-22T00:13:24Z

Lib/test/test_urllibnet.py

@@ -24,8 +24,8 @@ def tearDown(self):
        socket.setdefaulttimeout(None)

    def testURLread(self):
-        with support.transient_internet("www.example.com"):
-            f = urllib.request.urlopen("http://www.example.com/")
+        with support.transient_internet(support.TEST_HOSTNAME):


Would it be possible to extract the hostname from the URL rather than introducing another constant? It would avoid the risk of avoid 2 constants which would be inconsistent (if 1 is updated but not the other).

urllib.parse has tools for that.

yep, we can do it. I am going to fix it.

matrixise · 2019-02-22T13:01:01Z

@vstinner updated, when you have time.

vstinner

LGTM.

vstinner · 2019-02-22T13:44:35Z

Lib/test/test_urllibnet.py

@@ -24,8 +25,9 @@ def tearDown(self):
        socket.setdefaulttimeout(None)

    def testURLread(self):
-        with support.transient_internet("www.example.com"):
-            f = urllib.request.urlopen("http://www.example.com/")
+        domain = urllib.parse.urlparse(support.TEST_HTTP_URL).netloc


miss-islington · 2019-02-22T13:45:40Z

Thanks @matrixise for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2019-02-22T13:45:50Z

GH-11989 is a backport of this pull request to the 3.7 branch.

…pythonGH-11941) (cherry picked from commit a40681d) Co-authored-by: Stéphane Wirtel <[email protected]>

…GH-11941) (cherry picked from commit a40681d) Co-authored-by: Stéphane Wirtel <[email protected]>

…k tests (pythonGH-11941)

GH-12177) Use test_support.TEST_HTTP_URL (pythontest.net) instead of http://www.example.com/.

bpo-36019: Use pythontest.net instead of example.com

7204921

the-knights-who-say-ni added the CLA signed label Feb 19, 2019

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Feb 19, 2019

matrixise added 2 commits February 19, 2019 18:30

Add blurb entry

5f726be

Fix test_urllibnet

039455e

matrixise changed the title ~~WIP: bpo-36019: Use pythontest.net instead of example.com~~ bpo-36019: Use pythontest.net instead of example.com Feb 19, 2019

eamanu approved these changes Feb 20, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Feb 20, 2019

Add test.support.TEST_HTTP_URL for the tests

e885ac7

vstinner requested changes Feb 21, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label Feb 21, 2019

bedevere-bot added the awaiting changes label Feb 21, 2019

matrixise added 4 commits February 21, 2019 15:31

Fix a test

689fdc6

Revert the changes in the tests with pythontest.net/unicode/

a507631

Fix typo with TEST_HTTP_URL

b1c00eb

Remove the ReST role

8fec9c6

vstinner requested changes Feb 21, 2019

View reviewed changes

matrixise added 3 commits February 21, 2019 17:12

Fix with the recommenation of Victor Stinner

e90553c

Remove useless f-string

6559361

Use http://www.pythontest.net

37b5de3

matrixise added 2 commits February 21, 2019 17:39

Remove useless modifications for the review

25bf689

Reset TEST_HTTP_URL to http://www.pythontest.test - sorry it's my fau…

5329286

…lt maybe I am tired :/

vstinner reviewed Feb 21, 2019

View reviewed changes

matrixise added 2 commits February 21, 2019 20:16

Update with the remarks of Victor Stinner

c062224

Define TEST_HOSTNAME and use it for TEST_HTTP_URL and use it in test_…

cfafca6

…urllibnet

vstinner reviewed Feb 22, 2019

View reviewed changes

matrixise added 2 commits February 22, 2019 13:53

Revert the commit with the introduction of support.TEST_HOSTNAME

33a4744

Compute the domain from supoort.TEST_HTTP_URL

e4f8128

vstinner approved these changes Feb 22, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Feb 22, 2019

vstinner added the needs backport to 3.7 label Feb 22, 2019

vstinner merged commit a40681d into python:master Feb 22, 2019

bedevere-bot removed the awaiting merge label Feb 22, 2019

bedevere-bot removed the needs backport to 3.7 label Feb 22, 2019

miss-islington added a commit that referenced this pull request Feb 22, 2019

bpo-36019: Use pythontest.net instead of example.com in network tests (…

6163210

…GH-11941) (cherry picked from commit a40681d) Co-authored-by: Stéphane Wirtel <[email protected]>

matrixise added a commit to matrixise/cpython that referenced this pull request Mar 5, 2019

[2.7]: bpo-36019: Use pythontest.net instead of example.com in networ…

a50ac08

…k tests (pythonGH-11941)

vstinner pushed a commit that referenced this pull request Mar 5, 2019

[2.7] bpo-36019: Use pythontest.net in urllib network tests (GH-11941) (

84772e0

GH-12177) Use test_support.TEST_HTTP_URL (pythontest.net) instead of http://www.example.com/.

	urllib.request.urlretrieve(f'{support.TEST_HTTP_URL}/',
	urllib.request.urlretrieve(support.TEST_HTTP_URL,

		@@ -0,0 +1,2 @@
		Replace http://example.com by http://pythontest.net for some tests.
		Contributed by Stéphane Wirtel

Uh oh!

bpo-36019: Use pythontest.net instead of example.com #11941

bpo-36019: Use pythontest.net instead of example.com #11941

Uh oh!

Conversation

matrixise commented Feb 19, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Feb 19, 2019

Uh oh!

matrixise commented Feb 19, 2019

Uh oh!

eamanu left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Feb 20, 2019

Uh oh!

matrixise commented Feb 20, 2019

Uh oh!

vstinner commented Feb 20, 2019

Uh oh!

vstinner commented Feb 20, 2019

Uh oh!

matrixise commented Feb 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Feb 21, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matrixise commented Feb 22, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Feb 22, 2019

Uh oh!

bedevere-bot commented Feb 22, 2019

Uh oh!

Uh oh!

matrixise commented Feb 19, 2019 •

edited by bedevere-bot

Loading