MJ Convento

/

The tests we stopped writing

1,657 words, about 7 min read

I deleted 212 tests on a Thursday afternoon and the build dropped from 11 minutes 40 seconds to 6 minutes 5 seconds. Nobody noticed a regression. Two weeks later I am fairly sure nobody has opened those files either, because when I went looking for the last commit that touched any of them, the newest was from March.

This is a client project at the shop I joined earlier this year: PHP 7.1, Symfony on the older side, MySQL, one Jenkins box that also builds three other repositories. The suite was 912 tests when I started counting. It is 700 now. I want to write down which ones I killed and why, because “more tests is better” was the rule I inherited and it was making the suite worse.

Eleven minutes and forty seconds

The number matters more than it looks. Eleven minutes is past the point where a developer waits for the result. It is long enough to open something else, and once you have opened something else the build is no longer part of your feedback loop, it is a notification you deal with later.

So people stopped running the suite locally. They ran phpunit tests/Unit/Order or whatever directory they were in, pushed, and let Jenkins find out. On a busy day we pushed maybe fourteen times between three of us, which means Jenkins was the first thing to run the full suite roughly fourteen times a day, sequentially, with builds queueing behind each other. The record I saw was a 34 minute wait from push to red.

Red, usually, from a fixture. Not from the code in the commit.

We are on PHPUnit 6.4.4, released on 8 November per the 6.4 ChangeLog. PHPUnit 6 has required PHP 7 since 6.0 landed in February, which you can read straight off its composer.json: "php": "^7.0". The 6.0 ChangeLog says it plainly at the bottom of the list: PHPUnit is no longer supported on PHP 5.6. Upgrading to it was the easy part of this year. The namespaced PHPUnitFrameworkTestCase and the removal of setExpectedException(), deprecated back in 5.2 in favour of expectException(), cost an afternoon of search and replace. The hard part was admitting what the suite was actually doing with those eleven minutes.

The 212

Roughly 90 of them looked like this, in one form or another:

<?php
// tests/AppBundle/Entity/ShipmentTest.php

public function testSetAndGetReference()
{
    $shipment = new Shipment();
    $shipment->setReference('ABC-123');

    $this->assertEquals('ABC-123', $shipment->getReference());
}

There is no bug this test can catch that the code review would not. If setReference() writes to the wrong property, forty other tests break, because everything touches the entity. If it writes to the right property, this test is a statement that PHP assignment works. I found 90 of these across eleven entity test classes and deleted all of them, and the coverage report dropped by about four points and the suite lost nothing.

The second group was more interesting, because it looks like a real test:

<?php
// tests/AppBundle/Handler/OrderHandlerTest.php

public function testHandlerNotifiesTheMailer()
{
    $mailer = $this->createMock(MailerInterface::class);
    $mailer->expects($this->once())
        ->method('send')
        ->with($this->isInstanceOf(Message::class));

    $handler = new OrderHandler($mailer, $this->repository);
    $handler->handle($this->anOrder());
}

This test asserts that OrderHandler::handle() calls send() exactly once with a Message. That is not a fact about the order flow. It is a transcript of the current implementation, written in a second language, stored in a second file. Rename the method, or move the notification behind a dispatcher, and the test fails while the software works. That is the wrong direction for a test to fail in.

PHPUnit itself flirted with this problem. Issue #1902, “Mark a test as risky when it performs an assertion on a test double”, was implemented in 5.2.0 and then pulled back out two patches later in 5.2.2 because of the fallout, which you can see in the 5.2 ChangeLog. So the framework will not help you here. Worse, it quietly counts these tests as doing work. In TestCase::verifyMockObjects() there is this:

foreach ($this->mockObjects as $mockObject) {
    if ($mockObject->__phpunit_hasMatchers()) {
        $this->numAssertions++;
    }
    // ...
}

PHPUnit 6 is strict about useless tests by default, per the 6.0 ChangeLog, so a test body with no assertion in it gets reported as risky. A mock with an expectation on it counts as one assertion, so the test above sails through. The safety net is there, and this class of test walks under it.

