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

Wednesday 24 February 2021

Docker: using TDD to initialise my app's DB with some data

G'day:

I'll start by saying I am not convinced this exercise really ought to be a TDD-oriented one, but I'm gonna approach it that way anyhow because I suspect I'm going to need to mess around a bit to get this working. Secondly, this is very much a log of what I'm (trying to ~) work on today, and I doubt there will be any shrewd insights going on, given I'm basically googling and RTFMing, then doing what the docs say.

The exercise here is to take the MariaDB database that I already have in my Docker set up (see "Creating a web site with Vue.js, Nginx, Symfony on PHP8 & MariaDB running in Docker containers - Part 5: MariaDB"), which is currently empty and only accessible via the root login; and add some baseline tables and data to it. At the same time also create a user for code to connect to the DB with so I don't need code using root access. Another thing I want to do is stop storing the DB passwords in the Docker .env file like I am now:

COMPOSE_PROJECT_NAME=fullStackExercise
DATABASE_ROOT_PASSWORD=123

The data I need is to fulfil an exercise I have given myself (well: it wasn't me who gave me the original exercise, but I'm reinventing it a bit here) to construct an event registration form (personal details, a selection of workshops to register for), save the details to the DB and echo back a success page. Simple stuff. Less so for me given I'm using tooling I'm still only learning (Vue.js, Symfony, Docker, Kahlan, Mocha, MariaDB).

For my tests, I can already derive a bunch of test specs from those first few paragraphs above, so let's put them together now in spec/integration/baselineDatabase.spec.php:

<?php

namespace adamCameron\fullStackExercise\spec\integration;

describe('Tests for registration database', function () {
    describe('Connectivity tests', function () {
        it('can connect to the database with environment-based credentials', function () {
        });
    });

    describe('Schema tests', function () {
        it('has a workshops table with the required schema', function () {
        });

        it('has a registrations table with the required schema', function () {
        });

        it('has a registeredWorkshops table with the required schema', function () {
        });
    });

    describe('Data tests', function () {
        it('has the required baseline workshop data', function () {
        });
    });
});

And I can now run those to see them… not be implemented:

root@fde4be76c908:/usr/share/fullstackExercise# composer spec -- --spec=spec/integration/baselineDatabase.spec.php --reporter=verbose
> vendor/bin/kahlan '--spec=spec/integration/baselineDatabase.spec.php' '--reporter=verbose'


  Tests for registration database
    Connectivity tests
      ✓ it can connect to the database with environment-based credentials
    Schema tests
      ✓ it has a workshops table with the required schema
      ✓ it has a registrations table with the required schema
      ✓ it has a registeredWorkshops table with the required schema
    Data tests
      ✓ it has the required baseline workshop data


  Pending specifications: 5
  .spec/integration/baselineDatabase.spec.php, line 8
  .spec/integration/baselineDatabase.spec.php, line 13
  .spec/integration/baselineDatabase.spec.php, line 16
  .spec/integration/baselineDatabase.spec.php, line 19
  .spec/integration/baselineDatabase.spec.php, line 24


Expectations   : 0 Executed
Specifications : 5 Pending, 0 Excluded, 0 Skipped

Passed 0 of 0 PASS in 0.015 seconds (using 4MB)

root@fde4be76c908:/usr/share/fullstackExercise#

And now I can implement that first test:

