Skip to content

bpo-37421: test_urllib calls urlcleanup() #14529

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

Merged
merged 1 commit into from
Jul 2, 2019
Merged

bpo-37421: test_urllib calls urlcleanup() #14529

merged 1 commit into from
Jul 2, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 1, 2019

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

https://bugs.python.org/issue37421

Copy link
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

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

@vstinner vstinner requested a review from berkerpeksag as a code owner July 2, 2019 10:37
urllib.request tests now call urlcleanup() to remove temporary files
created by urlretrieve() tests and to clear the _opener global
variable set by urlopen() and functions calling indirectly urlopen().

regrtest now checks if urllib.request._url_tempfiles and
urllib.request._opener are changed by tests.
@vstinner
Copy link
Member Author

vstinner commented Jul 2, 2019

@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
Copy link
Member

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

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
Copy link
Member Author

vstinner commented Jul 2, 2019

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
Copy link
Member Author

vstinner commented Jul 2, 2019

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

@tirkarthi
Copy link
Member

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

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

@tirkarthi
Copy link
Member

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
Copy link
Member Author

vstinner commented Jul 2, 2019

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 vstinner merged commit 7cb9204 into python:master Jul 2, 2019
@vstinner vstinner deleted the urlcleanup branch July 2, 2019 12:50
@vstinner
Copy link
Member Author

vstinner commented Jul 2, 2019

@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
Copy link
Member

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

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
urllib.request tests now call urlcleanup() to remove temporary files
created by urlretrieve() tests and to clear the _opener global
variable set by urlopen() and functions calling indirectly urlopen().

regrtest now checks if urllib.request._url_tempfiles and
urllib.request._opener are changed by tests.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
urllib.request tests now call urlcleanup() to remove temporary files
created by urlretrieve() tests and to clear the _opener global
variable set by urlopen() and functions calling indirectly urlopen().

regrtest now checks if urllib.request._url_tempfiles and
urllib.request._opener are changed by tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants