When using mock to wrap an existing object, side_effect requires return_value

[noamraph]

BPO	35330
Nosy	@cjw296, @voidspace, @lisroach, @mariocj89, @tirkarthi, @noamraph
PRs	bpo-35330: Don't call the wrapped object if side_effect is set #10973 [3.6] bpo-35330: Don't call the wrapped object if side_effect is set (GH10973) #11034 [3.7] bpo-35330: Don't call the wrapped object if side_effect is set (GH10973) #11035

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-12-08.11:47:36.779>
created_at = <Date 2018-11-27.17:26:39.414>
labels = ['3.7', 'tests', '3.8', 'type-bug', 'library']
title = 'When using mock to wrap an existing object, side_effect requires return_value'
updated_at = <Date 2018-12-08.11:47:36.778>
user = 'https://github.com/noamraph'

bugs.python.org fields:

activity = <Date 2018-12-08.11:47:36.778>
actor = 'cjw296'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-08.11:47:36.779>
closer = 'cjw296'
components = ['Library (Lib)', 'Tests']
creation = <Date 2018-11-27.17:26:39.414>
creator = 'noamraph'
dependencies = []
files = []
hgrepos = []
issue_num = 35330
keywords = ['patch']
message_count = 9.0
messages = ['330540', '330595', '330596', '330599', '330602', '331155', '331378', '331379', '331380']
nosy_count = 6.0
nosy_names = ['cjw296', 'michael.foord', 'lisroach', 'mariocj89', 'xtreak', 'noamraph']
pr_nums = ['10973', '11034', '11035']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35330'
versions = ['Python 3.7', 'Python 3.8']

[noamraph]

When using mock to wrap an existing object, and using side_effect to set a function to wrap a method, I would expect the wrapper function to be called instead of the wrapped function, and its return value to be returned. Instead, both the wrapper function and the wrapped functions are being called, and the return value of the wrapped function is returned.

If, in addition to side_effect, return_value is set, the return_value is ignored, but my expected behavior actually happens: only the wrapper function is called, and its return value is returned.

Python 3.7.0 (default, Aug 22 2018, 20:50:05)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from unittest import mock
>>> class MyClass(object):
...     def func(self):
...         print('func called')
...         return 1
... 
>>> c = MyClass()
>>> m = mock.Mock(wraps=c)
>>> def func2():
...     print('func2 called')
...     return 2
... 
>>> m.func.side_effect = func2
>>> m.func()
func2 called
func called
1
>>> m.func.return_value = 3
>>> m.func()
func2 called
2

[tirkarthi]

I did some debugging with docstring for wraps.

wraps: Item for the mock object to wrap. If wraps is not None then
calling the Mock will pass the call through to the wrapped object
(returning the real result). Attribute access on the mock will return a
Mock object that wraps the corresponding attribute of the wrapped object
(so attempting to access an attribute that doesn't exist will raise an
AttributeError).
If the mock has an explicit return_value set then calls are not passed to the wrapped object and the return_value is returned instead.

So calling mock.Mock(wraps=c) sets the _mock_wraps with c. When we set m.func.side_effect and call m.func() it checks for the side_effect (func2) to be a callable and calls it [0]. It also checks if self._mock_wraps is not None which in this case is MyClass() and checks for the func of Myclass that is also called at [1] . As per the docstring since it wraps the actual object calling m.invalid_func without return_value set will cause attribute error like "AttributeError: 'MyClass' object has no attribute 'invalid_func'"

It seems to be a general case with mock itself where when side_effect and return_value are set then side_effect is called and ignores the return_value set unless the side_effect returns the sentinel value DEFAULT as in test [2]. I find this to be surprising.

