-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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-59215: unittest: _top_level_dir is incorrectly persisted #15242
gh-59215: unittest: _top_level_dir is incorrectly persisted #15242
Conversation
Co-Authored-By: Igor Sobreira <igor@igorsobreira.com>
Lib/unittest/test/test_discovery.py
Outdated
sys.path[:] = original_sys_path | ||
self.addCleanup(restore) | ||
|
||
os.path.isfile = lambda path: True |
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.
Is unittest.mock.patch
a better option here instead of addCleanup
?
def test_discover_should_not_persist_top_level_dir_between_calls(self):
with unittest.mock.patch('os.path.isfile', return_value=True):
with unittest.mock.patch('os.path.isdir', return_value=True):
loader = unittest.TestLoader()
loader.suiteClass = str
dir = '/foo/bar'
top_level_dir = '/foo'
loader.discover(dir, top_level_dir=top_level_dir)
self.assertEqual(loader._top_level_dir, None)
loader._top_level_dir = dir2 = '/previous/dir'
loader.discover(dir, top_level_dir=top_level_dir)
self.assertEqual(loader._top_level_dir, dir2)
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.
Nowadays that can be self.enterContext
. But, as is, the PR is consistent with other nearby tests; if this needs changing it would be best to change all of them (in a separate PR).
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.
IMO, this is the right solution, even if _top_level_dir
isn't perfect.
However, as evidenced by the changed test, this is a change in behaviour. IMO, it needs at least a versionchanged
note in the docs, saying that top_level_dir
is only stored for the duration of the discover
call.
I've sent proposed docs changes as a PR to this PR, here: https://github.com/ZackerySpytz/cpython/pull/39/files [now merged here]
If an unittest
expert has time to look over them, that would be great :)
Lib/unittest/test/test_discovery.py
Outdated
sys.path[:] = original_sys_path | ||
self.addCleanup(restore) | ||
|
||
os.path.isfile = lambda path: True |
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.
Nowadays that can be self.enterContext
. But, as is, the PR is consistent with other nearby tests; if this needs changing it would be best to change all of them (in a separate PR).
Thanks @ZackerySpytz for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
…ythonGH-15242) (cherry picked from commit fc5f68e) Co-authored-by: Zackery Spytz <zspytz@gmail.com>
…ythonGH-15242) (cherry picked from commit fc5f68e) Co-authored-by: Zackery Spytz <zspytz@gmail.com>
GH-117508 is a backport of this pull request to the 3.12 branch. |
GH-117509 is a backport of this pull request to the 3.11 branch. |
Co-Authored-By: Igor Sobreira igor@igorsobreira.com
https://bugs.python.org/issue15010