-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
gh-128731: fix ResourceWarning in test_robotparser
#128746
Conversation
Fixes python#128731 Address ResourceWarnings in `Lib/test/test_robotparser.py` tests. * **Lib/tempfile.py** - Modify `_TemporaryFileCloser.__del__` to call `self.cleanup()` instead of `_warnings.warn`. - Add a `__del__` method to `_TemporaryFileWrapper` to ensure explicit cleanup of temporary files. * **Lib/test/test_robotparser.py** - Add a `tearDown` method to `NetworkTestCase` to explicitly close the parser. - Add a `tearDown` method to `PasswordProtectedSiteTestCase` to explicitly close the parser. - Update `PasswordProtectedSiteTestCase.testPasswordProtectedSite` to use `self.parser` instead of a local variable.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
parser.set_url(url) | ||
parser.read() | ||
self.assertFalse(parser.can_fetch("*", robots_url)) | ||
self.parser = urllib.robotparser.RobotFileParser() # P9de7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's "P9de7"?
@@ -364,6 +365,9 @@ def url(self, path): | |||
self.base_url, path, '/' if not os.path.splitext(path)[1] else '' | |||
) | |||
|
|||
def tearDown(self): | |||
self.parser.close() # P080b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's P080b?
@@ -334,16 +334,17 @@ def tearDown(self): | |||
self.server.shutdown() | |||
self.t.join() | |||
self.server.server_close() | |||
self.parser.close() # P9de7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
@@ -477,10 +477,7 @@ def close(self): | |||
self.cleanup() | |||
|
|||
def __del__(self): | |||
close_called = self.close_called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be removed. We want to warn when there is a resource that is not closed. What should be fixed are the tests where we should explicitly close the corresponding resources.
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 |
test_robotparser
Actually, I just saw that there was already a PR for this one: #128733. Please do not duplicate PRs. I'm going to close this. Please read https://devguide.python.org/getting-started/pull-request-lifecycle/ beforehand as well. |
Fixes #128731
Address ResourceWarnings in
Lib/test/test_robotparser.py
tests.Lib/tempfile.py
_TemporaryFileCloser.__del__
to callself.cleanup()
instead of_warnings.warn
.__del__
method to_TemporaryFileWrapper
to ensure explicit cleanup of temporary files.Lib/test/test_robotparser.py
tearDown
method toNetworkTestCase
to explicitly close the parser.tearDown
method toPasswordProtectedSiteTestCase
to explicitly close the parser.PasswordProtectedSiteTestCase.testPasswordProtectedSite
to useself.parser
instead of a local variable.