My favourite kind of bug fix is one that only adds a new regression test and does not touch any existing test.
The new test proves the bug is fixed. The untouched existing tests prove the fix does not break any behaviour we rely on: at least none that is covered by a test.
When a bug fix forces you to change existing tests, that is worth a pause: either the tests were coupled to implementation details, or the "bug" was actually documented behaviour that someone depends on.
That is the whole idea, and I could stop here. But a claim like this is easy to make in the abstract. This article checks it against the history of a codebase I know rather well: PHPUnit itself. Two recent commits show the ideal case. Then, because a thesis that only ever confirms itself should make you suspicious, I searched PHPUnit's history for counterexamples: bug fixes that did change existing tests. For most of them, the two diagnoses above hold up. I learned the most from the one commit where they do not.
One refinement before we start
The claim needs to be stated precisely, or it is easy to refute with git log --stat. Many bug fixes in PHPUnit's history "modify" an existing test file by appending to it: a new test method, a new data provider case. That is the additive pattern; it merely lands in an existing file instead of a new one.
What the thesis is really about is expectations. A fix that adds a test case to an existing file leaves every existing expectation intact and therefore keeps the full evidentiary value of the suite. A fix that rewrites an existing expectation destroys evidence: an assertion that used to document behaviour now documents something else, and the question is why.
Stated precisely, the thesis is this: the ideal bug fix adds expectations and changes none.
The ideal case, twice
PHPUnit 13.2.4 contains two bug fixes, one committed directly after the other, that show what this looks like in practice. Both concern the same contract: how PHPUnit's error handler coexists with an error handler that the application under test registered before PHPUnit took over. Issue #6817 and issue #6818 each describe a situation in which PHPUnit did not play nicely with that previously registered handler.
The two fixes (1d90b396 and 6b667a5c) have the same shape:
- a change to
src/Runner/ErrorHandler.php, with a comment that explains why the code does what it does and links to the issue, - new end-to-end tests, each with a fixture that reproduces the reported scenario,
- a ChangeLog entry,
- and no change to any existing test.
Each of the two commits can be understood on its own: the code comment points to the issue, the issue to the regression test, the test to the ChangeLog. Six months from now, git log and git blame will still tell the whole story.
One detail deserves attention: the regression tests are end-to-end tests, not unit tests. That is not laziness. The contract being fixed is between PHPUnit's runner and a globally registered error handler: global state that is set up before PHPUnit runs a single test. No unit test can exercise that contract honestly; a .phpt test that registers an error handler in its script and then runs PHPUnit can. The regression test must live at the level where the bug lives. Writing a unit test against internals instead would have produced exactly the kind of coupled test that the next section is about.
And the untouched existing tests? PHPUnit's error handling was already covered by tests before these fixes. That these tests still pass is one half of the proof. The new tests show the bugs are gone; the old tests show that fixing them did not break any other documented behaviour.
When existing tests do change: the search
Two examples that confirm a thesis prove nothing. So I searched PHPUnit's history since 10.0.0 for the opposite pattern: bug-fix commits that modified pre-existing test files. The message and ChangeLog heuristics produced 191 candidates. 75 of them only appended to an existing file: new test methods, new data provider cases. The other 116 deleted or rewrote at least one line in an existing test; I went through those by hand, and a small set of clear examples remained.
They fall into the two categories I had predicted, and into a third that I had not.
Diagnosis one: the tests were coupled to implementation details
Issue #6470 reported a fatal error when mocking a class with a covariant property hook setter: the mock generator used the property's type for the generated setter instead of the (possibly wider) type accepted by the set hook. The fix (eaa3ae48) threads the setter type through an internal value object, HookedProperty, which gains a fifth constructor parameter.
The behavioural coverage for this fix is purely additive: a new fixture class and a new test in MockObjectTest. But PropertyTest, a unit test that constructs the internal HookedProperty value object directly, broke in every single test method:
public function testHasName(): void { $name = 'property-name'; - $property = new HookedProperty($name, Type::fromName('string', false), false, false); + $property = new HookedProperty($name, Type::fromName('string', false), false, false, Type::fromName('string', false)); $this->assertSame($name, $property->name()); }
Look at what this test asserts: that a name passed into a constructor comes back out of an accessor. None of the behaviour it checks changed. The test broke anyway, because it was pinned to the constructor signature of an internal class: an implementation detail.
This is the first diagnosis in its purest form, and the commit makes for a direct comparison: the same fix produced purely additive changes in the behavioural test and mechanical churn in the structural one. The churn says nothing about the fix, but a lot about the tests: they make every internal refactoring more expensive while adding little confidence that the mock generator behaves correctly. That signal is worth acting on independently of the fix.
Diagnosis two: the "bug" was documented behaviour
When every test of a class is skipped from a #[BeforeClass] hook, PHPUnit used to count zero tests as run. A test run consisting only of such classes printed No tests executed!, and the skipped tests were missing from the summary. A contributed fix (5a6ae728) counts these tests as run and skipped.
This sounds like a bug fix. But the old behaviour was not an accident that nobody had tested: it was pinned down by committed expectations. And here it gets interesting: one of the tests that had to change is tests/end-to-end/regression/5165.phpt, itself the regression test added for an earlier fix. Yesterday's regression test asserted exactly the output that today's fix declares wrong:
There was 1 skipped test suite: 1) Issue5165Test message -No tests executed! +OK, but some tests were skipped! +Tests: 2, Assertions: 0, Skipped: 2.
Everything in this hunk is observable, user-visible behaviour: the summary line, the counts, and (not visible in the diff) the exit path that distinguishes "nothing ran" from "everything was skipped". Any CI script that parses this output notices the change.
I agree with the new behaviour; the old counting was misleading. But calling this commit a bug fix undersells what it is: a deliberate change of documented output semantics, and the test suite said so before any human reviewer did. That is exactly the second diagnosis. Grumbling about "brittle tests" does not help here. The proper response is to recognise the change for what it is, and to communicate it accordingly: in the ChangeLog, and in versioning decisions.
Note the recursion here: the test that exposed this behavioural change was itself added by an earlier bug fix, following exactly the additive pattern. This is how the discipline pays off. Every bug fix that only adds a test makes it harder for the next mislabelled behavioural change to slip through unnoticed.
The diagnosis I missed: the neutralised test
One commit fit neither category, and it is the most uncomfortable one, because the failure it exposes is mine.
A cleanup commit changed a comparison in the defect-priority sorting from if ($priorityB <=> $priorityA) to if (($priorityB <=> $priorityA) > 0), subtly breaking defects-first test ordering for half the comparisons. An end-to-end test caught it, exactly as it should. But the breakage arrived on a maintenance branch and travelled upwards through merges, and in the merge that surfaced the failure, the test was parked with an --XFAIL-- section instead of being investigated. The eventual fix (0f19a4b5) repairs the comparison, and removes this:
--TEST-- Order by defects (with result cache): Test classes with defects ---XFAIL-- -After merging https://github.com/sebastianbergmann/phpunit/pull/6338 into PHPUnit 10.5 and merging it from there to PHPUnit 12.4 via PHPUnit 11.5, this test now fails. -This test failure needs to be investigated. Hopefully, it fails for the obvious reason: that the result cache file needs to be updated.
Neither of the two diagnoses applies here: the test was not coupled to implementation details, and the behaviour did not legitimately change. The test changed because it had earlier been edited to ratify a bug: silenced under merge pressure to keep the build green, with a hopeful note attached, and then forgotten until the real fix un-silenced it.
So the dichotomy in my opening paragraph is incomplete. There is a third pattern: the neutralised test. A correct test whose expectation, XFAIL marker, or skip condition was adjusted to make a failure go away rather than to document reality. When a bug fix touches a test like that, the test change is not part of the cost of the fix; it is the repair of earlier damage. An --XFAIL-- added under pressure does not solve a problem, it postpones it. And the postponed problem grows: by the time someone investigates, the offending change has been merged through three release branches.
I would like to claim this pattern only happens in other people's projects. The diff above says otherwise.
What the numbers say
The most reassuring finding came from the search itself. Most bug fixes since PHPUnit 10 do not touch an existing test file at all. When they do, the typical change removes only a line or two while appending new cases. Genuine rewrites of existing expectations are rare: a few dozen commits out of thousands. And when they occur, almost all of them turn out to be one of the three patterns above: coupling to internals, a behavioural change that was labelled as a bug fix, or the reactivation of a previously silenced test.
That is the thesis, now checked against the history of a real project: in a test suite that predominantly asserts behaviour rather than structure, bug fixes are additive by default. When a bug fix deviates from that default, there is a reason, and each of the three reasons calls for a different response: decouple the test, communicate the behavioural change, or stop silencing failures you do not have time to understand.
So the next time a bug fix makes you edit an existing test, do not silence the test with an --XFAIL-- section, and do not slip past the hook that runs your tests with git commit --no-verify. Pause and ask which of the three patterns you are looking at. The answer tells you what to do next.
