pytest-mock 1.13.0: catching side-effects breaks spy

[k4nar]

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.

[nicoddemus]

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?

[k4nar]

Yes, that sounds like a nice solution 👍 .

[nicoddemus]

@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.

[inderpreet99]

@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!

[nicoddemus]

Fixed in 2.0.0 🎉