Better PHP unit testing: avoiding mocks

What are mocks?

If you've ever seen a unit test, you've seen mocks. If you've written a unit test, you've (almost definitely) written mocks. Mocks are a type of test double which make assertions on how the object will be used; what methods will be called, what order, how many times, with what parameters. In PHP, they are instantly recognizable by code which looks like this:

$mockFoo->expects($this->once())->method('bar')->with($this->equalTo('baz'));

This is, on occasion, a legitimately useful thing to do. Sometimes we have complex business logic interacting with dependencies in such a way that it's difficult in practice to test without making direct assertions about how the dependency was used.

My contention, however, is that these cases are a rare exception, that - particularly in the PHP community but also more broadly - mocks are vastly overused and over-relied on, have a tendency to lead to badly constructed, uninformative, brittle tests and are simply neither needed nor preferable in the majority of cases.

This is the contention I want to explore in today's blog post.

Imagine a service to test

For the sake of argument, I've written a simple content negotiator class for an API which can return data in multiple formats. Its job in my imaginary API is to read the request path and headers and decide a) whether the client has requested a valid format at all and b) what is the highest priority supported format they have asked for.

We will give the highest weight to the file suffix on the end of the URI - if a client requests /foo.json, we'll assume they want JSON, even if the Accept header only specifies application/xml. If they don't supply a suffix, we'll go for the highest weighted Accept header we can support.

<?php
namespace App\Services;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException;

class ContentNegotiator
{
    private $supportedFormats = [
        'json' => 'application/json',
        'html' => 'text/html',
        'xml' => 'application/xml',
    ];

    private $defaultFormat = ['format' => 'json', 'mime' => 'application/json'];

    public function negotiate(Request $request)
    {
        $format = pathinfo($request->getPathInfo(), \PATHINFO_EXTENSION);
        if (!empty($format)) {
            if (isset($this->supportedFormats[$format])) {
                return ['format' => $format, 'mime' => $this->supportedFormats[$format]];
            }
            throw new NotAcceptableHttpException('Requested content type not supported');
        }
        $acceptableFormats = $request->getAcceptableContentTypes();
        if (empty($acceptableFormats)) {
            return $this->defaultFormat;
        }
        foreach ($acceptableFormats as $mime) {
            $format = $request->getFormat($mime);
            if (isset($this->supportedFormats[$format])) {
                return ['format' => $format, 'mime' => $mime];
            }
            if ($mime == '*/*') {
                return $this->defaultFormat;
            }
        }
        throw new NotAcceptableHttpException('Requested content type not supported');
    }
}

Writing the unit tests

Great, I've written a small service and I have a method I can test. This method has a dependency - it needs a Request object to operate against. Let's test it using a mock for that dependency.

<?php
use Symfony\Component\HttpFoundation\Request;

use App\Services\ContentNegotiator;

class ContentNegotiatorTest extends \PHPUnit\Framework\TestCase
{
     protected function setUp(): void { {
        $this->negotiator = new ContentNegotiator;
    }

    public function testNegotiateWithAcceptHeaderContainingOneSupportedAndOneUnsupportedContentTypeReturnsSupportedType()
    {
        $request = $this->createMock(Request::class);
        $request->expects($this->once())->method('getAcceptableContentTypes')->with()->willReturn(['application/other', 'application/json']);
        $request->expects($this->exactly(2))->method('getFormat')->withConsecutive(
            [$this->equalTo('application/other')], [$this->equalTo('application/json')]
        )->will($this->onConsecutiveCalls(null, 'json'));
        $expected = ['format' => 'json', 'mime' => 'application/json'];
        $format = $this->negotiator->negotiate($request);
        $this->assertEquals($expected, $format);
    }
}

The test passes and it ostensibly demonstrates what my test says it does; most PHP devs would look at it and say it's a good enough test for this particular scenario.

But look a little more closely. The test is tightly bound to the implementation logic of the negotiate method. If I change how my subject determines what content type to return, even if it would return the same result for the same input, I now need to rewrite large portions of my test.

The test is hard to read, you have to go through all those expectations and work out what's going on. I have to either repeat values like application/json or obscure them behind a variable name.

Worst of all, I'm not even testing what I claim I'm testing; the Accept header of a request has nothing to do with this test, rather it is a test on the result of one call to getAcceptableContentTypes and two calls to getFormat.

Writing a better unit test

Let's test this again without a mock.

