Skip to content

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jacksonriley
Copy link
Contributor

@jacksonriley jacksonriley commented Nov 20, 2019

This change implements the second suggestion of Issue 34716, changing the default behaviour of MagicMock()__divmod__ and MagicMock()__rdivmod__ to return a pair of MagicMock instances.

unittest/test/testmock/testmagicmethods.py:test_divmod_and_rdivmod has been expanded to test this new behaviour.

https://bugs.python.org/issue34716

@jacksonriley
Copy link
Contributor Author

@serhiy-storchaka would you mind please taking a look at this? :)

@jacksonriley
Copy link
Contributor Author

@serhiy-storchaka a friendly bump :)

@jacksonriley
Copy link
Contributor Author

Hi @serhiy-storchaka, sorry for the multiple bumps, but would you possibly have a chance to take a look at this PR? Thanks :)

Copy link
Contributor

@LewisGaul LewisGaul left a 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?

@LewisGaul
Copy link
Contributor

@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
Copy link
Contributor

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?

Copy link
Contributor Author

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__
Copy link
Contributor

@LewisGaul LewisGaul Oct 21, 2020

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?

Copy link
Contributor Author

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.

@LewisGaul
Copy link
Contributor

Docs also need updating (https://docs.python.org/3/library/unittest.mock.html#magic-mock) when the behaviour has been finalised.

@jacksonriley
Copy link
Contributor Author

jacksonriley commented Nov 10, 2020

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?

With the most recent commit (6bb2e07), the behaviour is (I think) more correct:

>>> m = MagicMock()
>>> m.__divmod__.return_value
(<MagicMock id='140534571186688'>, <MagicMock id='140534525562832'>)
>>> m.__divmod__.return_value[0]
<MagicMock id='140534571186688'>
>>> m.__divmod__.return_value[0] = 1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'tuple' object does not support item assignment
>>> divmod(m, 1)
(<MagicMock id='140534571186688'>, <MagicMock id='140534525562832'>)
>>> m.__divmod__.return_value = 1
>>> divmod(m, 1)
1

It should be noted that with this setup, the same pair of mocks is returned every time the magic method is called:

>>> divmod(m, 1)
(<MagicMock id='140715446614240'>, <MagicMock id='140715401200640'>)
>>> divmod(m, 1)
(<MagicMock id='140715446614240'>, <MagicMock id='140715401200640'>)
>>> 

However, as this behaviour is the same as other magic methods without specific implementations, I think this isn't a problem. For example, __add__:

>>> m + 1
<MagicMock name='mock.__add__()' id='140207358300704'>
>>> m + 2
<MagicMock name='mock.__add__()' id='140207358300704'>
>>> 

@jacksonriley
Copy link
Contributor Author

@jacksonriley Would be good to add yourself to Misc/ACKS :)

Docs also need updating (https://docs.python.org/3/library/unittest.mock.html#magic-mock) when the behaviour has been finalised.

Done, thank you for pointing these out :)

@jacksonriley
Copy link
Contributor Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants