AI-assisted code review z Copilotem - kwiecień 2023

Rok ghost textu i ogłoszenie z 22 marca

Jest 15 kwietnia. Za Copilota płacę od czerwca zeszłego roku, od dnia, w którym wyszedł z technical preview i dostał cennik. Wcześniej miałem go z waitlisty i opisałem to w lutym 2022. W edytorze przez te dziesięć miesięcy nie zmieniło się nic istotnego: szary tekst przed kursorem, Tab, Esc, faktura raz w miesiącu.

Zmieniło się otoczenie. 22 marca GitHub ogłosił Copilot X i wymienił rzeczy, których dzisiaj nie mam pod ręką: podsumowania i opisy pull requestów, odpowiedzi na pytania o dokumentację, wsparcie w terminalu. Do każdej z nich prowadzi formularz, nie przycisk instalacji. Czat w edytorze jest od 22 marca w technical preview za waitlistą i to jedyne zdanie, które o nim tu napiszę, bo nie mam go w rękach i nie zamierzam pisać tutoriala z cudzych zrzutów ekranu.

Zostaje mi więc temat, który mam od dawna i którego nie opisałem: co robię z sugestią, którą Copilot wstawia w projekcie testowym. To u mnie jest całe “AI review” w kwietniu 2023. Nie bot komentujący diff na GitHubie, tylko dyscyplina przy przyjmowaniu podpowiedzi i drugie czytanie tego, co po nich zostało w pliku.

Przez pierwsze dwa tygodnie kwietnia prowadziłem notes. Zapisywałem każdą wieloliniową sugestię, którą przyjąłem w dwóch projektach testowych, i to, co z nią potem zrobiłem. Wyszło 41 pozycji. To jest notes jednej osoby z dwóch tygodni, nie badanie, i tak będę o tym pisał.

Sugestię czytam jak review kolegi

Postawa, którą sobie wypracowałem, mieści się w jednym zdaniu: ghost text to propozycja zmiany, a nie kod, który już jest mój. Kiedy kolega przysyła patcha do zestawu testów, czytam go pod kątem tych samych czterech rzeczy co zawsze. Czy test może paść. Skąd bierze dane. Co zostawia po sobie. Czy nazwy mówią prawdę o tym, co się dzieje w środku. Sugestia dostaje ten sam przebieg.

Jest jedna asymetria, o której trzeba pamiętać. Za patchem kolegi stoi intencja i człowiek, którego mogę zapytać, dlaczego wybrał ten selektor. Za sugestią stoi prawdopodobieństwo, że tak wygląda kod w podobnym miejscu w innych repozytoriach. To brzmi jak drobiazg, a decyduje o kolejności czytania: przy patchu pytam najpierw “dlaczego”, przy sugestii najpierw “czy to w ogóle istnieje”.

Rok temu przy boilerplacie testów pytałem, czy ghost text skraca nudną część pisania testu. Odpowiedź była twierdząca i nie zmieniła się: using, klamry, powtórzony atrybut, kolejne [InlineData], pusta klasa page objectu z pięcioma lokatorami. Tam Copilot nadal wygrywa i tam prawie nic nie poprawiam. Ten wpis jest o drugiej połowie tej samej sugestii, czyli o miejscu, w którym trzeba wiedzieć, jak zachowuje się konkretna aplikacja.

Aplikacja pod testem to nadal to samo publiczne demo w stylu RealWorld, którego używam na blogu od 2021 roku: logowanie, nagłówek z linkami zalogowanego użytkownika, lista artykułów. Żadnego kodu z projektu klienta i żadnego logu z firmowego pipeline’u nie ma w tym wpisie i nie będzie. Wersje mam przypięte: NUnit 3.13.3 w projekcie UI, xUnit 2.4.2 w projekcie warstwy pomocniczej, runner Playwrighta 1.32.3 od 11 kwietnia.

Oto sugestia, którą dostałem po napisaniu nazwy metody testowej. Wpisałem Login_WithValidCredentials_ i po chwili miałem gotowe ciało testu:

[Test]
public async Task Login_WithValidCredentials_ShowsUserMenu()
{
    await Page.GotoAsync("https://demo.example.com/login");
    await Page.FillAsync("#email", "test@test.com");
    await Page.FillAsync("#password", "Password123!");
    await Page.ClickAsync("button[type=submit]");
    await Page.WaitForTimeoutAsync(2000);
    Assert.That(Page.Url, Is.Not.Null);
}

