Skip to content
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

Raise an error if mocker is used in a with context #165

Merged
merged 2 commits into from Oct 23, 2019
Merged

Raise an error if mocker is used in a with context #165

merged 2 commits into from Oct 23, 2019

Conversation

binarymason
Copy link
Contributor

Ref: #164

@nicoddemus nicoddemus merged commit 19a4e4d into pytest-dev:master Oct 23, 2019
@nicoddemus
Copy link
Member

Thanks a lot @binarymason!

1.11.2 released with this. 👍

@stefanvanburen
Copy link

fwiw, the CHANGELOG / release notes say TypeError while it looks like this actually raises a ValueError.

Also, I feel like this should probably catch situations in which multiple context managers are used?

with context_manager_1, mocker.patch(...):

Thoughts? Might be harder to detect.

@nicoddemus
Copy link
Member

fwiw, the CHANGELOG / release notes say TypeError while it looks like this actually raises a ValueError.

Oh right, I will update the CHANGELOG thanks

Thoughts? Might be harder to detect.

I thought about that too, but I couldn't come up with a good way to detect that too. In the end though I think this is fine, this will probably catch 99% of the mistakes. 👍

Thanks again!

AlexGascon added a commit to AlexGascon/pytest-mock that referenced this pull request Oct 30, 2019
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
@liamdawson
Copy link

Feedback: whilst presumably for a good reason, this did introduce a breaking change for a rarely-built project of ours.

@nicoddemus
Copy link
Member

Thanks @liamdawson, can you share more details? This error was introduced because otherwise you would get a criptic message otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants