-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-34716: Change MagicMock().__divmod__ to return a pair of MagicMock instances #17291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@serhiy-storchaka would you mind please taking a look at this? :) |
@serhiy-storchaka a friendly bump :) |
Hi @serhiy-storchaka, sorry for the multiple bumps, but would you possibly have a chance to take a look at this PR? Thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a play around with these changes and they seem to work as expected - returning type(self)()
appears to be standard and the correct approach. Also noted divmod and rdivmod seem to be the only special cases of dunder methods expected to return a fixed number of multiple values.
At a higher level there are some considerations for making this change, e.g. it's not strictly speaking backwards compatible (although I don't expect the change to cause many problems).
Previously:
>>> m = MagicMock()
>>> divmod(m, 1)
<MagicMock name='mock.__divmod__()' id='140147753274096'>
>>> divmod(m, 1).return_value = 1
>>> divmod(m, 1)()
1
Now:
>>> m = MagicMock()
>>> divmod(m, 1)
(<MagicMock id='139643052028928'>, <MagicMock id='139643052032640'>)
>>> divmod(m, 1).return_value = 1
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'tuple' object has no attribute 'return_value'
Another potential concern is the following behaviour:
>>> m = MagicMock()
>>> m.__divmod__.return_value
<MagicMock name='mock.__divmod__()' id='139643052068720'>
>>> m.__divmod__.return_value[0]
<MagicMock name='mock.__divmod__().__getitem__()' id='139643049016480'>
>>> m.__divmod__.return_value[0] = 1
>>> m.__divmod__.return_value[0]
<MagicMock name='mock.__divmod__().__getitem__()' id='139643049016480'>
>>> divmod(m, 1)
<MagicMock name='mock.__divmod__()' id='139643052068720'>
I think what's happening here is that m.__divmod__.return_value
initially holds sentinal.DEFAULT
rather than what will actually be returned (the tuple of mocks), and when you change something on it then it becomes a concrete mock and the tuple is no longer returned (when what I expected to happen was to change the first element of the tuple).
Perhaps it would be better to add to _return_values
rather than _side_effect_methods
to fix this?
@jacksonriley Would be good to add yourself to Misc/ACKS :) |
self.assertEqual(divmod(2, m), (2, 1)) | ||
self.assertEqual(divmod(None, m), (2, 1)) | ||
|
||
# Test what happens if two MagicMocks are passed into divmod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about if a subclass of MagicMock is passed - we expect to get two instances of the subclass out right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - will add a test for this, thanks :)
self.assertIsInstance(elem, MagicMock) | ||
|
||
# Test the behaviour of divmod with two MagicMock instances when one | ||
# has a return value set for __divmod__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you actually checking here, that __divmod__
on the first mock takes precedence over __rdivmod__
on the second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as it should from the docs. Will add a comment to this effect.
Docs also need updating (https://docs.python.org/3/library/unittest.mock.html#magic-mock) when the behaviour has been finalised. |
With the most recent commit (6bb2e07), the behaviour is (I think) more correct:
It should be noted that with this setup, the same pair of mocks is returned every time the magic method is called:
However, as this behaviour is the same as other magic methods without specific implementations, I think this isn't a problem. For example,
|
Done, thank you for pointing these out :) |
Hi @aeros - sorry for the random question but do you know if news items need to be regenerated via blurb? I added one for this PR a year ago, and wasn't sure whether it was collated into news based on the date on the autogenerated file in Misc/NEWS.d/next/Library or based on the date of merge :) |
This change implements the second suggestion of Issue 34716, changing the default behaviour of
MagicMock()__divmod__
andMagicMock()__rdivmod__
to return a pair ofMagicMock
instances.unittest/test/testmock/testmagicmethods.py:test_divmod_and_rdivmod has been expanded to test this new behaviour.
https://bugs.python.org/issue34716