PR Review - Historia i problemy
Wspólnie z Krzysztofem Witczakiem stworzyliśmy serię artykułów, w których zastanawiamy się skąd tak duża popularność Pull Request Review, oraz jakie problemy Pull Request Review tworzy w naszych zespołach. Następnie podjęliśmy temat rozwiązywania kwestii PR Review i mierzenia, czy jest lepiej.
W serii PR Review:
Historia i problemy [TEN ARTYKUŁ]
PR Review – z czym pracujemy
Z Krzyśkiem Witczakiem złapaliśmy się na Mastermind po publikacji na LinkedIn artykułu Pull Request Review - szybciej, lepiej, inaczej. Wymieniliśmy się spostrzeżeniami wokół problemów dookoła PR Review i jak poprawić ten stan rzeczy.
Wyszło z tego tyle notatek, że postanowiliśmy podsumować te informacje dla potomnych 😀
Jedna istotna uwaga:
Nie skupiamy się tutaj na Pull Request – wystawieniu kodu do zmergowania po przejściu buildu
Skupiamy się na Pull Request Review – procesie asynchronicznego sprawdzania kodu w ramach wystawionego PR.
Lecz najpierw krótka historia o tym, skąd PR Review się wzięło. I dlaczego jest ważne, by o tym pamiętać.
Historia Pull Request
Ciekawe podsumowanie historii PR możemy znaleźć na blogu Daniela Smith’a w artykule A Brief History of the Pull Request.
Git
Git został stworzony przez zespół pracujący nad jądrem Linuxa. Wraz z nim powstała też komenda Pull Request, aby uporządkować proces zgłaszania zmian. Wewnętrzni i zewnętrzni kontrybutorzy mogli tą prostą komendą zebrać proponowane zmiany do repozytorium. Następnie mieli możliwość je przesłać mailowo na listę mailingową Linux Kernel Mailing List. Tam oceniali ją natomiast główni maintenerzy.
Istotne jest wychwycenie tutaj kontekstu pracy nad jądrem Linuxa:
Duża różnorodność kontrybutorów.
Kontrybutorzy nie znają się wznajemnie.
Środowisko mocno asynchroniczne – nieskupione na projektach, wydaniach.
Przy takich założeniach zadaniem jest oczywiście szukanie błędów i wychwytywanie wrogich aktorów..
GitHub
Następnie, w 2008 roku, powstał GitHub. Same Pull Request zostały wdrożone niewiele później – Oh yeah, there’s pull requests now. Zawierały jednak tylko powiadomienia mailowe do osób wskazanych w GUI.
Aktualny kształt Pull Request przyjął w 2010 roku podczas aktualizacji Pull Requests 2.0. Mamy znane nam dyskusje, commity, zmienione pliki:
Jednak znamienity jest opis zawarty w oryginalnym powiadomieniu od GitHuba:
As of today, pull requests are living discussions about the code you want merged. They’re our take on code review and represent a big part of our vision for collaborative development.
Pull Request został bezpośrednio powiązany z przeprowadzeniem sprawdzenia kodu.
Jeśli zaś skupimy się na repozytorach firmowych to jednak kontekst pracy się zmienił:
Wąskie grono kontrybutorów.
Grupa zwykle się zna, jest powiązana wspólnymi kontraktami.
Środowisko synchroniczne – wydania, projektowa.
Więc skąd wykorzystanie PR w ramach sprawdzenia kodu?
Otoczenie narzędziowe 2010
Na powyższą sytuację trzeba nałożyć realia roku 2010.
W ramach tamtejszej sytuacji brakowało:
Możliwości łatwej współpracy zdalnej – narzędzia typu Skype były jeszcze w powijakach.
Potężnych IDE - wspomagających i automatyzujących naszą pracę.
Procesów CI/CD – stale integrujących kod i sprawdzających błędy.
Stabilnych środowisk testowych (nie mówiąc o efemerycznych).
Książki takie jak Continuous Delivery dopiero co wprowadzały tematy automatyzacji pod strzechy, tempo wypuszczania oprogramowania było mniejsze. Dużo więcej było więc wymagane od szeregowego pracownika – manualnego potwierdzenia, uruchomienia lokalnie, sprawdzenia testów.
Otoczenie narzędziowe 2024
Wszystko to, co zostało opisane powyżej jako braki, dziś już w zasadzie jest w standardzie. Mamy nowoczesne narzędzia (a nawet AI). Diametralnie zmieniło się też tempo dostarczania.
Więc czemu ludzie wciąż wykorzystują Pull Request Review w 2024 roku, w ten sam sposób co w 2010?
Ciężko powiedzieć.
Ale mamy kilka teorii. O nich za chwilę - gdy już opowiemy jakie problemy może zrodzić Pull Request Review stosowane jak w 2010.
Problemy dookoła PR Review
Konflikt interesów
Review staje jest poklepywaniem po plecach.
Autor czekający na review często już pracuje nad kolejnym zadaniem i chce jak najszybciej “wypchnąć” wiszące zmiany. Jakiekolwiek uwagi w review są tutaj opóźnieniem. A czas uruchomienia funkcji na produkcji jest widoczny dla wszystkich członków zespołu. Reviewer również jest tego świadomy.
Dodatkowo nierzadko dochodzi do błędów kognitywnych dookoła kosztu utopionego – Sunk Cost Fallacy:
Im więcej uwag znalezionych w kodzie tym mocniej zmieniamy napisany kod. Kod, na który poświęciliśmy czas by go napisać. Więc naturalnie będziemy chcieli z tym kodem iść dalej na produkcję.
W efekcie tworzymy środowisko, gdzie:
Autor zmiany skrycie ma nadzieję, że nic “dużego” nie zostanie wykryte.
Reviewer mający pomóc utrzymać jakość często “ogranicza” się, aby nie pogarszać sytuacji i nie doprowadzać do irytacji członka zespołu.
W takim środowisku błędne zmiany nie będą zawracane, ponieważ ich koszt socjotechniczny będzie zbyt duży.
Nie dowozi jakości produktu
Rozwijając poprzedni punkt, PR Review daje złudne poczucie jakości produktu.
Nasze systemy stają się coraz większe i coraz bardziej skomplikowane z każdym rokiem.
Liczba przypadków użycia i scenariuszy brzegowych staje się zbyt duża, aby mogła zostać w pełni pokryta podczas prostego przeglądu kodu, na który mamy mało czasu i robimy po oderwaniu od innej czynności.
Często przypadki testowe wychodzą poza zmiany kodu – są połączone z częściami nie zmienianymi.
Możemy zacytować tutaj naukowców z pracy Alberto Bacchelli i Christian Bird Expectations, Outcomes, and Challenges of Modern Code Review”
Quality Assurance: There is a mismatch between the expectations and the actual outcomes of code reviews. From our study, review does not result in identifying defects as often as project members would like and even more rarely detects deep, subtle, or “macro” level issues. Relying on code review in this way for quality assurance may be fraught.
W rezultacie, istotne błędy mogą zostać przeoczone, a złudne poczucie bezpieczeństwa i jakości prowadzi do problemów w produkcie końcowym. PR Review wielokrotnie to tylko minimum, “plaster” na problemy zespołu związane z faktycznym dbaniem o jakość produktu.
Różne percepcje jakości
Znane nam są przypadki, gdy zespół ma tak zwaną “tribal knowledge”: kto w teamie robi dokładny review, a kto “rozdaje approvale” 😉 To tworzy sytuacje, w których:
PR review jako ostatni bastion jakości łatwo złamać, jeżeli wiesz kogo zapytać…
W zespole powstaje dysbalans wysiłku, uczucie frustracji, “jedni się starają” a inni nie
Ci którzy się starają, nie rozumieją, dlaczego innym nie zależy na jakości
Ci którzy dają approvale, nie rozumieją, dlaczego innym nie zależy na prędkości
Niektórzy członkowie zespołu przestają czuć ownership za swój kod, dzięki “siatce bezpieczeństwa” w postaci doświadczonego reviewera. Jeżeli reviewer też przyklepał to rozwiązanie/też tego nie zauważył, no to musi być dobrze…
W pewnym sensie dzielą się odpowiedzialnością, co nie jest fair biorąc pod uwagę jak mało czasu reviewer ma na analizę kodu.
Kończymy z sytuacją, gdy PR Review zapewnia bardzo nieregularną jakość.
Nie dowozi jakości kodu
Kolejną ułudą jest postrzegane PR Review jako narzędzia do utrzymania jakości kodu. W praktyce często nie spełnia tej roli. Mamy w branży świadomość, że testy manualne czy automatyczne nie wyeliminują wszystkich błędów. Jednocześnie wydaje nam się, że krótkie review kodu podczas PR jest wystarczającą inwestycją w jakość.
Recenzenci często skupiają się na drobnych, powierzchownych problemach zamiast na głębokich, fundamentalnych aspektach kodu. Największą wartość mają zmiany dotyczące architektury i struktury kodu, które znajdują się na dole piramidy Code Review
Te zmiany są jednak najtrudniejsze do wykrycia (często brakuje nam czasu i kontekstu) oraz naprawy podczas przeglądu PR.
Co wpędza nas w…
Wydłużanie Lead Time
Ostatnio popularyzowany framework DevEx mocno podkreśla znaczenie dwóch filarów produktywności:
Krótkich pętli zwrotnych dostarczających informacji jak nam idzie.
Uczucia flow, które możemy dla uproszczenia przyjąć jako skupienie.
Poleganie na review podczas PR wprowadza wąskie gardła w obu tych obszarach - ponieważ wprowadzamy rozpraszacze dla reszty zespołu w losowych momentach (przerywając ich flow) oraz otrzymujemy pętlę zwrotną stosunkowo wolno (kwestia godzin, zamiast sekund).
W 2024 roku, kiedy wszyscy mówią o GenAI, możemy sobie wyobrazić, że wolumen produkowanego kodu na kontrybutora będzie rosnąć, co oznacza jeszcze większe problemy ze skalowaniem się tego procesu.
Ostatecznie tworzymy znaczne kolejki w zespole - jak Radek opisał w artykule Dlaczego tak wolno dowozimy.
Brak kontekstu i przeładowanie
Recenzent nie widzi szerszego kontekstu.
Pliki, które nie zostały zmienione, nie są widoczne, więc wymagany jest większy nakład pracy by zobaczyć kod, na który może mieć to wpływ (np. miejsca wywołujące zmienioną klasę).
Jeżeli pracujemy w projektach, które posiadają oddzielne repozytoria i jeden zespół dotyka ich wszystkich (np. osobne repo na frontend, bibliotekę komponentów, backend, testy e2e, konfigurację itd.), może być konieczne odnoszenie się do poprzednich lub równoległych PR, aby dostatecznie dobrze zrozumieć kontekst wprowadzanej zmiany.
Sytuacja robi się jeszcze gorsza, jeżeli:
review trwa długo lub PR oddalone są w czasie,
reviewer pracuje nad innym zadaniem,
jeżeli recenzenci mają małą znajomość wyżej wymienionych repozytoriów, ale konieczny jest approval z uwagi na zdefiniowany proces.
Wtedy musimy wczytać większy kontekst, aby faktycznie przeprowadzić jakościowe review.
Iluzja wymiany wiedzy
Na stronie GitHub PR Review zostały opisane jako proces, który wspiera wymianę wiedzy. Twierdzimy jednak coś innego – dają one iluzję wymiany wiedzy.
Komentarze dotyczą zmian, a nie wiedzy wymaganej do wprowadzenia tych zmian. Przez co albo:
Nie zdobędziemy tej wiedzy podczas PR Review.
Dopytamy o to w komentarzach, co da płytką wymianę wiedzy.
Prowadzi to do sytuacji, gdzie reviewer spoza zespołu może blokować zmianę (i zespół) na ostatniej prostej. Nie wynika to ze złych chęci tylko braku kontekstu – czy jest to prototyp zawężony do jednego klienta, a może fundament dla prac kolejnych zespołów?
Liderzy czy ownerzy repozytoriów/obszarów w kodzie często oczekują prawa approvala aby trzymać rękę na pulsie i wiedzieć co się dzieje. Jednak etap PR review jest zdecydowanie zbyt późnym w SDLC, aby drastycznie proponować zmianę kierunku rozwoju. Przez co zmiany są wprowadzane przy minimalnej wymianie wiedzy.
Przedwczesna optymalizacja
W sieci możemy znaleźć wiele artykułów pouczających programistów, aby podczas Review akceptowali inne rozwiązania problemu niż ich preferowane, jeżeli jest to tylko kwestia opinii.
W praktyce bardzo ciężko jest pozbyć się biasów i uniknąć takich komentarzy.
Uważamy jednak, że ten problem nasili się podczas PR Review:
PR Review nie zapewnia wymiany wiedzy – to sprawia, że nie rozumiejąc powodów wykonania zmiany będziemy ją optymalizowali pod inne kryteria.
PR Review jest bardzo widoczne – to sprawia, że będziemy chcieli przeprowadzić „rozwijające” zmiany
Reviewerzy z większym doświadczeniem, mają tendencję do proponowania bardziej złożonych podejść. Szczególnie w zespołach, gdzie kultura techniczna jest mocno rozwinięta.
To skutkuje kosztownymi zmianami w implementacji, które nie zawsze przynoszą oczekiwane rezultaty, a czasami wręcz łamią zasady KISS i YAGNI.
Niestety, na tym etapie jest często tak wcześnie, że jeszcze nie mamy pewności jaki wzorzec czy model abstrakcji będzie znacząco lepszy od prostszych do utrzymania alternatyw. W rezultacie zaczynamy optymalizować i inwestować w nasze wyobrażenie przyszłości, które może się nie spełnić.
Utrudnienie wprowadzania małych usprawnień
Jeśli każda zmiana musi przejść przez ten sam ciężki proces to wprowadzanie małych, inkrementalnych zmian jest utrudnione. Wobec czego łatwiej jest:
Wprowadzać duże zmiany raz a dobrze – rośnie wielkość mergowanego kodu (co zwiększa Lead Time), a im większa zmiana podczas PR review tym bardziej nasz cognitive load jest wysycony, przez co review staje się płytsze
Nie wprowadzadzać poprawek po zmergowaniu największej zmiany - ucieka nam cała praca po wdrożeniu.
Trudno w takim środowisku stosować zasadę scoutów
“Leave Things BETTER than you found them.” ~ Robert Baden Powell
Raczej budujemy środowisko, które odstrasza programistów od aplikowania jej.
Rozmiar Review wzrasta
Po zaakceptowaniu powyższych punktów, większość PR Review będzie dążyła do wprowadzania coraz większych zmian. A to wpływa na opisaną już jakość.
Biorąc na tapet artykuł From Async Code Reviews to Co-Creation Patterns z InfoQ:
A correlation between change size and review quality is acknowledged by Google and developers are strongly encouraged to make small, incremental changes.
Tak duże zmiany odstraszają reviewerów - albo przejdą przez proces bez żadnego sprawdzenia, albo utkną tam na wiele dni. Jeżeli wystarczy nam sił na to drugie, to dochodzi tutaj również problem integracji zmian, ponieważ PR wiszący długo zaczyna odstawać od głównego brancha - pojawiają się konflikty i zablokowani programiści, którzy na zmianę czekają.
Narzędzia PR powodują relacje ping-pong
Nie komunikujemy się ze sobą, przerzucamy PR do kolegi.
Utrudnia to budowanie relacji w zespole. Budowanie kultury pracy zespołowej (szczególnie w zdalnych zespołach) jest znacznie trudniejsze, gdy cała komunikacja opiera się na komentarzach w GitHub. To promuje relację siły – osoba zatwierdzająca ma władzę.
Zwłaszcza w zespołach asynchronicznych, konflikty, które na słuchawce można by rozwiązać w kilkanaście minut potrafią ciągnąć się dniami poprzez komentarze.
Jakie są powody występowania PR Review?
Wiele zespołów akceptuje status quo i wykonuje review, ponieważ takiej formy pracy ich nauczono i nikt tego nigdy nie zakwestionował, ani nie zastanowił się, czy nie da się inaczej 😉 Co dokładnie za tym stoi? O tym piszemy w kolejnej części: