Skip to content
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

Guidance on "un-spying" a method #259

Closed
melvinkcx opened this issue Oct 20, 2021 · 12 comments · Fixed by #319
Closed

Guidance on "un-spying" a method #259

melvinkcx opened this issue Oct 20, 2021 · 12 comments · Fixed by #319

Comments

@melvinkcx
Copy link

melvinkcx commented Oct 20, 2021

Hi, I couldn't quite figure out how to un-spy a spied method yet. I'm not sure if it's even possible after browsing through the source code.

spy_foo = mocker.spy(module, "foo")

# do something

# how to un-spy `spy_foo`?

Any ideas or suggestions on how to achieve it? 🙏

@nicoddemus
Copy link
Member

nicoddemus commented Oct 20, 2021

Hi @melvinkcx,

The only way right now is to call mocker.stopall(), but this will of course undo all others mocks for the test.

@melvinkcx
Copy link
Author

Thanks for replying.
Undoing all other mocks is the unintended side-effect here.
Any thoughts on supporting this in the future? I'm happy to work on this, but any guides on achieving it will be greatly appreciated too =)

@nicoddemus
Copy link
Member

No plans, but I'm not sure what a good API for it would be... suggestions welcome!

@melvinkcx
Copy link
Author

Could this be an option? It is along the line with .reset_mock(), I believe

spy_foo = mocker.spy(module, "foo")
# do something
spy_foo.stop()  # introduce .stop() ?

@nicoddemus
Copy link
Member

No because spy_foo is a MagicMock, so it is possible for it to be mocking an object that contains a stop() method. 😕

@melvinkcx
Copy link
Author

Right, you're utterly right.

The only alternative I can think of so far is:

spy_foo = mocker.spy(module, "foo")

mocker.stop(spy_foo)

I think this is quite possible given that we keep track of all mocks in MockerFixture()._mocks?

@nicoddemus
Copy link
Member

nicoddemus commented Oct 22, 2021

Ahh that's a great idea!

That would even work for any mock, not only spy. 😁

Would you like to work on a PR for that?

@melvinkcx
Copy link
Author

I was looking into the implementation of MockerFixture, and what happens internally when we call spy, patch, etc.

The method _start_patch() appends the patcher object and the mocked object into 2 separate lists internally, but it doesn't seem like we can easily associate the objects in MockedFixtures()._mocks with their respective patchers.

I'm not too familiar with the code base, of course, please correct me if I'm wrong.

It seemed to me that, unless we introduce a new internal state to track the associations of patcher and mocked objects, we cannot support this:

spy_foo = mocker.spy(module, "foo")
mocker.stop(spy_foo)

Let me know what you think =)

@nicoddemus
Copy link
Member

nicoddemus commented Oct 30, 2021

Hi @melvinkcx,

I see, you are right.

I think we can merge the two lists into one, a list of pairs (patcher, mock), so we can associate each mock with their patcher.

I played around with the idea and it works well:

diff --git a/src/pytest_mock/plugin.py b/src/pytest_mock/plugin.py
index 088e5ce..1b5d4c7 100644
--- a/src/pytest_mock/plugin.py
+++ b/src/pytest_mock/plugin.py
@@ -39,11 +39,10 @@ class MockerFixture:
     """

     def __init__(self, config: Any) -> None:
-        self._patches = []  # type: List[Any]
-        self._mocks = []  # type: List[Any]
+        self._patches_and_mocks = []
         self.mock_module = mock_module = get_mock_module(config)
         self.patch = self._Patcher(
-            self._patches, self._mocks, mock_module
+            self._patches_and_mocks, mock_module
         )  # type: MockerFixture._Patcher
         # aliases for convenience
         self.Mock = mock_module.Mock
@@ -76,8 +75,10 @@ class MockerFixture:
         else:
             supports_reset_mock_with_args = (self.Mock,)

-        for m in self._mocks:
+        for p, m in self._patches_and_mocks:
             # See issue #237.
+            if not hasattr(m, "reset_mock"):
+                continue
             if isinstance(m, supports_reset_mock_with_args):
                 m.reset_mock(return_value=return_value, side_effect=side_effect)
             else:
@@ -88,10 +89,18 @@ class MockerFixture:
         Stop all patchers started by this fixture. Can be safely called multiple
         times.
         """
-        for p in reversed(self._patches):
+        for p, m in reversed(self._patches_and_mocks):
             p.stop()
-        self._patches[:] = []
-        self._mocks[:] = []
+        self._patches_and_mocks.clear()
+
+
+    def stop(self, mock) -> None:
+        for index, (p, m) in enumerate(self._patches_and_mocks):
+            if mock is m:
+                p.stop()
+                del self._patches_and_mocks[index]
+                break
+

     def spy(self, obj: object, name: str) -> unittest.mock.MagicMock:
         """
@@ -167,9 +176,8 @@ class MockerFixture:

         DEFAULT = object()

-        def __init__(self, patches, mocks, mock_module):
-            self._patches = patches
-            self._mocks = mocks
+        def __init__(self, patches_and_mocks, mock_module):
+            self.__patches_and_mocks = patches_and_mocks
             self.mock_module = mock_module

         def _start_patch(
@@ -181,9 +189,8 @@ class MockerFixture:
             """
             p = mock_func(*args, **kwargs)
             mocked = p.start()  # type: unittest.mock.MagicMock
-            self._patches.append(p)
+            self.__patches_and_mocks.append((p, mocked))
             if hasattr(mocked, "reset_mock"):
-                self._mocks.append(mocked)
                 # check if `mocked` is actually a mock object, as depending on autospec or target
                 # parameters `mocked` can be anything
                 if hasattr(mocked, "__enter__") and warn_on_mock_enter:

Here are two tests which demonstrate that this works:

def test_stop_patch(mocker):

    class C:
        def foo(self):
            return 42

    m = mocker.patch.object(C, "foo", return_value=0)
    assert C().foo() == 0
    mocker.stop(m)
    assert C().foo() == 42


def test_stop_spy(mocker):

    class C:
        def foo(self):
            return 42

    spy = mocker.spy(C, "foo")
    assert C().foo() == 42
    assert spy.call_count == 1
    mocker.stop(spy)
    assert C().foo() == 42
    assert spy.call_count == 1

The code of course needs documentation, types, and perhaps a few more tests.

@melvinkcx
Copy link
Author

Hi, thanks for testing the idea out.
How do you think we can move forward further with this?
Perhaps, I can spare some time to contribute more on this

@nicoddemus
Copy link
Member

How do you think we can move forward further with this?
Perhaps, I can spare some time to contribute more on this

Yeah it would be a great opportunity to contribute if you like.

@sgaist
Copy link
Contributor

sgaist commented Oct 1, 2022

Hi,

Since it is still open, I would like to implement it for Hacktober fest. Can I be assigned ?

sgaist added a commit to sgaist/pytest-mock that referenced this issue Oct 1, 2022
sgaist added a commit to sgaist/pytest-mock that referenced this issue Oct 4, 2022
nicoddemus pushed a commit that referenced this issue Oct 5, 2022
Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>

Fixes #259
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants