PHPUnit: using a mocked-method call log to track its activity

G'day:
This is just a quick PHPUnit one, inspired by a trick my team lead suggested to me. I think it's quite handy and works around what seems to be a shortcoming of how PHPUnit mocks stuff. I'm sure there's strong opinions held on this sort of thing somewhere as to how I'm not supposed to want to do what my requirement has me needing to do here. I do so look forward to being told about it.

Anyway, I'm writing a script to repair some bung data we have. The details are irrelevant, but it's fairly "important" data (well: not to me... but to someone, apparently...), and I want to make sure we can track when things go wrong. And they will go wrong. Firstly I'm talking to an external third-party service, and it's foolhardy to trust those; secondly I'm relying on some of our own internal services which... ah... well yeah they were written in a different era by people with an unusual understanding of how to write managable, testable, and reliable code. And in the course of doing this work I turned-up a coupla bugs and had to fix those, so I don't back our own code to be stable here. I don't mean that judgementally: all code bases have better and not so good code in them. Anyway...

The upshot of this is the script logs a bunch of stuff - said script will be run by a cron job - and part of my testing is to verify the right things gets logged at the right time. A lot of it is fairly perfunctory "belt and braces" sort of logging, say "Records 1000-1999 processed OK", and I don't really care about that; but there's other stuff where the script deals with unexpected circumstances and I want to make sure that's done right.

For the purposes of this test I am mocking out all the dependencies so the script doesn't actually do anything, but the mocks return "workable" mocked data sufficient to walk through the stages of the script. And the logger is one of these dependencies, and is also mocked.

The test I was stuck on was one where an unhandled exception was thrown, and the script basically shuts itself down; having first logged what went wrong.

Here's a very pared-back version of the script:

public function myMethod($times){
    try {
        $this->logger->logMessage("Starting off");
        for ($i=1; $i <= $times; $i++) {
            $this->logger->logMessage("Starting processing iteration $i");
            $this->helper->doThing();
            $this->logger->logMessage("Finished processing iteration $i");
        }
        $this->logger->logMessage("Finishing off");
    }
    catch (\Exception $e) {
        $this->logger->logMessage(sprintf("Something went wrong: %s", $e->getMessage()));
        throw $e;
    }
    finally {
        $this->logger->logMessage("All done");
    }
}

Here I've just got the one doThing dependency call in the loop, but it makes the point. And around that, and before and after the the loop I log a line to track progress. And I log a line if there's an unhandled exception. As I said the real script is somewhat more complex than this, but this is an adequate analogy.

I have another test which tests the ongoing logging of a successful run with 0, 1, >1 records, but this test is to make sure the script a) reports the exception; b) still shuts down OK.

PHPUnit's approach to checking a specific argument was passed to a particular mock call is to use the at matcher method when setting an expectation. My first pass at the test used this matcher:

function testMyMethodUsingAt()
{
    $exceptionMessage = "EXPECT_THIS";
    $this->expectException(\RuntimeException::class);
    $this->expectExceptionMessage($exceptionMessage);

    $testIterations = 6;
    $errorAt = $testIterations / 2;

    $this->helper
        ->expects($this->at($errorAt))
        ->method("doThing")
        ->willThrowException(new \RuntimeException($exceptionMessage));

    $this->logger
         ->expects($this->at(7))
         ->method("logMessage")
         ->with("Starting processing iteration 4");
    $this->logger
         ->expects($this->at(8))
         ->method("logMessage")
         ->with("Something went wrong: $exceptionMessage");
    $this->logger
         ->expects($this->at(9))
         ->method("logMessage")
         ->with("All done");

    $this->sut->myMethod($testIterations);
}

Here I worked out the the call history for info would be nine entries long, and the entries I wanted to confirm the exception-tracking logic was correct was the 7th, 8th and 9th ones.

But this is problematic for a number of ways:

• it's not immediately clear what hat I pulled 7th, 8th and 9th from;
• it's fragile because it relies on there being precisely seven earlier log entries for it to work. EG: if we were to add another log line into the method, that would break this test, which is rubbish;
• semantically it's incorrect, as it's not the 7th, 8th and 9th entries I'm interested in; it's the last three. It'd be better to code it accordingly so the code was more readable to maintainers, later on.
• Lastly the at approach to things in PHPUnit is just a bit crap because the index is based on the mock object, not the mock method, despite almost all use-cases I've had for this sort of thing have been wanting to check sequential calls to the mocked method, not just to any method in the mocked object. So I try to steer clear of at at the best of times.