describe('Tests for registration database', function () {

    $this->getConnectionDetailsFromEnvironment = function () {
        return (object) [
            'database' => $_ENV['MYSQL_DATABASE'],
            'user' => $_ENV['MYSQL_USER'],
            'password' => $_ENV['MYSQL_PASSWORD']
        ];
    };

    describe('Connectivity tests', function () {
        it('can connect to the database with environment-based credentials', function () {
            $connectionDetails = $this->getConnectionDetailsFromEnvironment();
            $connection = new PDO(
                "mysql:dbname=$connectionDetails->database;host=database.backend",
                $connectionDetails->user,
                $connectionDetails->password
            );
            $statement = $connection->query("SELECT 'OK' AS test FROM dual");
            $statement->execute();

            $testResult = $statement->fetch(PDO::FETCH_ASSOC);

            expect($testResult)->toContainKey('test');
            expect($testResult['test'])->toBe('OK');
        });
    });

There's not much to this. I'm reading the DB connectivity details from the environment variables Docker has set for me, and using those to do a simple DB query from the database, and just verify the DB is responding as expected. To be honest I don't think I need / ought to be using the environment variable for the database name here: that environment variable is just for MariaDB to create a DB of that name when it first starts up. In the app itself, we'll have a static value for the database name, because the app wants to use that exact database, not simply whatever DB is in that environment variable. Hopefully you see the subtle difference in intent there. Anyhow, we now run our tests:

> vendor/bin/kahlan '--spec=spec/integration/baselineDatabase.spec.php' '--reporter=verbose'


  Tests for registration database
    Connectivity tests
      ✖ it can connect to the database with environment-based credentials
        an uncaught exception has been thrown in `spec/integration/baselineDatabase.spec.php` line 11

        message:`Kahlan\PhpErrorException` Code(0) with message "`E_WARNING` Undefined array key \"MYSQL_DATABASE\""

          [NA] - spec/integration/baselineDatabase.spec.php, line 7 to 11
          […etc…]

Cool. Now we can sort those credentials out and watch that test pass. The first thing I have done is to update docker/.env (see above) to get rid of the root password, and add the other credentials MariaDB expects to initialise a database when its container is first built (see the "Environment Variables" section in mariadb - Docker Official Images for info about that):

COMPOSE_PROJECT_NAME=fullStackExercise
MYSQL_DATABASE=fullstackExercise
# the following are to be provided to `docker-compose up`
DATABASE_ROOT_PASSWORD=
MYSQL_USER=
MYSQL_PASSWORD=

Those empty entries are not necessary, I've just left them there for the sake of documentation. The bit I do actually need to do is in docker/docker-compose.yml. This is best shown with a diff I think:

$ git diff docker/docker-compose.yml
diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml
index bc399a0..0eec553 100644
--- a/docker/docker-compose.yml
+++ b/docker/docker-compose.yml
@@ -24,7 +24,9 @@ services:
       context: ../backend
       dockerfile: ../docker/php-fpm/Dockerfile
     environment:
-      - DATABASE_ROOT_PASSWORD=${DATABASE_ROOT_PASSWORD}
+      - MYSQL_DATABASE=${MYSQL_DATABASE}
+      - MYSQL_USER=${MYSQL_USER}
+      - MYSQL_PASSWORD=${MYSQL_PASSWORD}
     volumes:
       - ../backend/config:/usr/share/fullstackExercise/config
       - ../backend/public:/usr/share/fullstackExercise/public
@@ -52,6 +54,9 @@ services:
       context: ./mariadb
     environment:
       - MYSQL_ROOT_PASSWORD=${DATABASE_ROOT_PASSWORD}
+      - MYSQL_DATABASE=${MYSQL_DATABASE}
+      - MYSQL_USER=${MYSQL_USER}
+      - MYSQL_PASSWORD=${MYSQL_PASSWORD}
     ports:
       - "3306:3306"
     volumes:

I've taken out PHP's access to the DB root password as it doesn't need it any more. It has two tests that will now fail, but they were only ever temporary ones until I did this work anyhow, so I'll be deleting those when I verify they now fail. And I've also added in the three new environment variables to both the MariaDB service, and the PHP one. MariaDB uses it to create the fullstackExercise DB, and PHP will use the same credentials to connect to it. I now have no DB credentials anywhere in the codebase. Instead, I pass them in when I first bring the containers up:

adam@DESKTOP-QV1A45U:/mnt/c/src/fullstackExercise/docker$ DATABASE_ROOT_PASSWORD=123 MYSQL_USER=fullstackExercise MYSQL_PASSWORD=1234 docker-compose up --build --detach

This is not completely secure. One can still see the passwords if one terminals into the containers, eg:

adam@DESKTOP-QV1A45U:/mnt/c/src/fullstackExercise/docker$ docker exec --interactive --tty fullstackexercise_php-fpm_1 /bin/bash
root@ac3872091c8e:/usr/share/fullstackExercise# set | grep MYSQL
MYSQL_DATABASE=fullstackExercise
MYSQL_PASSWORD=1234
MYSQL_USER=fullstackExercise
root@ac3872091c8e:/usr/share/fullstackExercise#

A better way would perhaps be to use Docker Secrets, but I could not work out how to get the values from the files it creates into environment variables in the docker-compose.yml file. But will also admit I pretty much read the docs and went "yeah CBA with that right now". It might be dead easy (UPDATE: just now when linking to the MariaDB docker image page a coupla paragraphs up, I noticed it's all actually explained there, and it is dead easy. I might look at doing this later then).

Now I will run my tests again. My expectations are that that test that failed before will now be passing; and one each of the Kahlan and PHPUnit tests will start to fail because they are testing connecting to the DB using the root credentials, which I've removed.

root@ac3872091c8e:/usr/share/fullstackExercise# composer spec
> vendor/bin/kahlan

..........E.PPPP.                                                 17 / 17 (100%)


  Pending specifications: 4
  .spec/integration/baselineDatabase.spec.php, line 37
  .spec/integration/baselineDatabase.spec.php, line 40
  .spec/integration/baselineDatabase.spec.php, line 43
  .spec/integration/baselineDatabase.spec.php, line 48

Tests database availability
  ✖ it should return the expected database version
    an uncaught exception has been thrown in `spec/integration/database.spec.php` line 14

    message:`Kahlan\PhpErrorException` Code(0) with message "`E_WARNING` Undefined array key \"DATABASE_ROOT_PASSWORD\""

      [NA] - spec/integration/database.spec.php, line 11 to 14
      Kahlan\Filter\Filters::run() - vendor/kahlan/kahlan/src/Suite.php, line 236
      […etc…]


Expectations   : 18 Executed
Specifications : 4 Pending, 0 Excluded, 0 Skipped

Passed 12 of 13 FAIL (EXCEPTION: 1) in 0.491 seconds (using 6MB)

Script vendor/bin/kahlan handling the spec event returned with error code 255

This is good: only one failing test: the one we expect to fail, and it's failing for the right reason. And with PHPUnit:

PHPUnit 9.5.2 by Sebastian Bergmann and contributors.

.....E                                                              6 / 6 (100%)

Time: 00:00.268, Memory: 14.00 MB

There was 1 error:

1) adamCameron\fullStackExercise\tests\integration\DatabaseTest::testDatabaseVersion
Undefined array key "DATABASE_ROOT_PASSWORD"

/usr/share/fullstackExercise/tests/integration/DatabaseTest.php:16

ERRORS!
Tests: 6, Assertions: 13, Errors: 1.

Generating code coverage report in HTML format ... done [00:00.374]
Script vendor/bin/phpunit handling the test event returned with error code 2

I'll get rid of those failing tests. They are redundant now.

The next test cases we have to address are these ones:

    Schema tests
      ✓ it has a workshops table with the required schema
      ✓ it has a registrations table with the required schema
      ✓ it has a registeredWorkshops table with the required schema

Looking at the docs for MariaDB's Docker image ("Docker Official Images > mariadb > Initializing a fresh instance"), when the DB starts up, it looks for files in a docker-entrypoint-initdb.d directory, and runs any scripts it finds in there. This makes things easy.

However let's not get ahead of ourselves. We need tests first. But first… a bit of an aside. I'm actually questioning the merits of these tests. They are handy when I'm doing the initial DB setup though. Later as the application develops, we'll have more finely-tuned integration tests that will implicitly test the table schemata are correct; but I guess at the moment all we need to have is the schema (then some baseline data), so as transient tests I suppose the have some merit. I'm not sure. One one hand it might be overkill; on another hand we're supposed to be developing the application iteratively, and these are a first iteration. I guess the situation is similar to the DB tests I had that were using the root connectivity details, because for that iteration that's where we were at. Now we've moved on so those tests are redundant, and these new tests replace them. And these tests will likely be replaced in the next coupla iterations as we go. Anyhow: I'm writing them. Here we go.

describe('Schema tests', function () {
    $schemata = [
        [
            'tableName' => 'workshops',
            'schema' => [
                ['Field' => 'id', 'Type' => 'int(11)'],
                ['Field' => 'name', 'Type' => 'varchar(500)']
            ]
        ],
        [
            'tableName' => 'registrations',
            'schema' => [
                ['Field' => 'id', 'Type' => 'int(11)'],
                ['Field' => 'fullName', 'Type' => 'varchar(100)'],
                ['Field' => 'phoneNumber', 'Type' => 'varchar(50)'],
                ['Field' => 'emailAddress', 'Type' => 'varchar(320)'],
                ['Field' => 'password', 'Type' => 'varchar(255)'],
                ['Field' => 'ipAddress', 'Type' => 'varchar(15)'],
                ['Field' => 'uniqueCode', 'Type' => 'varchar(36)'],
                ['Field' => 'created', 'Type' => 'timestamp']
            ]
        ],
        [
            'tableName' => 'registeredWorkshops',
            'schema' => [
                ['Field' => 'id', 'Type' => 'int(11)'],
                ['Field' => 'registrationId', 'Type' => 'int(11)'],
                ['Field' => 'workshopId', 'Type' => 'int(11)']
            ]
        ]
    ];

    array_walk($schemata, function ($tableSchema) {
        $tableName = $tableSchema['tableName'];
        $expectedSchema = $tableSchema['schema'];

        it("has a $tableName table with the required schema", function () use ($tableName, $expectedSchema) {
            $statement = $this->connection->query("SHOW COLUMNS FROM $tableName");
            $statement->execute();

            $columns = $statement->fetchAll(PDO::FETCH_ASSOC);

            expect($columns)->toHaveLength(count($expectedSchema));
            foreach ($expectedSchema as $i => $column) {
                expect($columns[$i]['Field'])->toBe($expectedSchema[$i]['Field']);
                expect($columns[$i]['Type'])->toBe($expectedSchema[$i]['Type']);
            }
        });
    });
});

There was an intermediary refactoring here: initially I had three "hard-coded" cases, as listed further up. As I wrote the test for the second case I noticed I was duplicating everything from the first test except the table name and the details of the schema, so I extracted those as test data, and looped over them. All the test does here is to get the table columns description, and verify they match the name, type and length of my expectations. The expectations were taken directly from the requirement I had been given to implement.

If I now run the tests, those three cases fail, as we'd expect given the tables don't yet exist:

Tests for registration database
  Schema tests
    ✖ it has a workshops table with the required schema
      an uncaught exception has been thrown in `spec/integration/baselineDatabase.spec.php` line 74

      message:`PDOException` Code(42S02) with message "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'fullstackexercise.workshops' doesn't exist"

        [NA] - spec/integration/baselineDatabase.spec.php, line 73 to 74
        […etc…]

    ✖ it has a registrations table with the required schema
      an uncaught exception has been thrown in `spec/integration/baselineDatabase.spec.php` line 74

      message:`PDOException` Code(42S02) with message "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'fullstackexercise.registrations' doesn't exist"

        [NA] - spec/integration/baselineDatabase.spec.php, line 73 to 74
        […etc…]

    ✖ it has a registeredWorkshops table with the required schema
      an uncaught exception has been thrown in `spec/integration/baselineDatabase.spec.php` line 74

      message:`PDOException` Code(42S02) with message "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'fullstackexercise.registeredWorkshops' doesn't exist"

        [NA] - spec/integration/baselineDatabase.spec.php, line 73 to 74
        […etc…]
[…etc…]

Now to add the tables.I've set up these files:

adam@DESKTOP-QV1A45U:/mnt/c/src/ttct$ tree docker/mariadb/do*
docker/mariadb/docker-entrypoint-initdb.d
├── 1.createAndPopulateWorkshops.sql
├── 2.createRegistrations.sql
└── 3.createRegisteredWorkshops.sql

Note: for now that first file name is slightly misnamed, as it'll only have the DDL statement in it at the moment, and the data-insertion will come in a subsequent step. The file contents are as follows:

/* docker/mariadb/docker-entrypoint-initdb.d/1.createAndPopulateWorkshops.sql */

USE fullstackExercise;

CREATE TABLE workshops (
    id INT NOT NULL AUTO_INCREMENT,
    name VARCHAR(500) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
    
    PRIMARY KEY (id)
) ENGINE=InnoDB;


/* docker/mariadb/docker-entrypoint-initdb.d/2.createRegistrations.sql */

USE fullstackExercise;

CREATE TABLE registrations (
   id INT NOT NULL AUTO_INCREMENT,
   fullName VARCHAR(100) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
   phoneNumber VARCHAR(50) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
   emailAddress VARCHAR(320) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
   password VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
   ipAddress VARCHAR(15) NOT NULL,
   uniqueCode VARCHAR(36) NOT NULL DEFAULT (UUID()),
   created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
   
   PRIMARY KEY (id)
) ENGINE=InnoDB;


/* docker/mariadb/docker-entrypoint-initdb.d/3.createRegisteredWorkshops.sql */

USE fullstackExercise;

CREATE TABLE registeredWorkshops (
   id INT NOT NULL AUTO_INCREMENT,
   registrationId INT NOT NULL,
   workshopId INT NOT NULL,
   PRIMARY KEY (id),
   FOREIGN KEY (registrationId) REFERENCES registrations(id),
   FOREIGN KEY (workshopId) REFERENCES workshops(id)
);

And lastly I need to copy that directory into my MariaDB container when I build it (docker/mariadb/Dockerfile):

FROM mariadb:latest
COPY ./docker-entrypoint-initdb.d/ /docker-entrypoint-initdb.d/
CMD ["mysqld"]
EXPOSE 3306

After I rebuild my containers, I run the tests and we're all good:

    Schema tests
       it has a workshops table with the required schema
       it has a registrations table with the required schema
       it has a registeredWorkshops table with the required schema

Finally I need some seed data in the workshops table. First I'm going to write my test cases for this:

describe('Data tests', function () {
    it('has the required baseline workshop data', function () {
        $expectedWorkshops = [
            ['id' => '2', 'name' => 'TEST_WORKSHOP 1'],
            ['id' => '3', 'name' => 'TEST_WORKSHOP 2'],
            ['id' => '5', 'name' => 'TEST_WORKSHOP 3'],
            ['id' => '7', 'name' => 'TEST_WORKSHOP 4']
        ];

        $statement = $this->connection->query("SELECT id, name FROM workshops ORDER BY id");
        $statement->execute();
        $workshops = $statement->fetchAll(PDO::FETCH_ASSOC);

        expect($workshops)->toEqual($expectedWorkshops);
    });

    it('correctly auto-increments the ID on new insertions', function () {
        $$expectedWorkshopName = 'TEST_WORKSHOP 5';

        $this->connection->beginTransaction();

        $statement = $this->connection->prepare(query: "INSERT INTO workshops (name) VALUES (:name)");
        $statement->execute(['name' => $expectedWorkshopName]);
        $id = $this->connection->lastInsertId();

        $statement = $this->connection->prepare("SELECT id, name FROM workshops WHERE id = :id");
        $statement->execute(['id' => $id]);
        $workshops = $statement->fetchAll(PDO::FETCH_ASSOC);

        expect($workshops)->toHaveLength(1)
        expect($workshops[0])->toContainKey('name')
        expect($workshops[0]['name'])->toBe($expectedWorkshopName)

        $this->connection->rollback();
    });
});

Those are reasonably self-explanatory. I need to insert four baseline workshop records, and in the first case I just SELECT the data and check it's what I expect it to be. The second case only occurred to me when I went to look at the changes to the SQL I needed to make in 1.createAndPopulateWorkshops.sql to insert that data. I needed to take the auto-increment off the table-create statement so I could insert records with the specific IDs I need, then after doing that I altered the table to have the ID auto-increment. I figured I had better test that that worked too. So I insert a new record (just the name, letting the DB handle the ID), get the ID back and use that to get the whole record back for that ID, verifying it's also got the correct name. I do no want that data cluttering my DB so I put the whole thing in a transaction so that it rolls-back when I'm done or if there's an error.

Running those, only the first one errors:

> vendor/bin/kahlan '--spec=spec/integration/baselineDatabase.spec.php'

F.                                                                  2 / 2 (100%)


Tests for registration database
  Data tests
    ✖ it has the required baseline workshop data
      expect->toEqual() failed in `.spec/integration/baselineDatabase.spec.php` line 102

      It expect actual to be equal to expected (==).

      actual:
        (array) []
      expected:
        (array) [
            0 => [
                "id" => "2",
                "name" => "TEST_WORKSHOP 1"
            ],
            […etc…]


Expectations   : 4 Executed
Specifications : 0 Pending, 0 Excluded, 0 Skipped

Passed 1 of 2 FAIL (FAILURE: 1) in 0.025 seconds (using 4MB)

Focus Mode Detected in the following files:
fdescribe - spec/integration/baselineDatabase.spec.php, line 89 to 124
exit(-1)

Script vendor/bin/kahlan handling the spec event returned with error code 255
root@e7d6aa6cf839:/usr/share/fullstackExercise#

This puzzled me at first, but then it occurred to me that the auto-increment test case really ought to have been added when I did the first round of tests before creating the table, because that is when that functionality was added. All I'm doing with the changes I'm about to make is insert some data-insertion code into the script. It's already doing the auto-increment on the ID, and all I'm doing with that is changing when it's being applied: from the table-creation statement to its own statement after the inserts are done. See below for what I mean.

And now I'll now update that docker/mariadb/docker-entrypoint-initdb.d/1.createAndPopulateWorkshops.sql to also insert the baseline data:

USE fullstackExercise;

CREATE TABLE workshops (
    id INT NOT NULL /* AUTO_INCREMENT <- this has been removed from here */,
    name VARCHAR(500) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL,
    
    PRIMARY KEY (id)
) ENGINE=InnoDB;

INSERT INTO workshops (id, name)
VALUES
    (2, 'TEST_WORKSHOP 1'),
    (3, 'TEST_WORKSHOP 2'),
    (5, 'TEST_WORKSHOP 3'),
    (7, 'TEST_WORKSHOP 4')
;

ALTER TABLE workshops MODIFY COLUMN id INT auto_increment;

(Note I've moved the auto-increment on the ID field when I create the table now, so the seed data can have specific IDs. Once I do the insert, then I make the column auto-increment).

Once I rebuild my containers, all the tests now pass:

> vendor/bin/kahlan '--spec=spec/integration/baselineDatabase.spec.php' '--reporter=verbose'


  Tests for registration database
    Connectivity tests
       it can connect to the database with environment-based credentials
    Schema tests
       it has a workshops table with the required schema
       it has a registrations table with the required schema
       it has a registeredWorkshops table with the required schema
    Data tests
       it has the required baseline workshop data
       it correctly auto-increments the ID on new insertions



Expectations   : 35 Executed
Specifications : 0 Pending, 0 Excluded, 0 Skipped

Passed 6 of 6 PASS in 0.033 seconds (using 5MB)

root@44850303b17a:/usr/share/fullstackExercise#

And I think that's about it. I'm not doing anything with the data yet, but that'll start to be fleshed out in the next article (or maybe the following one. Not sure). This was just an exercise in doing some stuff with Docker and MariaDB, and thinking about the merits of TDDing exercises like this. I think it was worth it, especially during this early phase of working with these containers as I'm still reconfiguring stuff a lot, so it's good to know things don't get messed up when I'm monkeying with stuff.

Righto.

--
Adam

Thursday 21 January 2021

Listening to the console log of a page loaded with Puppeteer

G'day:

This is a follow-up from something I touched on yesterday ("Polishing my Vue / Puppeteer / Mocha / Chai testing some more"). In that exercise I was using Puppeteer to load a web page I was testing, and then pulling some DOM element values out and checking they matched expectations. The relevant bits of code are thus:

describe.only("Tests of githubProfiles page using github data", function () {
    let browser;
    let page;
    let expectedUserData;

    before("Load the page and test data", async function () {
        await loadTestPage();
        expectedUserData = await loadTestUserFromGithub();
    });

    let loadTestPage = async function () {
        browser = await puppeteer.launch( {args: ["--no-sandbox"]});
        page = await browser.newPage();

        await Promise.all([
            page.goto("http://webserver.backend/githubProfiles.html"),
            page.waitForNavigation()
        ]);
    }

    it("should have the expected person's name", async function () {
        let name = await page.$eval("#app>.card>.content>a.header", headerElement => headerElement.innerText);
        name.should.equal(expectedUserData.name);
    });

  • Load the page with Puppeteer
  • Example test checking the page's DOM

This code seemed to be running fine, and the tests were passing. As I was adding more code ot my Vue component on the client end, I suddenly found the tests started to fail. Sometimes. If I ran them ten times, they'd fail maybe three times. At the same time, if I was just hitting the page in the browser, it was working 100% of the time. Odd. I mean clearly I was doing something wrong, and I'm new to all this async code I'm using, so figured I was using values before they were available or something. But it seemed odd that this was only manifesting sometimes. The way the tests were failing was telling though:

1) Tests of githubProfiles page using github data
       should have the expected person's name:

      AssertionError: expected '' to equal 'Alex Kyriakidis'
      + expected - actual

      +Alex Kyriakidis

The values coming from the DOM were blank. And note that it's not a case of the DOM being wrong, because if that was the case, the tests would barf all the time, with something like this:

Error: Error: failed to find element matching selector "#app>.card>.content>a.header"

The relevant mark-up here is:

<a class="header" :href="pageUrl">{{name}}</a>

So {{name}} isn't getting its value sometimes.

I faffed around for a bit reading up on Vue components, and their lifecycle handlers in case created was not the right place to load the data or something like that, but the code seemed legit.

My JS debugging is not very sophisticated, and it's basically a matter of console.logging stuff and see what happens. I chucked a bunch of log calls in to see what happens:

created () {
    console.debug(`before get call [${this.username}]`);
    axios.get(
        `https://api.github.com/users/${this.username}`,
        {
            auth: {
                username: this.$route.query.GITHUB_PERSONAL_ACCESS_TOKEN
            }
        }
    )
    .then(response => {
        console.debug(`beginning of then [${response.data.name}]`);
        this.name = response.data.name;
		// [etc...]
        console.debug("end of then");
    });
    console.debug("after get call");
}

Along with some other ones around the place, these all did what I expected when I hit the page in the browser:

beginning of js
githubProfiles.js:46 before VueRouter
githubProfiles.js:52 before Vue
githubProfiles.js:23 before get call [hootlex]
githubProfiles.js:43 after get call
githubProfiles.js:63 end of js
githubProfiles.js:33 beginning of then [Alex Kyriakidis]
githubProfiles.js:41 end of then

I noted that the then call was being fulfilled after the mainline code had finished, but in my test I was waiting for the page to fully load, so I'd catered for this. Repeated from above:

await Promise.all([
    page.goto("http://webserver.backend/githubProfiles.html"),
    page.waitForNavigation()
]);

I ran my tests, and was not seeing anything in the console which momentarily bemused me. But then I was just "errr… duh, Cameron. That stuff is logging in the web page's console. Not Node's console from the test run". I'm really thick sometimes.

This flumoxed me for a bit as I wondered how the hell I was going to get telemetry out of the page that I was calling in the Puppeteer headless browser. Then it occurred to me that I would not be the first person to wonder this, so just RTFMed.

It's really easy! The Puppeteer Page object exposes event listeners one can hook into, and one of the events is console. Perfect. All I needed to do is put this into my test code:

page = await browser.newPage();

page.on("console", (log) => console.debug(`Log from client: [${log.text()}] `));

await Promise.all([
    page.goto("http://webserver.backend/githubProfiles.html"),
    page.waitForNavigation()
]);

Then when I ran my tests, I was console-logging the log entries made in the headless browser as they occurred. What I was seeing is:

  Tests of githubProfiles page using github data
Log from client: [beginning of js]
Log from client: [before VueRouter]
Log from client: [before Vue]
Log from client: [before get call [hootlex]]
Log from client: [after get call]
Log from client: [end of js]
    1) should have the expected person's name
    2) should have the expected person's github page URL
    3) should have the expected person's avatar
    4) should have the expected person's joining year
    5) should have the expected person's description
