Skip to content

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

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

Closed
mentalAdventurer opened this issue Sep 11, 2024 · 6 comments
Closed
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@mentalAdventurer
Copy link

mentalAdventurer commented Sep 11, 2024

Bug report

Bug description:

The reset_mock(return_value=True) method behaves in a wrong/inconsistent way.
When used with MagicMock, the method reset_mock(return_value=True) does not reset the return values of the magic methods. Only if you call for example __str__ and then call the reset_mock function, the return value will be reset, but not to the default value.

from unittest import mock

mm = mock.MagicMock()
print(type(mm.__str__()))
mm.reset_mock(return_value=True)
print(type(mm.__str__()))
print(type(mm.__hash__()))
mm.reset_mock(return_value=True)
print(type(mm.__hash__()))

Output

<class 'str'>
<class 'unittest.mock.MagicMock'>
<class 'int'>
<class 'unittest.mock.MagicMock'>

Since Python 3.9 PR reset_mock now also resets child mocks. This explains the behaviour. Calling the __str__ method creates a child MagicMock with a set return value. Since this child mock now exists, its return value is reset when reset_mock(return_value=True) is called.
Although this can be logically explained, it's counter-intuitive and annoying as I'm never sure which values are being reset.

I would expect the same behaviour as Mock. The return value of __str__ and other magic methods should not be effected.

from unittest import mock

m = mock.Mock()
print(type(m.__str__()))
m.reset_mock(return_value=True)
print(type(m.__str__()))
print(type(m.__hash__()))
m.reset_mock(return_value=True)
print(type(m.__hash__()))

Output

<class 'str'>
<class 'str'>
<class 'int'>
<class 'int'>

CPython versions tested on:

3.10

Operating systems tested on:

Linux

Linked PRs

@mentalAdventurer mentalAdventurer added the type-bug An unexpected behavior, bug, or error label Sep 11, 2024
@mentalAdventurer mentalAdventurer changed the title reset_mock resets MagicMock's magic methods in an unexpected way reset_mock resets MagicMock's magic methods in an unexpected way Sep 11, 2024
@sobolevn sobolevn self-assigned this Sep 11, 2024
@sobolevn
Copy link
Member

Thank you for the report. I will take a look soon :)

@picnixz picnixz added the stdlib Python modules in the Lib dir label Sep 11, 2024
sobolevn added a commit to sobolevn/cpython that referenced this issue Sep 13, 2024
@sobolevn
Copy link
Member

Since this child mock now exists, its return value is reset when reset_mock(return_value=True) is called.

Yes, this is correct.

I would expect the same behaviour as Mock. The return value of str and other magic methods should not be effected.

Mock does not mock magic methods at all, so they are unaffected at any case. Compare:

print(type(mock.Mock().__str__))
# <class 'method-wrapper'>

print(type(mock.MagicMock().__str__))
# <class 'unittest.mock.MagicMock'>

But, I agree that retuning <class 'unittest.mock.MagicMock'> from __str__ is not correct.
So, my proposed solution is to ignore cases when:

  1. child is a MagicMock instance
  2. key is a magic method
  3. return_value is True