Other mocking frameworks I've used expose the call-log for a mocked method, but I cannot find any way to get it from PHPUnit. I can see where the data is stored, but to get it out would be deeply "Heath Robinson", and would be even worse than the approach above.

I was stumped.

Then me boss said "just use a callback in the will expectation, and build the log yerself". Initially I just looked at him like he was a loony (he's used to this), but then it dawned on me what he was suggesting. And it was easy, and not altogether awful, either:

function testMyMethodUsingCallLog()
{
    $exceptionMessage = "EXPECT_THIS";
    $this->expectException(\RuntimeException::class);
    $this->expectExceptionMessage($exceptionMessage);

    $testIterations = 6;
    $errorAt = $testIterations / 2;

    $this->helper
        ->expects($this->at($errorAt))
        ->method("doThing")
        ->willThrowException(new \RuntimeException($exceptionMessage));

    $callLog = [];
    $this->logger
        ->method("logMessage")
        ->will($this->returnCallback(function ($message) use (&$callLog) {
            $callLog[] = $message;
        }));

    try {
        $this->sut->myMethod($testIterations);
    }
    catch (\Exception $e) {
        $this->assertSame("All done", array_pop($callLog));
        $this->assertSame("Something went wrong: $exceptionMessage", array_pop($callLog));
        $this->assertNotSame("Finishing off", array_pop($callLog));

        throw $e;
    }
}

The first key bit is capturing the call-log. This creates an array, and with each call to the mocked logging method, we capture what was logged. Note that these expectations don't enforce any rules here. They just capture what's going on.

Because this method call throws an exception, I have to try/catch it so that I can regain control once it's errored, and inspect the call log to see it's what I expect. My approach here is far more semantic: I check the last entry is the "Finishing off" one, the one before that is logging the correct exception, and - this is slightly different now - as a failsafe I check that the one before that is anything other than "All Done". Because if it was that value, it'd mean the logic flow did not branch at the right stage of things. Lastly I throw the exception I caught so PHPUnit's own exception-expectation-checking also validates that too.

One of the best things about this is that this test doesn't care about what goes on in the log prior to the stuff it's testing, it is able to focus on precisely what it cares about. This is much clearer code. An example of this would be to change this line in each test:

$testIterations = 6;

Change it to be any other number, and the first test breaks, whereas the second test keeps working. Win.

That's pretty cool.

I hasten to add I would probably not usually be focusing quite this closely on this sort of sequencing, but in this case we need to know things are working properly.

It's a shame PHPUnit doesn't just expose the call-log like this, as I can't be the first person who's been in this situation. But I guess the tools are there to DIY like I have here, so no harm done.

Righto.

--
Adam

PHPUnit: setMethods and how I've been writing too much code

G'day:
Seems our technical architect is becoming my PHP muse. Good on ya, Pete.

Anyway, he just sent out what I thought was a controversial email in which he said (paraphrase) "we're calling setMethods way too often: in general we don't need to call it". To contextualise, I mean this:

$dependency = $this
    ->getMockBuilder(MyDependency::class)
    ->setMethods(['someMethod'])
    ->getMock();

$dependency
    ->method('someMethod')
    ->willReturn('MOCKED RESULT');

See here I'm explicitly saying "mock out the someMethod method". Pete's contention is that that's generally unnecessary, cos getMockBuilder mocks 'em all by default anyhow. This goes against my understanding of how it works, and I've written bloody thousands of unit tests with PHPUnit so it surprised me I was getting it wrong. I kinda believed him cos he said he tested it, but I didn't believe him so much as to not go test it myself.

So I knocked out a quick service and dependency for it:

namespace me\adamcameron\myApp;

class MyService
{

    private $myDecorator;

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

    public function decorateMessage($message)
    {
        $message = $this->myDecorator->addPrefix($message);
        $message = $this->myDecorator->addSuffix($message);
        return $message;
    }

}

namespace me\adamcameron\myApp;

class MyDecorator
{

    public function addPrefix($message)
    {
        return "(ACTUAL PREFIX) $message";
    }

    public function addSuffix($message)
    {
        return "$message (ACTUAL SUFFIX)";
    }

}

Nothing surprising there: the service takes that decorator as a constructor arg, and then when calling decorateMessage the decorator's methods are called.

In our tests of decorateMessage, we don't want to use the real decorator methods, we want to use mocks.

First I have a test the way I'd normally mock things: explicitly calling setMethods with the method names:

public function testWithExplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(['addPrefix', 'addSuffix'])
        ->getMock();

 $decorator
  ->method('addPrefix')
  ->willReturn('(MOCKED PREFIX)');

 $decorator
  ->method('addSuffix')
  ->willReturn('(MOCKED SUFFIX)');

 $service = new MyService($decorator);

 $result = $service->decorateMessage('TEST MESSAGE');

 $this->assertSame(
        '(MOCKED SUFFIX)',
        $result
    );
}