Log from client: [beginning of xxxxxx then [Alex Kyriakidis]]
Log from client: [end of then]
    6) should have the expected person's number of friends
     should have the expected person's friends URL


  1 passing (4s)
  6 failing

Note how the tests get underway before the then call takes place. And shortly after that, the tests start passing because by then the dynamic values have actually been loaded into the DOM. This is my problem! that page.waitForNavigation() is not waiting long enough! My first reaction was to blame Puppeteer, but I quickly realised that's daft and defensive of me, given this is the first time I've messed with this stuff, almost certainly I'm doing something wrong. Then it occurred to me that a page is navigable once the various asset files are loaded, but not necessarily when any code in them has run. Duh. I figured Puppeteer would have thought of this, so there'd be something else I could make it wait for. I googled around and found the docs for page.waitForNavigation, and indeed I needed to be doing this:

page.waitForNavigation({waitUntil: "networkidle0"})

After I did that, I found the tests still failing sometimes, but now due to a time out:

  Tests of githubProfiles page using github data
Log from client: [beginning of js]
Log from client: [before VueRouter]
Log from client: [before Vue]
Log from client: [before get call [hootlex]]
Log from client: [after get call]
Log from client: [end of js]
Log from client: [beginning of then [Alex Kyriakidis]]
Log from client: [end of then]
    1) "before all" hook: Load the page for "should have the expected person's name"


  0 passing (4s)
  1 failing

  1) Tests of githubProfiles page using github data
       "before all" hook: Load the page for "should have the expected person's name":
     Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/usr/share/fullstackExercise/tests/functional/public/GithubProfilesTest.js)

