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

[sobolevn]

Refs https://github.com/python/cpython/pull/17409/files

• Issue: reset_mock resets MagicMock's magic methods in an unexpected way #123934

[sobolevn]

I consider this a bug fix, so I propose to backport this to 3.12 and 3.13
@cjw296 what do you think? :)

[cjw296]

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]

[sobolevn]

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?

[cjw296]

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.

[sobolevn]

Sure!

[cjw296]

As I said inline, the docs aren't great. Any reason not to just add a type annotation of : bool to that parameter?

Anyway, I think this is good to land, thanks for the attention to detail here!

[cjw296]

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.

[miss-islington-app]

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[sobolevn]

Thanks a lot for the helpful reviews! I would prefer to work on docs in a separate PR, because it is unrelated to this problem. Working on it now! 👍

#124234

[bedevere-app]

GH-124231 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-124232 is a backport of this pull request to the 3.12 branch.