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

Parameterized doens't play nicely with unittest.mock.patch decorator #66

Open
thejcannon opened this issue Feb 4, 2019 · 23 comments
Open
Labels
bug needs-fix Good PR with failing test case, needs a fix implemented

Comments

@thejcannon
Copy link

thejcannon commented Feb 4, 2019

Howdy!

I've found that mixing Python's unittest.mock.patch decorator with parameterized.expand leads to unfortunate side-effects.

@parameterized.expand([("x", "y"),])
@patch("foo")
def test_bar(foo, x, y):
   pass

Will lead to UnboundLocalError: local variable 'patching' referenced before assignment (at unittest\mock.py:1181).
I'm sure this is due to the nature of how expand generates test cases.

There's a workaround, which is use patch as a context manager in the test body. However, I'd still love to use both as decorators for all that delicious syntactic sugar and saved whitespace 😄.

@thejcannon
Copy link
Author

Well this parametrized test which tests patch certainly makes my situation a puzzler.

@thejcannon
Copy link
Author

thejcannon commented Feb 4, 2019

I think the issue I'm running into only occurs if the test raises an exception.
So really the bug just makes tracking down the cause of failing tests a bit more challenging.

Edit: And looks like the root cause of the exception raised (in my instance) was the parameter ordering (foo should've been last).

@wolever wolever closed this as completed in a940fcd Feb 6, 2019
@wolever
Copy link
Owner

wolever commented Feb 6, 2019

Good to know, thanks! That seems like a common stumbling block, so I've updated the documentation to include an example.

@ltsampros
Copy link

ltsampros commented Mar 10, 2019

I think this is still an issue as I can reproduce it even with the correct ordering of the variable.

@parameterized.expand([("x", "y"),])
@patch("foo")
def test_bar(x, y, foo):
   assertEqual(1,2)

As expected it's only reproducible with py2 using the mocks backport. py3 works fine.

@korcek-juraj
Copy link

I have the same issue with v0.7.0.

The setup is following:

@parameterized.expand([("x", "y"),])
@patch("internal.lib.timezone_at", new=mock_timezone_at('Europe/Zurich'))
def test_bar(x, y):
   assertEqual(1,2)

If context manager is used instead, everything works:

@parameterized.expand([("x", "y"),])
def test_bar(x, y):
   with patch("internal.lib.timezone_at", new=mock_timezone_at('Europe/Zurich')):
       assertEqual(1,2)

In other words, UnboundLocalError is thrown instead of the actual exception when patch is used as decorator together with parameterized.

@wolever wolever reopened this Mar 15, 2019
@wolever
Copy link
Owner

wolever commented Mar 15, 2019

Hrm, I've added a test case for this, but I can't seem to recreate the issue: 85222cf

I've also tried to reproduce it by raising an explicit AssertionError in the test, and I still see the correct stack trace:

FAIL: parameterized.test.test_mock_patch_standalone_function(42, {})
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/wolever/code/parameterized/.tox/py27-nose/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/wolever/code/parameterized/parameterized/parameterized.py", line 387, in <lambda>
    nose_func = wraps(func)(lambda *args: func(*args[:-1], **args[-1]))
  File "/Users/wolever/code/parameterized/.tox/py27-nose/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/Users/wolever/code/parameterized/parameterized/test.py", line 207, in test_mock_patch_standalone_function
    raise AssertionError("foo")
AssertionError: foo

Could you try running this test with pip install tox then tox -e py27-nose (or replace nose with whatever testrunner you use) and let me know if you can reproduce it?

I'm wondering if it's an issue with a particular version of mock, or something similar.

@korcek-juraj
Copy link

The new test passed with pytest3, however, I managed to recreate the same issue by adding

    @parameterized.expand([(42,)])
    @mock.patch('os.umask', new=lambda x: x)
    def test_mock_patch_standalone_function2(foo):
        raise ValueError()

into TestParameterizedExpandWithNoMockPatchForClass TestCase.

@wolever
Copy link
Owner

wolever commented Mar 18, 2019

@korcek-juraj hrm… I'm still not able to reproduce the issue. When I use the example you've included, I get the correct stack trace:

============================================================================== FAILURES ==============================================================================
_______________________________________________________________ test_mock_patch_standalone_function[0] _______________________________________________________________

foo = 42

    @parameterized([(42, )])
    @mock.patch("os.umask", new=some_func)
    def test_mock_patch_standalone_function(foo):
>       raise ValueError()
E       ValueError

parameterized/test.py:210: ValueError
============================================================ 1 failed, 86 passed, 1 error in 0.37 seconds ============================================================
ERROR: InvocationError for command '/Users/wolever/code/parameterized/.tox/py27-pytest2/bin/py.test parameterized/test.py' (exited with code 1)
@parameterized([(42, )])
@mock.patch("os.umask", new=lambda x: x)
def test_mock_patch_standalone_function(foo, mock_umask):
    raise ValueError()

To help me debug this further, could you:

  1. Add the test case which is failing
  2. Run it with tox -e py27-nose (or whatever the test runner you're using is)
  3. Run .tox/py27-nose/bin/pip freeze and post the output?

Mine is:

$ .tox/py27-nose/bin/pip freeze
funcsigs==1.0.2
mock==2.0.0
nose==1.3.7
parameterized==0.7.0
pbr==5.1.1
six==1.12.0

@korcek-juraj
Copy link

@wolever When I run the test case given by you using tox -e py27-pytest3 I get:

Exception: Warning: '@parameterized' tests won't work inside subclasses of 'TestCase' - use '@parameterized.expand' instead.

When I change @parameterized to @parameterized.expand I get the original error:

UnboundLocalError: local variable 'patching' referenced before assignment

Output of .tox/py27-pytest3/bin/pip freeze is:

atomicwrites==1.3.0
attrs==19.1.0
funcsigs==1.0.2
mock==2.0.0
more-itertools==5.0.0
nose==1.3.7
parameterized==0.7.0
pathlib2==2.3.3
pbr==5.1.3
pluggy==0.9.0
py==1.8.0
pytest==3.10.1
scandir==1.10.0
six==1.12.0

Do you want me to create PR adding the test that is failing?

@wolever
Copy link
Owner

wolever commented Mar 19, 2019

Yes, a PR would be helpful, thanks!

korcek-juraj pushed a commit to korcek-juraj/parameterized that referenced this issue Mar 20, 2019
@korcek-juraj
Copy link

Okay, PR #72 created. In Travis you can see that most of the builds fail with UnboundLocalError.

@floer32
Copy link

floer32 commented Mar 20, 2019

Slightly tangential but could be relevant if there are doc updates here

Personally, I've had so many problems with brittleness in mock-patching, vs. rock-solid results from pytest's monkeypatch. They're not the same and sometimes mock is really what's needed (for checking called-with and so on). Many times the job is just to get some I/O out of the way and monkeypatch works.

So it may be worth noting that for pytest users, monkeypatch is a choice to consider to avoid the brittleness. https://docs.pytest.org/en/latest/monkeypatch.html

@wolever
Copy link
Owner

wolever commented Mar 20, 2019

Oof, okay, I see the issue now. This is a good bug - nice find! And thanks for taking the time to submit the PR.

Basically, the bug is happening because the patches are being copied to each expanded function by @functools.wraps(…). This means the patchings are also being copied, which is undesirable (ie, because they will be called twice).

Would you be able to take a look the potential fix I've push'd and see if it works for you?

Running the test suite shows the expected failure.

@korcek-juraj
Copy link

I have tested it by running the test suite as well as the tests in my code and I confirm that it indeed works now. Thanks a lot for fixing it!

@korcek-juraj
Copy link

@hangtwenty thanks for the insight

@dvirtz
Copy link

dvirtz commented Nov 27, 2019

I still see this error in 0.7.1. Hasn't it been fixed already?

@dvirtz
Copy link

dvirtz commented Nov 27, 2019

Can you please merge #72 and cut out a new release?

@justinbricker-eb
Copy link

Is this still unresolved?

@atleta
Copy link

atleta commented Jan 30, 2020

It seems so. I've just ran into it with 0.7.1 (Params are in the correct order: mock last)

@wolever wolever added bug needs-fix Good PR with failing test case, needs a fix implemented labels Apr 12, 2020
@wolever
Copy link
Owner

wolever commented Apr 12, 2020

Unfortunately this is still unresolved. The PR in question - #72 - is just a demonstration of the bug, not an actual fix.

@StefanoChiodino
Copy link

StefanoChiodino commented Apr 14, 2020

My workaround was to use patch as a context manager:

with patch('my_calendar.requests') as mock_requests:

@thejcannon
Copy link
Author

thejcannon commented Apr 14, 2020

I should mention I work around it by using pytest-mock which has other nicities.

@adamchainz
Copy link
Contributor

This was a bug in unittest.mock, addressed in https://bugs.python.org/issue40126 . Although the linked issue says it was only fixed for Python 3.7+, I have found the test from #72 passes on Python 3.6.10 as well. It still fails on Python 2.7.17 , so I guess the mock backport doesn't have the fix yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs-fix Good PR with failing test case, needs a fix implemented
Projects
None yet
Development

No branches or pull requests

10 participants