I had the time out set for five seconds, but now the tests are waiting for the client to finish its async call as well, I was just edging over that five second mark sometimes. So I just bumped it to 10 seconds, and thenceforth the tests all passed all the time. I've left the telemetry in for one last successful run here:

  Tests of githubProfiles page using github data
Log from client: [beginning of js]
Log from client: [before VueRouter]
Log from client: [before Vue]
Log from client: [before get call [hootlex]]
Log from client: [after get call]
Log from client: [end of js]
Log from client: [beginning of then [Alex Kyriakidis]]
Log from client: [end of then]
     should have the expected person's name
     should have the expected person's github page URL
     should have the expected person's avatar
     should have the expected person's joining year
     should have the expected person's description
     should have the expected person's number of friends
     should have the expected person's friends URL


  7 passing (5s)

OK so that was a bit of a newbie exercise, but I'm a noob so yer gonna get that. It was actually pretty fun working through it though. I'm really liking all this tooling I'm checking out ATM, so yer likely get a few more of these basic articles from me.

Righto.

--
Adam

Saturday 9 November 2019

I need to backtrack on some TDD guidance. I think

G'day:
Yes, this thing still exists.

Possibly foolishly my employer has made me tech lead of one of our software applications, and part of that is to lead from the front when it comes to maintaining code quality, including our TDD efforts. So I tend to pester the other devs about TDD techniques and the like. O how they love that. Some of them. Occasionally. Actually seldom. Poor bastards.

