Mocking properties

[bhardin]

Discussion on best practice of Mocking properties.

[adambom]

mock.patch should be considered harmful. I'm seeing it abused all over the code base. Here's an example of what I'm talking about. Let's say I have a class Cat that depends on a function from the sounds module.

# cat.py
from sounds import play_sound

class Cat:
    def meow(self):
        play_sound('meow')
        return True

# sounds.py

import midi

def play_sound(sound):
    return midi.play(sound)

Ok great. It's working! Now to write some tests:

from cat import Cat

def test_cat_can_meow(self, mocker):
    cat = Cat()
    result = cat.meow()
    assert result

Ok, but now I'm in trouble because we don't have midi support in our unit tests, so I have to mock the play_sound method...

from cat import Cat

def test_cat_can_meow(self, mocker):
    mocker.patch('sounds.play_sound', lambda self: 0)
    cat = Cat()
    result = cat.meow()
    assert result

But wait! My patch didn't work.... Oh now I see it.... because Cat imports play_sound first, mocker.patch has no effect. Hmm...

Aha!

I'll be sneaky...

# cat.py

class Cat:
    def meow(self):
        from sounds import play_sound
        play_sound('meow')
        return True

Hold on a second! Now we've created all sorts of problems for ourselves. For one, it's confusing. Anyone who's casually browsing your Cat class will have no idea why you're importing play_sound inside a function. Secondly, you'll have to repeat that every other place you call anything from the sounds module. Finally, if cat.py imports anything else that depends on sounds, and that code isn't being sneaky as well, your tests are going to start failing in mysterious ways. Not to mention that we've now hacked our pristine code base specifically so we can get the tests to pass.

Let's just all agree that we should stop doing this. And mocker.patch is a pretty surefire sign that this is already happening, or is likely to happen soon. There's a better way. Our old friend dependency injection will save us!

Here's what I'm talking about:

# cat.py

class Cat:
    def __init__(self, play_sound):
        self.play_sound = play_sound

    def meow(self):
        self.play_sound('meow')
        return True

Or better yet, make the constructor take an entire object that encapsulates all the functionality in the sounds module:

# cat.py
from sounds import SoundPlayer

class Cat:
    def __init__(self, sounds: SoundPlayer):
        self.sounds = play_sound

    def meow(self):
        self.sounds.play_sound('meow')
        return True

We can easily test this using good ol' MagicMock.

from cat import Cat

def test_cat_can_meow(self, mocker):
    cat = Cat(MagicMock())
    result = cat.meow()
    assert result

Problem solved. No patching necessary.

It's allowed to have a factory method on the Cat class, perhaps, to handle instantiating a class with some module-level imports if we decide we want that as a convenience:

# cat.py
from sounds import SoundPlayer

class Cat:
    def __init__(self, sounds: SoundPlayer):
        self.sounds = play_sound

    def meow(self):
        self.sounds.play_sound('meow')
        return True

    @classmethod
    def factory(cls):
        return cls(SoundPlayer.factory()) # See what I did there ? ;)

Anyway, this is something we should be doing more of...

[rbusquet]

from cat import Cat

def test_cat_can_meow(self, mocker):
    mocker.patch('sounds.play_sound', lambda self: 0)
    cat = Cat()
    result = cat.meow()
    assert result

this is covered in the docs: https://docs.python.org/3/library/unittest.mock.html#where-to-patch

to patch it correctly you need to patch where its used:

from cat import Cat

def test_cat_can_meow(self, mocker):
    mocker.patch('cat.play_sound', lambda self: 0)
    cat = Cat()
    result = cat.meow()
    assert result

there are other problems with the test above, but I don't think the usage of patch is necessarily a problem.

[adambom]

@rbusquet

Having to know where stuff is looked up and when is another layer of mental overhead that contributes to tech debt over time. We have to remember that our code base will be touched by hundreds of people, and they're not all going to be willing (or able) to spend the cycles to figure out how to surgically patch your code. I think there's abundant evidence of this in our code as it stands right now.

