PHP / PHPUnit / TDD: unit testing abstract classes. Or not.

G'day:

One of my colleagues at work asked me about this, but it's a good topic to think about, so am gonna write about it here.

The question was pretty much as stated in the subject line there "if we have an abstract class… how do we go about unit testing that?". There's a coupla things to unpack here, one practical and one theoretical. I'll do the practical bit first.

Practical

I have got an abstract class (thanks to ChatGPT for writing the example code for me today, btw ;-))

abstract class Shape
{

    public function __construct(protected string $colour)
    {
    }

    public function getColour(): string
    {
        return $this->colour;
    }

    abstract public function getArea();
}

And this will eventually have implementing classes like circles and squares and what not. The abstraction conceit being that all shapes have an area, but the algorithms for defining said area vary from shape to shape. That's easy to understand and pretty ubiquitous in "here's an example of abstract classes in action" situations. For now we're just testing the abstract class, so I'm not worrying about the implementations yet.

We want to test that getColour returns the shape's colour.

For a non-abstract class, we could do this:

/** @testdox getColour returns the colour set by the constructor */
public function testGetColour()
{
    $testColour = "red";
    $shape = new Shape($testColour);

    $actualColour = $shape->getColour();

    $this->assertEquals($testColour, $actualColour);
}

Job done. Except Shape is an abstract class, so we get this instead:

Error: Cannot instantiate abstract class adamcameron\php8\Shapes\Shape
/var/www/tests/Unit/Shapes/ShapeTest.php:15

All is not lost. Obviously this is well-trod ground and PHPUnit already deals with this sort of thing: We can use a partial mock to create an implemetation class at runtime:

public function testGetColour()
{
    $green = "karariki";
    $shape = $this->getMockForAbstractClass(Shape::class, [$green]);

    $actualColour = $shape->getColour();

    $this->assertEquals($green, $actualColour);
}

getMockForAbstractClass()

The getMockForAbstractClass() method returns a mock object for an abstract class. All abstract methods of the given abstract class are mocked. This allows for testing the concrete methods of an abstract class.

Easy. There's also a mock-builder variant of this too:

public function testGetColourUsingMockBuilder()
{
    $blue = "kikorangi";
    $shape = $this
        ->getMockBuilder(Shape::class)
        ->setConstructorArgs([$blue])
        ->getMockForAbstractClass();

    $actualColour = $shape->getColour();

    $this->assertEquals($blue, $actualColour);
}

That's it, really. Original question answered.

---

Theory

The problem is that "how to test methods of an abstract class" begs the question. If we're doing TDD: how do we get to a point where we have an abstract class to test anyhow? It sounds to me like we're getting ahead of ourselves. I hasten to add the question from my team mate was a theoretical one: something that just popped into his head, and it was a good thing to know the answer for. But it's also good to reason the situation through from a TDD perspective.

Let's say the end of the story is "an abstract Shape class, and concrete classes for Square and Circle". But at the beginning of the story we had no classes at all, and no code. We had a requirement, which was probably along the lines of "we need to be able to get the colour of our Circle". Why is it a Circle and not a Shape? Because it's unlikely we're going to have a real world requirement that deals in abstract terms. It's more likely there'll be a concrete requirement to start with. But it's a good question: I'll think about that. We write our first test, for the Circle:

/** @testdox getColour returns the Circle's colour */
public function testGetColour()
{
    $orange = "karaka";
    $circle = new Circle($orange, 1);

    $actualColour = $circle->getColour();

    $this->assertEquals($orange, $actualColour);
}

And once we see this failing, we implement what we need to get it to pass, which would be along these lines:

class Circle
{
    public function __construct(protected string $colour)
    {
    }

    public function getColour(): string
    {
        return $this->colour;
    }
}

Cool.

The next requirement comes along, which is that we need to get the circle's area. Fine, more of the same sort of thing:

/** @testdocs getArea returns the Circle's area */
public function testGetArea()
{
    $circle = new Circle("NOT_TESTED", 2);

    $actualArea = $circle->getArea();

    $this->assertEquals(pi() * 4, $actualArea);
}

And implementation:

class Circle
{
    public function __construct(private readonly string $colour, private readonly float $radius)
    {
    }

    // …

    public function getArea(): float
    {
        return pi() * $this->radius ** 2;
    }
}

Now the twist comes in. The next requirement is "OK, sometimes the shapes will be squares instead of circles, but otherwise behave the same". Remember red-green-refactor here. After TDDing the Square's behaviour, we would end up with this implementation:

class Square
{

    public function __construct(private readonly string $colour, private readonly float $side)
    {
    }

    public function getColour(): string
    {
        return $this->colour;
    }

    public function getArea(): float
    {
        return $this->side ** 2;
    }
}

That's "red" & "green" done. Now for "refactor". Clearly we come to the conclusion that Circles and Squares are both Shapes; the colour behaviour is identical in both, but whilst both have the concept of "area", how it's derived is different, and the naming of the property that we use to derive the area - radius or side, respectively - also differs. This is when we decide we need our Shape abstract class. During refactoring. But the thing is during refactoring, the tests don't change. We extract the colour-handling into a base class, but leave the area handling in the implementation classes. We just make sure the base class says "I don't care how you do it, but you need to be able to answer the question 'what is your area'".

abstract class Shape
{

    public function __construct(protected string $colour)
    {
    }

    public function getColour(): string
    {
        return $this->colour;
    }

    abstract public function getArea();
}

class Circle extends Shape
{

    public function __construct(string $colour, protected float $radius)
    {
        parent::__construct($colour);
    }

    public function getArea(): float
    {
        return pi() * $this->radius ** 2;
    }
}

class Square extends Shape
{

    public function __construct(string $colour, protected float $side)
    {
        parent::__construct($colour);
    }

    public function getArea(): float
    {
        return $this->side ** 2;
    }
}

We're not done refactoring yet. And this comes back to the requirement to test methods of an abstarct class. Currently in CircleTest and SquareTest we have these:

public function testGetColour()
{
    $orange = "karaka";
    $circle = new Circle($orange, 1);

    $actualColour = $circle->getColour();

    $this->assertEquals($orange, $actualColour);
}

public function testGetColour()
{
    $red = "whero";
    $square = new Square($red, 1);

    $actualColour = $square->getColour();

    $this->assertEquals($red, $actualColour);
}

Other than the colours I've chosen to use and one creates a Circle and one creates a Square, these are identical. As they should be as they're testing behaviour of their base class. So it does make a kind of sense to de-dupe this stuff, and push the one test up into ShapeTest. The test is the one from the first section of this article.

However I'm split either way on this. That we decided to use an abstract Shape class here is - IMO - "implementation detail", and we don't generally directly test implementation detail. So if we refactor those two tests, we might be catering to implementation detail too directly. I'm unsure. Also these tests happen to be very simple, so I think in a way this refactoring would be a case of "bad DRY" (see my earlier article "DRY: don't repeat yourself"). Maybe if the tests were more complicated then there'd be a better case for de-duping the complexity ("good DRY"). I think in this case, either way would be A-OK. Personally I'm a pedant, and a bit "a place for everything, and everything in its place", so I'm gonna go ahead and do the refactoring, and have a unit test testing an abstract class.

The code for the tests is here: /tests/Unit/Shapes, and for the source code: /src/Shapes.

Righto.

--
Adam

PHP: looking at spatie/async some more

G'day:

I'm getting back to this spatie/async library today (see "PHP: looking at spatie/async" for the first part of this).

Previously I have just been passing a callback to the add function when adding a task to the pool:

$pool->add(function () use ($connection, $i, $startTime) {
    $result = $connection->executeQuery("CALL sleep_and_return(?)", [2]);
    return sprintf(
        "%d:%d:%d",
        $i,
        $result->fetchOne(),
        microtime(true) - $startTime
    );
});

This requires the task to be tightly coupled to the pool-handling, which is probably not what one wants to do. Instead, one can put the task logic into a Task class:

namespace adamcameron\php8\Task;

use adamcameron\php8\tests\Integration\Fixtures\Database as DB;
use Doctrine\DBAL\Connection;
use Spatie\Async\Task;

class SlowDbCallTask extends Task
{
    readonly private Connection $connection;

    public function __construct(
        readonly private int $i,
        readonly private float $startTime
    ) {
    }

    public function configure()
    {
        $this->connection = DB::getDbalConnection();
    }

    public function run()
    {
        $result = $this->connection->executeQuery("CALL sleep_and_return(?)", [2]);
        return sprintf(
            "%d:%d:%d",
            $this->i,
            $result->fetchOne(),
            microtime(true) - $this->startTime
        );
    }
}

