How to reconcile "not mocking what you don't own" with "expectations" in unit tests?

Question

I'm putting together a set of models for use in a Zend Framework 2 application. So, each model has a table class which acts as the interface between the model and database for querying. Using the ZF2 TableGateway means that even a fairly simple query, like getting the number of hits from a page log between two dates, ends up with some complex calls:

public function getDailyHits($start, $end)
{
    $select = $this->tableGateway->getSql()->select();

    $select->columns(array(
            "date" => new Expression("DATE_FORMAT(timestamp, '%Y-%m-%d')"),
            "hits" => new Expression("COUNT(timestamp)"),
        ));

    $select->where->between("timestamp", $start, $end);

    $select->group("date");

    $results = $this->tableGateway->selectWith($select);



    $dailyHits = array();
    foreach ($results as $result) {
        $dailyHits[$result->date] = $result->hits;
    }

    return $dailyHits;
}

When it came to developing the unit test, I wrote up a bunch of expectations:

public function testGetDailySearches()
{
    // Setup
    $result = new \stdClass();
    $result->date = "2015-01-01";
    $result->searches = 42;

    $resultSet = array($result);

    $sql = $this->getMockBuilder("Zend\Db\Sql\Sql")
        ->disableOriginalConstructor()
        ->getMock();

    $select = $this->getMockBuilder("Zend\Db\Sql\Select")
        ->disableOriginalConstructor()
        ->getMock();

    $where = $this->getMockBuilder("Zend\Db\Sql\Where")
        ->disableOriginalConstructor()
        ->getMock();

    $tableGateway = $this->getMockBuilder("Zend\Db\TableGateway\TableGateway")
        ->disableOriginalConstructor()
        ->getMock();

    $searchLogTable = $this->getMockBuilder("Usage\Model\Table\SearchLog")
        ->setConstructorArgs(array($tableGateway))
        ->setMethods(null)
        ->getMock();



    // Expectations
    $tableGateway->expects($this->once())
        ->method("getSql")
        ->will($this->returnValue($sql));

    $tableGateway->expects($this->once())
        ->method("selectWith")
        ->with($this->equalTo($select))
        ->will($this->returnValue($resultSet));



    $sql->expects($this->once())
        ->method("select")
        ->will($this->returnValue($select));

    $select->expects($this->once())
        ->method("columns")
        ->will($this->returnValue($select)); // TODO: need to assert parameters

    $select->expects($this->once())
        ->method("__get")
        ->with($this->equalTo("where"))
        ->will($this->returnValue($where));

    $where->expects($this->once())
        ->method("between")
        ->with(
            $this->equalTo("time"),
            $this->equalTo("2015-10-01 00:00:00"),
            $this->equalTo("2015-10-07 23:59:59")
        )
        ->will($this->returnValue($select));

    $select->expects($this->once())
        ->method("group")
        ->with($this->equalTo("date"));



    // Assertions
    $this->assertEquals(
        array("2015-01-01" => 42),
        $searchLogTable->getDailySearches("2015-10-01 00:00:00", "2015-10-07 23:59:59")
    );
}

I had a go at using Prophecy to reduce the verbosity in these tests, but got tripped up on them magic __get method needed to read the $where property from the Select object.

After reading through a couple of threads on the Prophecy site, I uncovered a link to That's Not Yours. It makes a lot of sense. It also would reduce a lot of boilerplate in some tests if I just subclass TableGateway (or even maybe just mock it) so that all the Zend\Db behaviour is retained.

But, then what happens to my expectations? Is this a case of overengineered testing? Should I have these expectations, or should I rely on testing a mostly black box of a method?

If I did remove those expectations, and relied on a mocked result from the subclassed/mocked selectWith, I could write a passing test which didn't specify any of the select criteria, and would fail to function as required in the real world.

Answer 1

The point of your test should be that it tests if getDailyHits returns the expected results. It is not doing that right now. It's a meaningless test that simply checks that your implementation makes a bunch of calls, instead of testing the behaviour itself. When for some reason your implementation or database structure changes you should not have to edit this test, which is what you will be doing right now.

This is a kind of test where I would actually run against the database itself. By some standards that would not make this a 'unittest', but it makes a lot of sense to test this against the database.

Think of it like this: if you could, would you write a piece of code to test that the generated SQL from this statement matches some predefined SQL that you think might work? Or would you rather test that the result of that query matches what you expect and does not lack data or return things it shouldn't?

Answer 2

The basic idea behind the advice you quote is that where you have a piece of code that:

• interacts with some external service (eg your database), and
• also contains application logic (eg processes data retrieved from the database)

it is best to split the code in two, because those two different responsibilities need to be tested in different ways:

• you create a module that presents the simplest possible interface to your own code, and which encapsulates all of the complexity of interacting with the external service - this module is best tested with integration tests

• you then develop a module with your application logic that uses the simplified interface module. This simplified module should be easier to replace with a mock, and you're more likely to fully understand its behaviour so you are less likely to write a mock that behaves incorrectly. You can therefore test the complex logic more easily using simpler unit tests.

Note that your interface module shouldn't subclass an external class - to get the full benefit of the principle, you should only include as much complexity in its interface as you absolutely require. Prefer aggregation over inheritance is the maxim to apply here.

Answer 3

The recommendation in "Don't Mock What You Don't Own" is to wrap a third party component into an object of your own. You've already done that. Whatever object has the getDailyHits() method is already a wrapper around TableGateway. And it's a good wrapper since it describes its contract not in nasty low level terms from the ZF2 world, but in terms that make sense in your own system (getDailyHits, start, end).

All you need to do is give it an interface and change the way you test :

• Test the concrete wrapper object (let's call it ZF2DAO, for Data Access Object) in an integration test against a real TableGateway if you can. Just test happy paths. These tests are not here to bust corner cases, just to verify that everyhting is set up porperly. You can move them to a separate test suite if they are too slow.

• When testing objects that collaborate with the DAO, mock it. You do own this one.