-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
pytester: use monkeypatch.chdir() for dir changing
#11315
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
Conversation
| #: the method using them so refer to them for details. | ||
| self.plugins: List[Union[str, _PluggyPlugin]] = [] | ||
| self._cwd_snapshot = CwdSnapshot() | ||
| self._sys_path_snapshot = SysPathsSnapshot() |
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.
should we log a followup to ensure sys.path/sys.modules handling in coordination with monkeypatch
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.
Might be possible but some complications:
SysPathSnapshotalso savessys.meta_path, monkeypatch doesn't.- Monkeypatch doesn't save
sys.pathby default, though pytester can probably force it to do it SysModulesSnapshothas no equivalent in monkeypatch currenty as far as know. Also has somepreserve_modulesexclusion...
a9a1fd6 to
5670792
Compare
|
Updated; there were some calls to |
| last_reprtb = last_p.repr_traceback_entry(excinfo.traceback[-1], excinfo) | ||
| last_lines = last_reprtb.lines | ||
| monkeypatch.undo() | ||
| with monkeypatch.context() as mp: |
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.
Because others use pytester, I think it would be worth adding a trivial changelog explaining this change, in case others find/have the same pattern in their test suites.
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.
Agreed - added a changelog.
The current method as the following problem, described by Sadra Barikbin: The tests that request both `pytester` and `monkeypatch` and use `monkeypatch.chdir` without context, relying on `monkeypatch`'s teardown to restore cwd. This doesn't work because the following sequence of actions take place: - `monkeypatch` is set up. - `pytester` is set up. It saves the original cwd and changes it to a new one dedicated to the test function. - Test function calls `monkeypatch.chdir()` without context. `monkeypatch` saves cwd, which is not the original one, before changing it. - `pytester` is torn down. It restores the cwd to the original one. - `monkeypatch` is torn down. It restores cwd to what it has saved. The solution here is to have pytester use `monkeypatch.chdir()` itself, then everything is handled correctly.
5670792 to
81192ca
Compare
|
Should we deprecate monkeypatch.undo as public api now that . context is available |
|
We did add a note warning about its use some time ago. I don't know if we can entirely deprecate it because it doesn't have a replacement if someone wants to undo their own |
|
@bluetech customized contextmanager undo is controlled via |
The current method as the following problem, described by @sadra-barikbin in #11236 (comment):
The tests that request both
pytesterandmonkeypatchand usemonkeypatch.chdirwithout context, relying onmonkeypatch's teardown to restore cwd. This doesn't work because the following sequence of actions take place:monkeypatchis set up.pytesteris set up. It saves the original cwd and changes it to a new one dedicated to the test function.monkeypatch.chdir()without context.monkeypatchsaves cwd, which is not the original one, before changing it.pytesteris torn down. It restores the cwd to the original one.monkeypatchis torn down. It restores cwd to what it has saved.The solution here is to have pytester use
monkeypatch.chdir()itself, then everything is handled correctly.