To się kompiluje, wykonuje i świeci na zielono. Jest też bezużyteczne, bo świeci na zielono również wtedy, kiedy logowanie jest zepsute. Page.Url nigdy nie jest null, więc jedyny warunek w tym teście jest zawsze spełniony. Nazwa metody obiecuje sprawdzenie menu użytkownika i w ciele nie ma ani jednego zdania o menu.

Reszta problemów jest mniejsza, ale w zestawie regresji kosztuje tygodniami. Adres wpisany w metodzie omija konfigurację BaseURL. Dane logowania są magiczną stałą, o której nie wiem, czy istnieje na środowisku. WaitForTimeoutAsync to dwie sekundy oddane pod flake: na wolnym agencie za krótko, lokalnie niepotrzebnie. Po teście w bazie zostaje sesja i nic po sobie nie sprząta.

Tak wygląda to, co przyjąłem, po recenzji. Minusy to skreślenia, czyli linie, których nie zostawiłem:

 [Test]
 public async Task Login_WithValidCredentials_ShowsUserMenu()
 {
-    await Page.GotoAsync("https://demo.example.com/login");
-    await Page.FillAsync("#email", "test@test.com");
-    await Page.FillAsync("#password", "Password123!");
-    await Page.ClickAsync("button[type=submit]");
-    await Page.WaitForTimeoutAsync(2000);
-    Assert.That(Page.Url, Is.Not.Null);
+    var user = await _users.CreateAsync();
+    var login = new LoginPage(Page);
+    var header = new HeaderComponent(Page);
+
+    await login.OpenAsync();
+    await login.SignInAsync(user.Email, user.Password);
+
+    await Expect(header.UserMenu).ToBeVisibleAsync();
+    await Expect(header.SignInLink).ToBeHiddenAsync();
 }

Do tego dwie linie, których w sugestii nie było wcale, bo Copilot nie wie, że w tej klasie istnieje sprzątanie:

[TearDown]
public async Task CleanupAsync() => await _users.DeleteAsync();

Z sugestii zostały: nazwa metody, atrybut, sygnatura async Task i kolejność kroków. To jest realna oszczędność, bo to jest ta część, której nie chcę pisać po raz setny. Nie zostało nic, co dotyczy zachowania mojej aplikacji, i to też jest wynik, tylko trzeba go umieć zobaczyć zamiast wcisnąć Tab trzy razy pod rząd.

Notes z dwóch tygodni układa się tak:

A tak wyglądają powody 29 poprawek i odrzuceń. Jedna sugestia mogła mieć dwa problemy, więc suma tej tabeli nie jest sumą tamtej:

Widziałem w sieci procenty z zeszłorocznego badania GitHuba o szybszym kończeniu zadania z Copilotem. Nie mierzyłem tego u siebie i nie będę powtarzał tej liczby jako swojego wyniku. Moje liczby są z jednego notesu i mówią o czymś innym: o tym, ile razy musiałem tknąć przyjętą sugestię, zanim uznałem ją za test.

Gdzie Copilot kłamie przy testach

Trzy kategorie z tabeli powyżej powtarzają się tak regularnie, że warto je opisać osobno. Nie nazywam tego błędem narzędzia. Narzędzie robi dokładnie to, do czego zostało zbudowane: produkuje kod, który wygląda prawdopodobnie w tym miejscu pliku.

Selektory i identyfikatory testowe. To jest najczęstsza i najbardziej kosztowna kategoria, bo jest niewidoczna do pierwszego uruchomienia. Copilot podpowiada mi data-testid="user-menu", #login-form albo .article-preview__title, przy czym istnieje z tego dokładnie jedno, a pozostałe wyglądają jak coś, co ktoś kiedyś w internecie napisał w podobnej aplikacji. Nauczyłem się prostej reguły: żaden nowy selektor nie wchodzi do page objectu bez jednego przebiegu przy otwartej stronie. Kiedy sugestia trafia w klasę, w której lokatory już są zdefiniowane i nazwane, trafność rośnie wyraźnie, bo model ma czego się trzymać w tym samym pliku.