And this mocks out the method correctly. This is my baseline.

Secondly, I still call setMethods, but I give it an empty array:

public function testWithImplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods([])
        ->getMock();

 $decorator
  ->method('addPrefix')
  ->willReturn('(MOCKED PREFIX)');

 $decorator
  ->method('addSuffix')
  ->willReturn('(MOCKED SUFFIX)');

 $service = new MyService($decorator);

 $result = $service->decorateMessage('TEST MESSAGE');

 $this->assertSame(
        '(MOCKED SUFFIX)',
        $result
    );
}

This also mocks out the method correctly. Passing an empty array mocks out all the methods in the mocked class.

Next I try passing null to setMethods:

public function testWithNull()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(null)
        ->getMock();

    $decorator
        ->method('addPrefix')
        ->willReturn('(MOCKED PREFIX)');

    $decorator
        ->method('addSuffix')
        ->willReturn('(MOCKED SUFFIX)');

    $service = new MyService($decorator);

    $result = $service->decorateMessage('TEST MESSAGE');

    $this->assertSame(
        '(ACTUAL PREFIX) TEST MESSAGE (ACTUAL SUFFIX)',
        $result
    );
}

This creates the mocked object, but the methods are not mocked. So the result here is using the actual methods.

In the last example I mock one of the methods (addPrefix), but not the other (addSuffix):

public function testWithOneExplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(['addPrefix'])
        ->getMock();

    $decorator
        ->method('addPrefix')
        ->willReturn('(MOCKED PREFIX)');

    $service = new MyService($decorator);

    $result = $service->decorateMessage('TEST MESSAGE');

    $this->assertSame(
        '(MOCKED PREFIX) (ACTUAL SUFFIX)',
        $result
    );
}

Here we see the important bit: when one explicitly mocks one or a subset of methods, then the other methods are not mocked. Which is kinda obvious now I say it, but I guess we had been thinking one needed to call setMethods to be able to then mock the behaviour of the method. And, TBH, I don't think we thought through what was happening to the methods we were not mocking. I think we started calling setMethods all the time because we were slavishly following the PHPUnit docs, which always call it too, which is a less than ideal precedent to "recommend".

Generally speaking, one should want to mock all methods of a dependency, and it should really be the exception when explicitly not wanting to mock a method: ie, to leave it actually callable.

The win here is that we can now de-clutter our tests a bit. We really seldom need to call setMethods, and we sure want to make it clear when we do want to only mock some of a dependency's methods. So this will make our code cleaner and clearer, and easier to code review and maintain. Win!

Cheers Pete!

Righto.

--
Adam

PHPUnit: I question the merits of my work

G'day:
We take our TDD very seriously on our London-based team: all (PHP) code is developed using TDD, which means we spend a lot of time working with PHPUnit. And given the nature of testing, the test code can get pretty bloody repetitive at times. We also leverage dependency injection for all our dependencies, and as we take a Clean Code approach, we sometimes end up with a whole bunch of dependency methods to mock-out when we're testing the class depending on the... ah... dependency (I'll submit that one for "Clumsiest Sentence of the Day").

Furthermore, it seemed to me that our test classes were getting cluttered with a very high percentage of PHPUnit boiler plate code facilitating the mocking. It seemed to me that PHPUnit's fluent interface is just a mechanism for clutter.

Consider this contrived situation:

namespace me\adamcameron\someApp\service;

use \me\adamcameron\someApp\repository\SomeRepository;

class SomeService {

    private $someRepository;

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

    public function getTheThing($idOfTheThing){
        return $this->someRepository->getTheThing($idOfTheThing);
    }

    public function setTheThing($idOfTheThing, $valueOfTheThing){
        return $this->someRepository->setTheThing($idOfTheThing, $valueOfTheThing);
    }

    public function doesTheThingExist($idOfTheThing){
        return $this->someRepository->doesTheThingExist($idOfTheThing);
    }

}

