Showing posts with label TDD. Show all posts
Showing posts with label TDD. Show all posts

Tuesday, 31 January 2023

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:

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

Sunday, 29 January 2023

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:

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

Sunday, 9 October 2022

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

Saturday, 8 October 2022

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

Wednesday, 25 May 2022

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

Sunday, 15 May 2022

CFML: fixing a coupla bugs in my recent work on TinyTestFramework

G'day:

Last week I did some more work on my TinyTestFramework:

On Saturday, I found a bug in each of those. Same bug, basically, surfacing in two different ways. Here's an example:

describe("Demonstrating afterEach bug", () => {
    afterEach(() => {
        writeOutput("<br><br>This should be displayed<br><br>")
    })

    describe("Control", () => {
        it("is a passing test, to demonstrate expected behaviour", () => {
            expect(true).toBeTrue()
        })        
    })

    describe("Demonstrating bug", () => {
        it("should run even if the test fails", () => {
            expect(true).toBeFalse()
        })
    })
})

Output:

Demonstrating afterEach bug
Control
It is a passing test, to demonstrate expected behaviour:

This should be displayed

OK
Demonstrating bug
It should run even if the test fails: Failed
Results: [Pass: 1] [Fail: 1] [Error: 0] [Total: 2]

Note how the second This should be displayed is not being displayed. Why's this? It's because, internally, a failing test throws an exception:

toBeTrue = () => tinyTest.matchers.toBe(true, actual),

// ...

toBe = (expected, actual) => {
    if (actual.equals(expected)) {
        return true
    }
    throw(type="TinyTest.TestFailedException")
},

And in the implementation of it, an exception is caught before the code handling afterEach has a chance to run:

it = (string label, function implementation) => {
    tinyTest.inDiv(() => {
        try {
            writeOutput("It #label#: ")

            tinyTest.contexts
                .filter((context) => context.keyExists("beforeEachHandler"))
                .each((context) => context.beforeEachHandler())

            decoratedImplementation = tinyTest.contexts
                .filter((context) => context.keyExists("aroundEachHandler"))
                .reduce((reversed, context) => reversed.prepend(context), [])
                .reduce((decorated, context) => () => context.aroundEachHandler(decorated), implementation)
            decoratedImplementation()

            tinyTest.contexts
                .filter((context) => context.keyExists("afterEachHandler"))
                .reduce((reversedContexts, context) => reversedContexts.prepend(context), [])
                .each((context) => context.afterEachHandler())

            tinyTest.handlePass()
        } catch (TinyTest e) {
            tinyTest.handleFail()
        } catch (any e) {
            tinyTest.handleError(e)
        }
    })
},

To explain:

  • implementation is the callback from the it in the test suite. The actual test.
  • All that filter / reduce stuff is just handling aroundEach: don't worry about that.
  • After the decoration we run the test.
  • If it fails, it is caught down here.
  • Meaning the handling of the afterEach callback is never run.

This seems fairly easy to sort out:

try {
    decoratedImplementation()
} finally {
    tinyTest.contexts
        .filter((context) => context.keyExists("afterEachHandler"))
        .reduce((reversedContexts, context) => reversedContexts.prepend(context), [])
        .each((context) => context.afterEachHandler())
}

Now even if the test fails, the afterEachHandler handler will still be run. And as I'm not catching the exception, it'll still do what it was supposed to. Rerunning the tests demonstrates this bug is fixed:

Demonstrating afterEach bug
Control
It is a passing test, to demonstrate excpected behaviour:

This should be displayed

OK
Demonstrating bug
It should run even if the test fails:

This should be displayed

Failed
Results: [Pass: 1] [Fail: 1] [Error: 0] [Total: 2]

I also ran all the rest of the tests as well, and they all still pass, so I'm pretty confident my fix has had no repercussions.


I've got the same problem with aroundEach: the bit of the handler after the call to run the test was not being run, for the same reason we had with afterEach: a failing or erroring test throws an exception, and the exception is caught before the rest of the aroundEach handler can be run. This seems slightly trickier to handle, as the code to call the test is within the callback the tester provides:

aroundEach((test) => {
    // top bit before calling the test. No problem with this

    test()

    // bottom bit. This is not getting run after a failed / erroring test
})

