classification
Title: mock 3.9 bug: Wrapped objects without __bool__ raise exception
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Amir, Darragh Bailey, anthonypjshaw, aviso, cjw296, lisroach, mariocj89, michael.foord, pconnell, r.david.murray, rbcollins, serhiy.storchaka, xtreak
Priority: normal Keywords: 3.9regression, patch

Created on 2020-03-15 06:39 by aviso, last changed 2020-04-07 08:26 by xtreak.

Pull Requests
URL Status Linked Edit
PR 19102 open Amir, 2020-03-21 12:53
Messages (13)
msg364222 - (view) Author: Avram (aviso) * Date: 2020-03-15 06:39
This bug was introduced with Issue25597


Here's some code that demonstrates the error:

    import sys
    from unittest.mock import patch

    with patch.object(sys, 'stdout', wraps=sys.stdout) as mockstdout:
        bool(sys.stdout)

This works fine in 3.8 and earlier, but fails in 3.9

It seems the goal was to be able to access dunder methods for wrapped objects. Before this change __bool__ wasn't actually being checked, but was forced to True, which works for basic existence tests. The new code method._mock_wraps = getattr(mock._mock_wraps, name) has no fallthrough in case the attribute isn't there such as the case with __bool__ on sys.stdout.
msg364223 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-03-15 08:57
Thanks for the report. There should be a check to ensure that the attribute is present to handle the attribute error while attaching the wrapped object's value. Something like below could be done so that we check for wrapped value or fallback to the old behaviour. This needs a test too where the report can be added as a unittest. The news entry can also ensure that the wrapped value will be used when present or falls back to the default value. Marking it as a 3.9 regression.

diff --git Lib/unittest/mock.py Lib/unittest/mock.py
index 20daf724c1..86e832cc51 100644
--- Lib/unittest/mock.py
+++ Lib/unittest/mock.py
@@ -2036,7 +2036,7 @@ _side_effect_methods = {
 def _set_return_value(mock, method, name):
     # If _mock_wraps is present then attach it so that wrapped object
     # is used for return value is used when called.
-    if mock._mock_wraps is not None:
+    if mock._mock_wraps is not None and hasattr(mock._mock_wraps, name):
         method._mock_wraps = getattr(mock._mock_wraps, name)
         return
msg364663 - (view) Author: Amir Mohamadi (Amir) * Date: 2020-03-20 10:35
I'd like to work on it.
Should I add tests to 'Lib/unittest/test/testmock/testmock.py'??
msg364728 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-03-21 07:30
Amir, this needs a test like the initial report without preferably using sys.stdout and patch for cases below : 

1. Where the attribute is present on the wrapped object and uses it.
2. Where the attribute is not present on the wrapped object and uses the default value of the magic method.

@aviso Feel free to add in if I have missed any scenarios.
msg365144 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-03-27 11:43
On thinking more about this the wraps argument provides a way to wrap the calls for the mock to a given object. So when an attribute not present in the wrapped object is being accessed then it should act like the attribute is not present. Falling back to the default value like previous case would help but that will pose the question over whether the value is from the wrapped object's attribute or from the default value for the magic method that the user could never know. This could be better clarified in the docs I suppose which I missed in the previous issue. 

Thoughts on this fallback behavior?
msg365198 - (view) Author: Avram (aviso) * Date: 2020-03-28 03:25
They are documented. That said, the subsection section could use a header.

https://docs.python.org/3/library/unittest.mock.html#magic-mock

Patch looks good! Can't think of any other test scenarios right now.
msg365199 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-03-28 03:31
It's supposed to raise an AttributeError if the attribute is not present. The previous case was that magicmethods were not wrapped returning configured values. So this conflicts with the documentation returning a default value when not present instead of raising an AttributeError.

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).
msg365200 - (view) Author: Avram (aviso) * Date: 2020-03-28 03:55
Ah, I see, yes, the documentation is a bit inconsistent. What may make more sense for some magic methods is to call the underlying object and use the return value or raise any exception that occurs.
For example:
__bool__ return value is bool(mock._mock_wraps)
__int__ return value is int(mock._mock_wraps)
__lt__ return value is operator.lt(mock._mock_wraps, other)

This would take care of several of them and provide more consistent behavior with the wrapped object.
msg365201 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-03-28 04:00
> What may make more sense for some magic methods is to call the underlying object and use the return value or raise any exception that occurs.

Sorry, isn't that what the previous issue did? It tries to return the wrapped object attribute and raises exception if attribute is not present. Using wrapped object attribute and falling back to default value means that it will always return a value but the user can't figure out if it's from wrapped object or the default value for the magic method.
msg365202 - (view) Author: Avram (aviso) * Date: 2020-03-28 04:18
That is not necessarily the same thing, hence why there was a bug.

>>> hasattr(object, '__bool__')
False
>>> bool(object)
True

I think you may be right that magic methods shouldn't magically appear for wrapped objects, except those that do magically appear, like __bool__.
msg365203 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-03-28 04:27
Thanks for the example I didn't know hasattr can return False to return a value when the magicmethod was accessed. I will wait for others opinion on this then.
msg365204 - (view) Author: Avram (aviso) * Date: 2020-03-28 05:39
Honestly, I'm not sure if any of the other magic methods behave this way. It would take a little research or someone more familiar with it.

Here's more info about how __bool__ behaves.
https://docs.python.org/3/reference/datamodel.html#object.__bool__
msg365211 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2020-03-28 13:39
My guess is that it isn't so much that __bool__ is special, as that the evaluation of values in a boolean context is special.  What you have to do to make a mock behave "correctly" in the face that I'm not sure (I haven't investigated).  And I might be wrong.
History
Date User Action Args
2020-04-07 08:26:15xtreaksetnosy: + serhiy.storchaka
2020-03-28 13:39:27r.david.murraysetmessages: + msg365211
2020-03-28 05:39:16avisosetmessages: + msg365204
2020-03-28 04:27:03xtreaksetmessages: + msg365203
2020-03-28 04:18:49avisosetmessages: + msg365202
2020-03-28 04:00:02xtreaksetmessages: + msg365201
2020-03-28 03:55:16avisosetmessages: + msg365200
2020-03-28 03:31:18xtreaksetmessages: + msg365199
2020-03-28 03:25:23avisosetmessages: + msg365198
2020-03-27 11:43:55xtreaksetmessages: + msg365144
2020-03-21 12:53:06Amirsetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request18462
2020-03-21 07:30:12xtreaksetmessages: + msg364728
2020-03-20 18:52:10terry.reedysetstage: test needed
2020-03-20 10:35:26Amirsetnosy: + Amir
messages: + msg364663
2020-03-15 08:57:43xtreaksetkeywords: + 3.9regression

messages: + msg364223
2020-03-15 06:39:44avisocreate