This is all very contrived (in case you had not noticed from the class / method naming), but here we have SomeService which has three methods which depend on a repository to handle the actual data read/writes. When testing SomeService, we want to mock SomeRepository out completely as it's not being tested. It's the getMockedSomeRepository() method which is my focus here. Every method needs the same boilerplate code to mock it out: how many times it's called, what its name is:

namespace me\adamcameron\someApp\test\service;

use \me\adamcameron\someApp\service\SomeService;

class SomeServiceTest {
public function setup(){
        $mockedSomeRepository = getMockedSomeRepository();
        $this->someService = new SomeService($mockedSomeRepository);
    }

 public function testGetTheThing(){
        $result = $this->someService->getTheThing($this->testId);

        $this->assertEquals($this->mockedGetTheThingResult, $result);
    }

 public function testSetTheThing(){
        $this->someService->setTheThing($this->testId, $this->testThing);
    }

 public function testDoesTheThingExist(){
        $result = $this->someService->doesTheThingExist($this->testId);
        $this->assertEquals($this->mockedDoesTheThingExistResult, $result);
    }

 private function getMockedSomeRepository(){
  $mockedSomeRepository = $this
   ->getMockFactory('\me\adamcameron\someApp\repository\SomeRepository')
   ->disableOriginalConstructor()
   ->setMethods(["getTheThing", "setTheThing", "doesTheThingExist"])
   ->getMock();

  $mockedSomeRepository
   ->expects($this->once())
   ->method("getTheThing")
   ->with($this->testId)
   ->willReturn($this->mockedGetTheThingResult);

  $mockedSomeRepository
   ->expects($this->once())
   ->method("setTheThing")
   ->with($this->testId, $this->testThing);

  $mockedSomeRepository
   ->expects($this->once())
   ->method("doesTheThingExist")
   ->with($this->testId)
   ->willReturn($this->mockedDoesTheThingExistResult);

  return mockedSomeRepository;
 }

}

As an aside, don't ask why PHPUnit does it in that order, but that's the recommendation. I guess it's a metaphor for "expect the foo method to be called with these arguments and it will return this value". That reads OK when it's English, but when looking at the code, it's a bit like Yoda wrote it. Still: shrug.

Also there's the same set of variations on mocking the class itself: one either disables the constructor or passes it some arguments, and it needs to have the methods specified at this point (as well as individually when mocking the methods).

Back to considering the method mocking: I've demonstrated that it's not always the same set of expectations being set: on the mock for setTheThing() I do not check the return value, cos it doesn't have one. Similarly sometimes one doesn't want to test how many times a method is called (no expects()) or what arguments it receives (no with()); or sometimes willReturn might actually need to be a will() call (which allows for more complicated results, like using a callback which contrives the result from the arguments, and other good stuff).

But it's always the same variation on the same theme. And I did not like having to type ->expects($something), ->with($somethingElse) over and over. The "sensible" approach seemed to be abstract out the boilerplate code, and just pass the values into some helper which then had the boilerplate once.

With a bit of horsing around, I came up with this sort of thing:

class MockHelper extends \PHPUnit_Framework_TestCase {

    function mockObject($class, $methods=null, $constructorArgs=null){
        $mockObjectBuilder = $this->getMockBuilder($class);

        if (is_null($constructorArgs)){
            $mockObjectBuilder->disableOriginalConstructor();
        }else{
            $mockObjectBuilder->setConstructorArgs($constructorArgs);
        }

        if (!is_null($methods)){
            $mockObjectBuilder->setMethods(array_keys($methods));
        }
        $mockedObject = $mockObjectBuilder->getMock();

        if (is_null($methods)){
            return $mockedObject;
        }

        foreach ($methods as $method=>$mock){
            if (array_key_exists("expects", $mock)){
                $methodMocker = $mockedObject->expects($mock["expects"]);
            }else{
                $methodMocker = $mockedObject;
            }
            $mockedMethod = $methodMocker->method($method);

            if (array_key_exists("with", $mock)){
                call_user_func_array([$mockedMethod, 'with'], $mock["with"]);
            }

            if (array_key_exists("will", $mock)){
                $mockedMethod->will($mock["will"]);
            }

            if (array_key_exists("willReturn", $mock)){
                $mockedMethod->willReturn($mock["willReturn"]);
            }
        }
        
        return $mockedObject;
    }

}

An example call of this might be:

