Raise error if any version of patch is used as a context manager #168
Conversation
On pytest-dev#165, a change was added to raise an Exception when trying to use `mocker.patch.object` as a context manager, as its behavior is not supported in the plugin. However, the context managers for the remaining patch options (dict, multiple, and normal patch) still work as usual. With this change, we check if we are in a context manager when starting the patch, so all the methods will raise the exception if corresponds
Adding a new test to confirm that patch raises an exception when used as a context manager
|
Thanks @AlexGascon, just to let you know I will review this ASAP. 👍 |
|
It would have been nice that this was a WARNING only, rather than a flat out ERROR. Certainly important to cleanup this unsupported usage, but this PR & release subsequently cause breakage in otherwise unchanged code if using the latest version of pytest-mock. |
|
Hi @mcallaghan-bsm, Thanks for the feedback, but I kindly disagree: the previous usage was causing false-positives, it is better to loudly complain about it IMHO. Users unfortunately many times ignore warnings. |
|
Yes that is fair. Just saying this impacts MANY users, forces them to clean up immediately rather than at their convenience. We've already cleaned up our poor usages :) for the better. |
|
As a user myself I agree with @nicoddemus, I would definitely prefer getting an exception and knowing that what I am trying is not supported than introducing it in my codebase and not realizing the problem. Besides, it's not as if we were talking about a feature that suddenly stopped working, but about something that never worked and now is just notifying you about it. If I was using it on my tests I would definitely appreciate knowing that I'm doing something that doesn't work at all |
Good to hear. 😁
That's true, but the alternative would be to eventually turn the warning into an error, and then users who have been ignoring the warning would get the error anyway. Also users can always postpone the cleanup by pinning to <1.12 in their requirements. FTR in general I completely agree with you that introducing warnings to, well, warn about incoming changes/incompatibilities is the way to go, but in this case it was something that was really not working at all, so I think for those cases it makes sense to turn it into an error immediately. 😁 |
On #165, a change was added to raise an Exception when trying to use
mocker.patch.objectas a context manager, as its behavior is not supported in the plugin. However, the context managers for the remaining patch options (dict,multiple, and normalpatch) still work as usual.With this change, we check if we are in a context manager when starting the patch, so all the methods will raise the exception when corresponds.