pytest-mock 1.13.0: catching side-effects breaks spy
k4nar opened this issue ยท 5 comments
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
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
๐