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
bpo-40126: Fix reverting multiple patches in unittest.mock. #19351
bpo-40126: Fix reverting multiple patches in unittest.mock. #19351
Conversation
Patcher's __exit__() is now never called if its __enter__() is failed. Returning true from __exit__() silences now the exception.
Maybe ExitStack could be used here. |
Lib/unittest/mock.py
Outdated
patcher.__exit__(*exc_info) | ||
exit_stack = self._exit_stack | ||
del self._exit_stack | ||
return exit_stack.__exit__(*sys.exc_info()) |
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 this be passing exc_info
parameter passed to __exit__
? Is there a case where exc_info
can be different from sys.exc_info
?
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.
Good catch! Thank you.
Lib/unittest/mock.py
Outdated
patcher.__exit__(*exc_info) | ||
exit_stack = self._exit_stack | ||
del self._exit_stack | ||
return exit_stack.__exit__(*sys.exc_info()) |
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.
Good catch! Thank you.
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
Sorry @serhiy-storchaka, I had trouble checking out the |
Thanks for the refactor. Can these cases be tested to catch them in future? |
@serhiy-storchaka - I have to be honest, bit disappointed to see changes like these going in without unit tests to verify their behaviour. Would you be able to put in another PR to add these tests? |
I agree with you, but it is very hard to write any realistic tests for subtle behavior changes caused by this PR. I'll try more, but I don't have any tests yet. At least the code looks now clearer and the existing tests are passed. |
…thonGH-19351) Patcher's __exit__() is now never called if its __enter__() is failed. Returning true from __exit__() silences now the exception.. (cherry picked from commit 4b222c9) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
GH-19483 is a backport of this pull request to the 3.8 branch. |
GH-19484 is a backport of this pull request to the 3.7 branch. |
…thonGH-19351) Patcher's __exit__() is now never called if its __enter__() is failed. Returning true from __exit__() silences now the exception.. (cherry picked from commit 4b222c9) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
|
@serhiy-storchaka - what is the case that is difficult to test? I've had a quick scan of the issue, and neither the original bug report, around something attempting to clear patchings (just have the unit test to the same thing), nor your observation that |
Patcher's
__exit__()
is now never called if its__enter__()
is failed.Returning true from
__exit__()
silences now the exception.https://bugs.python.org/issue40126