So that's a bit nicer, especially when you see how the calling code in the test looks now:

$pool->add(new SlowDbCallTask($i, $startTime));

One thing that took me a while to straighten out in my head was the way the configure method works / needs to be used. Initially, my implementation of the SlowDbCallTask class was a bit literal, and my constructor was thus:

public function __construct(
    readonly private Connection $connection,
    readonly private int $i,
    readonly private float $startTime
) {
}

This errored-out when I ran the test:

Exception: Serialization of 'Closure' is not allowed
/var/www/vendor/spatie/async/src/Runtime/ParentRuntime.php:87
/var/www/vendor/spatie/async/src/Runtime/ParentRuntime.php:69
/var/www/vendor/spatie/async/src/Pool.php:140
/var/www/tests/Functional/SpatieAsync/TaskTest.php:20

I've run into this before: the object being used as the task handler needs to be serialised to get it into the PHP process that's running it, and not everything can be serialised. Less than ideal, but so be it. Then it occurred to me that I was being a div anyhow: I don't want to pass the Task's DB connection into it - there's no reason to - I can just initialise it in the Task's constructor, eg:

public function __construct(
    readonly private int $i,
    readonly private float $startTime
) {
    $this->connection = DB::getDbalConnection();
}

But same error, and same problem. When I handle this in the constructor, it's all being done in the calling code, so the same serialisation issue exists. I have to concede the penny did not drop without some googling, and I found this helpful comment on an issue on GitHub, which led me to this other comment:

It's important to remember that parallel processes are really separate processes. By using closures and a smart bootstrap, we're simulating the parent process as best as possible. There are however cases which won't work with that simple approach: not everything is serialisable.

That's where a Task comes in. Tasks allow more control over the bootstrap in the child process.

Start by taking a look at what the README says about tasks: https://github.com/spatie/async#working-with-tasks

The less objects and application things are passed to the child process, the better. […]

Whilst it doesn't directly say it, this made me click that that config method is for (I was previously going "not sure why I need that"); the docs - linked to above - are of the sort that state what thing are, but not why they are they way they are, so don't directly explain this either. Basically if there's anything the Task will need that won't be serialisable: sling it in the config method. Makes complete sense to me now.

If one looks at the base Task class, we can see how it works:

namespace Spatie\Async;

abstract class Task
{
    abstract public function configure();

    abstract public function run();

    public function __invoke()
    {
        $this->configure();

        return $this->run();
    }
}

---

There is a simplified way of implementing tasks too (not sure why this is needed: once one knows what's going on, it's not like the other approach is "complicated".

To prove I'm actually TDDing all this still, here's the test for this one:

/** @TestDox It supports a simplified version of a task */
public function testSimpleTask()
{
    $pool = Pool::create();

    $pool->add(new SimpleTask());

    $results = $pool->wait();

    $this->assertCount(1, $results);
    $this->assertEquals("G'day world from an async call", $results[0]);
}

And the implementation simply needs to be this:

class SimpleTask
{
    public function __invoke()
    {
        return "G'day world from an async call";
    }
}

(It's a bit mindless, fine. It's late).

As per the docs: provided one has an __invoke method: that's it.

---

Speaking of TDD: in the first part of this - despite not starting with the test code listing - I did TDD it. Although I took it as kind of a refactoring, so I duplicated the test for the inline-task-via-callback version, and changed the code to instantiate my SlowDbCallTask. I did not directly test the configure and run implementations (with their own tests I mean), because that literally is implementation detail. The feature here is "the slow DB call [blah blah, whatever the feature actually is]", and whether it's done by inline callback or a Task class is neither here nor there. And that test does actually exercise all the code in the task class anyhow, so: job done.

Interestingly, the test coverage report disagrees with me:

This is because the PHP process running that code is not the same one running the tests, I guess. Hrm. Maybe I should have separate tests. But: I am not of the disposition that I need to chase 100% LOC coverage … but (again) I discuss why ensuring 100% is something to aim for anyway in an article I wrote ages ago: "Yeah, you do want 100% test coverage" (which should then be measured against this other article on the topic: "Test coverage: it's not about lines of code"). I think in this case - as the class is so simple - I'd put a @codeCoverageIgnore annotation on it. If the task class ended up having a bunch of moving parts in it that became trickier to test via its calling context, then I might consider testing those discretely. "It depends" [etc].

OK. That's the end of that section of the docs (I have written more about it than there was actual documentation that said. Ha. So I'm gonna finish up here. I think I have another article or so to write on this stuff yet. Let's seem.

Righto.

--
Adam

TDD & Symfony & Monolog: adding some logging to that endpoint from last time

G'day:

Hopefully this one is shorter than the previous monster (TDD & Symfony: creating a small web service end point). I should perhaps have split that one in two, in hindsight (the Adapter, then the Controller). This article builds on that code.

I have create a wee webservice that I call like this: http://localhost:8008/postcode-lookup/XX200X, and that goes off and hits getaddress.io with much the same request. This web service of mine is two components:

• PostcodeLookupController which receives the request, and offloads it to…
• AddressService/Adapter which is an adapter for the HTTP request to getaddress.io.

Simple. Except I managed to write 4000-ish words on it on Sunday, somehow.

One shortfall I identified with the initial implementation is that it was just swallowing some failure situations that - whilst should not cause a problem for the consuming client of my web service - should be something I pay attention to if they occur. A quick fix for this is to chuck some logging in. And this is a good exercise as it will require me to revisit Monolog, and also I'll need to work out a) how to wire it into Symfony; b) and test my integration.

In the previous article I TDDed the first part of the exercise, and then backfilled the testing on the second part as I didn't even know how to wire things together in Symfony when I started, so I decided to spike that first. Today I'm taking a hybrid approach. I'm going to try to TDD it, but there will be some points at which I need to wrestle with Symfony/Monolog config, and I am just gonna go do that when I need to. NB: this is not to suggest any of this config is arduous: I've just forgotten how to do it, so I need to work it out again.

Here goes.

This unit test in tests/Unit/Controller/PostcodeLookupControllerTest.php describes the problem I'm trying to solve:

/**
 * @testdox It logs any issues we might need to deal with
 * @dataProvider provideCasesForLoggingTests
 */
public function testLogging(
    int $statusCode,
    string $expectedMessage,
    Level $expectedLogLevel
) {
    $testHandler = new TestHandler();

    $this->configureControllerWithTestLoggingHandler(
        $statusCode,
        $expectedMessage,
        $testHandler
    );

    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", TestConstants::POSTCODE_OK)
    );

    $this->assertLogEntryIsCorrect(
        $testHandler,
        $expectedLogLevel,
        $statusCode,
        $expectedMessage
    );
}

Especially with the data-provider method:

public function provideCasesForLoggingTests() : array
{
    return [
        "Unauthorized should log critical" => [
            Response::HTTP_UNAUTHORIZED,
            "Unauthorized",
            Level::Critical
        ],
        "Forbidden should log critical" => [
            Response::HTTP_FORBIDDEN,
            "Forbidden",
            Level::Critical
        ],
        "Too many requests should log critical" => [
            Response::HTTP_TOO_MANY_REQUESTS,
            "Too Many Requests",
            Level::Warning
        ],
        "Server error should log critical" => [
            Response::HTTP_INTERNAL_SERVER_ERROR,
            "Internal Server Error",
            Level::Warning
        ]
    ];
}

Basically the getaddress.io call could return each of those failures, and I wanna log when they occur. I don't care about "the client app passed an invalid postcode", but I do care if I'm using the wrong API key, or if I haven't paid my account (those're both bad, so: critical); and I also kinda wanna keen an eye on throttling issues, and unexpected server errors on their end ("good to know", so just warnings). If any of these occur, I'm still returning a usable response to the client so they don't need to care, but I keep an eye on issues I am having with getaddress.io

There's nothing interesting in that test method, but there's some stuff in the helper methods:

private function configureControllerWithTestLoggingHandler(
    int $statusCode,
    string $expectedMessage,
    TestHandler $testHandler
): void {
    $container = self::getContainer();
    $mockedAddressServiceAdapter = $this
        ->getMockBuilder(AddressService\Adapter::class)
        ->disableOriginalConstructor()
        ->onlyMethods(['get'])
        ->getMock();
    $mockedAddressServiceAdapter
        ->expects($this->once())
        ->method('get')
        ->willReturn(new AddressService\Response(
            [],
            $statusCode,
            $expectedMessage
        ));
    $container->set(AddressService\Adapter::class, $mockedAddressServiceAdapter);

    $logger = $container->get("monolog.logger.address_service");
    $logger->setHandlers([$testHandler]);
}