Atrybuty z innej wersji albo z drugiego frameworka. Cztery przypadki w dwa tygodnie, wszystkie tego samego typu. W projekcie NUnit dostałem [TestFixtureSetUp], czyli nazwę z NUnit 2, której w linii 3.13.x po prostu nie ma. W projekcie xUnit dostałem [TestCase] zamiast [Theory] z [InlineData] oraz Assert.Equal z trzecim argumentem jako komunikatem, którego xUnit nie przyjmuje, bo świadomie nie ma takich przeciążeń. To jest kategoria, która boli najmniej, bo kompilator odrzuca ją natychmiast. Warto jednak wiedzieć, dlaczego się pojawia: mam dwa frameworki w jednej solucji, tak jak opisywałem w styczniu 2022, a model widzi kontekst pliku, nie moją decyzję o podziale. Przy okazji: zostaję na NUnit 3.13.3 i xUnit 2.4.2 i nie mam żadnego powodu, żeby ruszać te piny w tym kwartale.

Kroki bez weryfikacji. Trzy razy dostałem test, który przechodzi całą ścieżkę przez interfejs i kończy się na ostatniej akcji. Sekwencja await bez asercji na końcu to nie test, to skrypt, który sprawdza, że nic nie rzuciło wyjątkiem. Bywa, że to wystarczy jako smoke, ale wtedy chcę mieć to napisane w nazwie i w komentarzu, a nie odziedziczone po podpowiedzi. Podobnie z asercjami, które zawsze przechodzą: Is.Not.Null na czymś, co nigdy nie jest null, albo Assert.That(list.Count, Is.GreaterThanOrEqualTo(0)). Dostałem oba w kwietniu.

Wniosek jest ten sam, który zapisałem miesiąc temu przy GPT-4 i szkicowaniu przypadków testowych, tylko o warstwę niżej. Model jest dobry w kształcie i słaby w konkretach zależnych od rzeczywistości, której nie widzi. Kształt testu, układ arrange i act, nazwa metody, powtarzalny atrybut - tak. Selektor, nazwa pola, wartość graniczna z reguły biznesowej, wersja atrybutu w moim csproj - nie.

Copilot for PRs to zapowiedź, nie mój pipeline

Z ogłoszenia 22 marca najbardziej interesuje mnie ta część, która dotyczy pull requestów: generowany opis zmiany na podstawie diffu i podpowiedzi w polu opisu. Wypełniłem formularz i tyle na dzisiaj mam. To jest waitlista i technical preview, nie funkcja, którą włączam w ustawieniach repozytorium, więc nie napiszę, jak to skonfigurować, i nie pokażę zrzutu, którego nie zrobiłem.

Mogę natomiast napisać, co postanowiłem, zanim dostęp przyjdzie, bo to jest decyzja procesowa i nie zależy od tego, czy funkcja będzie dobra.

Wygenerowany opis pull requesta jest opisem, nie werdyktem. Streszczenie diffu odpowiada na pytanie “co się zmieniło”, a review odpowiada na “czy to jest bezpieczne i czy testy to złapią”. To drugie pytanie wymaga wiedzy, której nie ma w diffie: co jest na produkcji, co poszło źle miesiąc temu, na czym stoi ten klient. Widzę tu też ryzyko, które łatwo przewidzieć: kiedy na górze pull requesta pojawi się gładkie streszczenie, część recenzentów przeczyta streszczenie zamiast diffu. To nie jest wina narzędzia, to znany mechanizm, ten sam, z którego bierze się klikanie “re-run” bez czytania loga.

Nie zamierzam wpinać podsumowania generowanego przez model jako kroku bramkującego merge do main. Bramka na pull requeście ma u mnie konkretny skład i opisałem to we wrześniu przy strategii testów w CI/CD: required checks to build, testy jednostkowe i integracyjne z kontenerem, wybrany zestaw UI, a resztę bierze nocny przebieg. Streszczenie tekstowe niczego z tej listy nie zastępuje, bo nie wykonuje kodu. Gdyby stanęło obok jako required check, dostałbym kolejne pole do przeklikania i pierwszą awarię pipeline’u w dniu, w którym cudze API ma zły dzień.

I rzecz, która wynika z jednego i drugiego: dopóki nie mam tego narzędzia, nie planuję pracy pod założenie, że wkrótce je dostanę. Kwiecień u mnie wygląda tak, że review pull requesta robi człowiek, opis pisze autor zmiany, a Copilot pomaga przy pisaniu, nie przy zatwierdzaniu.

Checklista recenzenta sugestii

To jest lista, którą przez kwiecień faktycznie przeszedłem 41 razy. Trzy minuty na sugestię, zawsze w tej samej kolejności.

