Mocking functions

[bhardin]

Discussion on best practice of Mocking functions.

[mannysz]

We should be careful mocking functions. You should only mock functions from instances, not classes. If you mock a class function, all tests that uses that method on tests running after your mock will be also mocked with the same MagicMock instance.

A classic example to avoid:

# bad example
Certificate.objects = MagicMock(spec=QuerySet)

# good example
mocker.patch.object(Certificate, 'objects', MagicMock(spec=QuerySet))

[emyller]

I would also encourage not to assign mocks to members of either classes or even instances. The reason is that it is easier to accidentally leaving a global mock just because they both share similar writing (member assignment), and also helps not to worry about creating an instance beforehand:

CertificateManager.get_queryset = MagickMock()  # REALLY bad example

# This calls `__init__`, which may or may not use the to-be mocked member
# and can/will potentially mask test failures / coverage.
manager = CertificateManager()
manager.get_queryset = MagicMock()  # Assignment

Instead, this is what I recommend:

with mock.patch.object(CertificateManager, 'get_queryset') as get_queryset:
    CertificateManager().another_method()
    assert get_queryset.assert_called_once_with()

Most of the time, though, I do decorate the test function with mock.patch.object; it is both safe and helps me keep the test function focused into one simple assertion rather than multiple concerns.

[mannysz]

@emyller if you read the styleguide for tests you'll realize we don't use the decorators.

[emyller]

@emanuelcds I read it but personally I can't see a reason not to use decorators, especially because:

• There is a clear ordering defined for the test function arguments: patches, parametrize members then fixtures, if I recall correctly.
• They are safe since they will always undo effects after each test -- the same behavior is also offered by pytest-mock.
• By forcing the mocked object throughout the entire test, it helps to granulate cases and keep the tests small and more readable.

That said though, I am perfectly fine with not using patch decorators in our projects -- we already have pytest-mock which is very handy. I just like them where they fit well. :wink:

[tizz98]

I think we should be consistent with how functions are mocked, whether at the class level or instance level.

From @emanuelcds example:

# bad example
Certificate.objects = MagicMock(spec=QuerySet)

# good example
mocker.patch.object(Certificate, 'objects', MagicMock(spec=QuerySet))

But I think that this should also apply to instances:

# bad example
cert = Certificate()
cert.save = MagicMock()

# good example
cert = Certificate()
mock.patch.object(cert, 'save')

[mannysz]

@emyller as you said They are safe since they will always undo effects after each test -- the same behavior is also offered by pytest-mock.

Why we don't use unit test decorators

Given pytest mock offers the same effect, there's no reason to use unittest.mock.patch and patch.object decorators since the outcome is an argument madness when you have more than a couple objects to mock.

e.g.:

@patch('package.module.method_one')
@patch('package.module.method_two')
@patch('package.module.method_three')
@patch('package.module.method_four')
def test_when_x_then_y(self, mock_four, mock_two, mock_three, mock_four):
    ...

Observe that it's very hard to keep the sanity on the argument order and make it readable with a long list of patch calls, and also sometimes you mock something that you don't need in fact to use the mock instance, making the argument passed useless.

When using mocker.patch gives you the ability to mock whatever you need whenever you need inside a code block making it more readable. e.g:

def test_when_certificate_saved_validates_data(self, mocker):
    # method that will be checked in test
    validation_method = mocker.patch.object(Certificate, 'validate')

    # method that won't be used
    mocker.patch.object(Certificate, 'external_hook')

    # object that needs specific implementation
    mocker.patch('package.module.Class', mocker.MagicMock(spec=Issuable))

    cert = Certificate()
    cert.save()
    assert validation_method.called

Why we don't use fixtures

Fixtures forces you in the majority of times to adapt your tests to fit under what they define, and if you need fixtures you're probably not writing unit tests (or you're testing too much in a single case inside your unit tests).

Acceptable cases to use fixtures (still dangerous)

I'm okay using them in integration tests since you really need to test several different situations but keep in mind these two situations: 1) if you will reuse fixtures declared in tests set_up (rebuilt once per test case), you'll probably run a lot of code that is not used inside all cases. 2) if you will reuse fixtures declared in tests set_up_class (rebuilt once per test suite), you need to keep in mind all the data you're using can't be changed in any test case, otherwise the results of a test will interfere in the next test case to be run, and test cases should be stateless and independent. If you need a stateful test considering the previous step, consider writing a browser or end-to-end test. 3) If you decide to go and use fixtures, don't do it in unit tests (since tests should be independent from each other in any level) and declare explicitly which cases depends on that given fixture.

[emyller]

@emanuelcds I see your point about using unittest.mock's decorators and context managers versus using pytest-mock's mocker utility and I agree with it. Been using it for a few days now and it's really handy.

About valid use cases for fixtures, I'd like to add another one:

3. Shortcuts to what's being tested in the current test module, such as the very subject being tested.

[mannysz]

I'm editing and adding your case to my response to let people read this from the same comment (and changing a little bit, because we don't test features based in modules anymore). Thanks for contributing with that @emyller!

Also, a test module should reflect a class, and test suites inside the test module should reflect each method to be tested (as far as you have a test case method inside each test suite representing each condition you want to test in each of those methods).

[jourdanrodrigues]

@tizz98 I replied to your proposal about mocking instances with patch.object in this comment.

TL;DR: I don't think the trade-off is good enough.