Serious performance issues from 1.12 and up

[wesleykendall]

The addition of this line (6587f79) that checks if one is inside a context manager caused our test suite to run over 3x slower (5 minutes to 17 minutes).

I understand the desire to report a more useful error message when a user tries to use it as a context manager, but the stack inspection on every patch causes a slowdown that makes pytest-mock unusable for us, especially since our test suite has hundreds of test files. Our observations indicate that the performance degradation is even more pronounced when using pytest-xdist.

One can observe the performance impacts in a simple test example:

In a test.py file:

def test_foo(mocker):
    mocker.patch('random.randint')

Install pytest-repeat and pytest-mock==1.12.0

pytest test.py --count 1000

total runtime = 4.45s

Install pytest-mock==1.11.2

pytest test.py --count 1000

total runtime = 2.28s

Now do the same experiment with pytest-xdist

with 1.12:

pytest test.py --count 1000 -n 4

total runtime = 6.67s

with 1.11.2:

pytest test.py --count 1000 -n 4

total runtime = 3.18s

Although this test case doesn't have performance impacts as pronounced as our test suite, which has hundreds of files and tests thousands of python files, it is still upwards of a 2x slowdown.

Can this check be removed altogether? IMO it is not worth it to slow down a test suite this much for a more useful error message in a few rare circumstances. Would it be possible to instead try to catch the unhelpful error message and then re-raise it as something helpful instead if this check is really aiding in the UX of the library? Thanks for your consideration

[tomage]

Seconded!

How about we actually make mocker into a context manager, but just raise that exception in the __enter_() method?

I suppose the only downside is that it having an __enter__() method might make it look as-if it is indeed a context manager.. but I suspect that it'd at least beat the performance of using inspect to look to see if with mocker. is in the code.

[wesleykendall]

I'm happy to open up a PR for the suggestion from @tomage. I'm assuming the intention is to prevent using it as a context manager. If the authors can verify this, I believe this will be an appropriate solution that doesn't change UX

[nicoddemus]

Thanks @wesleykendall and @tomage for writing!

Completely agree this small feature is not worth the large slowdown.

I've opened a PR with @tomage's suggestion and it seems to do the trick: #192

Would appreciate a review of you both if possible before releasing.

FTR I'm OK with removing this if #192 introduces then a regression for someone else. 👍