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

Add explicit support for unittest.mock.Mock in monkeypatch fixture #12145

Open
soceanainn opened this issue Mar 20, 2024 · 3 comments
Open

Add explicit support for unittest.mock.Mock in monkeypatch fixture #12145

soceanainn opened this issue Mar 20, 2024 · 3 comments
Labels
plugin: monkeypatch related to the monkeypatch builtin plugin type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@soceanainn
Copy link

soceanainn commented Mar 20, 2024

What's the problem this feature will solve?

In my organisation, we have a lot of tests split between unittest.mock.patch, pytest-mock and monkeypatch.

As we try to standardise usage to monkeypatch, we end up with a lot of cases looking like:

def my_test(monkeypatch: pytest.MonkeyPatch) -> None:
  mock_foobar: Mock = MagicMock()
  monkeypatch.setattr(foo, "bar", mock_foobar)
  # ...

There are two main reasons we do this:

  1. To inject Exceptions into unit tests (i.e. mock_foobar.side_effect = MyException("...")).
  2. To assert based on specific calls to the patched attribute.

Describe the solution you'd like

Support mocking in monkeypatch. I'm happy to contribute this feature, although maybe this is not something you want to actively support.

I propose introducing a new function to monkeypatch called mockattr (to match setattr). This function would patch the attribute to be an instance of MagicMock or AsyncMock (depending on whether the patched attribute is awaitable or not). Potentially new functions for mockitem and mockenv should be introduced too for parity for setitem and setenv.

def mockattr(
  self,
  target: str | object,
  name: str | notset = notset,
  raising: bool = True,
) -> MagicMock | AsyncMock:
  # ...

Taking my example above, the code would be simplified slightly. It would also prevent having to know to manually instantiate an instance of MagicMock vs AsyncMock depending on the return type of the function.

def my_test(monkeypatch: pytest.MonkeyPatch) -> None:
  mock_foobar: Mock =  monkeypatch.mockattr(foo, "bar")
  # ...

Alternative Solutions

What I have described above is essentially what we have done internally in my organisation by creating a pytest fixture that exposes mockattr. However we end up relying on monkeypatch internals (specifically derive_importpath) in order to inspect the attribute before monkeypatching it to determine whether it is awaitable or not, which we would like to avoid.

An alternative solve, would be to update pytest-mock to use monkeypatch, so that it has similar patching semantics as monkeypatch instead of using unittest patch semantics. However that would most likely result in breaking changes, and I think it's valid for that plugin to continue to exist as is (if people want to use unittest.mock semantics within pytest).

Another solve to simplify these use cases, would be to have setattr return the patched value, which would result in code that looked like this:

def my_test(monkeypatch: pytest.MonkeyPatch) -> None:
  mock_foobar: Mock =  monkeypatch.setattr(foo, "bar", MagicMock())
  # ...

Additional context

I see some similar conversations from the past:
#4576
pytest-dev/pytest-mock#9

@RonnyPfannschmidt
Copy link
Member

we should mirror the mock apis with a mock prefix and forego selective undo, people can use the monkeypatch.context context manager

@The-Compiler
Copy link
Member

An alternative solve, would be to update pytest-mock to use monkeypatch, so that it has similar patching semantics as monkeypatch instead of using unittest patch semantics.

Could you elaborate on this? How does pytest-mock not already do what you want? I'm -1 on introducing a different way to do the same thing into the core when we already have a well maintained and popular plugin that solves the same problem.

@soceanainn
Copy link
Author

soceanainn commented Mar 21, 2024

Actually thinking about it more carefully - I was wrong to suggest that pytest-mock could be updated to solve this. Extending pytest-mock to allow patching with non-mock objects wouldn't make a lot of sense.

The issue with using pytest-mock as the only means of patching is that it defaults to using MagicMock. MagicMock can be problematic / difficult to work with (it has a lot of 'magic' that leads to some pitfalls for developers). As a result I think a lot of teams end up in a situation where they use a combination of monkeypatch (for simpler cases) as well as one of either pytest-mock or unittest.mock (when they need more fully featured 'mocking').

It forces developers to understand the semantics of two libraries in order to patch objects in unit tests. We shouldn't need more than one tool to solve one problem (patching).

IMO developers should have a single way of patching (monkeypatch) with better support for patching with MagicMock/AsyncMock for the cases where that is needed.

If the answer was "just use pytest-mock" or any other plugin - I would be happy. But I think we currently exist in a limbo where you end up using multiple tools to patch objects when writing tests in Pytest.

@Zac-HD Zac-HD added type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature plugin: monkeypatch related to the monkeypatch builtin plugin labels Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: monkeypatch related to the monkeypatch builtin plugin type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests

4 participants