From d0c6c6b9222214e37422ee3198db3611acc7c7c0 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 13 Sep 2024 10:35:06 +0300 Subject: [PATCH 1/3] gh-123934: Fix `MagicMock` not to reset magic method return values --- .../testmock/testmagicmethods.py | 20 +++++++++++++++++++ Lib/unittest/mock.py | 13 ++++++++++-- ...-09-13-10-34-19.gh-issue-123934.yMe7mL.rst | 2 ++ 3 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-09-13-10-34-19.gh-issue-123934.yMe7mL.rst diff --git a/Lib/test/test_unittest/testmock/testmagicmethods.py b/Lib/test/test_unittest/testmock/testmagicmethods.py index 5ca753b8f20811..bd8b0346cf920c 100644 --- a/Lib/test/test_unittest/testmock/testmagicmethods.py +++ b/Lib/test/test_unittest/testmock/testmagicmethods.py @@ -331,6 +331,26 @@ 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_methods_and_spec(self): class Iterable(object): diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index 480c85bed9b31e..a656c1fcfefa1b 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -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 = [] @@ -648,9 +648,18 @@ def reset_mock(self, visited=None,*, return_value=False, side_effect=False): if side_effect: self._mock_side_effect = None - for child in self._mock_children.values(): + for key, child in self._mock_children.items(): if isinstance(child, _SpecState) or child is _deleted: continue + if ( + return_value + and issubclass(type(child), MagicMock) + and _is_magic(key) + ): + # Don't reset return values for magic methods, + # otherwise `m.__str__` will start + # to return `MagicMock` instances, instead of `str` instances. + return_value = False child.reset_mock(visited, return_value=return_value, side_effect=side_effect) ret = self._mock_return_value diff --git a/Misc/NEWS.d/next/Library/2024-09-13-10-34-19.gh-issue-123934.yMe7mL.rst b/Misc/NEWS.d/next/Library/2024-09-13-10-34-19.gh-issue-123934.yMe7mL.rst new file mode 100644 index 00000000000000..cec7741bcabbda --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-09-13-10-34-19.gh-issue-123934.yMe7mL.rst @@ -0,0 +1,2 @@ +Fix :class:`unittest.mock.MagicMock` reseting magic methods return values +after ``.reset_mock(return_value=True)`` was called. From e98fb62a2372eb38162e947ae86d6de8c4f7ff40 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Mon, 16 Sep 2024 10:29:07 +0300 Subject: [PATCH 2/3] Address review --- .../testmock/testmagicmethods.py | 11 ++++++++++ Lib/unittest/mock.py | 22 ++++++++++--------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_unittest/testmock/testmagicmethods.py b/Lib/test/test_unittest/testmock/testmagicmethods.py index bd8b0346cf920c..108d2fdd86ea48 100644 --- a/Lib/test/test_unittest/testmock/testmagicmethods.py +++ b/Lib/test/test_unittest/testmock/testmagicmethods.py @@ -352,6 +352,17 @@ def test_magic_mock_does_not_reset_magic_returns(self): 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])) + 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_methods_and_spec(self): class Iterable(object): def __iter__(self): pass diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index a656c1fcfefa1b..9d5d5c8d460162 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -648,18 +648,9 @@ def reset_mock(self, visited=None, *, return_value=False, side_effect=False): if side_effect: self._mock_side_effect = None - for key, child in self._mock_children.items(): + for child in self._mock_children.values(): if isinstance(child, _SpecState) or child is _deleted: continue - if ( - return_value - and issubclass(type(child), MagicMock) - and _is_magic(key) - ): - # Don't reset return values for magic methods, - # otherwise `m.__str__` will start - # to return `MagicMock` instances, instead of `str` instances. - return_value = False child.reset_mock(visited, return_value=return_value, side_effect=side_effect) ret = self._mock_return_value @@ -2229,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): From 1affdd2aecd750b81ae3eb87a9f81b8b0b17261b Mon Sep 17 00:00:00 2001 From: sobolevn Date: Wed, 18 Sep 2024 10:14:20 +0300 Subject: [PATCH 3/3] Add one more test --- Lib/test/test_unittest/testmock/testmagicmethods.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Lib/test/test_unittest/testmock/testmagicmethods.py b/Lib/test/test_unittest/testmock/testmagicmethods.py index 108d2fdd86ea48..2a8aa11b3284f6 100644 --- a/Lib/test/test_unittest/testmock/testmagicmethods.py +++ b/Lib/test/test_unittest/testmock/testmagicmethods.py @@ -363,6 +363,14 @@ def test_magic_mock_resets_manual_mocks(self): 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): def __iter__(self): pass