Fix performance when raising ValueError when used as context-manager

[nicoddemus]

Fix #191

[tomage]

Say, unless I'm missing something, I thought the issue was not really that the user was mocking an object that could be used as a context manager, but rather that mocker.patch (for example) would be used as a context manager, similar how python's mock module can be used, e.g. given a non-context manager method called some_method():

with mock.patch('some_method') as mock_some_method:
    # do stuff that invokes `some_method()`
    mock_some_method.assert_called()

whereas I understood the idea with mocker was that this would be disallowed, opting rather for something like:

mock_some_method = mocker.patch('some_method')
# do stuff that invokes `some_method()`
mock_some_method.assert_called()

So I figured we could add a method like this to mocker.patch and mocker.patch.object:

    def __enter__(self):
        raise ValueError("Using mocker ... etc.")

But TBH, I haven't dug all that deep into the code so I may be wrong here.

Thoughts..?

[wesleykendall]

@tomage I think the way it is implemented in this PR is the proper way to override the __enter__ on those objects. I'm not sure if there is a way to override the base patch class and then get python's mock module to return it

@nicoddemus the code lgtm. Thanks for getting this together! I tried it out locally just to make sure I could still mock out a context manager alright.

[nicoddemus]

@tomage the object returned by mocker.patch will have its __enter__ method called when executed in a with statement. I believe this is the correct approach.

@nicoddemus the code lgtm. Thanks for getting this together! I tried it out locally just to make sure I could still mock out a context manager alright.

Sure! I will add an example to ensure this works as well and we don't regress in the future.

[nicoddemus]

Actually, @wesleykendall can you post the test you did? I want to make sure we are discussing the same thing.

[wesleykendall]

@nicoddemus basically what I did was create this:

@contextlib.contextmanager
def my_dec():
    pass


def my_func():
    with my_dec():
        pass

And then verify that I can patch my_dec without issues. I was originally (and incorrectly) thinking that you were overriding the __enter__ of the mock object and not the patcher. I thought it might accidentally break a lot of other tests. Hope that explains it

[nicoddemus]

I was originally (and incorrectly) thinking that you were overriding the enter of the mock object and not the patcher.

But I am overriding the __enter__ method of the mock object...

Can you post the "And then verify that I can patch my_dec without issues" part as well? It is exactly that part I want to see what you expect to work. 😁

[tomage]

Oh, of course - I forgot that it's the mock module that makes the patched objects be context managers, not pytest-mock. So the fix makes good sense!

Barring any issue coming from your and Wes' discussion, I'm good with this patch.

[wesleykendall]

@nicoddemus @tomage this should be safe to merge. I'll try to elaborate on some things since it is difficult to explain in text :)

Take the following code

class my_dec:
    def __enter__(self, *args, **kwargs):
        pass

    def __exit__(self, *args, **kwargs):
        pass


def my_func():
    with my_dec():
        pass

And the following test:

def test_my_func(mocker):
    mocker.patch('some_module.my_dec')
    some_module.my_func()
  

I was originally concerned that this PR would override the __enter__ for my_dec in this example, which would always result in the side effect introduced by this PR. I.e. when my_func is executed and my_dec is entered, I thought it would hit the side effect.

However, this test works fine because my_dec is instantiated in my_func, which creates a new mock instance that has an __enter__ without a side effect.

If it was possible to use the decorator like this (note the missing parens), we would have issues with it hitting the side effect of the mock:

def my_func():
    with my_dec:
        pass

This, however, is invalid and shouldn't happen in practice unless a user has a typo in their code. If a user did have this error in their code, they would inadvertently run into the side effect in this PR and have a confusing message, but I think this is less likely to happen in practice than a user trying to use the mock as a context manager.

This was the source of my initial confusion. Happy to elaborate more, but this PR lgtm!

[wesleykendall]

Thanks again for following up and helping get a fix out for the performance issues. We love using this plugin!

[nicoddemus]

Thanks again for following up and helping get a fix out for the performance issues. We love using this plugin!

Thanks, appreciate it! 😁

3.1.1 released. 👍

[iforapsy]

If it was possible to use the decorator like this (note the missing parens), we would have issues with it hitting the side effect of the mock:

def my_func():
    with my_dec:
        pass

This, however, is invalid and shouldn't happen in practice unless a user has a typo in their code. If a user did have this error in their code, they would inadvertently run into the side effect in this PR and have a confusing message, but I think this is less likely to happen in practice than a user trying to use the mock as a context manager.

This change broke one of my unit tests and I write scarcely any unit tests so this situation is not that rare. In my case, I am testing that some code acquires a threading.Lock object. The threading.Lock object is global and created only once to properly synchronize different threads running the function.

Sample system under test:

import threading

CREATION_LOCK = threading.Lock()

def create_widgets():
    with CREATION_LOCK:
        print('Creating object')

Sample test code:

def test_create_widgets_acquires_lock(mocker):
    """Test that the creation lock is acquired."""
    lock_mock = mocker.patch('foo.CREATION_LOCK', autospec=True)
    create_widgets()
    lock_mock.__enter__.assert_called()

While the documentation about usage as context manager should stay, I suggest not raising the exception at all. One of the main principles of Python is that "we're all consenting adults" so it doesn't make much sense to actively block the context manager usage.

[nicoddemus]

Hey @iforapsy,

I write scarcely any unit tests so this situation is not that rare

Not sure, I'm into automated tests for more than 15 years and I don't recall once needing to mock __enter__, but of course your mileage may vary.

it doesn't make much sense to actively block the context manager usage.

Unfortunately not blocking the context-manager usage results in things not getting un-mocked properly, confused users, and bug reports, so I believe the exception should stay.

[iforapsy]

How about changing from raising an exception to a warning, à la warnings.warn()? When someone uses the mock as a context manager, a warning would still display a visible error message in pytest, but it would not abort control flow and cause test failure. If you are open to this change, I can make a PR for that.

[nicoddemus]

How about changing from raising an exception to a warning, à la warnings.warn()? When someone uses the mock as a context manager, a warning would still display a visible error message in pytest, but it would not abort control flow and cause test failure. If you are open to this change, I can make a PR for that.

OK sounds good; a warning should at least give a chance of users to spot the misuse when it happens, and for people really wanting that behavior they can silence the warning in the test which makes use of it by adding a @pytest.mark.filterwarnings mark.