[ten post jest częścią mojego minicyklu o testach, pełna lista postów: tutaj]
W komentarzach do ostatniego posta wywiązała się dyskusja na temat “a co z metodami prywatnymi?“. Odpowiedź najkrótsza z możliwych brzmi: NIC. Zainteresowanych odsyłam do tamtejszych wypowiedzi, a w niniejszej notce postaram się zawarte tam myśli rozwinąć.
Zaczynając przygodę z testami jednostkowymi często stawałem przed dylematem “jak mam przetestować funkcjonalność z metod prywatnych?“. Sporo się naszukałem i naczytałem o różnych rozwiązaniach, z czego dwa zdawały się być najpopularniejsze i najbardziej rekomendowane. I teraz, o kilka lat mądrzejszy, mogę z czystym sumieniem powiedzieć: oba były błędne.
Pierwsza rada to “skorzystaj z refleksji“. Fakt, da się w ten sposób wywołać metodę prywatną, ale czy to przypadkiem nie jest zbytnia ingerencja w sam testowany obiekt? Gdyby metoda miała z założenia być wołana z zewnątrz, to by nie była prywatna! I tu wchodzi druga rada: “upublicznij metodę“. Ale skoro dana metoda była prywatna, to jej upublicznienie tylko i wyłącznie na potrzeby testów zaśmieci nam interfejs klasy. I… śmierdzi.
Tak źle i tak niedobrze.
Moja rada, i rada Zacnych Komentatorów: nie testuj metod prywatnych. Z dwóch powodów:
1) one i tak zostaną “niejawnie” przetestowane poprzez testy metod publicznych…
2) … a jeśli nie, to oznacza że wcale nie są potrzebne, więc nie ma sensu ich testować
“Ale ja naprawdę czuję że powinienem bo są skomplikowane!”
Jeśli tak… to Twój kod wymaga zmian. Czy funkcjonalność w tej prywatnej metodzie nie jest przypadkiem zbyt przepchanym logiką helperem, który tak naprawdę w ogóle nie powinien znaleźć się w tej klasie? Pamiętaj o Single Responsibility Principle: “every object should have a single responsibility, and that responsibility should be entirely encapsulated by the class“.
Przytoczę przykład, który podałem we wspomnianych komentarzach: mamy klasę posiadającą ID, jednak nie dostaje go wprost. Zamiast tego z jakiegoś zewnętrznego systemu dajemy jej stringa i mówimy “znajdź tu sobie swoje ID“. Z życia wzięte: FIM reprezentuje referencje między obiektami poprzez taki string: “urn:uuid:b116583c-a03e-4ea8-a895-a9511a4714b3“, gdzie pierwsze dwa elementy są stałe, a trzeci to ID obiektu powiązanego.
Sytuacja wymagająca testowania metody prywatnej zakłada, że klasa taka ma zaszytą logikę poszukiwania GUIDa pod deklaracją “private string ExtractId(string source)“.
Ale czy naprawdę klasa reprezentująca jakiś obiekt w systemie powinna się takimi pierdołami zajmować? Najprawdopodobniej NIE! Wyrzućmy ten kod na zewnątrz, do, dajmy na to, FimReferenceIdExtractor. Tam nie będzie to już metodą prywatną – tam będzie to główną odpowiedzialnością klasy. I metoda “ExtractId” nie będzie prywatnym helperem, spychanym na sam dół pliku i chowanym w jakichś regionach jak bękart na królewskim dworze. Będzie po prostu częścią interfejsu! A co za tym idzie: idealnym kandydatem do przetestowania.
Do głowy przychodzą mi też momentalnie moje dawne porażki z testami kończące się stwierdzeniem:
– “moje rozwiązanie jest na tyle nietypowe że nie da się go przetestować”
– Dlaczego?
– “Bo nie umiem przetestować obsługi zdarzeń w *.aspx albo Form*.cs”
– Po pierwsze: co w tym nietypowego (niestety)? A po drugie: po co chcesz testować obsługę zdarzeń w tych plikach?
– “Bo tam mam najwięcej kodu”
– …
No i właśnie. Wtedy siedziałem całymi dniami i drapałem się po łbie kombinując jak to możliwe, że akurat MÓJ kod nie nadaje się do testowania. Dziś powiedziałbym sobie bardzo dosadnie: wyciągnij-że ten kod do klas, które naprawdę powinny go zawierać!
Z dużą dozą prawdopodobieństwa można stwierdzić, że prywatne metody wymagające osobnych testów łamią zasadę SRP i powinny być przeniesione do dedykowanego dla nich miejsca.
Bardzo fajnie ujął to w komentarzu @rek:
“Metod prywatnych nie testujemy, jeśli uważasz, że metoda prywatna powinna być otestowana to znaczy że zrobiłeś coś źle…. refactor it….. wyciągnij do osobnej klasy (SRP) wraz z testami”
A dobitniej podchwycił Paweł:
“Masz cos prywatnego, co musisz zewnetrznie przetestowac -> zabrnales w guwno (moze nawet po pas) -> refaktoryzuj ASAP“.
Może jeszcze ktoś coś doda, czy osiągnęliśmy stan globalnego współdzielenia poglądu, co w tym kraju jest sztuką niemałą?:)
Pomijając oczywiste (?) korzyści wynikające z wyciągnięcia logiki zakodowanej w prywatnej metodzie (albo raczej metodach) do zewnętrznej klasy; to jaką w tym przypadku mamy przewagę nad upublicznieniem tej prywatnej metody stricte do testów bezpośrednio? W przypadku klasy też to robimy tylko na innym poziomie. W obydwu tych przypadkach przynajmniej w .NET można użyć modyfikatora internal w połączeniu z atrybutem inernalsvisibleto i to chyba byłoby moje preferowane rozwiązanie (oczywiście jak najbardziej jestem za umieszczaniem wewnętrznej logiki gdzie faktycznie ona należy).
@daczkowski: SRP jest odpowiedzią na Twoje pytanie. Jeśli chodzi o modyfikator ‘internal’ lub atrybut ‘internalvisibleto’ to pierwsze wymusza by testy były w bibliotece z logiką (fuj!) a drugi tak samo tylko niejawnie. Pierwsze powoduje, że biblioteka rośnie i musisz całą przekazać klientowi (chyba że użyjesz ‘#if Debug’ ale to tylko zaśmiecanie kodu).
SRP jasne – tylko ja pytam, co jeśli mimo to nie chcesz, żeby akurat ta część była publicznym API dostępnym dla klientów biblioteki. Oczywiście można to udokumentować (lub właśnie nie dokumentować), ale internal (na klasach) + internalsvisibleto pozwala to zrobić explicite. I tak też mi się internalsvisibleto nie podoba, ale jest chyba mniejszym złem.
daczkowski,
Stosowałem kiedyś InternalsVisibleTo (nawet pierwszy post na tym blogu był o tym właśnie:) http://www.maciejaniserowicz.com/post/2008/02/26/Model-View-Controller-i-testy-jednostkowe.aspx )…
Jednak teraz właściwie nigdy nie widzę już takiej potrzeby. Wszelkie internale tak mi działają na nerwy w codziennym życiu (chociaż rozumiem argumenty za ich wykorzystaniem), że tego nie stosuję. Ale z drugiej strony – nie tworzę frameworków/kodu wykorzystywanego publicznie przez osoby trzecie, więc API moich dllek i tak ma scope 1 projektu.
Jak zwykle będę w mniejszości.
Globalne współdzielenie poglądu? Wszystko co globalne jest złe. ;)
[quote]oczywiste (?) korzyści wynikające z wyciągnięcia logiki zakodowanej w prywatnej metodzie (albo raczej metodach) do zewnętrznej klasy[/quote]
Pierwszą korzyścią będzie zapewne więcej klas w aplikacji. To może być ogromna korzyść, jeśli pracodawca płaci nam od ich ilości. W przeciwnym wypadku tworzenie klas po to, by je mieć jest mocno wątpliwe. No chyba, że ktoś lubi mieć w aplikacji milion helperów, żeby gorzej mu się czytało kod i zarządzało projektem.
Warto wydzielać odrębne odpowiedzialności, ale bez naciągania.
Jest takie powiedzenie "nigdy nie mów nigdy". Zapewne 99% metod prywatnych nie trzeba testować, ale zawsze zostaje jeszcze 1%.
Przykład? Klasa obliczająca wektory i wartości własne macierzy algorytmem Jacobiego. Oczywiście klasa musi zawierać publiczną metodę [i]Calculate[/i] zwracającą wynik. Algorytm wymaga w każdym kroku znalezienia indeksów po pierwsze największego elementu leżącego na diagonali, a po drugie największego leżącego poza diagonalą. Oczywiste jest, że warto wydzielić te dwie operacje to oddzielnych, pomocniczych metod prywatnych.
Można tych metod nie testować, bo przecież [quote]i tak zostaną "niejawnie" przetestowane poprzez testy metod publicznych[/quote]. To niewątpliwie prawdziwe stwierdzenie. Ale czy takie podejście jest wygodne? Jeśli ktoś wie, że od razu wszystko dobrze zaimplementuje, nie będzie potrzebował debugować i poprawiać kodu, to pewno tak. Z drugiej strony, skoro ktoś od razu wszystko dobrze implementuje, to po co mu w ogóle testy? :) Ja jednak wolę poświęcić 15 minut na napisanie testów do tych pomocniczych metod, tylko po to, żeby mieć pewność, że jeśli główna metoda zwróci zły wynik, to nie z powodu jakiejś pobocznej pierdoły tylko zasadniczej części algorytmu. Kurde, przecież po to właśnie są testy jednostkowe, żeby sobie pomóc, czyż nie?
Można te metody niby wyrzucić do oddzielnej klasy, tylko po co? Nigdy poza tym algorytmem nie będą potrzebne, jaki jest sens mnożyć klasy w aplikacji? Jak napisałem wyżej, korzyści w tym nie ma.
Przy implementacji wielu innych obliczeń matematycznych (o matematykę mi chodzi, a nie o sumowania pozycji faktury) znajdzie się więcej przykładów metod prywatnych, które warto testować.
[i]InternalsVisibleTo[/i] to chyba jakaś bajka o żelaznym wilku. W VS (przynajmniej 2010) wystarczy kliknąć "CreateUnitTests", wybrać metody (także prywatne), parę razy OK/Next i IDE wygeneruje dla klasy wrappera (z sufiksem _Accessor), z dostępem do metod prywatnych ukrywanej klasy i to wszystko, można już pisać testy.
Chyba, że ktoś wybitnie nie lubi MSTesta, no ale czyj to wtedy problem? :)
oo…… zostałem odfiltrowany :/
@rek,
Jak to odfiltrowany? W panelu komentarzy nie mam żadnej treści od Ciebie zablokowanej. Więcej: system dodał Cię do "white list"…
w takim razie gdzieś wessało komenatrz z linkiem do rozmowy Uncle Boba na temat testowania metod prywatnych – bywa. Odszukam i wyślę jeszcze raz
Opinia Roberta C. Martina na temat testowania metod prywatnych http://vimeo.com/20388419 (od 39 minuty)
procent: no i sprawa się wyjaśniła, zaginiony komentarz o którym pisałem wcześniej jest tutaj http://www.maciejaniserowicz.com/post/2011/12/08/UT-5-Kiedy-testowac.aspx najwyraźniej późno już było i się mi okna pomyliły. Mea culpa
@somekind: budując zaproponowaną przez Ciebie klasę, już na początku wydzieliłbym mechanizmy odpowiedzialne za rozbijanie macierzy na trzy wymagane przez metodę Jacobiego (niewiele pamiętam już z algebry, opis metody znalazłem w sieci). Umieściłbym je jako metody statyczne powiedzmy w klasie … Transformations przestrzeni nazw Matrix ;). Byłyby one oczywiście publiczne (a może byłaby tylko jedna metoda, ale zwracająca kolekcję trzech macierzy?). I to dla klasy Transformations powstałby pierwszy test. Dopiero jego zaliczenie upoważniałoby mnie do napisania klasy właściwej z jedną metodą [i]Calculate[/i].
Jak widać nie ma ani przerażającego przyrostu klas, ani konieczności testowania metod prywatnych, bo tak naprawdę nie są one odpowiedzialnością klasy obliczającej wektory i wartości własne. Ona z nich korzysta. Gdyby zaś przyszło pisać kolejne mechanizmy związane z transformacjami macierzy dodałbym je także do klasy Transformations (liczba klas by nie wzrosła).
Aha. Klasy tworzy się nie po to, aby je mieć, ale po to by mieć … porządek w kodzie :P.
Jeszcze korekta dla autora wpisu. O ile dobrze zrozumiałem, to w trzecim akapicie zamiast [i]to by nie była publiczna![/i], powinno być: [i]to by nie była [u]prywatna[/u]![/i]
PaSkol,
Thx, poprawione
@PaSkol,
Nazwanie przestrzeni nazw Matrix w programie numerycznym, to byłby chyba strzał w stopę, bo znacznie utrudniłoby korzystanie z klasy Matrix. ;)
Co do zasadniczej części postu – jeśli chodzi Ci o macierze wyznaczane w każdej iteracji (wg mojej wiedzy oznaczane przez A i W), to one są po prostu wynikiem prostych mnożeń, i nie potrzebują oddzielnych metod pomocniczych. Te są potrzebne, jak już pisałem, do przeszukiwania macierzy w poszukiwaniu największego elementu i zwrócenia jego indeksów. Ok, można zrobić klasę MatrixSearcher z metodami FindElementByCośtam1 i 2, ale jaki to ma sens, jeżeli metody te (a zarazem i cała klasa) byłyby używane tylko przez jedną metodę klasy JacobiSolver?
Moim zdaniem jest to działanie mocno na wyrost, mnożenie niepotrzebnych klas i śmiecenie w Intellisense. ;)
Oczywiście, wydzieliłbym je do oddzielnej klasy, gdybym miał jakiekolwiek podstawy sądzić, że użyję ich jeszcze gdzieś indziej. Być może, gdybym potrzebował przeszukiwać macierze na różne sposoby, to też umieściłbym wszystkie metody szukające w jednej pomocniczej klasie. Ale w tym przypadku takich potrzeb po prostu nie ma.
Tak, porządek w kodzie – o to tu chodzi. Moim zdaniem w takim przypadku dodatkowa klasa pomocnicza stworzona tylko po to, aby nie testować metod prywatnych, jest właśnie przykładem nieporządku.
somekind,
Nie mogę się zgodzić z tym rozumowaniem. Duża liczba klas nie ma negatywnego przełożenia na organizacją kodu. Gdyby tak było to każdy program posiadałby jedną klasę i wszyscy byliby super-zorganizowani. Klasa nie jest też worem na "reużywalny" kod i nowej klasy nie tworzy się tylko po to, aby wykorzystać jej funkcjonalność w kilku miejscach.
Pytanie: jak przetestować metodę X w projekcie, który jest biblioteką.
Sytuacja: mam DLL z publicznym interfejsem do WebService’u: GetItem(), SaveItems(Item[] items), gdzie Item jest skomplikowaną klasą typu DTO. Jedno z pól tej klasy to SN.
Przed wysłaniem do WS pole SN musi być walidowane gość skomplikowanym algorytmem, który jest w osobnej klasie typu internal SNHelper. Klasa ta nie może być publiczna!
X = SNHelper.Validate()
Nadmienię, że testowanie przez wywołanie SaveItems() jest zbyt skomplikowane, ponieważ walidacja SN to tylko czubek góry lodowej.
Możliwe rozwiązania – bez faworyzowania:
a) kompilacja warunkowa dla testów, która upublicznia SNHelper;
b) odwołanie przez reflection;
c) ?