This shows how to grab the DI container and replace my AddressService/Adapter with a mock that returns the values I need to exercise my controller code. Remember: I will be logging in the controller here, as it's a reaction to the response it's returning. I am not changing any Adapter logic here, I am adding some logging to the controller. TBH thinking about it now, maybe this should be in Service/Address instead of the controller. Hrm. Anyhow, I can refactor later if I want (100% test coverage so I'm safe to do that).

I'm also replacing the "live" logging handler in the container with a test one. This is so I don't actually log to the file system, it instead exposes an array of log entries, which I then check out in the custom assertion function:

public function assertLogEntryIsCorrect(
    TestHandler $testHandler,
    Level $expectedLogLevel,
    int $statusCode,
    string $expectedMessage
): void {
    $logRecords = $testHandler->getRecords();
    $this->assertCount(1, $logRecords);
    $this->assertEquals($expectedLogLevel->getName(), $logRecords[0]["level_name"]);
    $this->assertEquals(
        AddressService\Adapter::ERROR_MESSAGES[$statusCode],
        $logRecords[0]["message"]
    );
    $this->assertEquals(
        [
            "postcode" => TestConstants::POSTCODE_OK,
            "message" => $expectedMessage
        ],
        $logRecords[0]["context"]
    );
}

(Full disclosure, I am writing this after I have done the full implementation so like there's that ERROR_MESSAGES const array that came out of some refactoring I did after everything was working).

This assertion is simple enough: look for one log entry, and it needs to be the level, message and context that I should expect.

Before I can run that, I need to wire in Monolog, and before I do that, I need to install it. So I'm gonna have a quick functional test for that too:

/** @testdox It writes AddressService entries to the expected log file */
public function testAddressServiceLogFile()
{
    $kernel = new Kernel("test", false);
    $kernel->boot();
    $container = $kernel->getContainer();
    $logFile = $container->getParameter("kernel.logs_dir") . "/address_service.log";

    $logger = $container->get("monolog.logger.address_service");

    $this->assertEquals($logFile, $logger->getHandlers()[0]->getUrl());
}

This doesn't test any actual writing of data to a file: I figure that's Monolog's job to look after. I'm just verifying my config stays the same as I expect it. This is not a unit test, it's just an functional test: testing I've done the config right, and no-one monkeys with it later.

Now I will permit myself to actually install Monolog; or as it is in this case: symfony/monolog-bundle.

That needs a monolog.yaml file:

monolog:
    handlers:
        address_service_log:
            type: stream
            path: '%kernel.logs_dir%/address_service.log'
            level: debug
            channels: [address_service]

    channels: [address_service]

Having added that: my functional test works, so I'm happy I've configured my log.

Now I can do my implementation of the logging in the controller. I'll show you this in parts:

public function __construct(
    AddressService\Adapter $addressServiceAdapter,
    LoggerInterface $addressServiceLogger
) {
    $this->addressServiceAdapter = $addressServiceAdapter;
    $this->logger = $addressServiceLogger;
}

I need to add the logger parameter here, but I don't need to do anything to wire it in to the DI container. Symfony works out that if I ask for a LoggerInterface, then it'll take the parameter name, lop off "Logger" and look for a channel in my monolog.yaml file that is the snake-case version of that. So the paramter name here - $addressServiceLogger will find the address_service channel in my Monolog config. That's quite cool.

public function doGet(string $postcode) : JsonResponse
{
    try {
        $response = $this->addressServiceAdapter->get($postcode);

        $this->logUnexpectedFailures($response, $postcode);

        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => $response->getAddresses(),
                'message' => $response->getMessage()
            ],
            $response->getHttpStatus()
        );
    } catch (\Exception $e) {
        return new JsonResponse(
            [
                'postcode' => $postcode,
                'addresses' => [],
                'message' => $e->getMessage()
            ],
            HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR
        );
    }
}

There's just that one insertion into the controller logic, and that function is also pretty simple:

private function logUnexpectedFailures(
    AddressService\Response $response,
    string $postcode
): void {
    $statusCode = $response->getHttpStatus();

    if (array_key_exists($statusCode, self::RESPONSES_TO_LOG)) {
        $this->logger->log(
            self::RESPONSES_TO_LOG[$statusCode],
            AddressService\Adapter::ERROR_MESSAGES[$statusCode],
            ['postcode' => $postcode, 'message' => $response->getMessage()]
        );
    }
}

That also refers to this lot:

private const RESPONSES_TO_LOG = [
    HttpStatusCode::HTTP_UNAUTHORIZED => Level::Critical,
    HttpStatusCode::HTTP_FORBIDDEN => Level::Critical,
    HttpStatusCode::HTTP_TOO_MANY_REQUESTS => Level::Warning,
    HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR => Level::Warning
];

Level is an enum, which are new to PHP since the last time I used it. I like. I'll need to look into those in another article maybe (not least of all cos the docs are not as good as they could be).

All the code here is doing is checking if there's a case that we want to log via comparing the returned HTTP status code in that RESPONSES_TO_LOG array, and if one is there, log a message with the defined log level.

What's logged comes from the Adapter:

public const ERROR_MESSAGES = [
    HttpFoundationResponse::HTTP_UNAUTHORIZED => "API key is not valid",
    HttpFoundationResponse::HTTP_FORBIDDEN => "Permission denied",
    HttpFoundationResponse::HTTP_TOO_MANY_REQUESTS  => "Too many requests",
    HttpFoundationResponse::HTTP_INTERNAL_SERVER_ERROR => "Server error"
];

We don't really need too much detail here, we just need to know it's happened.

Again, I wonder if this is an adapter's job to define these. I think I do need a skinny wee service in between the adapter and controller here. I will do that refactor.

And that's it. I mean there are a few use statements about the place I didn't show you, but I'll link through to the code and you can look at everything, and there's really not much to it. It did take quite a while to dig out the docs for all this, given I was working with Symfony and Monolog, and testing of each, and being a newbie didn't help because some of the docs seem to assume whilst I was a n00b at (for example) Monolog then that's fine we'll document it slowly, but not thinking about the fact I also don't know the Symfony side of things either, I found the docs assume a level of knowledge that they shouldn't (at times). Being rusty with PHP (eg: not even knowing PHP did enums!) did not help. But I got there.

All the code is in tag 1.9 of this project on Github.

Righto.

--
Adam

TDD & Symfony: creating a small web service end point

G'day:

Background

I have a real-world requirement to get a small web service written: one that wraps up calls to the getaddress.io UK postcode look-up service. For those not familiar with UK post codes, they look something like "SW1W 0NY" and the chief conceit is that each postcode "generally represents a street, part of a street…". It's very common on an address-filling form for the first step to be trying to use the postcode to return a range of correct addresses, from which the user selects their own. This reduces keying errors or vagueness on their part. We have a few forms in our web apps that hit this webservice, and we need to migrate them over to PHP.

