Skip to content

bpo-39966: Revert "bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock (#16029)" #19734

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 28, 2020
Merged

bpo-39966: Revert "bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock (#16029)" #19734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6b6981
Revert "bpo-25597: Ensure wraps' return value is used for magic metho…
tirkarthi Apr 27, 2020
e0081c5
Add NEWS entry.
tirkarthi Apr 27, 2020
f44faeb
Add comments to tests. Revert back news entry.
tirkarthi Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions Lib/unittest/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2012,12 +2012,6 @@ def __aiter__():


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:
method._mock_wraps = getattr(mock._mock_wraps, name)
return

fixed = _return_values.get(name, DEFAULT)
if fixed is not DEFAULT:
method.return_value = fixed
Expand Down
16 changes: 10 additions & 6 deletions Lib/unittest/test/testmock/testmock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,12 +716,16 @@ def method(self): pass


def test_magic_method_wraps_dict(self):
# bpo-25597: MagicMock with wrap doesn't call wrapped object's
# method for magic methods with default values.
data = {'foo': 'bar'}

wrapped_dict = MagicMock(wraps=data)
self.assertEqual(wrapped_dict.get('foo'), 'bar')
self.assertEqual(wrapped_dict['foo'], 'bar')
self.assertTrue('foo' in wrapped_dict)
# Accessing key gives a MagicMock
self.assertIsInstance(wrapped_dict['foo'], MagicMock)
# __contains__ method has a default value of False
self.assertFalse('foo' in wrapped_dict)

# return_value is non-sentinel and takes precedence over wrapped value.
wrapped_dict.get.return_value = 'return_value'
Expand All @@ -732,14 +736,13 @@ def test_magic_method_wraps_dict(self):
self.assertEqual(wrapped_dict.get('foo'), 'bar')

self.assertEqual(wrapped_dict.get('baz'), None)
with self.assertRaises(KeyError):
wrapped_dict['baz']
self.assertIsInstance(wrapped_dict['baz'], MagicMock)
self.assertFalse('bar' in wrapped_dict)

data['baz'] = 'spam'
self.assertEqual(wrapped_dict.get('baz'), 'spam')
self.assertEqual(wrapped_dict['baz'], 'spam')
self.assertTrue('baz' in wrapped_dict)
self.assertIsInstance(wrapped_dict['baz'], MagicMock)
self.assertFalse('bar' in wrapped_dict)

del data['baz']
self.assertEqual(wrapped_dict.get('baz'), None)
Expand All @@ -759,6 +762,7 @@ def __custom_method__(self):
klass = MagicMock(wraps=Foo)
obj = klass()
self.assertEqual(obj.__getitem__(2), 2)
self.assertEqual(obj[2], 2)
self.assertEqual(obj.__custom_method__(), "foo")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about leaving these tests in, but change their expectations and add comments as to why things are the way they are and how they would be in a perfect world?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually reversions do a git revert of commit fixing conflicts and add a NEWS entry as needed if the change was released. I guess these can be added back as part of a separate tests only PR. I am new to doing a reversion so feel free to correct me if I am wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If cpython hadn't chose squashing commits as their appoach, I'd suggest doing the revert in one commit and then add the tests back in another commit.

However, given that squashing is the choice cpython has made, it makes little difference.
Maybe two commits in this PR which will get squashed down to one on merge?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have added back the test and added some comments referencing the issue.


Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2020-04-27-14-48-43.bpo-39966.N5yXUe.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Revert bpo-25597. :class:`unittest.mock.MagicMock` with wraps' set uses
default return values for magic methods.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need this if the new item is removed above?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change was published as part of an alpha release. So my belief with respect to reversion after a release is that someone might depend on it and to explicitly note tbe reversion as a NEWS entry from past reversions I looked in git log.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but perhaps that means the original news item should stay too? It's a little confusing to have a reference to a bpo that isn't anywhere else in the changelog.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, added it back. Each alpha has a separate section and it reads like added in alpha 4 to be reverted by alpha 6. If anyone has concern or feedback later I will change it.