Thursday, 3 March 2016

PHPUnit: trap for dumb players regarding mocking

G'day:
This is just a demonstration of me being a git. But who doesn't like that?

We use PHPUnit extensively to test our codebase. And along with that we use its mocking facilities extensively to mock-out our dependencies in these tests. It's more of a hassle to use than MockBox was on CFML, but this is down to PHP being more rigid with how it goes about things, and is not the fault of PHPUnit (or indeed anyone's "fault" at all: it's just "a thing").

Last week I was flummoxed for a lot longer than I ought to have been, because for some reason a method simply would not mock. After a number of hours I asked the other bods on the team to put their eyes on the code and my mate Amar spotted the problem instantly. The issue was down to me making assumptions about one of our code bases that I was not familiar with, and not bothering to check my assumptions. And also some user-unfriendliness in PHPUnit.

Here's a repro of the code I was running. Well: it bears absolutely no relation to the code I was running at all, but it demonstrates the issue.

Firstly I have a simple service class which has a method I want to test:

namespace me\adamcameron\mocking\service;

class MyService {

    private $logger;

    public function __construct($logger){
        $this->logger = $logger;
    }

    public function doesStuff($value){
        $preppedValue = "PREPPED $value";
        $processedValue = $this->doesRiskyStuff($preppedValue);
        $this->logger->logSomething($processedValue);
        return "RESULT OF DOING STUFF ON $processedValue";
    }

    private function doesRiskyStuff($preppedValue){
        return "$preppedValue (SURVIVED RISKY PROCESS)";
    }

}

This method uses a method from one external dependency, plus it also calls one of its own helper methods which does "risky stuff", so we actually want to mock-out both of those methods.

First things first I already know that one cannot mock private methods with PHPUnit, so we need to make that method protected. This is not ideal, but it's better than not being able to test safely, so it's a burden we are prepared to wear.

Having made that change, we need to partially mock the class we need to test:

function getTestMyService($logger){
    $partiallyMockedMyService = $this->getMockBuilder('\me\adamcameron\mocking\service\MyService')
        ->setConstructorArgs([$logger])
        ->setMethods(["doesRiskyStuff"])
        ->getMock();

    $partiallyMockedMyService->expects($this->once())
        ->method("doesRiskyStuff")
        ->with("PREPPED TEST VALUE")
        ->willReturn("MOCKED RESPONSE FROM DOESRISKYSTUFF");

    return $partiallyMockedMyService;
}

This just mocks-out doesRiskyStuff(), leaving everything else as is. So now when we call doesStuff() in our test, it won't call the real doesRiskyStuff(), but our mock instead.

Notice how some expectations are set on the mocked method itself here:

  • the number of times we expect it to be called;
  • what arguments it will be passed;
  • and a mocked value for it to return.

We call this from our setup() method:

function setup(){
    $logger = $this->getTestLogger();
    $this->myService = $this->getTestMyService($logger);
}


Our test method then is very simple:

/**
 * @covers doesStuff
 */
function testDoesStuff(){
   $result = $this->myService->doesStuff("TEST VALUE");

    $this->assertSame("RESULT OF DOING STUFF ON MOCKED RESPONSE FROM DOESRISKYSTUFF", $result);
}

It simply calls the method and checks what it returns. Simple.

But so far I've glossed over the logging part. 'ere 'tis:

namespace me\adamcameron\mocking\service;

use me\adamcameron\mocking\exception\NotImplementedException;

class LoggingService {

    final public function logSomething($text){
        throw new NotImplementedException(__FUNCTION__ . " not implemented yet");
    }

}


There's not much to that, and indeed I have not even implemented it yet. This is cool... we can just mock it out and test that it receives what it's supposed to be passed by our doesStuff() method. That completes the coverage of doesStuff(): our test of doesStuff() does not need to care whether logSomething() actually works.

Here's how we mock it:

function getTestLogger(){
    $mockedLogger = $this->getMockBuilder('\me\adamcameron\mocking\service\LoggingService')
        ->setMethods(["logSomething"])
        ->getMock();

    $mockedLogger->expects($this->once())
        ->method("logSomething")
        ->with("MOCKED RESPONSE FROM DOESRISKYSTUFF");

    return $mockedLogger;
}


All very obvious and the same as before.

When I run my tests, I see this:

C:\src\php\php.local\www\experiment\phpunit\mock>phpunit
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

E

Time: 733 ms, Memory: 5.00Mb

There was 1 error:

1) me\adamcameron\mocking\test\service\MyServiceTest::testDoesStuffme\adamcameron\mocking\exception\NotImplementedException: logSomething not implemented yet

C:\src\php\php.local\www\experiment\phpunit\mock\src\service\LoggingService.php:
10
C:\src\php\php.local\www\experiment\phpunit\mock\src\service\MyService.php:16
C:\src\php\php.local\www\experiment\phpunit\mock\test\service\MyServiceTest.php:
23
phpunit\phpunit\src\TextUI\Command.php:149
phpunit\phpunit\src\TextUI\Command.php:100

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.

C:\src\php\php.local\www\experiment\phpunit\mock>


OK, huh? It's still calling the original method... see how it's throwing that NotImplementedException exception. I rechecked my code over and over, and could see no reason why my mock wasn't working. I stuck other methods in the logging class and they mocked-out just fine. Just not that one?

Now I was aware of a "shortfall" in PHPUnit that if one accidentally tries to mock a private method then PHPUnit simply ignores you, and does not mock it. This is why I had to switch that from being private to being protected, remember?

But logSomething() is a public function in a dependency, so it's clearly public. Isn't it? Isn't it?

This is where I deployed Amar's eyes. And he actually paid attention:

final public function logSomething($text){
    throw new NotImplementedException(__FUNCTION__ . " not implemented yet");
}

Sure it's public. But it's also bloody final, innit? FFS.

Coming from CFML land - which doesn't have the concept of final - it's not something I really look out for. I mean I know what it's all about, but it's not something I have ever used, so it's not on my mental checklist of things to check. Amar spotted it immediately.

Once I remove the final, the tests run:

C:\src\php\php.local\www\experiment\phpunit\mock>phpunit
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

.

Time: 733 ms, Memory: 5.00Mb

OK (1 test, 3 assertions)

C:\src\php\php.local\www\experiment\phpunit\mock>


I'm sick of how PHPUnit  hides sh!t like this. First with ignoring attempts to mock private methods, and now with ignoring attempts to mock final methods. Do you know what it should do? It should bloody error! It's not like PHP itself ignores this sort of thing, or simply raises a warning or anything. Check this out:


PHP Fatal error:  Cannot override final method me\adamcameron\mocking\general\BaseClass::f() in C:\src\php\php.local\www\experiment\phpunit\mock\src\general\SubClass.php on line 5

That's what I'd expect to see happening. A fatal error. That's what one gets if one tries to override a final method in normal PHP code.

The other thing is that there's a flaw in our dev process here. Code that we write which will be dependencies of other code mustn't have final methods. Personally I think the notion of final is something that should be used very sparingly, and should be strongly justified. In this case there is no good reason the methods are final.

I managed to work around this, but that was another mission, and the topic for another day's article.

Bottom line: if you find yerself swearing at yerself for why methods don't seem to be mockable... check whether the method is private, or whether it's final.

And cheers Amar for helping me out here.

Righto.

--
Adam