private function getMockedSomeRepository()
{
    $mockHelper = new MockHelper();
    $mockedSomeRepository = $mockHelper->mockObject(
        '\me\adamcameron\someapp\repository\SomeRepository',
        [
            "getTheThing" => [
                "with" => [$this->testId],
                "willReturn" => $this->mockedGetTheThingResult
            ],
            "setTheThing" => [
                "with" => [$this->testId, $this->testThing]
            ],
            "doesTheThingExist" => [
                "with" => [$this->testId],
                "willReturn" => $this->mockedDoesTheThingExistResult
            ]
        ]
    );

    return $mockedSomeRepository;
}

The conceit is that - as I mentioned above - the data defines the behaviour here, not a bunch of repetitive boilerplate code. I just state the class I want to mock, and - in one fell swoop - a data structure of the methods I want to mock and their mocked behaviour. I don't need to worry about how the mocking takes place, this is all done within mockObject(). Lovely. And meself and me colleague started to use this on our team's work.

Come the Friday PHP Team meeting, I mentioned it in passing to the rest of the bods on the broader PHP team (we were only using this in one of our sub-teams), and they were appropriately hesitant of code they didn't have a hand in, but seemed to agree with the pain of repeated boilerplate needing a solution, but seemed ambivalent about this particular solution. Then after some reflection our team lead made a couple of very salient points (my paraphrasing of his original sentiment):

1. If we hire a new dev, there's a chance they might already know PHPUnit, but what they definitely won't know is how to use this approach. Is this a good thing?
2. Where's the actual gain here? Is it actually less code? Have you solved the problem you were trying to solve?

Point 1 is relevant, but in reality in my experience bugger-all devs know the first thing about unit testing, so it's slightly optimistic to think this will be an issue. In reaction to point 2 I was like "well of course it is! It must be!". Then I checked.

Let's look at the two approaches side by side:

Using PHPUnit's mock builder:

private function getMockedSomeRepository()
{
    $mockedSomeRepository = $this
        ->getMockBuilder('\me\adamcameron\someapp\repository\SomeRepository')
        ->disableOriginalConstructor()
        ->setMethods(["getTheThing", "setTheThing", "doesTheThingExist"])
        ->getMock();

    $mockedSomeRepository
        ->method("getTheThing")
        ->with($this->testId)
        ->willReturn($this->mockedGetTheThingResult);

    $mockedSomeRepository
        ->method("setTheThing")
        ->with($this->testId, $this->testThing);

    $mockedSomeRepository
        ->method("doesTheThingExist")
        ->with($this->testId)
        ->willReturn($this->mockedDoesTheThingExistResult);

    return $mockedSomeRepository;
}

Using this mock helper:

private function getMockedSomeRepository()
{
    $mockHelper = new MockHelper();
    $mockedSomeRepository = $mockHelper->mockObject(
        '\me\adamcameron\someapp\repository\SomeRepository',
        [
            "getTheThing" => [
                "with" => [$this->testId],
                "willReturn" => $this->mockedGetTheThingResult
            ],
            "setTheThing" => [
                "with" => [$this->testId, $this->testThing]
            ],
            "doesTheThingExist" => [
                "with" => [$this->testId],
                "willReturn" => $this->mockedDoesTheThingExistResult
            ]
        ]
    );

    return $mockedSomeRepository;
}

The former is 24 lines of code, and the latter 22. Not exactly a win there. If we were pedantic and went down to byte level, it's 756 vs 702. Until one thinks that one needs to import the class that the helper is in, so adding in that - in my instance, with my namespace - brings the helper version to 752 bytes.

Now I'm not interested in the number of bytes... that's daft. But I am interested in how many lines of code we're saving... and I'm sorry but 22 vs 24 doesn't cut it for me. It might be 8% or so, but... this is not the sort of code that's ever gonna be reflective of hundreds or even dozens of lines of code saved in a test class file. Over all it might save hundreds of lines of code in our code base, but it doesn't matter when it's spread through hundreds of files. It only really matters when looking at the one method in front of one at that point in time.

In the end I am rather disappointed that I don't think this solution is a good one, and I have reverted to using PHPUnit's mock builder approach.

Another of my colleagues pointed out another flaw - and this was a flaw in my approach to using PHPUnit's mock builder in general - one doesn't necessarily want to mock the object and the methods in one fell swoop anyhow. One might - and probably does - want to mock the dependency object itself in setup(), but then it's on a test-by-test basis that one needs to provide the method-mocking details. It's annoying that one needs to call the initial setMethods() at object level, and can't simply have that inferred from calling something like mockMethod() on the mocked object, but that's generally OK.