I can't expect the testing dev to stick handling of the test failure in there. I need to do this within the framework.

How to do this flummoxed me a bit, but I wrote some tests in the mean time to give my brain some time to think about things:

describe("Demonstrating aroundEach bug", () => {
    aroundEach((test) => {
        writeOutput("<br><br>Before the call to the test: this should be displayed<br>")
        test()
        writeOutput("<br>After the call to the test:This should be displayed<br><br>")
    })

    describe("Control", () => {
        it("is a passing test, to demonstrate expected behaviour", () => {
            expect(true).toBeTrue()
        })        
    })

    describe("Demonstrating bug", () => {
        it("should display the 'bottom' message even if the test fails", () => {
            expect(true).toBeFalse()
        })
    })
})

Results:

Demonstrating aroundEach bug
Control
It is a passing test, to demonstrate expected behaviour:

Before the call to the test: this should be displayed

After the call to the test: This should be displayed

OK
Demonstrating bug
It should display the 'bottom' message even if the test fails:

Before the call to the test: this should be displayed
Failed
Results: [Pass: 1] [Fail: 1] [Error: 0] [Total: 2]

See how the second test isn't outputting After the call to the test: This should be displayed.

it = (string label, function implementation) => {
    tinyTest.inDiv(() => {
        try {
            writeOutput("It #label#: ")

            tinyTest.contexts
                .filter((context) => context.keyExists("beforeEachHandler"))
                .each((context) => context.beforeEachHandler())

            decoratedImplementation = tinyTest.contexts
                .filter((context) => context.keyExists("aroundEachHandler"))
                .reduce((reversed, context) => reversed.prepend(context), [])
                .reduce((decorated, context) => () => context.aroundEachHandler(decorated), implementation)

        try {    
            decoratedImplementation()
        } finally {
            tinyTest.contexts
                .filter((context) => context.keyExists("afterEachHandler"))
                .reduce((reversedContexts, context) => reversedContexts.prepend(context), [])
                .each((context) => context.afterEachHandler())
        }

            tinyTest.handlePass()
        } catch (TinyTest e) {
            tinyTest.handleFail()
        } catch (any e) {
            tinyTest.handleError(e)
        }
    })
},

The culmination of that deocration code is how any aroundEach handler is called around the test implementation. Somehow I need to do the equivalent of that try / finally here. But it's not so straight forward as I basically need to prevent that call to implementation from erroring until after we bubble out of all the aroundEach handlers. Bear in mind there can be any number of aroundEach handlers to run: one for each level of describe in the tests:

describe("Test of a CFC", () => {

    aroundEach((test) => {
        // something before
        test()
        // something afterwards
    })

    describe("Test of a method", () => {

        aroundEach((test) => {
            // something before
            test()
            // something afterwards
        })

        describe("Test of a specific part of the method's behaviour", () => {

            aroundEach((test) => {
                // something before
                test()
                // something afterwards
            })
            
            it("will have all three of those `aroundEach` handlers run around it",  () => {
                // test stuff
            })
        })
    })
})

OK so I need to put a try / catch around the call to implementation so i can stop it erroring-out too soon. That's easy enough:

decoratedImplementation = tinyTest.contexts
    .filter((context) => context.keyExists("aroundEachHandler"))
    .reduce((reversed, context) => reversed.prepend(context), [])
    .reduce((decorated, context) => () => context.aroundEachHandler(decorated), () => {
        try {
            implementation()
        } catch (any e) {
            // ???
        }                            
    })
    
try {    
    decoratedImplementation()
    // ???
} finally {
    tinyTest.contexts
        .filter((context) => context.keyExists("afterEachHandler"))
        .reduce((reversedContexts, context) => reversedContexts.prepend(context), [])
        .each((context) => context.afterEachHandler())
}

But I still need to know about that exception after we finish calling the aroundEach handlers and the test implementation.

I'm not sure I like this implementation, but this is what I have done:

decoratedImplementation = tinyTest.contexts
    .filter((context) => context.keyExists("aroundEachHandler"))
    .reduce((reversed, context) => reversed.prepend(context), [])
    .reduce((decorated, context) => () => context.aroundEachHandler(decorated), () => {
        try {
            implementation()
            tinyTest.testResult = true
        } catch (any e) {
            tinyTest.testResult = e
        }                            
    })
    
