Refactoring - ist es angebracht, Code einfach neu zu schreiben, solange alle Tests bestanden sind?

9

Ich habe kürzlich "All the Little Things" von RailsConf 2014 gesehen. Während dieses Vortrags überarbeitet Sandi Metz eine Funktion, die eine große verschachtelte if-Anweisung enthält:

def tick
    if @name != 'Aged Brie' && @name != 'Backstage passes to a TAFKAL80ETC concert'
        if @quality > 0
            if @name != 'Sulfuras, Hand of Ragnaros'
                @quality -= 1
            end
        end
    else
        ...
    end
    ...
end

Der erste Schritt besteht darin, die Funktion in mehrere kleinere zu unterteilen:

def tick
    case name
    when 'Aged Brie'
        return brie_tick
    ...
    end
end

def brie_tick
    @days_remaining -= 1
    return if quality >= 50

    @quality += 1
    @quality += 1 if @days_remaining <= 0
end

Was ich interessant fand, war die Art und Weise, wie diese kleineren Funktionen geschrieben wurden. brie_tickZum Beispiel wurde nicht durch Extrahieren der relevanten Teile der ursprünglichen tickFunktion geschrieben, sondern von Grund auf unter Bezugnahme auf die test_brie_*Komponententests. Nachdem alle diese Unit-Tests bestanden waren, brie_tickwurde dies als erledigt angesehen. Nachdem alle kleinen Funktionen ausgeführt wurden, wurde die ursprüngliche monolithische tickFunktion gelöscht.

Leider schien der Moderator nicht zu wissen, dass dieser Ansatz dazu führte, dass drei der vier *_tickFunktionen falsch waren (und die andere leer war!). Es gibt Randfälle, in denen sich das Verhalten der *_tickFunktionen von dem der ursprünglichen tickFunktion unterscheidet. Zum Beispiel @days_remaining <= 0in brie_ticksollte < 0- so brie_tickfunktioniert nicht richtig , wenn sie aufgerufen mit days_remaining == 1und quality < 50.

Was ist hier falsch gelaufen? Ist dies ein Testfehler - weil es für diese speziellen Randfälle keine Tests gab? Oder ein Fehler beim Refactoring - weil der Code Schritt für Schritt hätte transformiert und nicht von Grund auf neu geschrieben werden müssen?

user200783
quelle
2
Ich bin mir nicht sicher, ob ich die Frage bekomme. Natürlich ist es in Ordnung, Code neu zu schreiben. Ich bin mir nicht sicher, was Sie konkret mit "Ist es in Ordnung, Code einfach neu zu schreiben?" Wenn Sie fragen: "Ist es in Ordnung, Code neu zu schreiben, ohne viel darüber nachzudenken?", Lautet die Antwort "Nein", genauso wie es nicht in Ordnung ist , Code auf diese Weise zu schreiben .
John Wu
Dies geschieht häufig aufgrund von Testplänen, die sich hauptsächlich auf das Testen von erfolgreichen Anwendungsfällen konzentrieren, und nur sehr wenig (oder gar nicht) auf das Abdecken von Fehleranwendungsfällen oder Unteranwendungsfällen. Es handelt sich also hauptsächlich um ein Leck der Abdeckung. Ein Testleck.
Laiv
@JohnWu - Ich hatte den Eindruck, dass das Refactoring im Allgemeinen als eine Reihe kleiner Transformationen des Quellcodes ("Extraktionsmethode" usw.) durchgeführt wurde, anstatt einfach den Code neu zu schreiben (womit ich meine, ihn von Grund auf neu zu schreiben, ohne ihn gerade zu schreiben) Betrachten des vorhandenen Codes (wie in der verknüpften Präsentation).
user200783
@JohnWu - Ist das Umschreiben von Grund auf eine akzeptable Refactoring-Technik? Wenn nicht, ist es enttäuschend zu sehen, dass eine so angesehene Präsentation zum Thema Refactoring diesen Ansatz verfolgt. OTOH, wenn es akzeptabel ist, können unbeabsichtigte Verhaltensänderungen auf fehlende Tests zurückgeführt werden - aber gibt es eine Möglichkeit, sicher zu sein, dass Tests alle möglichen Randfälle abdecken?
user200783
@ User200783 Nun, das ist eine größere Frage, nicht wahr? (Wie stelle ich sicher, dass meine Tests umfassend sind?) Pragmatisch würde ich wahrscheinlich einen Bericht zur Codeabdeckung erstellen, bevor ich Änderungen vornehme, und alle Codebereiche, die dies nicht tun, sorgfältig untersuchen Lassen Sie sich trainieren und stellen Sie sicher, dass das Entwicklungsteam sie beim Umschreiben der Logik berücksichtigt.
John Wu

Antworten:

11

Ist dies ein Testfehler - weil es für diese speziellen Randfälle keine Tests gab? Oder ein Fehler beim Refactoring - weil der Code Schritt für Schritt hätte transformiert und nicht von Grund auf neu geschrieben werden müssen?

Beide. Das Refactoring, bei dem nur die Standardschritte aus Fowlers Originalbuch verwendet werden, ist definitiv weniger fehleranfällig als das Umschreiben. Daher ist es häufig vorzuziehen, nur diese Art von Babyschritten zu verwenden. Selbst wenn es keine Komponententests für jeden Randfall gibt und die Umgebung keine automatischen Refactorings bereitstellt, hat eine einzelne Codeänderung wie "Erklärungsvariable einführen" oder "Extraktionsfunktion" eine viel geringere Chance, die Verhaltensdetails des zu ändern vorhandener Code als ein vollständiges Umschreiben einer Funktion.