  public function testNegotiateReturnsHighestWeightedSupportedFormatInAcceptHeaderForRequestWithNoPathSuffix()
  {
      $request = Request::create(
        '/some/path', 'GET', [], [], [], ['HTTP_ACCEPT' => 'application/xml;q=0.9, application/json, */*;q=0.8']
      );
      $expected = ['format' => 'json', 'mime' => 'application/json'];
      $format = $this->negotiator->negotiate($request);
      $this->assertEquals($expected, $format);
  }    

Sweet Jesus, that's better! You can tell straight away at a glance what this test is doing and more cruicially, I'm only testing the public interface of the negotiate method. This test doesn't care how that method decides what format to return, it only cares what it returns. I can completely change the internal workings of the negotiate method and as long as I haven't broken the method, I haven't broken the test.

Best of all, I am now testing what I claim to be testing - the actual functionality I care about, the thing I wanted to test, which is that the format I expect is what's returned for a request with a particular Accept header.

So why do some people prefer the first approach with the mock?

Misunderstanding unit tests

To answer the question immediate above, some people will insist that the second test is not a unit test because it uses a real dependency object (Request).

To me, this is a fundamental misunderstanding of what is meant by unit testing and specifically the isolation of tests. Mockists take the view they do because they believe the first test, using the mock, somehow achieves a greater degree of isolation for the feature I'm testing by not "polluting" the test subject with a real object.

Part of the confusion here comes from the fact that the term unit test isn't quite as unequivocal as you might think. Different people have different ideas about the scope of these tests. When I call something a unit test, I have four essential criteria:

1. It tests a single feature of a single component (probably a method of a class, but not necessarily).

2. It doesn't require any environment or system outside of the test harness to run. No database, no network access, etc., just the code we're testing and the test framework itself.

3. It does not alter the state of the system or test harness, it does not affect the environment it runs on.

4. It is not affected by the state of the system or environment it runs on.

The corollary of (3) and (4) is that unit tests are repeatable, can run in parallel and run in any order. They do not cause side effects for each other, they are isolated from each other.

That's it. If a test meets these criteria, I call it a unit test and it will live in my unit test suite.

There are certainly many cases where the use of a real dependency isn't possible in a unit test and we need to switch out for some kind of double - we're never going to be injecting a real database or a real HTTP client, for example. But in our case above, nothing about the behaviour of a real Request instance violates these criteria.

Misunderstanding isolation

The confusion of mockists, in my view, comes from misunderstanding this point of isolation. Unit tests are supposed to be isolated from the environment, from each other, not from any other bit of code in your code base.

These mockists may claim in the case of my second test that if it fails, I haven't isolated the unit under test sufficiently and won't know where the problem occurred. But is this true? If the Request class also has a passing suite of unit tests, a failure in this test is, in anything except some highly unusual edge case, going to be in the domain of the negotiate() method I'm testing. So I don't accept the argument that it would be meaningfully harder to track down the problem if it failed (or, as I'll get to in a moment, that the mockist alternative is achieving any better isolation). And if the Request class itself actually is broken, I want tests on other components which make use of it to fail, because it will instantly highlight to me the reach of the problem throughout my system.

The other test using a mock, however, is brittle, verbose, requires knowledge of the implementation details of the function it's testing, will break if I change that implementation, will continue to pass even if the behaviour of the real Request class changes in some incompatible manner and it's much harder to read. Moreover, if the claimed motivation for the mockist technique is that it prevents the unit under test interacting with external code outside the test's domain, it has failed that objective.

The mock Request class is a child of the real Request class. It's just had a lot of magic added behind the scenes by PHPUnit to manipulate its behaviour and track its method calls. That, if anything, makes it a more complicated external code interaction than using the real object.

The mockist assumes the mock works as intended - but why should that be a more reliable assumption than the unit-tested Request class works as intended? The very point of unit tests is to assure us that library-level components probably do work correctly.

The point here is that using mocks or other types of test double does not somehow magically prevent your unit from interacting with external code, you're just giving it different external code to play with. So I reject the mockist claim that the second example is not a unit test just because it has a "real dependency". And more importantly, the second test is a qualitatively better test than the first one.

There are many, many cases where it's useful to switch out a real dependency in a unit test for some kind of double, be it a mock, stub, fake or whatever. But the reason we write tests is to catch unexpected deviations in system behaviour. This means you should always prefer to test interface over implementation and to test against real objects unless doing so is awkward.

The other alternative - other types of test double

If it is awkward to use a real dependency - say for some reason my Request object relied on some external environment factor not available to my test harness - you will almost always fair better to use a stub or fake than a mock. A lot of people forget you don't need to rely on a mocking framework for test doubles. Quite often simply writing your own StubRequest (or whatever) class which implements the interface you need it to results in cleaner, more readable, more reliable tests. Fakes are something I intend to explore more in an upcoming blog post.

Further reading

• Martin Fowler: Mocks aren't stubs
• Code without rules: what is unit testing
• Stack exchange: Dependency problems when mocking
• Stack overflow: When should I mock?

---

Comments