Dependency injection is foolproof, robust, and it won't break. It has other advantages too -- the other day I was running a jupyter notebook that called a method that writes to the database. Since my class allowed for dependency injection, I could easily swap in a read-only version of the dependency.

[luismmontielg]

👍 lets reduce the number of patches by having our objects depend on the stuff they need, and require it at initialization

[bhardin]

I’m all for it. Isn’t this the way the style guide suggests? Should we have another bad example for mock.patch

[adambom]

Well I'm still of the opinion that mock.patch should be considered harmful. I don't think the style guide does anything to discourage it in favor of dependency injection. I'd propose adding some discussion about that, but I'm not sure that I've sufficiently convinced @bhardin or @rbusquet.

[levinsamuel]

I agree with @adambom. I think patch as a concept should be treated with inherent suspicion.

Important functionality

What is patching doing? Inherently, it is saying "I'm not testing this." So we're writing a test and we're specifically excluding things from our test. How do we know that what we've patched isn't so critical to the purpose of the method that by patching it, we'd made our test useless?

def important_method(param):
   result_1 = _complex_calculation_1(param)
   result_2 = _complex_calculation_2(result_1)
   ...
   result_N = _complex_calculation_N(result_N_minus_1)
   return result_N * 2

And I'll see a test that patches _complex_calculation_N with a mock value, and asserting that the result is that value times 2. This is clearly not in any way a substantial test of important_method, but it appears to be one. After all, a test is really calling that method.

Some methods are so large and complex that they can't be unit tested without a lot of mocking, but I think mocking is just a way of glossing over a method that needs to be refactored such that its parts can be tested without mocking.

Dependency injection avoids this because it allows the code itself to tell you what implementation details are important and necessary to the method and which ones can be outsourced, meaning that you can still test the method if you change those details. In the Cat example, it's important that meow invokes a sound player, but it doesn't care about the implementation of that player.

Locking implementation details

I've seen this test I don't know how many times:

def my_public_method(data):
    result = _my_inner_method(data)
    return result

class TestMyPublicMethod:
    def test_my_inner_method_is_called(self, mocker)
        patched_inner_method = mocker.patch('_my_inner_method')
        my_public_method({})
        patched_inner_method.assert_called_once()

This isn't just a useless test. This is a test that actively prevents what might be a beneficial refactor. It eliminates the benefit of encapsulation.

The point of encapsulation is that callers to my_public_method can continue to use it if the implementation changes. A test that calls the method and asserts the result can ensure that changes to the method don't break it. But this test is guaranteed to break if you change the implementation. It's actively working against best development practices.

Of course we can avoid this through testing standards, but I would argue that the best testing standard to adopt here is to treat every patch with suspicion: why is it necessary to patch here? The only really good reason that comes to mind for patching is to avoid integrations, such as DB calls. But even then, it would be much better to develop service boundaries/dependency injection such that test environments run with mock services that make patching unnecessary.

def test_cat_can_meow(self, mocker):
    mocker.patch('cat.play_sound', lambda self: 0)
    cat = Cat()
    result = cat.meow()
    assert result

there are other problems with the test above, but I don't think the usage of patch is necessarily a problem.

It is a problem here because it locks us into the cat module importing a method called play_sound.

[levinsamuel]

I’m all for it. Isn’t this the way the style guide suggests? Should we have another bad example for mock.patch

I think there should be a section on how mock.patch potentially indicates bad design and how to structure code in order to avoid it @bhardin

[erickmendonca]

I believe this discussion about mock.patch makes a lot of sense. Excessive mocking/patching is a sign of bad code design somewhere. I think we are free to add a section with some advice on how to avoid abusing it. By adding some actual examples, we can guide new engineers into best practices (e.g. mocking Django models or private methods vs creating services and/or dependency injection).