Yesterday I took a stab at implementing this in my own PHP8 project (the one I've been building in my recent articles), and got so focused I forgot to document what I was doing and why. So today I'm revisiting the code again, and writing this thing up. Warning: it's ended up being a monster.

---

Implementation plan

I need to produce an endpoint along the lines of /postcode-lookup/XX200X, and from the response from that, derive a reliable list of matching addresses to pass to the UI. I'm going to implement this in two parts:

• An adapter to sit in front of / around the HTTP call to the getaddress.io webs service. I mean "adapter" in the sense of the "Adapter Pattern"; I'll build a PHP interface to getaddress.io's HTTP one.
• A controller handler which receives a post-code, calls the adapter, gets a result from it and determines how best to respond to the client based on that result.

There's two approaches I could take with this:

• Top down: start with the interface of my own endpoint, nail that - mocking the adapter to start with - and then move downwards to the adapter and sort out the HTTP calls to getaddress.io.
• Bottom up: start with the HTTP calls to getaddress.io, build and adapter around it, finish that and then create a controller that is driven by how the adapter works.

Either would work. I've chosen the latter for a coupla reasons:

• I wasn't in the mood for dealing with Symfony when I thought about doing this work; I wanted to focus on code, not config.
• I have already documented an exercise in taking the first approach a coupla years ago:
  • Vue.js: using TDD to develop a data-entry form
  • Symfony & TDD: adding endpoints to provide data for front-end workshop / registration requirements
  Indeed that's a way more thorough handling of the exercise as it handles the front-end integration too.

Not in themselves great reasons to take that approach, but I doubt I would have bothered to do the work had I started with the Symfony side of things (reminding myself how routing, DI and controllers work). I was also only intending to do the adapter part for this exercise, even though I ultimately wired it up to the controller too (and it was dead easy as it turns out).

So. Time for some PHP code, some TDD via good old unit tests.

---

getaddress.io's web service

I had never used this web service before, but it's straight forward and well documented (again: documentation.getaddress.io). I clearly need an API key to access this thing, so signed-up for one of those (free, very limited usage), but they give some testing postcodes one can use to emulate the various responses one can get from their end point:

These postcodes yield both successful and unsuccessful responses to your request.

• XX2 00X or TR19 7AA or KW1 4YT Returns a 'successful' response 200. Your request was successful.
• XX4 00X Returns 'bad request' response400. Your postcode is not valid.
• XX4 01X Returns 'unauthorized' response 401. Your api-key is not valid.
• XX4 03X Returns 'forbidden' response 403. Your api-key is valid but you do not have permission to access to the resource.
• XX4 29X Returns 'too many requests' response 429. You have made more requests than your allowed limit.
• XX5 00X Returns 'server error' response 500. Server error, you should never see this.

Request made with these postcodes will not affect your usage.

(ibid.)

Oh, incidentally, they offer far more services than I need to use. I only need to deal with this call: https://api.getAddress.io/find/{postcode}?api-key={API key}.

Adapter

The adapter will need to deal with all those variants in some fashion, and returning the data in PHP rather than JSON; just return a PHP object that reflects success / and relevant failure states, along with a native-PHP representation of the returned address data. It can also through an exception in situations where it couldn't make sense of the response from getaddress.io. As it's only an adapter, it should not be performing any business logic; just interfacing between my app and the getaddress.io service. This is also why it's still returning HTTPish concepts rather than removing that entirely from the mix. That would be the job of a repository/service/some-other-domain-model-object sitting between it and the controller, if I chose to have one. I think an intermediary layer is overkill here, so have not bothered.

Routing and Controller

As mentioned, the public interface to my app needs to be to support GET requests to /postcode-lookup/{postcode}. The controller will be initialised with an adapter instance, the handler method will call its get method, and that will return a Response object, which will have properties for the returned addresses (if any), a message explaining potentially why no addresses were fetchable, and the HTTP status code of the upstream request. Or it - the adapter call - could throw an exception that will also need to be dealt with. The client won't be needing to know about those.

---

Code

API key

I've added a "missing" integration test here:

public function testEnvironmentVariables($expectedEnvironmentVariable)
{
    $this->assertNotFalse(
        getenv($expectedEnvironmentVariable),
        "Expected environment variable $expectedEnvironmentVariable to exist"
    );
}

public function expectedEnvironmentVariablesProvider() : array
{
    return [
        ["MARIADB_HOST"],
        ["MARIADB_PORT"],
        ["MARIADB_USER"],
        ["MARIADB_DATABASE"],
        ["MARIADB_ROOT_PASSWORD"],
        ["MARIADB_PASSWORD"],
        ["ADDRESS_SERVICE_API_KEY"]
    ];
}

I need all these environment variables to actually exist and be reachable by PHP, so I ought to have an integration test to make sure they do exist. I've added in the ADDRESS_SERVICE_API_KEY on there as this is what I'm about to add, and I want a failing test. You might or might not remember I'm loading my environment variables like this (in docker-compose.yml):

php:
  build:
    context: php
    dockerfile: Dockerfile

  env_file:
    - envVars.public
    - envVars.private

I have two files: the envVars.public one is in source control. It's got stuff like this in it:

MARIADB_HOST=mariadb
MARIADB_PORT=3306
MARIADB_DATABASE=db1
MARIADB_USER=user1

In contrast the envVars.private one is not in source control, as it has stuff like this in it:

# do not commit this file to your repository
MARIADB_ROOT_PASSWORD=[redacted]
MARIADB_PASSWORD=[redacted]
ADDRESS_SERVICE_API_KEY=[redacted]

You can see I've slung the ADDRESS_SERVICE_API_KEY in there. After cycling my containers, that test passes.

---

Adapter testing

Unit tests

I'm gonna eschew the "lets walk through the TDD steps" here as there's 10 unit tests, two integration tests, and 250 lines of test code that went through a chunk of refactoring after I got it all working. I'm just gonna show the tests - all of them - and discuss them.

/** @testdox It throws an AddressService\Exception if the getaddress.io call returns an unexpected status */
public function testThrowsExceptionOnUnexpectedStatus()
{
    $statusToReturn = Response::HTTP_NOT_IMPLEMENTED;

    $this->assertCorrectExceptionThrown(
        AddressServiceException::class,
        "Unexpected status code returned: $statusToReturn"
    );

    $adapter = $this->getTestAdapter($statusToReturn, "CONTENT_NOT_TESTED");

    $adapter->get("POSTCODE_NOT_TESTED");
}

If you refer back to the list of test postcodes above, and the HTTP response statuses they result in, 501 / not implemented is not one of them. So I am testing my handling of this situation throws an exception, because we have NFI what's going on with getaddress.io if we get one of these back from them, so there's no point continuing processing. The relevant fragment of the Adapter code is:

private const SUPPORTED_SERVICE_RESPONSES = [
    HttpFoundationResponse::HTTP_OK,
    HttpFoundationResponse::HTTP_BAD_REQUEST,
    HttpFoundationResponse::HTTP_UNAUTHORIZED,
    HttpFoundationResponse::HTTP_FORBIDDEN,
    HttpFoundationResponse::HTTP_TOO_MANY_REQUESTS,
    HttpFoundationResponse::HTTP_INTERNAL_SERVER_ERROR
];

// ...

if (!in_array($statusCode, self::SUPPORTED_SERVICE_RESPONSES)) {
    throw new AddressService\Exception("Unexpected status code returned: $statusCode");
}

Don't worry, I'll list the whole thing further down, I just wanna focus on which bits are being tested for now.

The test code above has a few helper methods I've extracted out during refactor:

private function assertCorrectExceptionThrown(string $type, string $message): void
{
    $this->expectException($type);
    $this->expectExceptionMessage($message);
}

private function getTestAdapter(int $statusToReturn, string $content): AddressServiceAdapter
{
    $client = $this->getMockedClient($statusToReturn, $content);

    return new AddressServiceAdapter("NOT_TESTED", $client);
}

private function getMockedClient(int $statusToReturn, string $content): MockObject
{
    $response = $this->getMockedResponse($statusToReturn, $content);

    $client = $this
        ->getMockBuilder(HttpClientInterface::class)
        ->disableOriginalConstructor()
        ->getMock();

    $client
        ->expects($this->once())
        ->method("request")
        ->willReturn($response);

    return $client;
}

private function getMockedResponse(int $status, string $content): MockObject
{
    $response = $this
        ->getMockBuilder(ResponseInterface::class)
        ->disableOriginalConstructor()
        ->getMock();
    $response
        ->expects($this->atLeastOnce())
        ->method("getStatusCode")
        ->willReturn($status);
    $response
        ->expects($this->any())
        ->method("getContent")
        ->willReturn($content);

    return $response;
}

The strategy here is that I am testing the Adapter's logic, and how it deals with different responses from the HTTP call. So I mock the HTTP client to return the various responses I need to exercise the Adapter logic I need to write. As you'll see from the rest of the tests, they all use the mocked client, and all the exception handling tests use that assertCorrectExceptionThrown custom assertion. This refactoring keeps the tests simple and clear, and just removes necessary boilerplate machinery.

It's just as important to refactor one's tests as it is to refactor one's source code. Test code still needs to be read by humans.

/** @testdox It throws an AddressService\Exception if the body is not JSON */
public function testThrowsExceptionOnBodyNotJson()
{
    $this->assertCorrectExceptionThrown(
        AddressServiceException::class,
        "json_decode returned [Syntax error]"
    );

    $adapter = $this->getTestAdapter(Response::HTTP_OK, "NOT_JSON");

    $adapter->get("NOT_TESTED");
}

You can see how simple this test is, and following the same approach as the previous one. Just testing a slightly different facet of the logic.

And the implementation code for this:

$body = $response->getContent(false);
$lookupResult = json_decode($body, JSON_OBJECT_AS_ARRAY);
if (json_last_error() != JSON_ERROR_NONE) {
    throw new AddressService\Exception(
        sprintf("json_decode returned [%s]", json_last_error_msg())
    );
}

I'll just give you the @testdox line from the other exception-handling tests, followed by the implementation code. The test implementations are all similar: just different return codes and body, so no point reproducing them.

/** @testdox Throws an AddressService\Exception if the body is not an array */

if (!is_array($lookupResult)) {
    throw new AddressService\Exception("Response JSON schema is not valid");
}

/** @testdox it throws an AddressService\Exception if there is no address data in the response json */
// …

/** @testdox it throws an AddressService\Exception if the addresses data is not an array */
// …

/** @testdox it throws an AddressService\Exception if the addresses data is not an array of strings */
// …

These three test different subexpressions in the one if expression:

if (
    !array_key_exists("addresses", $lookupResult)
    || !is_array($lookupResult["addresses"])
    || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
) {
    throw new AddressService\Exception("Response JSON schema is not valid");
}

I'm not normally crazy about compound if expressions like that, but they're all testing variants of garbage that we don't want to pass back to the calling code.

Those are all the exception-handling tests, now for the situations where we can actually return something:

/** @testdox returns empty addresses with status code on a non-200-OK response */
public function testReturnsEmptyAddressesOnNon200Response()
{
    $statusToReturn = Response::HTTP_BAD_REQUEST;

    $adapter = $this->getTestAdapter(
        $statusToReturn,
        '{"Message": "Bad Request: Invalid postcode."}'
    );

    $result = $adapter->get("NOT_TESTED");

    $this->assertEquals($statusToReturn, $result->getHttpStatus());
    $this->assertEquals([], $result->getAddresses());
}
// …

/** @testdox it returns the message on a non-200 response */
// …

/** @testdox it returns a standard message if the non-200 response doesn't include a valid one */
// …

These are pretty similar to the exception-handling tests, just different assertions. And the implementation being tested for all of those is in the one function:

private function handleFailureResponse(
    array $lookupResult,
    int $statusCode
): AddressService\Response {
    if (array_key_exists("Message", $lookupResult) && is_string($lookupResult["Message"])) {
        return new AddressService\Response([], $statusCode, $lookupResult["Message"]);
    }
    return new AddressService\Response([], $statusCode, "No failure message returned from service");
}

And finally the happy path:

/** @testdox it returns a Response object if the response is valid */
public function testReturnsResponseObject()
{
    $statusToReturn = Response::HTTP_OK;
    $expectedAddresses = [
        "TEST_ADDRESS_1",
        "TEST_ADDRESS_2"
    ];

    $adapter = $this->getTestAdapter(
        $statusToReturn,
        sprintf('{"addresses": %s}', json_encode($expectedAddresses))
    );

    $result = $adapter->get("NOT_TESTED");

    $this->assertEquals($statusToReturn, $result->getHttpStatus());
    $this->assertEquals($expectedAddresses, $result->getAddresses());
}

Implemenation:

private function handleSuccessResponse(array $lookupResult): AddressService\Response
{
    if (
        !array_key_exists("addresses", $lookupResult)
        || !is_array($lookupResult["addresses"])
        || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
    ) {
        throw new AddressService\Exception("Response JSON schema is not valid");
    }

    return new AddressService\Response(
        $lookupResult["addresses"],
        HttpFoundationResponse::HTTP_OK
    );
}

---

Integration tests

All of these unit tests only test the logic in the Adapter, having mocked-out the HTTP call. They don't actually test it will actually do what it's supposed to, which requires a call to the actual getaddress.io. This is where some integration tests come in.

/** @testdox Tests of the Adapter */
class AdapterTest extends TestCase
{
    private $adapter;

    protected function setUp(): void
    {
        $client = HttpClient::create();
        $this->adapter = new AddressServiceAdapter(getenv("ADDRESS_SERVICE_API_KEY"), $client);
    }

    /** @testdox It can get addresses from a valid postcode */
    public function testCanGetAddress()
    {
        $response = $this->adapter->get(TestConstants::POSTCODE_OK);

        $this->assertEquals(Response::HTTP_OK, $response->getHttpStatus());
        $this->assertGreaterThanOrEqual(1, count($response->getAddresses()));
        $this->assertEmpty($response->getMessage());
    }

    public function provideErrorTestCases(): array
    {
        return [
            [TestConstants::POSTCODE_INVALID, Response::HTTP_BAD_REQUEST],
            [TestConstants::POSTCODE_UNAUTHORIZED, Response::HTTP_UNAUTHORIZED],
            [TestConstants::POSTCODE_FORBIDDEN, Response::HTTP_FORBIDDEN],
            [TestConstants::POSTCODE_OVER_LIMIT, Response::HTTP_TOO_MANY_REQUESTS],
            [TestConstants::POSTCODE_SERVER_ERROR, Response::HTTP_INTERNAL_SERVER_ERROR]
        ];
    }

    /**
     * @testdox It returns the expected HTTP status code and a message but no addresses on an error
     * @dataProvider provideErrorTestCases
     */
    public function testReturnsExpectedHttpStatusAndMessageButNoAddressesOnError($postcode, $expectedHttpStatus)
    {
        $response = $this->adapter->get($postcode);

        $this->assertEquals($expectedHttpStatus, $response->getHttpStatus());
        $this->assertNotEmpty($response->getMessage());
        $this->assertEquals(0, count($response->getAddresses()));
    }
}

These test a happy-path response, as well as how the other "failure" responses from getaddress.io are handled. One might wonder why I don't test the "utter failure" cases here, where I throw an exception. I almost did, but then I thought that's nothing to do with the integration with getaddress.io, it's all down to how I handle that integration, and that's my code, and covered in the unit tests.

The only interesting / non-standard thing in that lot is the code to get the API key:

$this->adapter = new AddressServiceAdapter(getenv("ADDRESS_SERVICE_API_KEY"), $client);

The app never needs to know the key. Just the environment does.

Oh this code references some TestConstants:

<?php

namespace adamcameron\php8\tests\Fixtures\AddressService;

class TestConstants
{
    // provided by https://documentation.getaddress.io/ (these do not impact look-up usage)
    public const POSTCODE_OK = "XX2 00X";
    public const POSTCODE_INVALID = "XX4 00X";
    public const POSTCODE_UNAUTHORIZED = "XX4 01X";
    public const POSTCODE_FORBIDDEN = "XX4 03X";
    public const POSTCODE_OVER_LIMIT = "XX4 29X";
    public const POSTCODE_SERVER_ERROR = "XX5 00X";
}

These are separate from the integration test class as the controller tests need them too. I'll get to that.

---

Adapter implementation

namespace adamcameron\php8\Adapter\AddressService;

use adamcameron\php8\Adapter\AddressService;
use Symfony\Component\HttpFoundation\Response as HttpFoundationResponse;
use Symfony\Contracts\HttpClient\HttpClientInterface;
use Symfony\Contracts\HttpClient\ResponseInterface;

class Adapter
{

These are the details we need from getaddress.io:

    private const SUPPORTED_SERVICE_RESPONSES = [
        HttpFoundationResponse::HTTP_OK,
        HttpFoundationResponse::HTTP_BAD_REQUEST,
        HttpFoundationResponse::HTTP_UNAUTHORIZED,
        HttpFoundationResponse::HTTP_FORBIDDEN,
        HttpFoundationResponse::HTTP_TOO_MANY_REQUESTS,
        HttpFoundationResponse::HTTP_INTERNAL_SERVER_ERROR
    ];

    private const SERVICE_URL_TEMPLATE = "https://api.getAddress.io/find/{postcode}?api-key={api-key}";

    private string $apiKey;
    private HttpClientInterface $client;

    public function __construct(string $apiKey, HttpClientInterface $client)
    {
        $this->apiKey = $apiKey;
        $this->client = $client;
    }

This is the only public functions in here:

    public function get(string $postCode) : AddressService\Response
    {
        $response = $this->makeRequest($postCode);
        $lookupResult = $this->extractValidLookupResult($response);

        return $this->handleValidatedResponse($response, $lookupResult);
    }

Everything else is just refactoring to keep each task (ie: function) separate:

    private function makeRequest(string $postCode): ResponseInterface
    {
        $url = strtr(
            self::SERVICE_URL_TEMPLATE,
            ["{postcode}" => $postCode, "{api-key}" => $this->apiKey]
        );

        return $this->client->request("GET", $url);
    }

    private function extractValidLookupResult(ResponseInterface $response): array
    {
        $statusCode = $response->getStatusCode();

        if (!in_array($statusCode, self::SUPPORTED_SERVICE_RESPONSES)) {
            throw new AddressService\Exception("Unexpected status code returned: $statusCode");
        }

        $body = $response->getContent(false);
        $lookupResult = json_decode($body, JSON_OBJECT_AS_ARRAY);
        if (json_last_error() != JSON_ERROR_NONE) {
            throw new AddressService\Exception(
                sprintf("json_decode returned [%s]", json_last_error_msg())
            );
        }

        if (!is_array($lookupResult)) {
            throw new AddressService\Exception("Response JSON schema is not valid");
        }
        return $lookupResult;
    }

    private function handleValidatedResponse(
        ResponseInterface $response,
        array $lookupResult
    ): AddressService\Response {
        $statusCode = $response->getStatusCode();

        if ($statusCode == HttpFoundationResponse::HTTP_OK) {
            return $this->handleSuccessResponse($lookupResult);
        }
        return $this->handleFailureResponse($lookupResult, $statusCode);
    }

    private function handleSuccessResponse(array $lookupResult): AddressService\Response
    {
        if (
            !array_key_exists("addresses", $lookupResult)
            || !is_array($lookupResult["addresses"])
            || count(array_filter($lookupResult["addresses"], fn($address) => !is_string($address)))
        ) {
            throw new AddressService\Exception("Response JSON schema is not valid");
        }

        return new AddressService\Response(
            $lookupResult["addresses"],
            HttpFoundationResponse::HTTP_OK
        );
    }

    private function handleFailureResponse(
        array $lookupResult,
        int $statusCode
    ): AddressService\Response {
        if (array_key_exists("Message", $lookupResult) && is_string($lookupResult["Message"])) {
            return new AddressService\Response([], $statusCode, $lookupResult["Message"]);
        }
        return new AddressService\Response([], $statusCode, "No failure message returned from service");
    }
}

Hopefully given the explanations of all that in the tests, that's all clear. The code is pretty straight forward.

I'm using a custom exception so any consuming code can seprate out out "stuff this Adapter might throw" from "anything else that went wrong" (say for example the Symfony HttpClient spat the dummy for some reason):

namespace adamcameron\php8\Adapter\AddressService;

class Exception extends \RuntimeException
{
}

I will get murderous looks from some ppl for having a class that only holds a coupla data points and some accessors for same, but the adapter does simply return data, but it's a specific sort of data, and the consuming code ought only be reading it, not writing it. I think this is legit usage:

namespace adamcameron\php8\Adapter\AddressService;

class Response
{
    private array $addresses;
    private int $httpStatus;
    private string $message;

    public function __construct(array $addresses, int $httpStatus, string $message = "")
    {
        $this->addresses = $addresses;
        $this->httpStatus = $httpStatus;
        $this->message = $message;
    }

    public function getAddresses()
    {
        return $this->addresses;
    }

    public function getHttpStatus()
    {
        return $this->httpStatus;
    }

    public function getMessage()
    {
        return $this->message;
    }
}

If PHP allowed it, I'd probably make this an inner class of the Adapter as well. But PHP can't so just having it in the same package will have to do. I realise PHP now allows for class expressions, or anonymous classes or whatever they are, but I don't think that's quite right for this application. I just need to return an object from the adapter for the calling code to get predictable / save / checked values from.

---

Controller integration

I cheated with this bit and did not TDD it. I could not remember how controllers in Symfony worked, so I got the controller operational first, and then backfilled the tests. Soz.

Routing and service config

Firstly the routing (in routes.yaml):

controllers:
    resource:
        path: ../src/Controller/
        namespace: adamcameron\php8\Controller
    type: attribute

PostcodeLookup:
    path: /postcode-lookup/{postcode}
    controller: adamcameron\php8\Controller\PostcodeLookupController::doGet
    methods: GET

Then some DI config (in services.yaml):

# …
adamcameron\php8\Adapter\AddressService\Adapter:
    public: true
    arguments:
        $apiKey: '%env(ADDRESS_SERVICE_API_KEY)%'
        $client: '@Symfony\Component\HttpClient\HttpClient'

Symfony\Component\HttpClient\HttpClient:
    factory: ['\Symfony\Component\HttpClient\HttpClient','create']
# …

I only need to put the services that have non-obvious constructor argument requirements, like how I need to specify to use the environment variable for its $apiKey argument, and I need to specify to use the HttpClient::create factory method to create the $client value. Everything else is auto-wired on the basis of the type in the constructor's method signature.

Accordingly, the controller will be taking an instance of the Adapter, but Symfony can work that out from the type specification in its constructor method signature, and know to get the one I have configured here. Dead clever.

---

Controller implementation

namespace adamcameron\php8\Controller;

use adamcameron\php8\Adapter\AddressService\Adapter as AddressServiceAdapter;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Response;

class PostcodeLookupController extends AbstractController
{
    private AddressServiceAdapter $addressServiceAdapter;

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

    public function doGet(string $postcode)
    {
        try {
            $response = $this->addressServiceAdapter->get($postcode);

            return new JsonResponse(
                [
                    'postcode' => $postcode,
                    'addresses' => $response->getAddresses(),
                    'message' => $response->getMessage()
                ],
                $response->getHttpStatus()
            );
        } catch (\Exception $e) {
            return new JsonResponse(
                [
                    'postcode' => $postcode,
                    'addresses' => [],
                    'message' => $e->getMessage()
                ],
                Response::HTTP_INTERNAL_SERVER_ERROR
            );
        }
    }
}

As all good controller methods ought to be: simple. It receives a value from the request… passes it to the model tier to get some data… based on vagaries of the data, works out how to build the response… and sends it.

Here the conceit is that I really don't care what exceptions the Adapter might throw: I don't want to break the response, so I just swallow them and return an emptyish response, with the appropriate server error.

---

Controller testing

These are all in PostcodeLookupControllerTest.

private KernelBrowser $client;

protected function setUp(): void
{
    $this->client = static::createClient();
}

/** @testdox It retrieves addresses when the post code is valid */
public function testRetrievesAddressesWhenPostCodeIsValid()
{
    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", TestConstants::POSTCODE_OK)
    );
    $response = $this->client->getResponse();

    $this->assertEquals(
        Response::HTTP_OK,
        $response->getStatusCode()
    );

    $result = json_decode($response->getContent(), false);
    $this->assertObjectHasAttribute('addresses', $result);
    $this->assertGreaterThanOrEqual(1, count($result->addresses));
}

