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

given's interaction with mock.patch decorator is broken in almost every way #198

Closed
DRMacIver opened this issue Oct 12, 2015 · 5 comments · Fixed by #1611
Closed

given's interaction with mock.patch decorator is broken in almost every way #198

DRMacIver opened this issue Oct 12, 2015 · 5 comments · Fixed by #1611
Assignees
Labels
bug something is clearly wrong here

Comments

@DRMacIver
Copy link
Member

DRMacIver commented Oct 12, 2015

@waynew is trying to use unitest.mock's patch decorator together with Hypothesis's given and py.test's fixtures. He's not having a very good time of it, which is unsurprising because this combination almost entirely doesn't work.

The first way it goes wrong:

You can mostly use mock.patch with given if you don't care about py.test fixtures. For example, the following will work:

import thing
import pytest
import hypothesis.strategies as st
from hypothesis import given
from unittest import mock

@mock.patch('thing.fnord')
@given(thing=st.text())
def test_a_thing(fnord, thing):
    pass

But if you try to add an extra named argument then everything will go wrong: Hypothesis will expose something with just _args and *_kwargs because that's what mock.patch gives it, so py.test won't be able to find the fixture.

It's unclear that there is a correct thing Hypothesis could do at this point. mock.patch is using functools.wrap, so in Python 3 we could propagate the wrapped attribute, but AFAIK there's no equivalent in Python 2. Additionally, note that the signature of the wrapped function is not correct, because the first arguments are filled in manually. Historically, propagating wrapped has broken py.test (pytest-dev/pytest#677), but this may be affected differently now?

Anyway: Can of worms, unclear there's a correct thing to do here.

The second way it goes wrong is more amusing and is probably fixable (update: was fixed in #779), or at least can be mitigated:

import thing
import pytest
import hypothesis.strategies as st
from hypothesis import given
from unittest import mock

@given(thing=st.text())
@mock.patch('thing.fnord')
def test_a_thing(fnord, thing):
    assert False

This test passes.

Why does this test pass? It passes because Hypothesis can't currently tell the difference between being called as a bound method and just having an argument explicitly passed to it as the first positional argument. This means that it thinks that the mock is self and begins the shenanigans for the executor API. It looks for a method called execute_example, finds it, and then calls it with the test function.

It then of course immediately returns, never executing the test.

Possible ways to work around:

  • Have given return a descriptor, so that we can actually detect if it's been bound as a method.
  • Explicitly check for being passed a mock instance as self (I don't want to do this one, but it would work)
  • Tell people not to do this, or to use autospec when using patch.
@DRMacIver DRMacIver added the bug something is clearly wrong here label Oct 12, 2015
@DRMacIver
Copy link
Member Author

Note: Let me know if you really really care about this bug, because the combination of "good workarounds", "all the fixes are awful and/or partial" and "I don't actually like mocking" means that unless this is actually bothering people I am very unmotivated to work on it.

@dangitall
Copy link

Thanks for the details on this. Fixing this is not really important to me, I don't think the workarounds are burdensome at all. I've been using pytest and hypothesis for years and this has never been an issue before. I was just perplexed at the behavior but I understand the reasons it happens better now.

@killthrush
Copy link
Contributor

killthrush commented Aug 9, 2017

@DRMacIver I ran into this same issue (i.e. the second of the two reported problems) when injecting mocks using pytest.fixture instead of mock.patch. Specifically, when mocks appear first in the arg list, they will be misidentified as self. If you have other non-mock args that appear in the arg list first, you'll not see the problem. I independently verified the same behavior under the hood as the OP. Though there are decent workarounds, I'm inclined to at least take a stab at a PR to fix this. Having your tests return false positives, possibly MANY false positives that are hard to see is a serious and insidious issue IMO.

By hacking some code in executors.py, I was able to avoid the bug. It's ghetto as all hell, literally checking the type string looking for mocks. If you're OK with doing this sort of stuff, I'd be happy to roll up my sleeves and try making a few tests and fixing this. I really like hypothesis, and I really like mocks and fixtures so I'm suitably motivated =) If you have any suggestions on other code that might be relevant let me know.

Thanks!

@DRMacIver
Copy link
Member Author

If you're OK with doing this sort of stuff, I'd be happy to roll up my sleeves and try making a few tests and fixing this.

Hypothesis is full of hacky workarounds for weird behaviour, so no objections on that front. :-) Very happy to accept a pull request for this.

The other thing I thought about but never got around to investigating as a way of detecting mocks is to check for the presence of an arbitrary attribute. e.g. if hasattr(selfy, 'hypothesis_internal_this_attribute_should_not_exist') is True then it's probably a mock, or something sufficiently mocklike that it will cause the same problems. Might be worth trying?

A thing worth noting BTW is that pytest fixtures also interact unexpectedly with Hypothesis (function level fixtures are once for the whole test, not once per example), so you may run into problems on that front too. Fixing this one is a rather larger task though (or requires work on pytest's side).

@killthrush
Copy link
Contributor

Alright @DRMacIver , so I made a pull request to address a portion of this issue: #779. As you mentioned, this is not really going to do much in relation to the other challenges re: fixtures, but it's an improvement nonetheless. I wonder - should this bug (198) be broken down into smaller issues? Seems kind of overwhelming in its current form due to its scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is clearly wrong here
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants