Title: When using mock to wrap an existing object, side_effect requires return_value
Type: behavior Stage: resolved
Components: Library (Lib), Tests Versions: Python 3.8, Python 3.7
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cjw296, lisroach, mariocj89, michael.foord, noamraph, xtreak
Priority: normal Keywords: patch

Created on 2018-11-27 17:26 by noamraph, last changed 2018-12-08 11:47 by cjw296. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 10973 merged mariocj89, 2018-12-06 13:42
PR 11034 merged miss-islington, 2018-12-08 11:25
PR 11035 merged miss-islington, 2018-12-08 11:25
Messages (9)
msg330540 - (view) Author: Noam Yorav-Raphael (noamraph) Date: 2018-11-27 17:26
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
>>> m.func.return_value = 3
>>> m.func()
func2 called
msg330595 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-11-28 12:32
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
>>> f = mock.Mock()
>>> f.return_value = 3
>>> f() # side_effect is not set thus returns only return_value

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
>>> m.func.return_value = 3
>>> print(m.func())
>>> f = mock.Mock(wraps=c)
>>> f.func.side_effect = func2
>>> f.func.return_value = 3
>>> print(f.func())
func2 called

msg330596 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-11-28 12:39
A little more discussion over side_effect and return_value precedence : issue22541
msg330599 - (view) Author: Mario Corchero (mariocj89) * (Python triager) Date: 2018-11-28 13:50
I can indeed reproduce the issue. The problem seems to be here:

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

Results in:
Side effect func called
Original func called

Whilst the expected is:
Side effect func called

## 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 :)
msg330602 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-11-28 15:01
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.
msg331155 - (view) Author: Mario Corchero (mariocj89) * (Python triager) Date: 2018-12-05 20:24
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 :)
msg331378 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2018-12-08 11:25
New changeset f05df0a4b679d0acfd0b1fe6187ba2d553b37afa by Chris Withers (Mario Corchero) in branch 'master':
bpo-35330:  Don't call the wrapped object if `side_effect` is set (GH10973)
msg331379 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2018-12-08 11:41
New changeset 12b9fb603eea9298c835bae5b8742db4fa52892e by Chris Withers (Miss Islington (bot)) in branch '3.6':
bpo-35330:  Don't call the wrapped object if `side_effect` is set (GH11034)
msg331380 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2018-12-08 11:47
New changeset ee2c5a8e2dcf662048dbcf4e49af9b4aaf81f7d3 by Chris Withers (Miss Islington (bot)) in branch '3.7':
bpo-35330:  Don't call the wrapped object if `side_effect` is set (GH11035)
Date User Action Args
2018-12-08 11:47:36cjw296setstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-12-08 11:47:03cjw296setmessages: + msg331380
2018-12-08 11:41:57cjw296setmessages: + msg331379
2018-12-08 11:25:48miss-islingtonsetpull_requests: + pull_request10272
2018-12-08 11:25:40miss-islingtonsetpull_requests: + pull_request10271
2018-12-08 11:25:07cjw296setmessages: + msg331378
2018-12-06 13:42:40mariocj89setkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request10234
2018-12-05 20:24:54mariocj89setmessages: + msg331155
2018-12-05 03:29:33terry.reedysetnosy: + cjw296
stage: test needed

components: + Tests
versions: + Python 3.7, Python 3.8
2018-11-28 15:01:36xtreaksetmessages: + msg330602
2018-11-28 13:50:59mariocj89setmessages: + msg330599
2018-11-28 12:39:39xtreaksetmessages: + msg330596
2018-11-28 12:32:08xtreaksetnosy: + michael.foord, mariocj89
messages: + msg330595
2018-11-28 09:59:54xtreaksetnosy: + xtreak
2018-11-27 20:36:01rhettingersetnosy: + lisroach
2018-11-27 17:26:39noamraphcreate