try {    
    decoratedImplementation()
    if (!tinyTest.testResult.equals(true)) {
        throw(object=tinyTest.testResult)
    }
} finally {

I set a variable in the calling code either flagging the test worked, or if not: how it failed (or errored). The if it failed, I throw the exception I originally caught.

This works, and both those tests I wrote above, and all the rest of the test suite still passes too. Bug fixed.

I'm still thinking about this though. I feel I have nailed the red/green part of this process, but I still possibly have some refactoring to do. But obvs now I'm safe to do so because everything is tested. Well: except any other bugs I haven't noticed yet :-)

Righto.

--
Adam

Friday, 13 May 2022

CFML: adding aroundEach to TinyTestFramework was way easier than I expected

G'day:

I'm still pottering around with my TinyTestFramework. Last night I added beforeEach and afterEach handlers, but then thought about how the hell I could easily implement aroundEach support, and I could only see about 50% of it, so I decided to sleep on it.

After a night's sleep I spent about 30min before work doing a quick spike (read: no tests, just "will this even work?"), and surprisingly it did work. First time. Well except for a coupla typos, but I nailed the logic first time. I'm sorta halfway chuffed by this, sorta halfway worried that even though what I decided would probably work - and it did - I haven't quite got my head around how it works, or even quite what it's doing. So let's blog about that.

This evening after work I ignored my spike, and wrote some tests. This is another TDD lesson: it's OK to do a spike in yer code without tests and stuff to just prove a proof of concept, but then once yer good with that, put it to once side and go back to writing tests. My spike code is in a completely different environment from the environment I'm writing the tests in.

For the tests, I consider a lot of the testing of the hierarchical mechanics of how a describe / it testing framework works has already been tested extensively by the beforeEach tests (see "CFML: Adding beforeEach handlers to my TinyTestFramework. Another exercise in TDD"), and I don't need to restest that. This is the same reason I tested the afterEach stuff quite lightly too ("CFML: for the sake of completeness, here's the afterEach treatment"). In those afterEach tests I just tested the "afterness" of the way it works, which is the only difference in the implementation of afterEach compared to beforeEach. The chief difference being that when there is a hierarchy of afterEach handlers, they are called in the reverse order from equivalent beforeEach handlers.

For aroundEach what I am testing is the "aroundness" of how it works.

Here are the tests:

describe("Tests of aroundEach", () => {
    describe("Tests hierarchical sequencing", () => {
        result = []
        aroundEach((test) => {
            result.append("aroundEach top level before test")
            test()
            result.append("aroundEach top level after test")
        })
        describe("Tests hierarchical sequencing (second level: no aroundEach in this one)", () => {
            describe("Tests hierarchical sequencing (third level)", () => {
                aroundEach((test) => {
                    result.append("aroundEach third level before test")
                    test()
                    result.append("aroundEach third level after test")
                })
                describe("Tests hierarchical sequencing (inner level)", () => {
                    aroundEach((test) => {
                        result.append("aroundEach inner before test")
                        test()
                        result.append("aroundEach inner after test")
                    })
                    
                    it("is the baseline test", ()=> {
                        expect(true).toBeTrue()
                    })

                    it("tests the aroundEach handlers are called in the correct order", ()=> {
                        result.append("tests the aroundEach handlers are called in the correct order")

                        expect(result).toBe([
                            "aroundEach top level before test",
                            "aroundEach third level before test",
                            "aroundEach inner before test",
                            "aroundEach inner after test",
                            "aroundEach third level after test",
                            "aroundEach top level after test",
                            "aroundEach top level before test",
                            "aroundEach third level before test",
                            "aroundEach inner before test",
                            "tests the aroundEach handlers are called in the correct order"
                        ])
                    })
                })
            })
        })
    })

    describe("Tests with beforeEach and afterEach", () => {
        result = []

        afterEach(() => {
            result.append("set by afterEach")
        })

        beforeEach(() => {
            result.append("set by beforeEach")
        })

        aroundEach((test) => {
            result.append("set by aroundEach before test")
            test()
            result.append("set by aroundEach after test")
        })

        it("is the baseline test", ()=> {
            expect(true).toBeTrue()
        })

        it("tests the aroundEach handlers are called in the correct order", ()=> {
            result.append("tests the aroundEach handlers are called in the correct order")

            expect(result).toBe([
                "set by beforeEach",
                "set by aroundEach before test",
                "set by aroundEach after test",
                "set by afterEach",
                "set by beforeEach",
                "set by aroundEach before test",
                "tests the aroundEach handlers are called in the correct order"
            ])
        })
    })
})

So we have:

  • Test how a collection of hierarchical aroundEach handlers work. It looks like a lot of code, but it's just three nested describe blocks each with an aroundEach handler, before finally a test to observe, and a test that does the observation and tests some expectations on same.
  • Test that aroundEach plays nice with beforeEach and afterEach. Again a few lines of code, but pretty simple stuff, and the expectations are pretty clear and straight forward.

Um. OK. So.

The implementation bit is going to be a bit of an anti-climax after that lot. Firstly there's some scaffolding stuff that's obviously needed:

beforeEach = (callback) => {
    tinyTest.contexts.last().beforeEachHandler = callback
},
afterEach = (callback) => {
    tinyTest.contexts.last().afterEachHandler = callback
},
aroundEach = (callback) => {
    tinyTest.contexts.last().aroundEachHandler = callback
},
// ...
beforeEach = tinyTest.beforeEach
afterEach = tinyTest.afterEach
aroundEach = tinyTest.aroundEach

And the implementation is changing this:

it = (string label, function implementation) => {
    tinyTest.inDiv(() => {
        try {
            writeOutput("It #label#: ")

            tinyTest.contexts.each((context) => {
                context.keyExists("beforeEachHandler") ? context.beforeEachHandler() : false
            })

            implementation()

            tinyTest.contexts.reduce((reversedContexts, context) => reversedContexts.prepend(context), []).each((context) => {
                context.keyExists("afterEachHandler") ? context.afterEachHandler() : false
            })

            tinyTest.handlePass()
        } catch (TinyTest e) {
            tinyTest.handleFail()
        } catch (any e) {
            tinyTest.handleError(e)
        }
    })
},

To be this:

decoratedImplementation = tinyTest.contexts
    .filter((context) => context.keyExists("aroundEachHandler"))
    .reduce((reversed, context) => reversed.prepend(context), [])
    .reduce((decorated, context) => () => context.aroundEachHandler(decorated), implementation)

decoratedImplementation()

That's not much code, but it's a wee bit dense, even with putting each step on a separate line. What are we doing:

  • Taking the context collection.
  • Filtering out all the context objects with no aroundEachHandler element. We don't care about those.
  • Flipping the remainder around, as we need to apply them from the one nearest the test to the furthest.
  • Then, starting with the test implementation callback, sequentially pass it to the preceding callback in the hierarchy, if that makes sense. It's kind of a partial-application of each aroundEach callback I guess.
  • Once we've done that, call the returned function.

In effect what I think I am doing is calling a function that calls each aroundEachHandler callback from outermost to innermost, passing it the next handler down as its argument, all the way until the test implementation gets called at the end.

Once that was working, I realised that the filtering step could be applied to the beforeEach and afterEach calls too:

tinyTest.contexts.each((context) => {
    context.keyExists("beforeEachHandler") ? context.beforeEachHandler() : false
})

Becomes:

tinyTest.contexts
    .filter((context) => context.keyExists("beforeEachHandler"))
    .each((context) => context.beforeEachHandler())

And equivalently with afterEach:

tinyTest.contexts.reduce((reversedContexts, context) => reversedContexts.prepend(context), []).each((context) => {
    context.keyExists("afterEachHandler") ? context.afterEachHandler() : false
})

Becomes:

tinyTest.contexts
    .filter((context) => context.keyExists("afterEachHandler"))
    .reduce((reversedContexts, context) => reversedContexts.prepend(context), [])
    .each((context) => context.afterEachHandler())

The usage of ?: didn't sit well with me before, and I like this approach. Of course YMMV.

To copy and paste from last night's article: that's it. I'll add these to the source code:

You know what? I'm quite pleased that the aroundEach handling was so easily solved with just four chained method calls. It seems amazing to me for some reason. I don't mean my code is amazing: I mean the technique is.

Righto.

--
Adam