Anyway, recently I started to think about the difference between "fixing tests that would break if we made this new change to the code" and "writing a TDD-oriented test case before making the code change". Just to back up a bit... this is in the context of "write a failing test for the case being tested... then writing the code to pass the test". The red and green from the "red green refactor" trope. This started from doing code review and seeing existing tests being update to accommodate new code changes, but not see any actual new test case explicitly testing the new requirement. This did not sit well with me, so I've been asking the devs "where's yer actual test case for the change we're making here?"

To help with this notion I wrote a quick article on WTF I was on about. I was initially pleased with it and thought I'd nailed it, but when I asked my more-knowledgeable colleague Brian Sadler to sanity check it, he wasn't convinced I had nailed it, and moreover wasn't sure I was even right. Doh. I've been mulling over this for the rest of the week and I've come to the conclusion... yeah, he's right and I'm wrong. I think. I'm writing this article to put my thinking down in public, and see what you lot think. If anyone still reads this I mean. I will be chasing some ppl to read it, that said...

I can't just reproduce the original article cos it was an internal document and work owns it. So I'm reproducing a similar thing here.

Here's some sample code. I stress the code used here bears no relation to any code or situation we have in our codebase. It's just a simplified case to contextualise the discussion:

class ThingRepository {

    public function getThing($id) {
        $dao = DaoFactory::getThingDao();
        $dbData = $dao->getThing($id);

        return Thing::createFromArray($dbData);
    }
}

It gets raw data for the requested thing from a DAO and returns the data as a Thing object.

It also has an existing test:


class ThingRepositoryTest extends TestCase {

    private $repository;

    protected function setUp() : void {
        $this->repository = new ThingRepository();
    }

    public function testGetThingReturnsCorrectThingForId(){
        $testId = 17;
        $expectedName = "Thing 1";
        $thing = $this->repository->getThing(17);

        $this->assertInstanceOf(Thing::class, $thing);
        $this->assertSame($testId, $thing->id);
        $this->assertSame($expectedName, $thing->name);
    }
}

And this all works fine:

C:\src\php\general>vendor\bin\phpunit test\tdd\fixingExistingTests\ThingRepositoryTest.php
PHPUnit 8.4.3 by Sebastian Bergmann and contributors.

.                                                                   1 / 1 (100%)

Time: 54 ms, Memory: 4.00 MB

OK (1 test, 3 assertions)

C:\src\php\general>

If you were paying attention, you'll've spotted we're tightly coupling the DAO implementation here to its repository. At least we're using a factory, but still it is tightly coupled to the Repository implementation. We want to use dependency injection to provide the DAO to the repository.

Here's where my stance gets contentious.

Someone has addressed this requirement, and what they've done is updated that existing test to expect the passed-in DAO:

class ThingRepositoryExistingTestUpdatedTest extends TestCase {

    private $repository;
    private $dao;

    protected function setUp() : void {
        $this->dao = $this->createMock(ThingDao::class);
        $this->repository = new ThingRepositoryWithDaoDependency($this->dao);
    }

    public function testGetThingReturnsCorrectThingForId(){
        // implementation that now uses mocked DAO
    }
}

Note that this is exactly how I presented this information in the original work article. With the test implementation changes elided. This becomes relevant later.

Having prepped that test - and if fails nicely, cool - the dev then goes and sorts out the repo class.

I went on to reject that approach as not being valid TDD. My position is that "fixing other tests so they won't fail after we make the change" is not TDD, it's just technical debt recovery. If we're doing TDD, then we need to start with an explicit test case for the change we're going to make. I'll put this in a highlight box to draw attention to it:

Fixing other tests so they won't fail after we make the change is not TDD, it's just technical debt recovery. If we're doing TDD, then we need to start with an explicit test case for the change we're going to make.
This is what I'd expect to see:

class ThingRepositoryTest extends TestCase {

    private $repository;
    private $dao;

    protected function setUp() : void {
        $this->dao = $this->createMock(ThingDao::class);
        $this->repository = new ThingRepositoryWithDaoDependency($this->dao);
    }

    public function testGetThingUsesDaoPassedIntoConstructor(){
        $testId = 17;
        $mockedThingData = ['id'=>$testId, 'thing' => 'stuff'];

        $this->dao
            ->expects($this->once())
                        ->method('getThing')
                        ->with($testId)
                        ->willReturn($mockedThingData);

        $result = $this->repository->getThing($testId);

        $this->assertEquals(
            new Thing($mockedThingData['id'], $mockedThingData['thing']),
            $result
        )
    }
    
    // ... rest of test cases that were already there, including this one...

    public function testGetThingReturnsCorrectThingForID(){
        // updated implementation that doesn't break because of the DAO change
    }
}

Here we have an explicit test case for the change we're making. This is TDD. We're explicitly injecting a DAO into the test repo, and we're checking that DAO is being used to get the data.

This test is red:

C:\src\php\general>vendor\bin\phpunit test\tdd\fixingExistingTests\ThingRepositoryTest.php
PHPUnit 8.4.3 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 61 ms, Memory: 4.00 MB

There was 1 failure:

1) me\adamcameron\general\test\tdd\fixingExistingTests\ThingRepositoryTest::testGetThingUsesDaoPassedIntoConstructor
Expectation failed for method name is "getThing" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

FAILURES!
Tests: 1, Assertions: 4, Failures: 1.

C:\src\php\general>

But that means we're now good to make the changes to the repository:

class ThingRepository {

    private $dao;

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

    public function getThing($id){
        $dbData = $this->dao->getThing($id);

        return Thing::createFromArray($dbData);
    }
}

Now our test is green, and we're done.

I rounded out the article with this:

As a rule of thumb... if you don't start your TDD work by typing in a case name - eg testGetThingUsesDaoPassedIntoConstructor - then you're not doing TDD, you are just doing maintenance code. These are... not the same thing.

Brian read the article and - my words not his - didn't get the distinction I was trying to make. Why was the first test update not sufficient? I could not articulate myself any differently than repeat what I'd said - no, not helpful - so we decided to get together later and look at how the implementation of the test I had elided would pan out, and how that wouldn't itself be a decent test. Due to work being busy this week we never had that confab, but I continued to think about it, and this morning concluded that Brian was right, and I was mistaken.