Happy path: it returns addresses. Hurrah. I don't check the contents of the addresses as I am happy that the Adapter code will only let legit ones through, and the content of the addresses is outwith my control: that's on getaddress.io.

/**
 * @testdox It returns an error status code and no addresses when the postcode is invalid
 * @dataProvider provideCasesForClientErrorTests
 */
public function testReturnsErrorStatusCodeAndNoAddressesWhenPostCodeIsInvalid(
    string $postcode,
    int $statusCode
) {
    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", $postcode)
    );
    $response = $this->client->getResponse();

    $this->assertEquals($statusCode, $response->getStatusCode());

    $result = json_decode($response->getContent(), false);
    $this->assertObjectHasAttribute('addresses', $result);
    $this->assertEmpty($result->addresses);
}

public function provideCasesForClientErrorTests() : array
{
    return [
        [TestConstants::POSTCODE_INVALID, Response::HTTP_BAD_REQUEST],
        [TestConstants::POSTCODE_UNAUTHORIZED, Response::HTTP_UNAUTHORIZED],
        [TestConstants::POSTCODE_FORBIDDEN, Response::HTTP_FORBIDDEN],
        [TestConstants::POSTCODE_OVER_LIMIT, Response::HTTP_TOO_MANY_REQUESTS],
        [TestConstants::POSTCODE_SERVER_ERROR, Response::HTTP_INTERNAL_SERVER_ERROR]
    ];
}

