bpo-37421: test_urllib calls urlcleanup()

[vstinner]

urlretrieve_HttpTests tests now explicitly call urlcleanup() to
remove temporary files.

https://bugs.python.org/issue37421

[tirkarthi]

LGTM. It seems test_urllibnet already uses a wrapper to urlretrieve to delete temporary files automatically. Thanks.

[vstinner]

@tirkarthi: Would you mind to review the updated PR?

LGTM. It seems test_urllibnet already uses a wrapper to urlretrieve to delete temporary files automatically. Thanks.

See also https://bugs.python.org/issue37475 "What is urllib.request.urlcleanup() function?" In this issue, I also discovered that urlopen() sets a "_opener" global variable and so have a side effect on following tests. I modified my PR to test in regrtest that urllib.requests._url_tempfiles and urllib.requests._opener are not modified, and I modified way more tests to call urlcleanup().

urlopen() and urlretrieve() side effects are really surprising, but let's discuss that in https://bugs.python.org/issue37475

[tirkarthi]

If I understand regrtest correctly, clear_caches is called before save_test_environment is called where urllib.request.cleanup is called by clear_caches at

cpython/Lib/test/libregrtest/refleak.py

Line 223 in 61bf97e

urllib_request.urlcleanup()

. So essentially all the tests should start with urllib.request._url_tempfiles as empty due to clear_caches and by restore_urllib_requests__url_tempfiles it will just be looping over an empty list? Similar thing with _opener being set as None. Is it worth doing the changes using get and restore?

[vstinner]

clear_caches() is not used by default. It's only used when running tests with -R 3:3.

Is it worth doing the changes using get and restore?

It allows to detect bugs in tests. Try to modify my PR to remove some calls to urlcleanup() and run modified tests, you will see tests failing with ENV_CHANGED. Pass --fail-env-changed option to make the command fail.

[vstinner]

save_test_environment() helps to detect side effects of unit tests. Unit tests should have no side effect.

[tirkarthi]

Okay, I just grepped for save_test_environment and saw only usage at runtest.py where clear_caches was always called before save_test_environment at

cpython/Lib/test/libregrtest/runtest.py

Line 263 in 039fb49

with saved_test_environment(test_name, ns.verbose, ns.quiet, pgo=ns.pgo) as environment:

.

[tirkarthi]

One more note that I found is that test_urllib has it's own urlopen function defined at the top level and sets global variable _opener at the top as a shortcut to use FancyURLOpener. There are interchanged usages of urllib.request.urlopen and urlopen which is slightly confusing to read the tests :(

Also I just commented out the part in urlcleanup where _opener is set as None and could see no test failures in test_url* related tests. So I think there is no test to actually see urlcleanup is actually working as intended to set urllib.request._opener as None. Maybe a test like below could help?

def test_url_cleanup_opener(self):
    url = 'http://docs.python.org/library/urllib.html'
    self.fakehttp(b"HTTP/1.1 200 OK\r\n\r\nHello!")
    try:
        fp = urllib.request.urlopen(url)
        self.assertIsInstance(urllib.request._opener, urllib.request.OpenerDirector)
        urllib.request.urlcleanup()
        self.assertIsNone(urllib.request._opener)
    finally:
        self.unfakehttp()

[vstinner]

Okay, I just grepped for save_test_environment and saw only usage at runtest.py where clear_caches was always called before save_test_environment at (...)

Oh ok, I didn't know. Well, my save_env.py change is still relevant.

One more note that I found is that test_urllib has it's own urlopen function defined at the top level and sets global variable _opener at the top as a shortcut to use FancyURLOpener.

Oh wow, that's super weird. Why test_urllib tests is test urlopen() function rather than testing urllib.request module?! I propose to address this issue in a separated PR and/or in https://bugs.python.org/issue37475

Also I just commented out the part in urlcleanup where _opener is set as None and could see no test failures in test_url* related tests.

I tried this change on top of my PR:

diff --git a/Lib/urllib/request.py b/Lib/urllib/request.py
index f6ce9cb6d5..2668115f1c 100644
--- a/Lib/urllib/request.py
+++ b/Lib/urllib/request.py
@@ -298,9 +298,9 @@ def urlcleanup():
             pass
 
     del _url_tempfiles[:]
-    global _opener
-    if _opener:
-        _opener = None
+    #global _opener
+    #if _opener:
+    #    _opener = None
 
 # copied from cookielib.py
 _cut_port_re = re.compile(r":\d+$", re.ASCII)

The test fails as expected:

$ ./python -m test test_urllib --fail-env-changed
Run tests sequentially
0:00:00 load avg: 0.57 [1/1] test_urllib
Warning -- urllib.requests._opener was modified by test_urllib
  Before: None
  After:  <urllib.request.OpenerDirector object at 0x7f50115bf1e0> 
test_urllib failed (env changed)

== Tests result: ENV CHANGED ==

1 test altered the execution environment:
    test_urllib

Total duration: 488 ms
Tests result: ENV CHANGED

[vstinner]

@berkerpeksag: Thanks for the review. I merged my PR to unblock https://bugs.python.org/issue37421

It seems like there are other issues which can be addressed separately.

[tirkarthi]

Thanks Victor, I tried without fail-env-changed and hence the confusion over expecting an assertion error.