From a personal perspective: one thing I am taking away from all this is that my work - which I was quite pleased with - was challenged, and I didn't get all defensive about it, which is an easy trap to fall into. I went "hmmm... yeah, best I look at that...", and... I turned out to be wrong. And it actually felt good to find out I was wrong (disappointing, but good). Also: in the discovery process I now know a bit more about how PHPUnit works, and I am a better dev for it. Also it gave me a good topic to write about.

--
Adam

Unit testing: mocking out final, type-checked dependency

G'day:
This is a follow-on from my earlier article: "PHPUnit: trap for dumb players regarding mocking". In that article I described how I fell foul of trying to mock a method that had been declared final, and - basically - wasting time from a mix of me not thinking to RTFM, validating my assumptions, and PHPUnit not being as declarative with its feedback as it could be.

Recap of previous article

To recap/augment the situation, I had a Logger class like this:

namespace me\adamcameron\mocking\service;

use me\adamcameron\mocking\exception\NotImplementedException;

class LoggingService {

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

}

And we have a service using that Logger as a dependency:

namespace me\adamcameron\mocking\service;

class MyService {

    private $logger;

    public function __construct(LoggingService $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";
    }

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

}

We've established we cannot mock that dependency because I cannot mock that final method.

Final prevents mocking

Our tactic to work around this was to create a different stubbed LoggingService which has the same methods, but not the final restriction:

namespace me\adamcameron\mocking\stub;

use me\adamcameron\mocking\exception\StubMethodCalledException;

class LoggingService {

    public function logSomething($text){
        throw new StubMethodCalledException(__FUNCTION__ . " must be mocked");
    }

}

And then we use StubMethodCalledException for our mocked dependency:

class MyServiceTest extends \PHPUnit_Framework_TestCase {

    // ...

    function setup(){
        $mockedLogger = $this->getMockedLogger();
        $this->myService = $this->getTestMyService($mockedLogger);
    }

    // ...

    function getMockedLogger(){
        $mockedLogger = $this->getMockBuilder('\me\adamcameron\mocking\stub\StubbedLoggingService')
            ->setMethods(["logSomething"])
            ->getMock();

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

        return $mockedLogger;
    }

    // ...

}

Type-checking can be unhelpful

And now we run our tests and...

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

.E

Time: 764 ms, Memory: 5.00Mb

There was 1 error:

1) me\adamcameron\mocking\test\service\MyServiceTest::testDoesStuff
Argument 1 passed to me\adamcameron\mocking\service\MyService::__construct() must be an instance of me\adamcameron\mocking\service\LoggingService, instance of Mock_StubbedLoggingService_e4bcfaaf given

C:\src\php\php.local\www\experiment\phpunit\mock\src\service\MyService.php:9
C:\src\php\php.local\www\experiment\phpunit\mock\test\service\MyServiceTest.php:44
C:\src\php\php.local\www\experiment\phpunit\mock\test\service\MyServiceTest.php:16
C:\Users\adam.cameron\AppData\Roaming\Composer\vendor\phpunit\phpunit\src\TextUI\Command.php:149
C:\Users\adam.cameron\AppData\Roaming\Composer\vendor\phpunit\phpunit\src\TextUI\Command.php:100

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

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

Ah FFGS.

Indeed, here's that constructor:

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

Note how it type-checks the $logger argument.

For web application code, I would simply never bother type checking arguments. PHP is a dynamic, run-time compiled language, so type-checking really makes little sense in most situations. In this situation it's one of our API libraries, so it kinda makes sense. Normally I'd go "an API should typecheck as much as possible", but to me that only applies to APIs which will be used as a third-party module. For internal libraries - even shared ones - it's just boilerplate for the sake of it, IMO.


This is important, so I'm gonna make it bold:

That aside, in a situation you know the library is going to be used as a dependency of other code - especially code you know is going to be using TDD and dependency injection - do not type check on an implementation. Use an interface.

If we'd used an interface here, I could still use my stub, just by saying it implements the interface (which it does already!).

PHPUnit: very bloody impressed with Sebastian Bergmann

G'day:
Yesterday I wrote about an issue I had encountered with PHPUnit ("PHPUnit: trap for dumb players regarding mocking"), which was irking me a bit. I guess cos I tagged my Twitter note about he article with #PHPUnit, the owner of the Project, Sebastian Bergmann got back to me and said he'd have a look at it:

@DAC_dev Thank you for bringing this to my attention. I will implement https://t.co/6BDr3r6DLn ASAP to improve this.