It comes down to how I omitted the implementation of that first test we updated. I did not do this on purpose, I just didn't want to show an implementation that was not helpful to the article. But in doing that, I never thought about what the implementation would be, and set-up a straw man to support my argument. I went back this morning and implemented it...

public function testGetThingReturnsCorrectThingForId(){
    $testId = 17;
    $expectedName = "Thing 1";

    $this->dao->expects($this->once())
        ->method('getThing')
        ->with($testId)
        ->willReturn(['id' => $testId, 'name' => $expectedName]);

    $thing = $this->repository->getThing(17);

    $this->assertInstanceOf(Thing::class, $thing);
    $this->assertSame($testId, $thing->id);
    $this->assertSame($expectedName, $thing->name);
}


This is what I'd need to do to that first test to get it to not break when we change the DAO to being injected. And... erm... it's the same frickin' logic as in the test I said we need to explicitly make. I mean I wrote the two versions at different times so solved the same problem slightly differently, but it's the same thing for all intents and purposes.

So... let's revisit my original position then:

Fixing other tests so they won't fail after we make the change is not TDD, it's just technical debt recovery. If we're doing TDD, then we need to start with an explicit test case for the change we're going to make.

It's not intrinsically true. Actually fixing existing tests can indeed be precisely the TDD-oriented cover we need.

I love being wrong. No sarcsm there: I really do. Now I'm thinking better about this topic. Win.

The only thing left after the distillation is the difference between these two:

Original test name (detailing its original test case requirement):

testGetThingReturnsCorrectThingForId

And my suggested case name for the new injected-DAO requirement:

testGetThingUsesDaoPassedIntoConstructor

Now I do still think there's merit in being that explicit about what we're testing. But it's well into the territory of "perfect is the enemy of good". We're fine just updating that original test.

I thought of how to deal with this, and came up with this lunacy:

public function testGetThingUsesDaoPassedIntoConstructor()
{
    $this->testGetThingReturnsCorrectThingForId();
}

But I think that's too pedantic even for me. I think we can generally just take it as given that the existing test covers us.



I'll close by saying I do think this is good advice when starting to do the testing for a code change to first come up with the test case name. It gets one into the correct mindset for approaching things in a TDD manner. But one oughtn't be as dogmatic about this as I clearly tend to be... existing tests quite possible could provide the TDD-oriented cover we need for code changes. We do still need to start with a breaking test, and the test does actually need to cover the case for the change... but it might be an existing test.

Oh and making the code change then fixing the tests that break as a result is not TDD. That's still wrong ;-)

I'm dead keen to hear what anyone might have to say about all this...

Righto.

--
Adam

Saturday 19 August 2017

Mishmash: code review as a learning exercise, loops vs higher-order-functions and testing

G'day:
There's a few things going on in my head here, and none individually are worthy of an article, but perhaps if I dwell on a bunch of stuff something might shake out. I have no idea as I have not drafted this one in my head like I usually do, I'm just "let's start typing and see if I get anywhere". Eek.

OK so yesterday I was code reviewing one of my mates' work, and breezed past this code (this is not the exact code, but it's a replication of it):

public function getSomeThings($rowsToGet)
{
    $thingsValues = $this->dao->getThingsFromStorage($rowsToGet);
    
    $things = [];
    foreach ($thingsValues as $thingValues) {
        $things[] = $this->thingFactory->createFromRawData($thingValues);
    }

    return $things;
}

I just put a note in the pull request "array_map?".

Now I do a few things in a code review. Firstly I check for obvious flaws, unclean code, bad test coverage, typos, etc, and I will raise those as "tasks" in BitBucket: stuff that needs to be addressed before I'll press the "Approved" button. This is our standard approach to code review, everyone expects it, and it's fine. It works.

But secondly I'll just make observations about the code, ask questions, suggest alternative approaches we could have taken, etc. Just as food for thought, a discussion point, or a chance for some learning. This was the latter. I was not suggesting to my mate they needed to change their code to match my whim... the code is fine as it is. It was just "did you think of this?".

An interesting conversation ensued, which I will paraphrase here. Note that Fred and I do not work in the same physical office.