Manchmal ist es jedoch genau das, was Sie tun müssen oder möchten, wenn Sie einen Codeabschnitt neu schreiben. Und wenn das der Fall ist, brauchen Sie bessere Tests.

Beachten Sie, dass selbst bei Verwendung eines Refactoring-Tools immer ein gewisses Risiko besteht, dass beim Ändern von Code Fehler auftreten, unabhängig davon, ob kleinere oder größere Schritte angewendet werden. Deshalb braucht Refactoring immer Tests. Beachten Sie auch, dass Tests nur die Wahrscheinlichkeit von Fehlern verringern können, aber niemals deren Fehlen beweisen. Die Verwendung von Techniken wie dem Betrachten des Codes und der Zweigabdeckung kann Ihnen jedoch ein hohes Konfidenzniveau geben, und im Falle eines Umschreibens eines Codeabschnitts ist dies der Fall oft wert, solche Techniken anzuwenden.

Doc Brown
quelle
1
Danke, das macht Sinn. Wenn die ultimative Lösung für unerwünschte Verhaltensänderungen umfassende Tests sind, gibt es dann eine Möglichkeit, sicher zu sein, dass die Tests alle möglichen Randfälle abdecken? Zum Beispiel wäre es möglich, eine 100% ige Abdeckung zu haben, brie_tickwährend der problematische @days_remaining == 1Fall immer noch nie getestet wird, indem beispielsweise mit @days_remainingset to 10und getestet wird -10.
user200783
2
Sie können niemals absolut sicher sein, dass die Tests alle möglichen Randfälle abdecken, da es nicht möglich ist, mit allen möglichen Eingaben zu testen. Es gibt jedoch viele Möglichkeiten, mehr Vertrauen in Tests zu gewinnen. Sie könnten sich Mutationstests ansehen , mit denen Sie die Wirksamkeit der Tests testen können.
BDSL
1
In diesem Fall könnten die fehlenden Zweige während der Entwicklung der Tests mit einem Code-Coverage-Tool abgefangen worden sein.
Cbojar
2

Was ist hier falsch gelaufen? Ist dies ein Testfehler - weil es für diese speziellen Randfälle keine Tests gab? Oder ein Fehler beim Refactoring - weil der Code Schritt für Schritt hätte transformiert und nicht von Grund auf neu geschrieben werden müssen?

Eines der Dinge, die bei der Arbeit mit Legacy-Code wirklich schwierig sind: ein umfassendes Verständnis des aktuellen Verhaltens zu erlangen.

Legacy-Code ohne Tests, die alle Verhaltensweisen einschränken, ist ein weit verbreitetes Muster in freier Wildbahn. Was Sie mit einer Vermutung zurücklässt: Bedeutet das, dass die uneingeschränkten Verhaltensweisen freie Variablen sind? oder Anforderungen, die nicht spezifiziert sind?

Aus dem Vortrag :

Dies ist nun echtes Refactoring gemäß der Definition von Refactoring. Ich werde diesen Code überarbeiten. Ich werde seine Anordnung ändern, ohne sein Verhalten zu ändern.

Dies ist der konservativere Ansatz; Wenn die Anforderungen möglicherweise nicht genau festgelegt sind und die Tests nicht die gesamte vorhandene Logik erfassen, müssen Sie sehr vorsichtig sein, wie Sie vorgehen.

Mit Sicherheit können Sie behaupten, dass Sie einen "Testfehler" haben, wenn die Tests das Verhalten des Systems nicht ausreichend beschreiben. Und ich denke, das ist fair - aber nicht wirklich nützlich; Dies ist ein häufiges Problem in freier Wildbahn.

Oder ein Fehler beim Refactoring - weil der Code Schritt für Schritt hätte transformiert und nicht von Grund auf neu geschrieben werden müssen?

Das Problem ist nicht ganz, dass die Transformationen Schritt für Schritt hätten erfolgen sollen. Vielmehr stimmte die Wahl des Refactoring-Tools (menschlicher Tastaturbediener statt geführter Automatisierung) aufgrund der höheren Fehlerrate nicht gut mit der Testabdeckung überein.

Dies hätte entweder durch die Verwendung von Refactoring-Tools mit höherer Zuverlässigkeit oder durch die Einführung einer größeren Anzahl von Tests zur Verbesserung der Systembeschränkungen behoben werden können.

Ich denke, Ihre Konjunktion ist schlecht gewählt. ANDnicht OR.

VoiceOfUnreason
quelle
2

Refactoring sollte das von außen sichtbare Verhalten Ihres Codes nicht ändern. Das ist das Ziel.

Wenn Ihre Komponententests fehlschlagen, bedeutet dies, dass Sie das Verhalten geändert haben. Das Bestehen von Unit-Tests ist jedoch nie das Ziel. Es hilft mehr oder weniger, Ihr Ziel zu erreichen. Wenn das Refactoring das von außen sichtbare Verhalten ändert und alle Komponententests bestanden wurden, ist Ihr Refactoring fehlgeschlagen.

Arbeitseinheitentests geben in diesem Fall nur das falsche Erfolgsgefühl. Aber was ist schief gelaufen? Zwei Dinge: Das Refactoring war nachlässig und die Unit-Tests waren nicht sehr gut.

gnasher729
quelle
1

Wenn Sie "richtig" als "Test bestanden" definieren, ist es per Definition nicht falsch, nicht getestetes Verhalten zu ändern.

Wenn eine bestimmte Kante Verhalten sollte definiert, fügen Sie einen Test für ihn, wenn nicht, dann ist es OK , um es egal , was passiert. Wenn Sie wirklich pedantisch sind, können Sie einen Test schreiben, der überprüft, truewann in diesem Randfall, um zu dokumentieren, dass es Ihnen egal ist, wie sich das Verhalten verhält.

Caleth
quelle