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

Advice request: monkeypatch vs mock.patch #4576

Closed
dimaqq opened this issue Dec 25, 2018 · 15 comments
Closed

Advice request: monkeypatch vs mock.patch #4576

dimaqq opened this issue Dec 25, 2018 · 15 comments
Labels
plugin: monkeypatch related to the monkeypatch builtin plugin type: docs documentation improvement, missing or needing clarification

Comments

@dimaqq
Copy link

dimaqq commented Dec 25, 2018

I'm working on a codebase where tests use a mixture of mock.patch and pytest's monkeypatch, seemingly based on authors' personal preferences.

The official docs for the latter, https://docs.pytest.org/en/latest/monkeypatch.html, refer to a blog post that's nearing its 10th anniversary; meanwhile the earlier made it into Python proper.

I wonder if there's official advice, like "use X", or perhaps "if you need feature Y, use Z" to choose between the two.

@asottile
Copy link
Member

It does seem to come down to personal preference as far as I've seen so far. There's really three options that work well for pytest (imo) so I'll outline them here. Note these are my opinions and not necessarily representative of others involved with the project or any sort of "official" stance. It's possible we should put something together in the documentation since it is a pretty common subject 👍

  1. monkeypatch
    • PRO: comes with pytest, no extra dependencies in python2 / python 3
    • PRO (or CON depending on your attitude here, MagicMock is some crazy shenanigans): is dead simple, no MagicMock, no call tracking, just normal attribute setting
    • CON: as it's a fixture, the scope is often more broad than expected instead of "just part of the function" or "just the function", it can often lead to patches "leaking" into other fixtures / etc. It's also difficult to control the ordering in some cases
      • ok this isn't strictly fair, there is a context manager version, it's just not the "default style"
    • CON: potentially less battle tested than mock (for example, monkeypatch.delattr doesn't handle classmethod or staticmethod #4536)
  2. mock
    • CON: for python2.x you need a dependency on the mock backport
    • PRO: if you're python3.x only, it comes with the standard library (unittest.mock)
    • PRO: many more features than monkeypatch (call tracking, MagicMock, assertion framework (though it has a really bad api (out of date article before mock 2.x made this ~slightly better))
    • PRO: tight control over mocked context via context managers / decorators (if you want fixtures, you can make those too with yield
    • CON: MagicMock is some crazy magic
  3. pytest-mock: adds a mocker fixture which uses mock under the hood but with a surface area / api similar to monkeypatch
    • Basically all of the features of mock, but with the api of monkeypatch. I don't like using it due to the same reasons I mentioned about scoping of patches in monkeypatch

In code that I write, I tend to stick to mock (if it's even necessary at all)

I also wonder if pytest should gut monkeypatch when dropping python2.x and replace the internals with unittest.mock 🤔

@RonnyPfannschmidt
Copy link
Member

personally, i despise mock, needing to use it implies a structural error, so i certainly want to keep monkey-patch for when doing controlled changes of 3rd parties not under my control, but for own code - the moment a mock becomes necessary its a indicator that a re-factoring is needed

@Zac-HD Zac-HD added type: docs documentation improvement, missing or needing clarification plugin: monkeypatch related to the monkeypatch builtin plugin labels Dec 26, 2018
@tuukkamustonen
Copy link

tuukkamustonen commented Mar 7, 2020

I understand this is a complicated topic, and that this ticket is not really a place to discuss the architectural patterns, and that I'm quite late with this reply, but could @RonnyPfannschmidt summarize what you mean by "needing to use it [mock] implies a structural error"? Or could you link to an article that describes the ideology that you phrase here?

@bluetech
Copy link
Member

bluetech commented Mar 7, 2020

I'm not @RonnyPfannschmidt but here is my opinion on why mock.patch/monkeypatch is usually better avoided.

Monkeypatching, by definition, breaks the abstraction barrier. Some code reaches into some other code and changes it bowls. The code calls some_function(), but what actually runs is patched_in_function(). The trouble with relying on internal details is that it is brittle. If the code is refactored to call some_other_function() instead, the test breaks, even if the behavior is exactly the same.

A better alternative is to "formalize" the relationship between the test and the code. The conventional way to do it is give the test explicit control over the particular thing it wants to patch, usually using dependency injection.

For an example I'll use the post linked by @asottile. In it, they have this code

def restart_server(server):
    ...

def restart_servers_in_datacenter(servers, datacenter):
    for server in servers:
        if server.endswith("-%s" % datacenter):
            restart_server(server)

and they want to write a test for restart_servers_in_datacenter, but without it actually going to restart actual servers. What they did was to patch the restart_server function, and they explain some problems they ran into and how they fixed them.

The alternative to patching is do something like this:

class ServerManager:
    # Takes some dependencies itself (for example).
    def __init__(self, http_client: HttpClient, auth_token: str) -> None:
        self.http_client = http_client
        self.auth_token = auth_token

     def restart_server(self, server: Server) -> None:
          self.http_client.post("restart server", auth_token=self.auth_token, {server: server.id})

def restart_servers_in_datacenter(server_manager: ServerManager, servers, datacenter):
    for server in servers:
        if server.endswith("-%s" % datacenter):
            server_manager.restart_server(server)

Now the test doesn't need to patch. It can do this:

fake_server_manager = mock.create_autospec(ServerManager)
restart_servers(fake_server_manager, servers, datacenter)
assert len(fake_server_manager.restart_server.calls) == 1

Now the test cannot make mistakes, it most provide its own implementation of the dependency. It is not possible for the real code to run accidentally. And there is no abstraction being broken, no peace is disturbed, just regular argument passing.

You can decide to fake at a deeper level, if you want to increase the coverage:

fake_http_client = mock.create_autospec(HttpClient)
server_manager = ServerManager(fake_http_client, 'fake token')
restart_servers(server_manager, servers, datacenter)
assert len(fake_server_manager.restart_server.calls) == 1

Sometimes it's beneficial to go "full Java" and do this:

class ServerManager(meta=ABCMeta):
      @abstractmethod
     def restart_server(self, server: Server) -> None: pass

# As before.
class RealServerManager(ServerManager): ...

# Tests can use this.
class FakeServerManager(ServerManager):
     def __init__(self) -> None:
          self.restarted_servers = {}  # type: Set[Server]

     def restart_server(self, server: Server) -> None:
          self.restarted_servers.add(server)

depending on the case.

This intersects with general OO good practices for testable code, see here for example. This style of programming is also enforced in the object-capability security model, which I (personally) hope will gain more prominence in the future.

As a disclaimer, I should say that sometimes monkeypatching in tests is necessary, for example when dealing with external code you have no control over. And sometimes it is just easiest and putting more effort is not worth it. And sometimes you intentionally want to test some internal detail. I don't mean to be dogmatic.

@tuukkamustonen
Copy link

tuukkamustonen commented Mar 10, 2020

@bluetech Thanks for explaining that (and sorry for late response as I'm travelling). It's a good writeup, I agree with that. However:

The distinction you make in your post is monkeypatching vs dependency injection / inversion of control (whatever we want to call it). My question, however, was (what I thought RonnyPfannschmidt was referring to) about mocking vs not mocking (using mock objects, not mock.patch or monkeypatch). Ronny wrote:

personally, i despise mock, needing to use it implies a structural error, so i certainly want to keep monkey-patch -- the moment a mock becomes necessary its a indicator that a re-factoring is needed

Maybe I'm interpreting this wrong, but to me it seems that he says "mock objects are bad" but "monkeypatching (either mock.patch or pytest monkeypatch) is good"?

@RonnyPfannschmidt
Copy link
Member

i consider both practices bad for different reasons,
i consider monkeypatches as acceptable practice for systems where certain hook point are not given (like a 3rd party library not intended to be set up for partial tests)

i consider mock objects bad because they encode expectations that may eventually differ with real systems, and generally try to have in memory or limited interface implementations instead of mocks when possible

however even if there is a systematic weakness to both approaches, they still win over playing architecture astronaut

so there is a number of situations where the use of those approach beats making them structurally possible for YAGNI or no controll of the upstream anyway

@dimaqq
Copy link
Author

dimaqq commented Mar 11, 2020

Since I've started this discussion, allow me to share what I've learned from experience over the past year a bit.

(The examples below are real use cases from the codebase, stripped of project specifics and simplified for clarity. Pundits may offer better solutions for each case, but I'll stand by my examples, they are the work of several smart individuals, constrained by requirements of a particular problem domain, and lived through many PRs)

Mocking is a valuable technique, especially for unit tests, that is focused on only one aspect of code under test, for example:

def test_reject_ancient():
    with mock.patch("time.time", return_value=42):
        old_request = sign_request(data)
    with pytest.raises(TooOld):
        verify_request(old_request)

Here, something specific is patched just to set up some tiny detail. I don't care about the exact value of time, as long as it's way long ago. The "cost" of the tight coupling in the test is justified by keeping the implementation simple.

Which can be extended to something a bit more complex (may or may not be the best choice):

@asynctest.mock.patch("mymodule.backend.SomeSideEffect", autospec=True)
async def test_simple_login(_, ephemeral_server, test_user):
    async with aiohttp.ClientSession() as session:
        async with session.get(ephemeral_server.url) as resp:
            assert resp.status == 200

Here, a simple test is done over 2 fixtures and 1 other thing is mocked away, because it's outside of test scope.

As test complexity and purpose gets closer to functional (or integration) testing, fixtures rule, and some fixtures are likely to ** monkey-patch**, for example:

@pytest.fixture
def config():
    monkeypatch("some.config", TEST_DEFAULTS)

@pytest.fixture
async def server(config):
    async with start_a_server() as s:  # uses global config
        yield s

...

async def test_login(server, client, user, parser, logger):
    resp = await client.post(server, user)
    assert parser.parse(resp.text) == "yes, it worked"
    assert logger.recorded({"result": "success", "username": user.name})

Here, fixtures and fixture dependencies are used extensively to control the "life cycle" of the monkey patches. The fixtures are essentially fake objects and perhaps, more generally, test doubles and their implementation relies on mokeypatch to insert (and later remove) the double into the codebase.

Note that nowhere here I've seemingly concerned myself with theoretical difference between mocking and monkeypatching. I'm being practical and use whatever works best (🎉pytest🎉) balancing complexity of code/fixture/test.

@bluetech
Copy link
Member

@tuukkamustonen

Maybe I'm interpreting this wrong

Yes, I misread what @RonnyPfannschmidt said. FWIW I think about the opposite -- I try to avoid patching, but I'm perfectly OK with mock.create_autospec() mocks as a shortcut for unittests.

@dimaqq

If I apply my suggestion to your examples, then I would avoid mock.patch in these cases.

Instead of

    with mock.patch("time.time", return_value=42):
        old_request = sign_request(data)

I would have sign_request accept a asof_time: float parameter, and use that. Real code can pass time.time(), test can pass a hardcoded value -- no patch needed!

For test_simple_login, I guess it is more of an integration test (running against a real server)? For these cases I try to setup a "test environment" (usually configured through some settings object given in initialization), and for this environment I provide fake/mock implementation of "side effect"-y services. Each dependency (sometimes called "service") requires an implementation. Patching can be fine, but it's very fragile. If mymodule.backend.SomeSideEffect changes its name in any way, suddenly the tests start to perform this side effect (hopefully it doesn't launch nuclear missiles 🚀 😃 ).

For monkeypatch("some.config", TEST_DEFAULTS), I would have start_a_server() take the config, instead of having it use a hardcoded import path. This would avoid the need to patch here as well. Usually web frameworks have some App or Application entry point which allows this.

@The-Compiler
Copy link
Member

I also want to point out that sometimes (especially for common libraries or usecases), there are third-party patching solutions which avoid having to deal with mocks by hand. I think they can make a lot of sense when dealing with things which are inherently "in the way" (like external HTTP services). In those cases, changing the code to pass in e.g. fake responses might mean those responses aren't the same as they would be in reality, and the "over-generalization" might lead to much more complex code.

Some examples I'm aware of:

@MartinThoma
Copy link

I was just about to ask the same question: In Python 3.6+, does pytest.monkeypatch provide any value over unittest.mock.patch? A small concrete example would be pretty awesome 🙂

I have seen the Monkeypatching/mocking modules and environments article (and the linked article) and was wondering if this is only interesting for applictions which have to handle Python versions before Python 3.3 where unittest.mock with the patch decorator was introduced.

It would be awesome if you could help me here. If it's desired, I can make a DOC-PR to add the outcome.

I didn't completely read this issue as most of the discussions seemed to be about "is mocking a sign for bad code". However, I was confused in the beginning by @asottile stating MagicMock is a con of patch. I might just not know something, but can't you use the new parameter of patch to use whatever you want instead of a MagicMock?

@The-Compiler
Copy link
Member

@MartinThoma It boils down to "it's just much more simple to use, with less magic involved" in my eyes. You get a pytest fixture (rather than a decorator), and it's essentially just monkeypatch.setattr(thing, 'attribute', value), rather than having a quite awkward signature which does a lot of things at once and is hard to explain.

@asottile
Copy link
Member

I believe there's no official recommendation because it's really about opinions and trade offs -- for instance I will never use monkeypatch because I've been burned by it's unknown scope duration (sometimes leaking to places I don't expect) whereas the context manager form of unittest.mock is explicit on what it affects. my con above about MagicMock is it's all too easy to leak those into apis that should TypeError / AttributeError but magically succeed (specs can help with this for the most part though). The design of MagicMock's assertions is also problematic (a typo'd assert_whatever can lead to a test silently succeeding!)

@MartinThoma
Copy link

Cool, thank you @The-Compiler and @asottile ! (I miss "thank you" as a Github reaction :-) )

As people might stumble over this via Google searches: I can recommend Demystifying the Patch Function (Lisa Roach, Pycon 2018) if you just get started with patching / MagicMock + spec / autospec / spec_set. She explains this really well.

@keu
Copy link

keu commented Oct 27, 2022

As of 2022 pytest-mock covers 100% of cases for me and is much clear to use than mock and definitely than monkeypatch

@nicoddemus
Copy link
Member

Thanks everyone for the great discussion, but I think we can close this as there's no actionable item. 👍

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: docs documentation improvement, missing or needing clarification
Projects
None yet
Development

No branches or pull requests

10 participants