Fred (his name's not Fred): I'm not sure the benefit of swapping simple code for a more complex solution, unless there's some performance gain I'm unaware of.

Adam (my name is Adam): Hmmm... well array_map is one expression, and it reduces the complexity here to a single variable assignment statement(*). The loop version requires building the array by hand, and requires two assignments and the whole loop thing too. So I think the array_map approach is less complex innit?

return array_map(function ($thingValues) {
    return $this->thingFactory->createFromRawData($thingValues);
}, $thingsValues);

(*) OK it's two statements. Never let it be said I don't hyperbolise to better make my point.

One thing I did not get around to saying is that performance is not a consideration here. One of the calls is hitting an external DB, so any processing time differences are going to be eclipsed - by orders of magnitude - by the external call. And, yes, I know that if we'd be inclined to worry about micro-optimisations, then calling a higher-order function is slower than a loop. One should simply not care.

Adam (cont'ed): further more the array_map version only requires a single test, whereas the loop version needs three. That in itself demonstrates the increased complexity.

Fred: err... why's that then?

I'll break away from the conversation here.

The dev process here is that we identify the need to have a function to get stuff from the DB, and then model that and return it to the calling code. So doing TDD we write a test which uses a mocked-out DAO to return known data, and we test that the values in the resultant models are what we expected them to be based on the mocked DAO data.

Here's a very simplified, self-contained example:

First our test:

describe('comparing testing for foreach vs array_map', function() {
    given('sut', function () {
        return new ForeachVsArrayMap();
    });
    given('allObjects', function () {
        return [
            (object) ['id' => 1, 'mi' => 'tahi'],
            (object) ['id' => 2, 'mi' => 'rua'],
            (object) ['id' => 3, 'mi' => 'toru'],
        ];
    });
    describe('tests of foreach', function() {
        it('returns the numbers as objects', function () {
            $result = $this->sut->getObjects();
            expect($result)->toEqual($this->allObjects);
        });
    });
});

And now our implementation:

class ForeachVsArrayMap
{
    private $allData;

    public function __construct()
    {
        $this->allData = [
            ['id' => 1, 'mi' => 'tahi'],
            ['id' => 2, 'mi' => 'rua'],
            ['id' => 3, 'mi' => 'toru']
        ];
    }

    private function getNumbers($rows)
    {
        return array_slice($this->allData, 0, $rows);
    }

    public function getObjects($rows = 3)
    {
        $data = $this->getNumbers($rows);
        $numbers = [];
        foreach ($data as $row) {
            $numbers[] = (object) $row;
        }

        return $numbers;
    }
}

And that passes. That's our TDD side of things done.

However we also now have to do edge testing as well. We've got a loop in there, and all loops need tests for three things: zero iterations, one iteration, and more than one iterations. There are easily introduced logic errors around the plurality of loop iteration.

Here our TDD test has coverd the "more than one iterations" case, so we only need the other two:

it('handles one result', function () {
    $result = $this->sut->getObjectsUsingForeach(1);
    $expected = [$this->allObjects[0]];

    expect($result)->toEqual($expected);
});

it('handles zero results', function () {
    $result = $this->sut->getObjectsUsingForeach(0);
    $expected = [];

    expect($result)->toEqual($expected);
});

Pretty simple tests, but still: two additional tests.

Why don't we need these two tests when using array_map? Because array_map's iteration is all handled within its implementation. It's PHP's job to test that, not ours. So all we need to test is that the inline callback works. And we only need to test that once: when it's called, it does what we expect it to do. We can safely stipulate that array_map will always return an array with the same number of elements as the input value. PHP deals with that; we don't need to.

One good thing about TDD (and test coverage in general) is now we can safely swap in the array_map version, and the test coverage still passes, so we know - with a high degree of certainty - that our code is A-OK.

public function getObjects($rows = 3)
{
    return array_map(function ($row) {
        return (object) $row;
    }, $this->getNumbers($rows));
}

Coincidentally I had another conversation, in another code review, on Friday about the necessity of always testing loops for 0, 1, >1 iterations. "One can just look at the code and know it's fine". This statement didn't come up in the conversation, but I've heard it before.

Three weeks ago I was show-stopped by a bug in one of our libraries. Taking the analogy above, this was the code:

public function getObjectsUsingForeach($rows = 3)
{
    $data = $this->getNumbers($rows);

    foreach ($data as $row) {
        $numbers[] = (object) $row;
    }

    // do something else with $numbers here
    // ...
}

The code had no tests. Can you see what's wrong? The general expectation when calling a function to get data is that there's actually data to get, given the provided criteria. In the case I fell foul of, the value of $rows was zero. And this was legit. So if $rows is 0, what value does $numbers have after the loop? Well: $numbers was undefined.

Had the dev done their testing properly, then they'd've noticed their bug. They never initialised $numbers outside of the code in the loop, so there's a chance it might never get defined. In this case not only did the dev not do the edge-case testing, they didn't even do the baseline TDD testing either, but that's a different story.

I needed to down-tools and fix the library before I could continue my own work. And it was made very tricky by the fact the code in the library had no tests at all, and wasn't written in a clean-code fashion (so was basically untestable). But we are not allowed to work that way any more, so I needed to do a bunch of work the original developer ought to have done.

I have also fell foul of this brainfart too:

public function getObjectsUsingForeach($rows = 3)
{
    $data = $this->getNumbers($rows);

    foreach ($data as $row) {
        $numbers = [];
        $numbers[] = (object) $row;
    }

    // do something else with $numbers here
    // ...
}

This usually crops up when code that was previously used once suddenly needs to be used for multiple iterations. Note that the initialisation of $numbers is inisde the loop, so it will re-initialise every iteration. This would be caught by the "more than one iterations" test. This is rarer, and usually only crops up in completely untested code: it'd be a shit tester who tested a loop with only one iteration as the test, but I've seen it done. Saves time not having to mock so much data, you see? Berks.

So, anyhow... always test yer loops. And try to avoid loops if there are built-in functions that handle the iteration for you.

Also always do TDD, but also understand that there is more testing to do after the TDD cycle. TDD is not the end of the story there.



In the title of this article I mention "code review as a learning exercise". I greatly summarised the back and forth between Fred and myself here... there was an order of magnitude more discussion than there was code under discussion. this sort of interaction is gold. Some people get the idea that code review is kinda like a teacher marking homework. It's partly that, but that's only part. As programmers and coders our job is one of communication: communication of instructions to a computer, both to the computer itself, but also to our future colleagues and selves about the nature of the instructions. Programming languages are a means to communicate instructions to other humans. It's tangential the computer can also be made to understand them. Similarly reviewing each other's code should be a communications and collaboration process, and we absolutely should be having these discussions. Both Fred and I benefited from this conversation: a lot of what I typed here wasn't formalised in my brain before explaning it, but now I have a much clearly picture of what I do and why. And Fred knows too. This was not a waste of time, as we're both now better developers for it. And it might have seemed like a lot of back and forth in the code review, but it really only represents about 10min of discussion. And all this came from just a conversation point, not me saying "change the code".

If we were in the same office, we might have had the conversation via voice instead of typing in the code review, and some people might think that's better. In a way it is, but it also means that the details of the conversation don't get recorded, so benefit fewer people. That said, when programmers are not agreeing on things, often it is better to switch to voice instead of continue the discussion via text-based back-and-forth. It's just easier and more collaborative sometimes.

Right, that's that. A bit random, but didn't turn out so bad I think. And I seem to have found a point to make.

Righto.

--
Adam

Monday 31 July 2017

Yeah, you do want 100% test coverage

G'day:
I've read a number of blog articles recently that have been eshewing 100% test coverage. The rationale being that there's little difference between 90% and 100%, and that that last 10% can be quite tricky to attain as - we being human 'n' all - sometimes leave the worst to last. Obviously we should all be doing TDD so that situation would not arise, but - again - we're human so don't always do TDD for one reason or another ("can't be arsed" is the usual one).

Another rationale is that it's not a race to 100%. IE: the object of the exercise is delivering working software which adds business value; it's not "getting 100% test coverage".

The latter is true. But we still aim for 100% test coverage for basically one good reason: visibility.

First, look at this:



Not bad I guess. Not great.

Now look at this:



We've accidentally forgotten to cover something new. Can you see it (it's one of the provider methods).

On the other hand look at these two:



Somewhat easier to spot, yeah?

This is bloody handy. In this case I just commented-out one of my tests to get the report off 100%, but in the real world I'd see that and go "huh, WTF?", and perhaps discover I had a logic branch untested, or a test I thought was covering something wasn't actually doing so. Or I just forgot to put the @covers annotation on the test. Whatever the situation: it's handy for the shortfall to be so obvious.

What it also means is part of our pull-request process is running a Bamboo build which runs all our tests (plus some other code quality tools):

 Everything's green here, so it's OK for code review. If it was failing, I'd get a nudge from my reviewers going "erm... you sure this is ready to go?" And even if I thought it was, we can't merge anything whilst the build is failing anyhow. The issue needs to be addressed.

Now the key thing here is this:


We don't test everything, but we cover everything. Where "cover" might be "we don't need to test this". In this case a single variable assignment does not need testing. We trust PHP to follow simple instructions. The thing is: this still shows up as green in the test coverage report.

We do this in a few situations:

  • the method has not branching logic and is overwise pretty simple. Ignore it.
  • we're backfilling coverage on old code we need to maintain, but it's such a mess we simply don't have time to solve all the testing shortfalls right now, so we test what is urgent (the bit we're changing), but just mark as ignore the rest of it. Now that file is green, and we can see if it stops being green.
  • similar to the above but we know the code the code in question is  not long for this world. So we ignore it and stick a TODO cross referencing to the ticket that will be dealing with it. And the ticket must already be lined-up to be worked on "soon".

There might be other situations where we need to be pragmatic and skip some stuff, but this needs to be explained to the code reviewers, and get their agreement.

But being able to run a report that says "100%", and shows up anything that isn't 100% is gold.

Get yer coverage report green, but don't be afraid to be pragmatic and skip stuff, if there's an informed reason to do so.

Now this isn't to say one can just pepper ignore annotations around the place just to get the report green. That's daft. The object of the exercise is not to make some report green / say 100%. It's to get good code quality. This is just a tool to help identify the quality: it's still up to the devs to provide the quality. Don't forget that. It needs to be an informed, honest decision for one to ignore code.

Righto.

--
Adam

Tuesday 7 February 2017

TDD: still testing too much

G'day:
A while back I wrote this article - Maintaining untested code, and mocking too much when backfilling test (with JavaScript & Jasmine) - wherein my colleague pulled me up for testing too much when I'm doing my TDD. I'm working with PHP this time, and I've just been pulled up on it again. For much the same reason. It created an interesting thought exercise... well interetsing ish... so I'll write it up here.

Firstly, I cannot use the actual code I am working with due to the usual over-baked "commercial sensitivity" considerations, so this is simply an analogy, but it captures the gist of what I was doing.

