Meine liebste Art von Bugfix ist die, die nur einen neuen Regressionstest hinzufügt und keinen bestehenden Test anfasst.
Der neue Test beweist, dass der Fehler behoben ist. Die unveränderten bestehenden Tests beweisen, dass die Korrektur kein Verhalten bricht, auf das wir uns verlassen. Zumindest keines, das von einem Test abgedeckt ist.
Wenn ein Bugfix dich zwingt, bestehende Tests zu ändern, lohnt sich eine Pause: Entweder waren die Tests an Implementierungsdetails gekoppelt, oder der „Bug“ war in Wirklichkeit dokumentiertes Verhalten, auf das sich jemand verlässt.
Das ist die ganze Idee, und ich könnte hier aufhören. Aber so eine Behauptung ist schnell aufgeschrieben. Dieser Artikel überprüft sie anhand der Geschichte einer Codebasis, die ich ziemlich gut kenne: PHPUnit selbst. Zwei aktuelle Commits zeigen den Idealfall. Und weil eine These, die sich immer nur selbst bestätigt, misstrauisch machen sollte, habe ich anschließend in der Geschichte von PHPUnit nach Gegenbeispielen gesucht: nach Bugfixes, die bestehende Tests doch verändert haben. Für die meisten davon treffen die beiden Diagnosen oben zu. Am meisten gelernt habe ich von dem einen Commit, für den sie nicht zutreffen.
Eine Präzisierung vorab
Die Behauptung muss präzise formuliert sein, sonst lässt sie sich leicht mit git log --stat widerlegen. Viele Bugfixes in der Geschichte von PHPUnit „verändern“ eine bestehende Testdatei, indem sie anfügen: eine neue Testmethode, ein neuer Fall in einem Data Provider. Das ist das additive Muster; es landet nur in einer bestehenden Datei statt in einer neuen.
Worum es der These wirklich geht, sind Erwartungen. Ein Fix, der einer bestehenden Datei einen Testfall hinzufügt, lässt jede bestehende Erwartung intakt und erhält damit die volle Beweiskraft der Testsuite. Ein Fix, der eine bestehende Erwartung umschreibt, zerstört Beweismaterial: Eine Zusicherung, die bisher Verhalten dokumentiert hat, dokumentiert jetzt etwas anderes. Die Frage ist, warum.
Präzise formuliert lautet die These also: Der ideale Bugfix fügt Erwartungen hinzu und ändert keine.
Der Idealfall, gleich zweimal
PHPUnit 13.2.4 enthält zwei Bugfixes, direkt nacheinander committet, die zeigen, wie das in der Praxis aussieht. Beide betreffen denselben Vertrag: wie der Error Handler von PHPUnit mit einem Error Handler koexistiert, den die getestete Anwendung registriert hat, bevor PHPUnit übernommen hat. Issue #6817 und Issue #6818 beschreiben jeweils eine Situation, in der PHPUnit nicht gut mit diesem zuvor registrierten Handler zusammengespielt hat.
Die beiden Fixes (1d90b396 und 6b667a5c) haben dieselbe Form:
- eine Änderung an
src/Runner/ErrorHandler.php, mit einem Kommentar, der erklärt, warum der Code tut, was er tut, und der auf das Issue verlinkt, - neue End-to-End-Tests, jeweils mit einer Fixture, die das gemeldete Szenario reproduziert,
- ein ChangeLog-Eintrag,
- und keine Änderung an irgendeinem bestehenden Test.
Jeder der beiden Commits lässt sich für sich allein nachvollziehen: Der Kommentar im Code verweist auf das Issue, das Issue auf den Regressionstest, der Test auf das ChangeLog. Auch in sechs Monaten erzählen git log und git blame noch die ganze Geschichte.
Ein Detail verdient Aufmerksamkeit: Die Regressionstests sind End-to-End-Tests, keine Unit Tests. Das ist keine Faulheit. Der Vertrag, um den es geht, besteht zwischen dem Runner von PHPUnit und einem global registrierten Error Handler. Also globalem Zustand, der eingerichtet wird, bevor PHPUnit auch nur einen einzigen Test ausführt. Kein Unit Test kann diesen Vertrag ehrlich prüfen; ein .phpt-Test, der in seinem Skript einen Error Handler registriert und dann PHPUnit ausführt, kann es. Der Regressionstest muss auf der Ebene leben, auf der der Fehler lebt. Stattdessen einen Unit Test gegen Interna zu schreiben, hätte genau die Art von gekoppeltem Test hervorgebracht, um die es im nächsten Abschnitt geht.
Und die unveränderten bestehenden Tests? Die Fehlerbehandlung von PHPUnit war schon vor diesen Fixes durch Tests abgedeckt. Dass diese Tests weiterhin bestehen, ist die eine Hälfte des Beweises. Die neuen Tests zeigen, dass die Fehler weg sind; die alten Tests zeigen, dass ihre Behebung kein anderes dokumentiertes Verhalten gestört hat.
Wenn bestehende Tests sich doch ändern: die Suche
Zwei Beispiele, die eine These bestätigen, beweisen nichts. Also habe ich die Geschichte von PHPUnit seit 10.0.0 nach dem gegenteiligen Muster durchsucht: Bugfix-Commits, die zuvor existierende Testdateien verändert haben. Die Heuristiken über Commit-Nachrichten und ChangeLog lieferten 191 Kandidaten. 75 davon haben einer bestehenden Datei nur etwas hinzugefügt: neue Testmethoden, neue Data-Provider-Fälle. Die übrigen 116 haben mindestens eine Zeile in einem bestehenden Test gelöscht oder umgeschrieben; diese habe ich von Hand durchgesehen, und am Ende blieb eine kleine Menge klarer Beispiele.
Sie fallen in die beiden Kategorien, die ich vorhergesagt hatte, und in eine dritte, mit der ich nicht gerechnet hatte.
Diagnose eins: Die Tests waren an Implementierungsdetails gekoppelt
Issue #6470 meldete einen Fatal Error beim Mocken einer Klasse mit einem kovarianten Property-Hook-Setter: Der Mock-Generator verwendete für den generierten Setter den Typ der Property statt des (möglicherweise breiteren) Typs, den der set-Hook akzeptiert. Der Fix (eaa3ae48) reicht den Setter-Typ durch ein internes Wertobjekt, HookedProperty, das dafür einen fünften Konstruktor-Parameter erhält.
Die verhaltensbezogene Abdeckung für diesen Fix ist rein additiv: eine neue Fixture-Klasse und ein neuer Test in MockObjectTest. Aber PropertyTest, ein Unit Test, der das interne Wertobjekt HookedProperty direkt konstruiert, brach in ausnahmslos jeder Testmethode:
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()); }
Schau dir an, was dieser Test zusichert: dass ein Name, der in einen Konstruktor hineingegeben wird, aus einem Accessor wieder herauskommt. Nichts von dem Verhalten, das er prüft, hat sich geändert. Der Test brach trotzdem, weil er an die Konstruktor-Signatur einer internen Klasse gepinnt war: an ein Implementierungsdetail.
Das ist die erste Diagnose in Reinform, und der Commit erlaubt einen direkten Vergleich: Derselbe Fix erzeugte rein additive Änderungen im verhaltensbezogenen Test und mechanische Anpassungen im strukturellen. Diese Anpassungen sagen nichts über den Fix aus, aber viel über die Tests: Sie machen jedes interne Refactoring teurer und stiften dabei wenig Vertrauen, dass der Mock-Generator sich korrekt verhält. Dieses Signal verdient eine Reaktion, unabhängig vom Fix.
Diagnose zwei: Der „Bug“ war dokumentiertes Verhalten
Wenn alle Tests einer Klasse aus einem #[BeforeClass]-Hook heraus übersprungen werden, zählte PHPUnit früher null ausgeführte Tests. Ein Testlauf, der nur aus solchen Klassen bestand, druckte No tests executed!, und die übersprungenen Tests fehlten in der Zusammenfassung. Ein beigesteuerter Fix (5a6ae728) zählt diese Tests als ausgeführt und übersprungen.
Das klingt nach einem Bugfix. Aber das alte Verhalten war kein Zufall, den niemand getestet hatte: Es war durch committete Erwartungen festgeschrieben. Und hier wird es interessant: Einer der Tests, die geändert werden mussten, ist tests/end-to-end/regression/5165.phpt, selbst der Regressionstest, der für einen früheren Fix hinzugefügt worden war. Der Regressionstest von gestern sicherte exakt die Ausgabe zu, die der Fix von heute für falsch erklärt:
There was 1 skipped test suite: 1) Issue5165Test message -No tests executed! +OK, but some tests were skipped! +Tests: 2, Assertions: 0, Skipped: 2.
Alles in diesem Hunk ist beobachtbares, für Nutzende sichtbares Verhalten: die Zusammenfassungszeile, die Zähler und (im Diff nicht sichtbar) der Exit-Pfad, der „nichts wurde ausgeführt“ von „alles wurde übersprungen“ unterscheidet. Jedes CI-Skript, das diese Ausgabe parst, bemerkt die Änderung.
Ich stimme dem neuen Verhalten zu; die alte Zählweise war irreführend. Aber diesen Commit einen Bugfix zu nennen, verkauft ihn unter Wert: Er ist eine bewusste Änderung dokumentierter Ausgabe-Semantik, und die Testsuite hat das gesagt, bevor irgendein menschlicher Reviewer es tat. Genau das ist die zweite Diagnose. Über „fragile Tests“ zu murren, hilft hier nicht weiter. Die richtige Reaktion besteht darin, die Änderung als das zu erkennen, was sie ist, und sie entsprechend zu kommunizieren: im ChangeLog und in Versionierungsentscheidungen.
Beachte auch die Rekursion: Der Test, der diese Verhaltensänderung aufgedeckt hat, wurde selbst von einem früheren Bugfix hinzugefügt, genau nach dem additiven Muster. So zahlt sich die Disziplin aus. Jeder Bugfix, der nur einen Test hinzufügt, macht es schwerer, dass die nächste falsch etikettierte Verhaltensänderung unbemerkt durchrutscht.
Die Diagnose, die mir fehlte: der neutralisierte Test
Ein Commit passte in keine der beiden Kategorien. Und er ist der unbequemste, denn das Versagen, das er offenlegt, ist meins.
Ein Aufräum-Commit änderte einen Vergleich in der Sortierung nach Defekt-Priorität von if ($priorityB <=> $priorityA) zu if (($priorityB <=> $priorityA) > 0). Und brach damit subtil die Testreihenfolge „Defekte zuerst“ für die Hälfte der Vergleiche. Ein End-to-End-Test bemerkte das, genau wie er sollte. Aber der Bruch kam auf einem Maintenance-Branch an und wanderte durch Merges nach oben. Und in dem Merge, der den Fehlschlag sichtbar machte, wurde der Test mit einer --XFAIL---Sektion stillgelegt, statt untersucht zu werden. Der spätere Fix (0f19a4b5) repariert den Vergleich. Und entfernt dies:
--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.
Keine der beiden Diagnosen passt hier: Der Test war nicht an Implementierungsdetails gekoppelt, und das Verhalten hat sich auch nicht legitim geändert. Der Test änderte sich, weil er zuvor bearbeitet worden war, um einen Fehler abzusegnen: unter Merge-Druck stillgelegt, damit der Build grün bleibt, mit einer hoffnungsvollen Notiz versehen und dann vergessen, bis der echte Fix ihn wieder scharf schaltete.
Das Entweder-oder aus meinem ersten Absatz ist also unvollständig. Es gibt ein drittes Muster: den neutralisierten Test. Einen korrekten Test, dessen Erwartung, XFAIL-Markierung oder Skip-Bedingung angepasst wurde, um einen Fehlschlag verschwinden zu lassen, statt die Realität zu dokumentieren. Wenn ein Bugfix einen solchen Test berührt, gehört die Teständerung nicht zu den Kosten des Fixes; sie ist die Reparatur eines früheren Schadens. Ein unter Druck hinzugefügtes --XFAIL-- löst kein Problem, es verschiebt es. Und das verschobene Problem wächst: Bis jemand nachschaut, ist die verursachende Änderung durch drei Release-Branches gemergt.
Ich würde gern behaupten, dieses Muster gebe es nur in den Projekten anderer Leute. Das Diff oben sagt etwas anderes.
Was die Zahlen sagen
Der beruhigendste Befund kam aus der Suche selbst. Die meisten Bugfixes seit PHPUnit 10 fassen gar keine bestehende Testdatei an. Wenn doch, entfernt die typische Änderung nur ein oder zwei Zeilen, während sie neue Fälle anfügt. Echte Rewrites bestehender Erwartungen sind selten: ein paar Dutzend Commits unter Tausenden. Und wo sie vorkommen, erweisen sich fast alle als eines der drei Muster oben: Kopplung an Interna, eine Verhaltensänderung, die als Bugfix etikettiert wurde, oder die Reaktivierung eines zuvor stillgelegten Tests.
Das ist die These, jetzt an der Geschichte eines echten Projekts überprüft: In einer Testsuite, die überwiegend Verhalten statt Struktur zusichert, sind Bugfixes standardmäßig additiv. Weicht ein Bugfix von diesem Standard ab, hat das einen Grund, und jeder der drei Gründe verlangt eine andere Reaktion: den Test entkoppeln, die Verhaltensänderung kommunizieren oder aufhören, Fehlschläge stillzulegen, für deren Untersuchung du keine Zeit hast.
Wenn dich also das nächste Mal ein Bugfix zwingt, einen bestehenden Test zu ändern, schalte den Test nicht mit einer --XFAIL---Sektion still. Und geh nicht mit git commit --no-verify an dem Hook vorbei, der deine Tests ausführt. Halte inne und frage dich, welches der drei Muster du vor dir hast. Die Antwort sagt dir, was als Nächstes zu tun ist.