— Sebastian Bergmann (@s_bergmann) March 3, 2016

That was impressive enough (and quite pleased he read my blog article!), but I arrive at work this morning and he's only gone and fixed it already! And by the looks of the pull req: it was no small amount of work, too.

Thanks Sebastian: you're a star!

Right. Back to more unit testing.

--
Adam

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>

Unit testing (PHP): keeping sight of the intent of the tests

G'day:
An article with some actual code in it! That'd become a rarity. This is actuallya real-world challenge I'm faced with in backfilling some unit tests. Yeah, I know I'm an advocate of TDD so that should mean there's no "backfilling" of tests: they all get done up front. But for reasons I won't go into... I'm backfilling testing for this function.

Here's the function:

public static function loadValidatorMetadata(ClassMetadata $metadata)
{
    $metadata->addPropertyConstraint('languageId', new Assert\Range(
        ['min' => 1, 'max' => self::$maxLegacyLanguageId]
    ));

    $metadata->addPropertyConstraint('destinationId', new Assert\Range(['min' => 1]));

    $metadata->addConstraint(new Assert\Callback('validateArrivalDate'));
    $metadata->addConstraint(new Assert\Callback('validateNights'));

    $metadata->addPropertyConstraint('firstName', new Assert\NotBlank());
    $metadata->addPropertyConstraint('firstName', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    ));
    $metadata->addPropertyConstraint('surName', new Assert\NotBlank());
    $metadata->addPropertyConstraint('surName', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    ));

    $metadata->addPropertyConstraint('email', new Assert\NotBlank());
    $metadata->addPropertyConstraint('email', new Assert\Email());

    $metadata->addPropertyConstraint('groupTypeId', new Assert\Range(['min' => 1]));
    $metadata->addPropertyConstraint('groupSize', new Assert\NotNull());
    $metadata->addPropertyConstraint('groupSize', new Assert\Range(
        ['min' => GroupEnquiryService::MIN_GROUPS_THRESHOLD, 'max' => BookingService::MAXIMUM_PEOPLE]
    ));
    $metadata->addPropertyConstraint('youngestAge', new Assert\Range(
        ['min' => 1, 'max' => GroupEnquiryService::MAX_AGE]
    ));

    $metadata->addPropertyConstraint('telephoneCountryCode', new Assert\NotBlank());
    $metadata->addPropertyConstraint('telephoneCountryCode', new Assert\Length(
        ['min' => 1, 'max' => GroupEnquiryRepository::SHORT_STRING_MAX_LENGTH]
    ));
    $metadata->addPropertyConstraint('phone', new Assert\NotBlank());
    $metadata->addPropertyConstraint('phone', new Assert\Length(
        ['min' => GroupEnquiryService::MIN_TELEPHONE_NUMBER_LENGTH]
    ));
    $metadata->addConstraint(new Assert\Callback('validateMobileCountryCode'));
    $metadata->addConstraint(new Assert\Callback('validateMobile'));

    $metadata->addConstraint(new Assert\Callback('validateAdditionalInfo'));
}

First things first: yeah, normally I'd not write a function that's 40-odd lines long, but in this case there's very little actual logic, plus it's only setting some validation config. BTW this is using the Symfony 2 validation framework (Symfony: validation), as implemented by the Silex microframework (ValidatorServiceProvider: validating objects).

To provide context, we have a class - basically a validation bean - thus:

class GroupEnquiry
{
    public $languageId;
    public $destinationId;
    public $arrivalDate;
    public $nights;
    public $firstName;
    public $surName;
    public $email;
    public $telephoneCountryCode;
    public $phone;
    public $mobileCountryCode;
    public $mobile;
    public $groupTypeId;
    public $groupSize;
    public $youngestAge;
    public $additionalInfo;
}

And that loadValidatorMetadata() function provides the constraints for Symfony to enforce. As you can see: there's range validation, existence validation, length validation, date validation, and a coupla compound constraints implemented via callbacks.

So, anyway, I need to write the testing for loadValidatorMetadata(). Initially I was staring at the code going and going "how the hell?".

Then I thought: I'll assert the addConstraint() and addPropertyConstraint() methods are called for the correct properties, and the correct number of times. But that seemed unhelpful. What's the point of checking that some constraint is applied to a property, but not checking what sort of constraint. IT all seemed too contrived.

Then I thought that the constraints themselves don't need testing here because I'm either testing our callbacks elsewhere, or Symfony have done their own testing. In theory this is a fine notion, but in reality it does not actually help me test our validation code works.