Czy ten test może paść. Pytanie pierwsze i najważniejsze, bo test, który nie umie paść, jest gorszy od braku testu: kosztuje czas na CI i daje fałszywe poczucie pokrycia. Sprawdzam to najprościej, jak można. Psuję oczekiwaną wartość albo podaję złe hasło i chcę zobaczyć czerwony wynik z sensownym komunikatem. Dziewięć razy w dwa tygodnie ta próba pokazała, że asercja z sugestii jest zawsze prawdziwa.

Skąd są dane i gdzie znikają. Magiczna stała w ciele testu to pytanie bez odpowiedzi: nie wiem, czy ten użytkownik istnieje, kto go założył i czy inny test go właśnie nie zmienia. Dane wjeżdżają przez fixture albo helper, a po teście są usuwane. Copilot nie zna mojego sprzątania, więc [TearDown] i IAsyncLifetime dopisuję ręcznie. Tu wracam do zasady, którą spisałem przy porządkowaniu xUnit i NUnit: najpierw ustal, gdzie żyje stan, potem wybieraj atrybuty.

Czy to przetrwa równoległość. Sugerowany kod chętnie sięga do współdzielonego pola statycznego, do jednego użytkownika o stałym loginie i do rekordu, który “na pewno tam jest”. Lokalnie na jednym wątku przechodzi. Na czterech jobach zaczyna migotać raz na kilka przebiegów, a czas na diagnozę takiego przypadku liczy się w dniach. Rozbiłem zestaw na joby półtora roku temu i cena za bałagan w izolacji nie spadła ani o złotówkę, bo żadne narzędzie AI nie zdejmuje tego wymagania. Ono jest o architekturze testu, nie o szybkości pisania.

Czy nazwa mówi prawdę. Ostatnie pytanie zajmuje sekundy, a ratuje przyszłe czytanie. Nazwa Login_WithValidCredentials_ShowsUserMenu przy asercji na Page.Url jest kłamstwem, które za pół roku ktoś przeczyta jako dokumentację. Albo poprawiam ciało do nazwy, albo nazwę do ciała.

Do tego jedna zmiana nawyku, nie punkt checklisty. Przy sugestii dłuższej niż dwie linie nie przyjmuję jej seriami Tab. Przyjmuję całość i czytam blok w pliku, bo w edytorze łatwo zaakceptować pięć linii, których się nie przeczytało, i nie zauważyć, że w środku zniknął arrange. Ten jeden nawyk odpowiada za większość moich 22 poprawek przed commitem.

Podsumowanie

Po dwóch tygodniach notowania mam wniosek, który da się zapisać w jednym zdaniu: Copilot skraca u mnie pisanie i czytanie kodu testów, a nie skraca odpowiedzialności za merge.

Rozbijam to na trzy zdania, które chcę móc przeczytać za rok. Z 41 przyjętych sugestii 12 zostało bez zmian i wszystkie były boilerplate’em, więc obietnica z marca 2022 trzyma się dalej. 29 razy musiałem poprawić albo odrzucić, a powody były zawsze te same cztery: asercja, dane, sprzątanie i selektor, którego nie ma. Żaden z tych powodów nie jest nowy, wszystkie umiem nazwać i dlatego trzy minuty na sugestię wystarczają, żeby nie wpuścić do zestawu testu, który nie umie paść.

Czego z tego nie wynika. Nie wynika, że mam AI review na pull requestach, bo Copilot for PRs jest za waitlistą od 22 marca i mam tylko wypełniony formularz. Nie wynika, że przestałem czytać diffy. Nie wynika, że ktokolwiek przestał być potrzebny, bo narzędzie, które w kwietniu sześć razy podało mi selektor nieistniejący w aplikacji, nie jest kandydatem na recenzenta.

Ownerem merge’a zostaje człowiek i nie jest to kokieteria. Podpisuję się pod zestawem testów, który po mnie zostaje w repozytorium, a podpisać się można tylko pod czymś, co się przeczytało. Sugestia skraca drogę do pierwszej wersji testu i nie skraca ani o minutę drogi od pierwszej wersji do wersji, którą wpuszczam na main. To drugie zawsze było droższe i w kwietniu 2023 nadal jest.

Za miesiąc albo dwa będę wiedział, czy rzeczy zapowiedziane 22 marca cokolwiek w tym obrazku zmieniają. Jak dostanę dostęp, opiszę to na własnym stacku i z własnymi liczbami. Do tego czasu zostaje mi ta sama praktyka co dziś: szary tekst jako propozycja, checklista jako filtr i czerwony wynik jako dowód, że test w ogóle coś sprawdza.