@picnixz picnixz moved this from Todo to In Progress in Unittest issues Sep 13, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 19, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 19, 2024
sobolevn added a commit that referenced this issue Sep 19, 2024
…lues (GH-124038) (#124232)

gh-123934: Fix `MagicMock` not to reset magic method return values (GH-124038)
(cherry picked from commit 7628f67)

Co-authored-by: sobolevn <[email protected]>
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this issue Sep 22, 2024
Yhg1s pushed a commit that referenced this issue Sep 30, 2024
…lues (GH-124038) (#124231)

gh-123934: Fix `MagicMock` not to reset magic method return values (GH-124038)
(cherry picked from commit 7628f67)

Co-authored-by: sobolevn <[email protected]>
@hroncok
Copy link
Contributor

hroncok commented Nov 18, 2024

I see that this test in mu-editor fails with infinite RecursionError:

https://github.com/mu-editor/mu/blob/7641d7ada71a986f895ebd8a26d2ee2f52c3f1fe/tests/modes/test_python3.py#L486-L490

    with mock.patch("builtins.super") as mock_super:
        pm = PythonMode(editor, view)
        pm.set_buttons = mock.MagicMock()
        mock_super.reset_mock()

I believe this is because it is calling reset_mock on a mock of super and after this PR reset_mock calls super.

I think that mocking super is probably dangerous (and always has been), but I thought I should mention this here.

@sobolevn
Copy link
Member

Hm, I think that we can not use super() for this feature technically. But, I think that mocking super() is dangerous indeed.

What do others think? Should we fix this?

hroncok added a commit to hroncok/mu that referenced this issue Nov 18, 2024
…sionError

Starting with Python 3.12.7 or 3.13.0rc3, the reset_mock() method calls super().

python/cpython#123934

This lead to:

    __________________________ test_python_remove_plotter __________________________
        def test_python_remove_plotter():
            """
            Ensure the button states are returned to normal before calling super
            method.
            """
            editor = mock.MagicMock()
            view = mock.MagicMock()
            with mock.patch("builtins.super") as mock_super:
                pm = PythonMode(editor, view)
                pm.set_buttons = mock.MagicMock()
    >           mock_super.reset_mock()
    tests/modes/test_python3.py:489:
    _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
    /usr/lib64/python3.13/unittest/mock.py:2232: in reset_mock
        super().reset_mock(*args, return_value=return_value, **kwargs)
    /usr/lib64/python3.13/unittest/mock.py:2232: in reset_mock
        super().reset_mock(*args, return_value=return_value, **kwargs)
    /usr/lib64/python3.13/unittest/mock.py:2232: in reset_mock
        super().reset_mock(*args, return_value=return_value, **kwargs)
    E   RecursionError: maximum recursion depth exceeded
    !!! Recursion detected (same locals & position)

We avoid the problem by not needlessly calling reset_mock() in this test at all.
@hroncok
Copy link
Contributor

hroncok commented Nov 18, 2024

FWIW this particular use case was simple enough to fix: mu-editor/mu#2528

carlosperate pushed a commit to mu-editor/mu that referenced this issue Dec 13, 2024
…sionError (#2528)

Starting with Python 3.12.7 or 3.13.0rc3, the reset_mock() method calls super().

python/cpython#123934

This lead to:

    __________________________ test_python_remove_plotter __________________________
        def test_python_remove_plotter():
            """
            Ensure the button states are returned to normal before calling super
            method.
            """
            editor = mock.MagicMock()
            view = mock.MagicMock()
            with mock.patch("builtins.super") as mock_super:
                pm = PythonMode(editor, view)
                pm.set_buttons = mock.MagicMock()
    >           mock_super.reset_mock()
    tests/modes/test_python3.py:489:
    _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
    /usr/lib64/python3.13/unittest/mock.py:2232: in reset_mock
        super().reset_mock(*args, return_value=return_value, **kwargs)
    /usr/lib64/python3.13/unittest/mock.py:2232: in reset_mock
        super().reset_mock(*args, return_value=return_value, **kwargs)
    /usr/lib64/python3.13/unittest/mock.py:2232: in reset_mock
        super().reset_mock(*args, return_value=return_value, **kwargs)
    E   RecursionError: maximum recursion depth exceeded
    !!! Recursion detected (same locals & position)

We avoid the problem by not needlessly calling reset_mock() in this test at all.
@hugovk
Copy link
Member

hugovk commented Feb 5, 2025

Triage: PR merged and backported.

The output on 3.12 and 3.14 is now as expected:

<class 'str'>
<class 'str'>
<class 'int'>
<class 'int'>

Please re-open if there's more to do.

@hugovk hugovk closed this as completed Feb 5, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Unittest issues Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

5 participants