The rest of the 212 were fixture-driven functional tests of the admin screens: load 40 rows, boot the kernel, click through, assert a table has 40 rows. They were the slow ones. Eight of them took about 90 seconds between them and the fixtures had drifted so far from production shapes that two had been marked skipped in April and never unskipped.

The rounding test that paid for the suite

Here is the other side, and it is the reason I am not writing an anti-testing post.

We split a delivery fee across the items in an order so each line carries its share on the invoice. The original implementation worked in pesos as floats and rounded each share with round($share, 2). The round() documentation is clear that the default PHP_ROUND_HALF_UP rounds away from zero at the half, and the float precision warning in the manual is equally clear about the other half of the problem: values like 0.1 and 0.7 have no exact binary representation, which is why floor((0.1+0.7)*10) gives 7 and not 8.

Put those two together across three line items and the shares do not add back to the fee. The test that found it is six lines of table:

<?php
// tests/AppBundle/Billing/FeeSplitterTest.php

/**
 * @dataProvider splitCases
 */
public function testSharesAlwaysSumToTheTotal($totalCentavos, $parts, array $expected)
{
    $this->assertSame($expected, (new FeeSplitter())->split($totalCentavos, $parts));
}

public function splitCases()
{
    return [
        'exact thirds'    => [30000, 3, [10000, 10000, 10000]],
        'one centavo over' => [10001, 3, [3334, 3334, 3333]],
        'two centavos over' => [10000, 3, [3334, 3333, 3333]],
        'single part'     => [999, 1, [999]],
    ];
}

Three of those four rows failed on the old implementation. The fix was to move the whole calculation to integer centavos and hand the remainder out one unit at a time, which is what the expected values above encode. Note the assertSame. assertEquals() takes a $delta argument precisely because comparing floats is a judgement call, and once you are writing a delta for money you have already lost the argument.

Total damage before the fix: one centavo missing on 1 invoice in about 40, for months, found by a finance person and not by us.

Half past midnight in Manila

The other test I would not give up covers a date boundary. Timestamps are stored in UTC. The daily operations report is read by people in Manila, which is UTC+8 with no daylight saving, and for a long time the report grouped rows by the UTC date. Everything created between 16:00 and 23:59 UTC therefore landed on the previous business day.

<?php
public function testLateEveningManilaBelongsToTheNextBusinessDay()
{
    $createdAt = new DateTimeImmutable('2017-03-01 16:30:00', new DateTimeZone('UTC'));

    $this->assertSame('2017-03-02', $this->report->businessDay($createdAt));
}

16:30 UTC is 00:30 the next morning in Manila. DateTime::setTimezone() changes the zone the object presents without moving the instant it represents, which is exactly the operation the report needed and was not doing. One test, one line of production code changed, and a report that had been quietly wrong for every order placed after eight in the evening.

What earns its place

The rule I now apply before writing a test is one question: what is the plausible bug that makes this fail? If I cannot name it in a sentence, I do not write the test. “Someone deletes the property” is not plausible. “Someone changes the VAT rate and the rounding goes the other way” is.

That question sorts our code cleanly. Money arithmetic, date and timezone boundaries, permission rules, and anything that parses input from outside the system are all places where a plausible bug exists, is silent, and is expensive. The courier tracking webhook parser has 31 tests and every one of them is a real payload shape we have received, including the one that sends an empty string where a timestamp should be. Those tests are the cheapest insurance in the repository.

Entities, DTOs, wiring, and the question of whether a collaborator was called are not those places. Delete on sight.

Six minutes is still too long

I have not fixed the build. Six minutes is better than eleven and it is still not fast enough that people run it before pushing, which was the actual goal. Of the 6 minutes, about 4 belongs to the 130 functional tests that boot the kernel and hit the database, and I do not yet have a plan for those that does not end in either a much smaller fixture set or a second Jenkins executor we do not have hardware for.

The next thing I am trying is splitting the suite into two test suites in phpunit.xml, unit and functional, running unit on every push and functional on merge only. It is a worse safety net than running everything every time. It is a better safety net than the one we had in October, which was everything every time and nobody reading the result.

Sources