Türker Yıldırım

PHPUnit 12, and the tests we deleted instead

Deep dive9 min read

PHPUnit 12 arrived in February and took two hours, which is the third consecutive cheap major and is entirely the return on one uncomfortable week in 2023. The interesting part of the upgrade was a directory it drew attention to: 188 tests excluded from Rector, excluded from the analyser, and running on every push since 2023.

The symptom

$ cat rector.php | grep -A3 withSkip
  ->withSkip([
      __DIR__ . '/tests/Legacy',
  ]);

$ cat phpstan.neon | grep -A2 excludePaths
  excludePaths:
    - tests/Legacy

$ ls tests/Legacy/*.php | wc -l
11
$ grep -c 'public function test' tests/Legacy/*.php | 
    awk -F: '{s+=$2} END {print s}'
188

An exclusion added in 2023 to make a migration tractable, with no ticket, no comment and no expiry. Two years later it reads as a deliberate architectural boundary rather than as a note to self, which is what every undocumented exclusion becomes.

Why it happens

A skip list is the correct tool for scoping a migration and it has no mechanism for saying “come back to this”. The entry outlives the migration, and the next person reads a path in a configuration file as a decision.

The fix

What the tests actually covered

$ vendor/bin/phpunit --testsuite=legacy 
    --coverage-clover=/tmp/legacy.xml
$ ./bin/coverage-paths /tmp/legacy.xml
  src/Http/Controllers/LegacyImportController.php   94%
  src/Import/CsvFormatV1.php                       100%
  src/Import/CsvFormatV2.php                        98%
  src/Support/LegacyRouteResolver.php               88%

# four files. nothing else in the codebase is touched
# by these 188 tests.

Measuring what the tests reach rather than reading their names is the step that makes this tractable — the directory is called Legacy and that tells you nothing about which code is legacy. Four files out of nine hundred is a small enough surface to investigate properly.

Three independent checks before deleting

# 1. is it routed?
$ grep -rn 'LegacyImportController' routes/
# (nothing)

# 2. is it called?
$ ./bin/route-usage --since=180d | grep -ci legacy
0
$ ./bin/access-log-paths --since=180d | grep -c '/import/v1'
0

# 3. is it referenced?
$ grep -rn 'LegacyRouteResolver|CsvFormatV1|CsvFormatV2' 
    src/ config/ app/ --exclude-dir=Import --exclude-dir=Support
# (nothing)

Any one of these alone is a guess. The reference search is the one people skip because the routing already said no, and it is the one that catches a class constructed by a container binding or named in a configuration file — which is exactly how dead code turns out not to be.

The three tests worth keeping

// of the 188, three asserted behaviour that still
// exists elsewhere:

//   the CSV delimiter detection heuristic, which moved
//   to the current importer in 2022 and had no test
//   there

//   a date parsing edge case for a supplier format
//   that is still received

//   an encoding fallback for latin-1 input

// moved to tests/Import/, rewritten against the
// current classes, and two of the three failed on the
// first run.

Two of three failing against the current implementation is the finding that justifies the whole exercise — behaviour that was tested in the old code path and had never been tested in the new one, for three years. The delimiter heuristic had a genuine regression in it, on files using semicolons.

Deleting the code, not just the tests

$ git rm -r tests/Legacy src/Import/CsvFormatV1.php 
    src/Import/CsvFormatV2.php src/Support/LegacyRouteResolver.php 
    src/Http/Controllers/LegacyImportController.php

$ git diff --stat HEAD~1 | tail -1
 16 files changed, 41 insertions(+), 4,112 deletions(-)

$ vendor/bin/phpstan analyse
 [OK] No errors
$ vendor/bin/phpunit
  Tests: 1,229 passed        # was 1,414

# and the exclusions, removed from both configurations.

Removing the exclusions is the part that matters more than the deletion, because an empty skip list cannot silently protect the next directory. The test count dropping by 185 looks like a regression in any dashboard and is the correct outcome — 185 tests covering nothing are not coverage.

The upgrade itself

$ composer require --dev phpunit/phpunit:^12.0
$ vendor/bin/phpunit 2>&1 | grep -c Deprecat
3

# three deprecations, all of them the same: a data
# provider returning a Generator, which 12 requires to
# be an iterable of arrays rather than of scalars.

$ vendor/bin/phpunit
  Tests: 1,229 passed

# elapsed: 1h 50m, of which 1h 40m was the deletion.

Verifying it worked

$ vendor/bin/phpunit --coverage-text | tail -2
  Lines: 76.41%      # was 74.11% — the denominator
                     # shrank more than the numerator

$ ./bin/suite-duration
  4m 02s             # was 4m 40s

$ grep -rc 'excludePaths|withSkip' phpstan.neon rector.php
phpstan.neon:0
rector.php:0

# a fortnight of production
$ grep -c 'CsvFormatV1|LegacyRoute' /var/log/app/*.log
0

Coverage going up because the denominator shrank is worth stating plainly rather than presenting as an improvement — deleting untested code raises the percentage and improves nothing. The fortnight of logs is the check that nothing constructed those classes by a path none of the three searches covered.

What this costs

A deletion that cannot be undone by anything except git, on four files nobody has read in two years. The three checks are strong and they are not proof — a reflection-based construction from a string in a database would have passed all three, and the fortnight of logs is the only thing standing between this and a production error.

The wider lesson is about the skip list rather than the code. An exclusion added during a migration needs a comment with a date and a reason, and there is no tooling for that — the closest thing is a test asserting the skip list is empty, which we now have and which is the most passive-aggressive test in the suite.