We have an adapter that handles HTTP calls. It's basically a generic wrapper that masks the API differences between Guzzle 5 and Guzzle 6, both of which we are still using, but we want a unified adapter to work with both. It's very generic, and basically has methods for GET, POST, PUT etc, and doesn't encompass any of our business logic.

Business reality is that we use this mostly to transmit JSONified objects, so we're adding a layer in front of it to take the raw text responses and rehydrate them. I'm writing the GET method.

When I start designing my solution, I know that the happy path will return me a Response object that has JSON as its content. I just need to deserialise the JSON and return it. This is easy enough:

public function testGetAsObject() {
    $expectedResult = ['result' => 'valid JSON'];
    $expectedContent = json_encode($expectedResult);

    $adapter = $this->mockAdapterWithResponse(200, $expectedContent);

    $client = new HttpClient($adapter);
    $result = $client->getWithJson($this->uri);

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

mockAdapterWithResponse just does this:

private function mockAdapterWithResponse($statusCode, $content){
    $response = $this->getMockBuilder(Response::class)
        ->setMethods(['getStatusCode', 'getContent'])
        ->getMock();
        
    $response->expects($this->once())
        ->method('getStatusCode')
        ->willReturn(statusCode);

    $response->expects($this->once())
        ->method('getContent')
        ->willReturn(content);

    $adapter = $this->getMockBuilder(Adapter::class)
        ->setMethods(['get'])
        ->getMock();
        
    $adapter->expects($this->once())
        ->method('get')
        ->willReturn($this->response);
        
    return $adapter;
}

(there are other tests that already needed this). All it's doing is enabling us to set what the content for the call to the HttpClient will return, because we don't care how the client goes about doing this; we just care what we do with the content afterwards.

So back to my test code:

public function testGetAsObject() {
    $expectedResult = ['result' => 'valid JSON'];
    $expectedContent = json_encode($expectedResult);

    $adapter = $this->mockAdapterWithResponse(200, $expectedContent);

    $client = new HttpClient($adapter);
    $result = $client->getWithJson($this->uri);

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

All I'm doing here is mocking the adapter to return a predetermined result, and I then test that what comes back from my function is the deserialisation of that value. That's it. I then implement some code to make that test pass:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);

    return $object;
}

And that's green: all good.

I've already done too much in my test, but have not noticed (and not put it in for code review, so no-one's pointed it out).

The next requirement for this method is that if the content coming back from the adapter isn't JSON, then just throw an exception instead of trying to deserialise it. Again, pretty easy to TDD that:

/** @expectedException \Exception */
public function testGetAsObjectThrowsExceptionOnNonJsonContent() {
    $content = 'NOT_VALID_JSON';

    $adapter = $this->mockAdapterWithResponse(200, $content);

    $client = new HttpClient($adapter);
    $client->getWithJson($this->uri);
}

Note there's not even any explicit assertions in there: the @expectedException annotation takes care of it.

This test fails at present: no exception is thrown thanks to PHP's stupid-arse behaviour if json_decode is given non-JSON: it just returns null. Bravo, PHP. But that's what we want... we've designed our code and have tests to prove the design, now we need to implement the code:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);
    
    if (is_null($object)) {
        throw new \Exception('Unable to decode JSON result');
    }
    return $object;
}

The tests are green, so all good.

But what have I done wrong? Nothing major, but once again I'm testing too much. Actually my tests are fine. It's my helper method that's "wrong":

private function mockAdapterWithResponse($statusCode, $content){
    $response = $this->getMockBuilder(Response::class)
        ->setMethods(['getStatusCode', 'getContent'])
        ->getMock();
        
    $response->expects($this->once())
        ->method('getStatusCode')
        ->willReturn(statusCode);

    $response->expects($this->once())
        ->method('getContent')
        ->willReturn(content);

    $adapter = $this->getMockBuilder(Adapter::class)
        ->setMethods(['get'])
        ->getMock();
        
    $adapter->expects($this->once())
        ->method('get')
        ->willReturn($this->response);
        
    return $adapter;
}

Why am I testing that those methods are called once? It is completely irrelevant to what I'm testing how many times they're called.

What got into my head is I was being too dogmatic when I was writing my code, and I also cheated a bit with the TDD. Instead of designing the test variation first, and then implemented the code to pass the test, I was doing both at the same time. And it shows from my test implementation. Basically I didn't design my solution first, I designed it as I went, so - whilst tricking myself into thinking I was doing TDD - I was asking myself "right, what does my code need to do? It needs to call get on the adapter. OK, so I'll test I call it. What next... I need to get the content from the response. Right: I'll make sure that getContent gets called". All very stolid and admirable, but: wrong. All this is doing is testing my code (almost line by line!). TDD is not about testing my code; it's about testing my design. And it's the design and behaviour of the API of the method I'm testing. As I've said before, it'd be excellent it TDD was retconned to mean "Test Driven Design", not "Test Driven Development". The former is closer to the intent of the exercise.

I don't need to test those methods are called once: there's no logic in my function that will vary how many times they might be called, so they will be called once. We can trust PHP to do what we tell it to do by giving it source code.

The logic flaw here was demonstrated by my code reviewer observed: if you think you need to check how many times that functionality is called, why are you also not concerned with how many times json_decode is called too? It's an important part of your functionality which definitely needs to be called. Ha... yeah "duh", I thought. This drove home I was testing too much.

Another thing is that I was getting too distracted by what my implementation was whilst writing my test. The requirements of that function are:

  • make a request, the response of which might be JSON:
  • if it is: return the deserialised content;
  • if it's not: raise an exception.

That first step is implementation detail from the POV of testing. What we're testing with TDD is the outcome of the function call. Which is: an object, or an exception. We're also only supposed to be testing the logic in the function the test is actually calling. I emphasise "logic" there because we're not testing the code. TDD is not about that. We're testing the logic. This is why we have two test variations: one wherein the logic to throw the exception is not true, so we get our object back; another when that if condition is true, so we get the exception.

We only need to test logic branching code (conditionals and loops, basically).

We do not need to prove every single statement is executed.

I was reminded a rule of thumb to apply when considering whether to use expects or not. Basically it boils down to if you have test variations which might result in a dependency being called a different amount of times, and that variation is germane to the result, then verify the call count.

Another good point made by my other colleague Brian was that when mocking stuff... only mock the bare minimum to get the test passing. Then stop. And that's probably the right place to leave it be.

An example of this might be if we had this logic in our method:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);
    
    if (is_null($object)) {
        $this->logger("Bung content returned: $object");
        throw new \Exception('Unable to decode JSON result');
    }
    return $object;
}

Logging is an optional outcome of that method call, so we might want to test that the logger definitely gets called once in that second test. We could also be belt-and-braces about it and test it's not called at all in the happy-path test.

Or let's say the code was a caching proxy and one is testing the uncached and cached variations:

function get($id){
    if ($this->cache->exists($id)){
        return $this->cache->get($id);
    }
    return $this->dao->get($id);
}

In the uncached variation, the data is not found in the cache, so we want to make sure that the DAO is called to get the data from the DB. In the cached variation, we want to make sure the DAO isn't called. Even here, in the first example we're not really testing the DAO call was made specifically; we're testing the logic around the cache check didn't trigger the early return. We're testing the if statement, not the DAO call.

Bottom line, I think that if I find myself starting to do a call count on a mocked method, I need to stop to think about whether I will have cases where the expected call count will be different, and only then ought I even be bothering if the call-count variation is important to the outcome of the function call. If I'm only checking for one call count value... I'm doing it wrong.

And now rather than talking about my day job... I should actually be doing it...

Righto.

--
Adam