$ ./python.exe
Python 3.8.0a0 (heads/master:b7278736b3, Nov 28 2018, 10:26:47)
[Clang 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from unittest import mock
>>> m = mock.Mock()
>>> m.side_effect = lambda : 2
>>> m.return_value = 3
>>> m() # side_effect and return_value are set returning side_effect
2
>>> f = mock.Mock()
>>> f.return_value = 3
>>> f() # side_effect is not set thus returns only return_value
3

As per the original report when there is a return_value set without the side_effect it returns the set return_value. When there is a return_value set with the side_effect then return value of the side effect is returned though return_value is explicitly set like above also with wraps

>>> m = mock.Mock(wraps=c)
>>> print(m.func())
func called
1
>>> m.func.return_value = 3
>>> print(m.func())
3
>>> f = mock.Mock(wraps=c)
>>> f.func.side_effect = func2
>>> f.func.return_value = 3
>>> print(f.func())
func2 called
2

[0]

cpython/Lib/unittest/mock.py

Line 1031 in bde9d6b

if not _callable(effect):

[1]

cpython/Lib/unittest/mock.py

Line 1041 in bde9d6b

if (self._mock_wraps is not None and

[2]

cpython/Lib/unittest/test/testmock/testmock.py

Line 156 in 54ba556

mock = Mock(side_effect=side_effect, return_value=sentinel.RETURN)

[tirkarthi]

A little more discussion over side_effect and return_value precedence : bpo-22541

[mariocj89]

I can indeed reproduce the issue. The problem seems to be here:

cpython/Lib/unittest/mock.py

Line 1041 in 54ba556

if (self._mock_wraps is not None and

The simplified current logic in that code is:

• call side_effect, save the return value in ret.
• if return_value is not set, call the wraps object and return it.
• return the value in ret from the first step.

That explains why you see your "expected behavior" to happen only when return value is set.

Basically, the logic disables the current wrapped object ONLY if return value is set.
I am not sure why it was not done for side_effect as well.
Trying to perform that change in the sourcecode (nor run wraps if side_effect is set) results in no failure from the tests, which might mean it is a bug

One might claim the code is there because side_effect might still be used to cause a side effect but not return, I would disagree, especially as setting return_value totally breaks it. As once the return_value is set, both return_value and wraps are ignored and side_effect takes preference due to line 1044.
Especially as the docs say about side_effect:
unless it returns DEFAULT, the return value of this function is used as the return value.

I'd suggest a patch adding the and not effect to line 1041, so side_effect takes preference over wraps, the same way return_value does today.

The behavior of side_effect taken precedence over return_value is fine, that is how Mock works.

This bug can be reproduced without a class at all, see:

from unittest import mock
def func():
    print('Original func called')
    return "ORIGINAL"

m = mock.Mock(wraps=func)

def side_effect():
    print('Side effect func called')
    return "SIDE EFFECT"


m.side_effect = side_effect
print(m())

Results in:

Side effect func called
Original func called
ORIGINAL

Whilst the expected is:

Side effect func called
SIDE EFFECT

## TL;DR;

Indeed, side_effect seems broken when applied to an object with wraps and setting return_value kind of "fixes it".
I'd send a fix to add the check for effect unless michael.foord or someone else knows a good reason of why things are not like that. + tests

Noam Yorav-Raphael I am happy to send the patch if you don't have time :)

[tirkarthi]

Thanks @mariocj89 for the explanation. I just got to the docs part about side_effect and return_value precedence. I am curious to know about the behavior as well and at least this can be added as a test as I see only around 3 tests for side_effect and there is no test for wraps behavior in this report I assume given that there were no test failures due to changing the logic.

[mariocj89]

I'll get ready a PR with a good set of tests and the fix for the original issue. This is quite an interesting bug :)

[cjw296]

New changeset f05df0a by Chris Withers (Mario Corchero) in branch 'master':
bpo-35330: Don't call the wrapped object if side_effect is set (GH10973)
f05df0a

[cjw296]

New changeset 12b9fb6 by Chris Withers (Miss Islington (bot)) in branch '3.6':
bpo-35330: Don't call the wrapped object if side_effect is set (GH11034)
12b9fb6

[cjw296]

New changeset ee2c5a8 by Chris Withers (Miss Islington (bot)) in branch '3.7':
bpo-35330: Don't call the wrapped object if side_effect is set (GH11035)
ee2c5a8