Superficial tests of all the expected failure situations, making sure they "work".

This next one is more interesting:

/** @testdox it returns an error status and no addresses when there's been a server error */
public function testReturnsErrorStatusCodeAndNoAddressesWhenServerError()
{
    $container = self::getContainer();
    $mockedAddressServiceAdapter = $this
        ->getMockBuilder(AddressService\Adapter::class)
        ->disableOriginalConstructor()
        ->onlyMethods(['get'])
        ->getMock();
    $mockedAddressServiceAdapter
        ->expects($this->once())
        ->method('get')
        ->willThrowException(new AddressService\Exception("TEST_ERROR_MESSAGE"));
    $container->set(AddressService\Adapter::class, $mockedAddressServiceAdapter);

    $this->client->request(
        "GET",
        sprintf("/postcode-lookup/%s", TestConstants::POSTCODE_OK)
    );

    $response = $this->client->getResponse();

    $this->assertEquals(
        Response::HTTP_INTERNAL_SERVER_ERROR,
        $response->getStatusCode()
    );

    $result = json_decode($response->getContent(), false);
    $this->assertObjectHasAttribute('addresses', $result);
    $this->assertEmpty($result->addresses);
    $this->assertEquals("TEST_ERROR_MESSAGE", $result->message);
}

Here I'm mocking the dependency injection container to mock-out the HttpClient to fake a server error on "their" end. It's good to know how to do that, and it's fairly straight forward (he says, after googling for over an hour to find out how to do it!). I'm actually thinking now that this is a functional test, not an integration test, as it's not actually hitting the external service. Hrm. I shall move it I think. Oh: I think "functional test" not "unit test" because it's cutting across a couple of concerns: Adapter logic and Controller logic, and that's where I make the (admittedly fairly arbitrary) distinction: if the "unit" is in one object: unit test. If it goes across one object into a dependent object: functional test.

