Skip to content

gh-123934: Fix MagicMock not to reset magic method return values #124038

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
Sep 19, 2024
Merged

gh-123934: Fix MagicMock not to reset magic method return values #124038

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
d0c6c6b
gh-123934: Fix `MagicMock` not to reset magic method return values
sobolevn Sep 13, 2024
e98fb62
Address review
sobolevn Sep 16, 2024
1affdd2
Add one more test
sobolevn Sep 18, 2024
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
39 changes: 39 additions & 0 deletions Lib/test/test_unittest/testmock/testmagicmethods.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,45 @@ def test_magic_methods_fspath(self):
self.assertEqual(os.fspath(mock), expected_path)
mock.__fspath__.assert_called_once()

def test_magic_mock_does_not_reset_magic_returns(self):
# https://github.com/python/cpython/issues/123934
for reset in (True, False):
with self.subTest(reset=reset):
mm = MagicMock()
self.assertIs(type(mm.__str__()), str)
mm.__str__.assert_called_once()

self.assertIs(type(mm.__hash__()), int)
mm.__hash__.assert_called_once()

for _ in range(3):
# Repeat reset several times to be sure:
mm.reset_mock(return_value=reset)

self.assertIs(type(mm.__str__()), str)
mm.__str__.assert_called_once()

self.assertIs(type(mm.__hash__()), int)
mm.__hash__.assert_called_once()

def test_magic_mock_resets_manual_mocks(self):
mm = MagicMock()
mm.__iter__ = MagicMock(return_value=iter([1]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, okay, things I didn't know about MagicMock and I'm not sure I like ;-) :

>>> m = MagicMock()
>>> list(iter(m))
[]

However, what happens if you change the above line to:

Suggested change
mm.__iter__ = MagicMock(return_value=iter([1]))
mm.__iter__ = MagicMock(return_value=[])

I'm not sure it makes any difference, but mainly 'cos of the weird behaviour I started with an example of ;-)

Here's some playing around in a Python 3.12 interpreter that might give more of a feel for what I'm trying to communicate:

>>> from unittest.mock import MagicMock
>>> m = MagicMock()
>>> m.__iter__
<MagicMock name='mock.__iter__' id='4307758752'>
>>> m.__iter__.return_value = []
>>> iter(m)
<list_iterator object at 0x10088f8b0>
>>> list(iter(m))
[]
>>> list(iter(m)) is m.__iter__.return_value
False
>>> m.__iter__.return_value = [1, 2, 3]
>>> list(iter(m))
[1, 2, 3]

Copy link
Member Author

Choose a reason for hiding this comment

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

I added one more test with the behavior you are showing here. I executed these two tests

    def test_magic_mock_resets_manual_mocks(self):
        mm = MagicMock()
        mm.__iter__ = MagicMock(return_value=iter([1]))
        mm.custom = MagicMock(return_value=2)
        self.assertEqual(list(iter(mm)), [1])
        self.assertEqual(mm.custom(), 2)

        mm.reset_mock(return_value=True)
        self.assertEqual(list(iter(mm)), [])
        self.assertIsInstance(mm.custom(), MagicMock)

    def test_magic_mock_resets_manual_mocks_empty_iter(self):
        mm = MagicMock()
        mm.__iter__.return_value = []
        self.assertEqual(list(iter(mm)), [])

        mm.reset_mock(return_value=True)
        self.assertEqual(list(iter(mm)), [])

on main (with no changes from this PR) and it was also successful.

Should I cover anything else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think this is where I got confused; the docs aren't great here. I believe the intention may be that return_value is only a boolean, or it may be that it's intended as a way to provide a new value.

If you feel inclined, it'd be good to clarify the docs on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

mm.custom = MagicMock(return_value=2)
self.assertEqual(list(iter(mm)), [1])
self.assertEqual(mm.custom(), 2)

mm.reset_mock(return_value=True)
self.assertEqual(list(iter(mm)), [])
self.assertIsInstance(mm.custom(), MagicMock)

def test_magic_mock_resets_manual_mocks_empty_iter(self):
mm = MagicMock()
mm.__iter__.return_value = []
self.assertEqual(list(iter(mm)), [])

mm.reset_mock(return_value=True)
self.assertEqual(list(iter(mm)), [])

def test_magic_methods_and_spec(self):
class Iterable(object):
Expand Down
13 changes: 12 additions & 1 deletion Lib/unittest/mock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ def __set_side_effect(self, value):
side_effect = property(__get_side_effect, __set_side_effect)


def reset_mock(self, visited=None,*, return_value=False, side_effect=False):
def reset_mock(self, visited=None, *, return_value=False, side_effect=False):
"Restore the mock object to its initial state."
if visited is None:
visited = []
Expand Down Expand Up @@ -2220,6 +2220,17 @@ def mock_add_spec(self, spec, spec_set=False):
self._mock_add_spec(spec, spec_set)
self._mock_set_magics()

def reset_mock(self, /, *args, return_value=False, **kwargs):
if (
return_value
and self._mock_name
and _is_magic(self._mock_name)
):
# Don't reset return values for magic methods,
# otherwise `m.__str__` will start
# to return `MagicMock` instances, instead of `str` instances.
return_value = False
super().reset_mock(*args, return_value=return_value, **kwargs)


class MagicProxy(Base):
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2024-09-13-10-34-19.gh-issue-123934.yMe7mL.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix :class:`unittest.mock.MagicMock` reseting magic methods return values
after ``.reset_mock(return_value=True)`` was called.
Loading