I was stumped as to how to test this stuff, without basically replicating the function in parallel for test. And that clearly sucks as far as effort and common sense goes. It was the wrong approach.

PHP: unit testing private methods... PHP 5.5 version

G'day:
The Central Line isn't running at the moment and the buses are all shagged due to everyone wanting now to catch one, so I've given up trying to get to work and gone home until things settle down a bit. Not wanting to be completely useless to my employers, I've decided to solve an issue I created yesterday by recommending using some PHP 5.6-only code on out 5.5 environment. Duh.

PHP: unit testing private methods (a bit of reflection)

G'day:
Yes, I still do PHP as well as Lucee. Well, indeed, I don't do Lucee, I just seem to "invest" my time blogging about it. I am very much doing PHP all day every day again now.

Recently I revisited the notion of unit testing private methods. I realise a lot of people poopoo this notion as one is only supposed to unit test the interface, so only public methods. However when doing TDD, one needs to maintain one's private methods too, so this means one needs to test the behaviour of the change before making it.


Some argue these days that one should not have private methods: having them suggests bad class composition and a private method should be a public method of another class. I can see where they're coming from, but I also suspect the person deciding this was a Java developer and revels in the theory of OO put into practice, rather than... you know... getting shit done. I am in the "getting shit done" camp. I see no benefit in making classes for the sake of it, and I also like the idea of having code as close as possible to where it'll be used. So private helper methods have their place. And they need to be tested whilst being developed.

Others make the very good point that what actually needs testing is the nature of the impact on the result of the public method is what's important, so a private method ought to be tested "by proxy": call the public method, give it inputs that will invoke the to-be-developed new behaviour, and don't care - at test level - where the code happens to be. I have come to appreciate this idea, and it's how we approach a lot of our testing now.

One hitch with this is sometimes it's difficult to only hit the new functionality without also having to horse around appeasing other bits of functionality on the way through. Mocking makes this a lot easier, but not everything can be mocked, and even mocking stuff can be time and effort consuming. I guess if our code was perfect we'd not have this issue so much, but it's not (we're not), so we need to deal with that reality.

Sometimes it's just bloody easier to test a private method.

Back in my CFML days this was piss-easy. MockBox comes with a method makePublic(), which simply makes a private method public. It achieves this by injecting a proxy into the object under test which is public, and this wraps a call to the private method. One then calls that proxy in one's test.

Using makePublic() is as easy as:

mockbox.makePublic(object, "methodName");

Then continue to call methodName as per usual in one's tests. Cool.

This ain't so easy in PHP. One cannot just inject proxy functions into one's methods. However one can use reflection to make changes to an object's existing methods, including its access restrictions.

So what I set out to do was to make my own makePublic() method.

I arrived at this:

private function makePublic($object, $method){
    $reflectedMethod = new \ReflectionMethod($object, $method);
    $reflectedMethod->setAccessible(true);
    return $reflectedMethod;        
}

(it's private as it's just inline in my test class, the entire code for which can be seen here: SomeClassTest.php SomeClass.php).

This is simple enough. However actually using it is inelegant compared to the MockBox approach:

/**
@covers isIndexedArray
*/
function testIsIndexedArray(){
    $testValue = [53,59,61];
    $exposedMethod = $this->makePublic($this->testObject, 'isIndexedArray');
    $actual = $exposedMethod->invoke($this->testObject, $testValue);
    $this->assertTrue($actual);
}

That's no so clear.

So I decided to abandon that approach, instead coming up with this method:

private function executePrivateMethod($object, $method, $arguments){
    $reflectedMethod = new \ReflectionMethod($object, $method);
    $reflectedMethod->setAccessible(true);

    $result = $reflectedMethod->invoke($object, ...$arguments);
    return $result;        
}

This does the same thing, but it looks far more understandable in the calling code:

/**
@covers isIndexedArray
*/

function testIsIndexedArray_withAssociativeArray(){
    $testValue = ['a'=>67,'b'=>71,'c'=>73];

    $actual = $this->executePrivateMethod($this->testObject, 'isIndexedArray', [$testValue]);
    $this->assertFalse($actual);
}

It's really clear what executePrivateMethod does, I reckon.

I'mm going to show this to the lads at work tomorrow, and see what they think. Note: my position in general will be to test via the public interface, but in those situations where this is going to be a lot of work, or make the testing unclear, hopefully we can use this instead.

Thoughts?

--
Adam