I'm pretty sure that's it. Just as well. This is a monster of an article. Sorry.

---

Conclusions, and "next…"

I need to improve this by paying attention to a couple of those statuses back from getaddress.io, namely the 401, 403 and 429 responses. These might need remediation on our end, so I want to log something if we get those responses. And also log something in the "catch all" catch there. If there's been an unexpected error, whilst the client might not care, we should.

I'm gonna need to mess around with Monolog to do that, so I'll take that as a separate refresher exercise.

Well done, if you got this far. All the code for this is in the 1.7.3 tag on GitHub (I've linked everything relevant directly as well).

Righto.

--
Adam

Kotlin / TDD: writing the tests for a small web service

G'day:

One of my tasks @ work is to check out how to test a web service. I started with this yesterday's article: Kotlin: getting the khttp library installed and running… then… getting rid of it and using something else, but that was justa "proof of concept" of making an HTTP call, and examining its results. Today I'm gonna write the actual tests we need for the interface for a web service. Note: I still don't know enough about Ktor to create a web service with it, so I'm gonna fall back to using CFML for that end of things. I'm not gonna bother with the code for that here; the tests will demonstrate whether or not the web service is fulfilling its contract. But I will be TDDing this. Currently I have zero tests and zero code to test. Let's get on with it.

---

It should return a 200-OK on the root URI on a valid request

fun `It should return a 200-OK on the root URI on a valid request`() {
    runBlocking {
        HttpClient().use { client ->
            val response = client.get(webserviceUrl)
            response.status shouldBe HttpStatusCode.OK
        }
    }
}

NB: I will only comment if there's something note-worthy or not obvious. All tests will initially fail until I implement the relevant bit of the web service, eg:

expected:<200 OK> but was:<404 Not Found>
Expected :200 OK
Actual   :404 Not Found

---

It should return a 406-NOT-ACCEPTABLE and suggest the expected type if the Accepts header is not application/json

@Test
fun `It should return a 406-NOT-ACCEPTABLE and suggest the expected type if the Accepts header is not application-json`() {
    runBlocking {
        HttpClient().use { client ->
            val response = client.get(webserviceUrl) {
                header("Accept", "text/plain")
            }

            response.status shouldBe HttpStatusCode.NotAcceptable
            response.body() as String shouldBe """["application/json"]"""
        }
    }
}

Note: It's a Kotlin/JVM limitation that I have to use application-json rather than application/json in the method name there. See Why Kotlin does not allow slash in identifiers, which in turn points the reader to Java Virtual Machine Specification › Chapter 4. The class File Format:

4.2.2. Unqualified Names

Names of methods, fields, local variables, and formal parameters are stored as unqualified names. An unqualified name must contain at least one Unicode code point and must not contain any of the ASCII characters . ; [ / (that is, period or semicolon or left square bracket or forward slash).

When I initially just referenced response.body() shouldBe """["application/json"]""" Kotlin was saying "Not enough information to infer type variable", and after some googling I landed on Type checks and casts › "Unsafe" cast operator, which explains that as String thing I have there.

Also note I needed to update the 200-OK test to pass the correct Accept header.

---

It returns an array of Numbers as a JSON array

This one was a bit trickier, but the docs were reasonably helpful, and I'm pleased with the outcome.

@Test
fun `It returns an array of Numbers as a JSON array`() {
    @Serializable
    data class Number(val id: Int, val en: String, val mi: String)

    runBlocking {
        HttpClient() {
            install(ContentNegotiation) {
                json()
            }
        }.use { client ->
            val response = client.get(webserviceUrl) {
                header("Accept", "application/json")
            }
            response.status shouldBe HttpStatusCode.OK
            response.body() as List<Number> shouldBe listOf(
                Number(1, "one", "tahi"),
                Number(2, "two", "rua"),
                Number(3, "three", "toru"),
                Number(4, "four", "wha")
            )
        }
    }
}

I had guessed that there'd be a way to deliver objects straight to the app from an HTTP request, and a quick google set me on the right path here, landing me on these docs: Content negotiation and serialization. What's super helpful in these docs as they link through to examples of everything (including the build.gradle.kt file, as I needed to add some dependencies - see further down), eg: ktor-documentation/codeSnippets/snippets/client-json-kotlinx/src/main/kotlin/com/example/Application.kt and ktor-documentation/codeSnippets/snippets/client-json-kotlinx/build.gradle.kts. I just followed those and changed the bits I needed to change.

Steps:

• tell the client that I'm gonna expect it to work out what the content represents (boilerplate).
• Create a data class that the client will use to deserialize the data as. NB: it needs to be tagged as being serializable, because well: that's what we're doing here.
• Specify that type - or in this case a list of that type - as the body value.

Done.

I will admit than initially I thought I had messed-up because instead of getting a "nup, it's not a list of Numbers" in my failing test, I got this monstrousity:

io.ktor.client.call.NoTransformationFoundException: No transformation found: class io.ktor.utils.io.ByteBufferChannel -> class kotlin.collections.List

But it turns out that just means "um… yer request ain't returning JSON". Which it indeed was not. Once I got it to return something (anything) and the correct content-type, I got a more on-point error:

Expected start of the array '[', but had 'EOF' instead at path: $
JSON input: ""  

And from there I tweaked the web service to return a coupla wrong things to see how Ktor reported on deserialization failures, and it was all helpful.

All the examples I saw for this was only deserializing one object, so I was slightly cautious as to how to deal with a JSON array, but I took a punt on just specifying response.body() as List<Number>, thinking Ktor's probably clever enough to expect this sort of thing, and - lo - it did. Nice one.

As I mentioned above, I also had to tweak the dependencies and plugins a bit (build.gradle.kts):

plugins {
    kotlin("jvm") version "1.7.10"
    kotlin("plugin.serialization").version("1.7.10")
    application
}

// …

dependencies {
    // …
    implementation("io.ktor:ktor-client-core:$ktorVersion")
    implementation("io.ktor:ktor-client-cio:$ktorVersion")
    implementation("io.ktor:ktor-client-content-negotiation:$ktorVersion")
    implementation("io.ktor:ktor-serialization-kotlinx-json:$ktorVersion")
    // …
}

---

It will accept a POST request of an object as JSON and return the same object as confirmation, and its URL

This was really straight-forward, having done all the hard-bit in the previous one:

@Test
fun `It will accept a POST request of an object as JSON and return the same object as confirmation, and its URL`() {
    val five = Number(5, "five", "rima")
    runBlocking {
        HttpClient() {
            install(ContentNegotiation) {
                json()
            }
        }.use { client ->
            val response = client.post(webserviceUrl) {
                contentType(ContentType.Application.Json)
                setBody(five)
            }
            response.status shouldBe HttpStatusCode.Created
            response.body() as Number shouldBe five
            response.headers["Location"] shouldBe "${webserviceUrl}5"
        }
    }
}

By now there's absolutely nothing "unexpected" in this, I think.

---

OK that's enough for a Sunday afternoon. This weekend I've managed to work out how to make HTTP requests in my tests, how to set/check headers, response codes and the body of the responses. I've posted an object and received objects back again, letting Ktor handle the (de)serialization.

These tests are only testing the interface of the web webservice, which is fine and an essential part of building a web service. However the next thing on the list is to do end to end tests: check the underlying data store that new objects are being created (cos they absolutely are not at the moment ;-)), and the correct data is being returned, etc. I have no idea how to make a DB call in Kotlin yet. Something for the coming week, I guess.

The code is on GitHub @ /src/test/kotlin/junit/practical/WebServiceTest.kt, and the web stub service I was creating to make the tests pass is this lot: adamcameron/Numbers.cfc.

Righto.

--
Adam

TDD: are tests that just "measure twice, cut once" legit?

G'day:

There's a bit of archaeology going on here: I started writing this in Jan 2022, but never got past the first para. I have a few articles like this, and I've decided to either complete them if they have merit, or get rid if they don't.

Part of my problem previously on this blog is that if I didn't think I could spin the article out to be say ~1500 words, I didn't think it was worth writing about. I think that was wrong-headed, and there's nothing wrong with taking a quick look at something I've been thinking about. Let's see if this works.

The situation I describe herein actually happened after I had wondered about the subject matter. It bore out/validated my hypothesis quite nicely.

Today I had an interesting TDD case. We have a features that rely on finding the "next working day" in various countries. The code behind these features ultimately calls a function isBankHoliday, and the logic in that function maintains a list of bank holidays within its implementation. This is perhaps not the ideal approach, but it's the approach we have.

I was loading in the holidays for 2023, and me being me, the first thing I did was to look for the unit tests for the function. We didn't have one. That was odd because we had a job a few months prior to add the 2022 holidays, and I can recall discussing the need for tests with the dev concerned. Apparently this never happened, and I can't follow-up because said dev is no longer with us. Harrumph.

I can't reproduce any code relating to this here because it's my employer's code, but I can describe what I did. I opened the official govt sites that list public holidays (eg in the UK it is https://www.gov.uk/bank-holidays), and wrote a test that iterated through the list of 2023(*) holidays there and called isBankHoliday on each of them, expecting it to be true. I then picked some edge-case days around bank holidays and tested those expected the result to be false. This gives both a control group (the tests returning false), and the TDD tests for the work I was about to do. I got the expected pass/failures: all good.

(*) why did I not do all of them, and only tested the 2022 ones I was adding? We have a policy of not backfilling missing unit tests, because we'd be there all day/month/year if we did. We only have maybe 25% code coverage of our codebase for… "historical reasons".

OK, so the tests are in place. I then closed the test file, went back to https://www.gov.uk/bank-holidays and re-copied all the holiday dates again. I did not pull the prior work out of the test and refactor to suit the method implementation. I even made sure to specifically use a different data structure in the tests than I knew we had in the method implementation: the tests used an array because I'm not a muppet; the method implementation used a comma-delimited string (ugh).

Why did I do this? Well: measure twice, cut once. If I wrote the tests, then lifted the test data out of the tests and put them in the implementation, all I'd be doing is testing my ability to copy and paste. I would not be testing that I had keyed the source data I was testing against correctly. A typo in the test data would translate to a typo in the implementation, but the test would still pass.

That code went into production and has not caused us any problems.

Back in May this year we had a system glitch where a bunch of processing didn't run. It was 2022-05-03. We looked into it and found that the dev who had loaded in the 2022 holidays had miskeyed, and had entered the Queen's Platinum Jubilee holiday as 2022-05-03 not 2022-06-03. And with no tests: we did not catch it. It went through code review, that said, and we didn't actually catch it there either.

This situation caused a bunch of work for us because not only did we have to remediate the unprocessed work from 2022-05-03, but the system had also started to queue-up work for 2022-06-03 when it shouldn't have, and we had to unpick that lot too.

All because we didn't measure twice, and cut once.

Righto.

--
Adam

A super-quick Observer Pattern implementation in CFML, and I skip TDD. Or do I?

G'day:

There's a possible "Betteridge's law of headlines" situation there. I'm not actually sure yet.

A few days back I was chatting to someone about application-scope-usage, and how to trigger an event when any changes to the application scope took place. I mentioned that the application scope should never be accessed directly in one's application code, and it should be wrapped up in an adapter. And it's easy for one's adapter to trigger events if needs must. An implementation of the Observer Pattern would do the trick here.

Then it occurred to me that I have never actually implemented the Observer Pattern, and I thought I might give it a go.

I've read that Wiki article a few times, and know what it needs to do, but hadn't thought it through at all before. So I opened a file and settled in to work out what I'd need to do. Then I paused and went "Adam: tests. Don't let yerself down here". Grumble. But I reasoned that I didn't even know if I was gonna go anywhere with this code, so I decided to call it a spike (random code which will be thrown away, and no tests).

I decided to implement it as an ObserverService: an instance of it would be injected into other objects that needed to either attach a handler to an event, or trigger an event. Internally the service would have to maintain some data structure of events and handlers, and probably only needed a coupla methods. Following jQuery (my JS is very out of date, I admit this), I decided on on for the method to attach a handler to an event; and trigger to fire the event (and run the handlers). As I type this, I realise I'm already thinking of the bases for test cases. I shoulda just done the tests up front. Or at least made the test cases, eg:

describe("test for the on method",  () => {
    it("adds an event handler to an event", () => {
    })
})

describe("test for the trigger method",  () => {
    it("runs all the event handlers for the event", () => {
    })
})

Oh well.

Right so I spiked away, and it all came together rather more easily than I expected. Here's the first pass (untested):

component {

    variables.eventRegistry = {}

    public void function on(required string event, required function handler) {
        registerEvent(event)
        variables.eventRegistry[event].append(handler)
    }

    public boolean function trigger(required string event) {
        registerEvent(event)
        return variables.eventRegistry[event].some((handler) => handler())
    }

    private void function registerEvent(required string event) {
        if (!variables.eventRegistry.keyExists(event)) {
            variables.eventRegistry[event] = []
        }
    }
}

That seems to have everything I need to start with:

• I can attach a handler to an event.
• I can trigger the event, causing the handlers to run in sequence.
• If any handler returns false, stop calling handlers.
• A bug that never would have cropped up had I done TDD properly.

Right so the way to check if this thing works is still via tests, so I knocked some tests together now. I'll just list the cases, as the implementations don't matter so much. I wrote all of these before running any of them (on purpose because I was being a dick, not cos it's good practice).

import cfml.forBlog.observerService.ObserverService

component extends=Testbox.system.BaseSpec {
    function run() {
        describe("Tests for ObserverService", () => {
            describe("Tests for on method", () => {
                it("registers an event handler", () => {
                })

                it("registers multiple event handlers", () => {
                })

                it("registers multiple handlers for multiple events", () => {
                })
            })

            describe("tests for trigger method", () => {
                it("triggers a registered event", () => {
                })

                it("does nothing when triggering an unregistered event", () => {
                })

                it("triggers event handlers until one returns false", () => {
                })
            })
        })
    }
}

And I triumphantly ran them, and then…

… I did not feel very triumphant any more. That was the result of every single test. So that's the bug. Did you spot it, or can you see it now?

return variables.eventRegistry[event].some((handler) => handler())

The handlers I was attaching to the event didn't necessarily return true if they worked. Just "not erroring" is a good enough sign an event handler ran OK. EG:

observerService.on("testEvent", () => {
    eventResults.append("test event")
})

So here handler() returns an array. No good as a boolean.

This was easily fixed:

return variables.eventRegistry[event].some((handler) => handler() == false)

Had I done TDD I'd've first done the minimum implementation without the "returning false from a handler terminates the handler loop" feature, then I would have added a test for the feature, and then my attention would have been focused on what I was actually doing, and I'd not have made such a stupid mistake.

Ah well.

After that first iteration, I realised I needed to also be able to pass data to the handler, so I did that too. Via TDD this time. This was the end result:

component {

    variables.eventRegistry = {}

    public void function on(required string event, required function handler, any data) {
        registerEvent(event)
        variables.eventRegistry[event].append({
            handler = handler,
            data = arguments?.data
        })
    }

    public boolean function trigger(required string event) {
        registerEvent(event)
        return variables.eventRegistry[event].some((eventData) => eventData.handler({name=event, data=eventData?.data}) == false)
    }

    private void function registerEvent(required string event) {
        if (!variables.eventRegistry.keyExists(event)) {
            variables.eventRegistry[event] = []
        }
    }
}

I'm pretty pleased with this simple little solution. It does what it needs to do, and is only ~700 bytes of code.

I'll get to why I'm creating this thing in a coupla days, showing how I'll use dependency injection to inject an instance of it into both publisher and subscribers of events, and how I solve that "firing an event every time the application scope is changed" challenge I mentioned in the beginning of this article.

Righto.

--
Adam