pytest-mock 1.13.0: catching side-effects breaks spy

Hello,

Since #173 was merged (and pytest-mock 1.13.0 released), mocker.spy can't be called successfully once a spied function raised an exception.

The issue is that mocker.spy relies on a side-effect to wrap all the calls:

pytest-mock/src/pytest_mock/plugin.py

Line 125 in 7bddcd5

result = self.patch.object(obj, name, side_effect=wrapper, autospec=autospec)

But now that we assign a new side-effect after an exception was raised, the spy will always raise the exception instead of calling the wrapper.

Here is a test case to reproduce the issue:

def test_spy_side_effect(mocker):

    class Foo:
        def bar(self, arg):
            if arg > 0:
                return arg
            raise RuntimeError("I'm an error")

    foo = Foo()
    mocker.spy(foo, 'bar')

    assert foo.bar(42) == 42
    foo.bar.assert_called_with(42)

    with pytest.raises(RuntimeError) as exc_info:
        foo.bar(-1)
    assert str(exc_info.value) == "I'm an error"
    foo.bar.assert_called_with(-1)

    # with pytest-mock 1.13.0 this will raise a RuntimeError instead of returning 21
    assert foo.bar(21) == 21
    foo.bar.assert_called_with(21)

A possible solution would be to assign the exception to result.return_value instead of result.side_effect as proposed initially in #173. However I understand that this is not perfect either.

Hi @k4nar,

Thanks for writing up, and sorry about the delay.

Hmm indeed that's unfortunate, shame we didn't detect it before.

I think we might make spy assign to a new attribute instead of side_effect, say spy_exception_raised (to make sure we don't clash with other attributes that might be in use). I've tried this locally and then the issue goes away.

What do you think?

Yes, that sounds like a nice solution 👍 .

@inderpreet99 any thoughts? The proposal is to save exceptions in a new attribute, spy_exception_raised, instead of in the side_effect attribute, because of the reasons mentioned above.

Unfortunately this will also require a 2.0 release, given that this is a breaking change. Fortunately I don't think this will affect many people as this is a feature that was just released.

@nicoddemus, I agree with using spy_exception_raised. I generally prefer these spy-special goodies in appropriately-named attributes.

As a side note, overriding return_value is also not perfectly ideal. return_value will return the MagicMock object before any calls made to the spy. Though after a spy is invoked, return_value will get overridden by our pytest-mock code. The problem with this behavior is it makes it tough to make easy-enough assertions. One would have to do something along the lines of assert type(spy.return_value) is not type(MagicMock) (or maybe check mock_calls or call_count to see if its usable) to ensure that we actually can use the return_value. Furthermore, if the spied method returns a MagicMock or if an exception is thrown, the return_value will continue to being some type of MagicMock. Gets a bit tough to reason about. I would prefer if we had spy_return_value as well on that side to make it perfectly clear.

Finally, @k4nar seems to have found the bug that kept me from implementing a spy that could catch multiple call results. Won't need to figure out much there anymore. Thanks!

Fixed in 2.0.0 🎉