Jak przetestować metodę która zmienia prywatne pole?

Trochę wstyd, ale złapałem się na tym, że do końca nie wiem jak mogę dobrze napisać testy jednostkowe do metod które modyfikują prywatne pole.

SUT:

private List<ComponentModel> _indexComponents = new List<ComponentModel>();

public List<ComponentModel> GetIndexComponents(string indexName)
{
    return
        _indexComponents.Any(x => x.IndexName == indexName) ?
        _indexComponents.Where(x => x.IndexName == indexName).ToList() :
        new List<CompanyModel>();
}

public void SetIndexComponents(List<ComponentModel> components, string indexName)
{
    if (_indexComponents.Any(x => x.IndexName == indexName))
    {
        _indexComponents.RemoveAll(x => x.IndexName == indexName);
    }

    _indexComponents.AddRange(components);
}

Test:

[Fact]
private void SetIndexValue_SetsTheValue()
{
    var value = 12.00M;
    var indexName = "XXX";

    _service.SetIndexValue(value, indexName);
    var result = _service.GetIndexValue(indexName); //żeby przetestować poprawność działania SetIndexValue muszę skorzystać z GetIndexValue :/

    var expected = value;
    Assert.Equal(expected, result);
}

Czy private List<ComponentModel> _indexComponents = new List<ComponentModel>(); powinien być tak naprawdę własnością publiczną, żebym mógł poprawnie przetestować metodę? Czy to dobra praktyka tak upubliczniać pola?

W ogóle, bracie, jeżeli nie masz na utrzymaniu rodziny, nie grozi ci głód, nie jesteś Tutsi ani Hutu i te sprawy, to wystarczy, że odpowiesz sobie na jedno zajebiście, ale to zajebiście, ważne pytanie:

Co chcę tu tetować. A potem zacznij to tetować.

Czy na pewno chcesz testować co jest dokładnie w tym prywatnym polu, skoro jest prywatne? Nie wystarczy testować oczekiwanych zachowań kompletu metod set i get?

Ale po co to w ogóle testować? Co cię obchodzi co klasa sobie robi w środku, testować masz jej zachowanie na zewnątrz. A prywatne pola i metody to często ukryte klasy.

W dodatku kod niepotrzebnie skomplikowany i iterujący wielokrotnie po kolekcjach bez sensu, można uprościć do:

private List<ComponentModel> _indexComponents = new List<ComponentModel>();

public List<ComponentModel> GetIndexComponents(string indexName) => _indexComponents.Where(x => x.IndexName == indexName).ToList();

public void SetIndexComponents(List<ComponentModel> components, string indexName)
        {
            _indexComponents.RemoveAll(x => x.IndexName == indexName);
            _indexComponents.AddRange(components);
    }

Jak już musisz coś upublicznić dla testów (czasem tak jest dużo łatwiej, np wyłączyć w ten sposób timeouty do debugowania) to częstą praktyką jest użycie internal i InternalsVisibleTo

Testy jednostkowe, które sięgają do czegokolwiek prywatnego to testy, które utrudniają utrzymanie kodu.
Celem testów jest sprawdzanie zewnętrznie widocznej funkcjonalności, a nie detali implementacyjnych.
Tak napisane testy potem blokują refactoring kodu i stają się kulą u nogi.

Zalecałbym pisanie najpierw testów, a potem kodu produkcyjnego. Coś w miarę zbliżonego to TDD lub BDD, wtedy nie piszę się testów naruszających prywatność.

Ok, do tej pory żyłem w przekonaniu, że jeśli znajduje się logika biznesowa w metodzie która jest publiczna, to należy ją przetestować. Jeśli nie w SetIndexComponents, to w GetIndexComponents jest Where, które mimo wszystko Daje możliwość zwrócenia różnych wartości. Czy chociaż ona powinna być przetestowana?

W ogóle nie powinieneś pisać testu pod tą klasę.

Znajdź miejsce gdzie jej używasz, i w tym miejscu napisz test.

bakunet napisał(a):

Czy private List<ComponentModel> _indexComponents = new List<ComponentModel>(); powinien być tak naprawdę własnością publiczną, żebym mógł poprawnie przetestować metodę? Czy to dobra praktyka tak upubliczniać pola?

Nie i Nie.

Po pierwsze dlatego że to nie ma sensu, po drugie dlatego że złamiesz wtedy enkapsulację, po trzecie że wtedy testy staną się bardziej podatne na false negative, po czwarte dlatego że wtedy refaktor tej klasy będzie trudniejszy, a po piąte dlatego że nawet jak wystawisz to pole, to nic to nie da, bo i tak faktyczny kod tej klasy nie będzie przetestowany tylko to pole - a to wnosi wartość zerową.

To napisz taki zestaw testów żeby pokryć wszystkie możliwe przypadki wynikające z logiki wewnątrz get i set.
Dalej da się to zrobić bez upubliczniania listy.

opiszon napisał(a):

To napisz taki zestaw testów żeby pokryć wszystkie możliwe przypadki wynikające z logiki wewnątrz get i set.
Dalej da się to zrobić bez upubliczniania listy.

Ok, po uwadze @obscurity przyznaję, że w SetIndexComponents nie ma logiki. A w testach GetIndexComponents można użyć SetIndexComponents do zapełnienia listy. Nic innego nie przychodzi mi do głowy

bakunet napisał(a):

opiszon napisał(a):

To napisz taki zestaw testów żeby pokryć wszystkie możliwe przypadki wynikające z logiki wewnątrz get i set.
Dalej da się to zrobić bez upubliczniania listy.

Ok, po uwadze @obscurity przyznaję, że w SetIndexComponents nie ma logiki. A w testach GetIndexComponents można użyć SetIndexComponents do zapełnienia listy. Nic innego nie przychodzi mi do głowy

Pokaż miejsce gdzie używasz tych metod.

bakunet napisał(a):

Ok, do tej pory żyłem w przekonaniu, że jeśli znajduje się logika biznesowa w metodzie która jest publiczna, to należy ją przetestować.

No to bardzo błędne przekonanie, to znaczy częściowo prawdziwe. Kod powinien być przetestowany ale nie bezpośrednio pisząc testy specjalnie do tej klasy, tylko któryś z testów powinien zahaczyć o tę klasę i z niej skorzystać. Pokrycie testami powinno być idealnie 100% a jeśli testy w jakiś sposób nie odpalają kodu to znaczy że jest bezużyteczny i wypada go wywalić.

Tak jak wyżej - nie pisz testów do każdej klasy i metody, pisz testy do każdej funkcjonalności biznesowej. Czyli potrzebujesz prawdopodobnie testu do klasy która korzysta z tej klasy, wtedy pokrywasz testami je obie. Pisanie testów do wszystkiego osobno niepotrzebnie betonuje kod, zniechęca do testów, zachęca do mockowania wszystkiego i jest zazwyczaj bezsensowne i nie przynosi żadnych korzyści. Idealnie jest NIE używać mocków zupełnie, a jeśli już to tylko do zewnętrznych zależności a nie do własnego kodu.

Riddle napisał(a):

Pokaż miejsce gdzie używasz tych metod.

public bool VerifyIfComponentsExists(int year, int month, int day, string indexName)
{
  var components = new List<ComponentModel>();

  components = _appState.GetIndexComponents(indexName);

  return _resultsProcessor.VerifyIfComponentsExists(components, year, month, day);
}

bakunet napisał(a):

Riddle napisał(a):

Pokaż miejsce gdzie używasz tych metod.

public bool VerifyIfComponentsExists(int year, int month, int day, string indexName)
{
  var components = new List<ComponentModel>();

  components = _appState.GetIndexComponents(indexName);

  return _resultsProcessor.VerifyIfComponentsExists(components, year, month, day);
}

No to z kolei teraz pokaż gdzie używasz tej metody?

Riddle napisał(a):

No to z kolei teraz pokaże gdzie używasz tej metody?

bool alreadyExistsInAppState = _dataDistributor.VerifyIfComponentsExists(utcNowTimestamp.Year, utcNowTimestamp.Month, utcNowTimestamp.Day, indexName);

if (!alreadyExistsInDb && !_dataDistributor.GetAppStateIsComponentsUpdateInProgress())
{
  (...)
}

Chyba już rozumiem do czego zmierzasz (zmierzacie). Będę musiał się z tym przespać :)

bakunet napisał(a):

Riddle napisał(a):

No to z kolei teraz pokaże gdzie używasz tej metody?

bool alreadyExistsInAppState = _dataDistributor.VerifyIfComponentsExists(utcNowTimestamp.Year, utcNowTimestamp.Month, utcNowTimestamp.Day, indexName);

if (!alreadyExistsInDb && !_dataDistributor.GetAppStateIsComponentsUpdateInProgress())
{
  (...)
}

No to teraz się zastanów, w jaki sposób Twoje prywatne pole wpływa na wynik .VerifyIfComponentsExists() - dla jakich stanów tego prywatnego pola, ta metoda zwróci true i false. Co by się stało, gdybyś usunął swoje prywatne pole? Jaki test mógłbyś napisać, który wykryłby czy to pole tam jest czy nie. Jaki bug powstałby w aplikacji, gdyby tego pola nie było?

Takie pytania musisz sobie zadać żeby napisać dobre testy.

Riddle napisał(a):

Takie pytania musisz sobie zadać żeby napisać dobre testy.

Tak więc, jeśli GetIndexComponents zwróci złą wartość, to będzie błąd. Jeśli SetIndexComponents zapisze złą wartość, będzie błąd. Choć już zadążyliśmy ustalić, że z SetIndexComponents można się pozbyć ifologii i testy dlań są zbędne.

bakunet napisał(a):

Riddle napisał(a):

Takie pytania musisz sobie zadać żeby napisać dobre testy.

Tak więc, jeśli GetIndexComponents zwróci złą wartość, to będzie błąd. Jeśli SetIndexComponents zapisze złą wartość, będzie błąd.

Nie chodzi o wyjątki i błędy.

Chodzi o to w jaki sposób Twoja aplikacja działa, i zachowa się zależnie od tego czy ten .VerifyIfComponentsExists() zwróci true lub false. Pisząc testy, nie podchodź do aplikacji z myślą "błąd lub nie błąd", tylko w kontekście zachowania - co ta aplikacja zrobi.

@Riddle: Ok, dalej nie kumam czemu nie mam testować GetIndexComponents skoro niepoprawna wartość alreadyExistsInAppState wpłynie na prawidłowe funkcjonowanie aplikacji. Co z tego, że przetestuję przypadki true i false dla alreadyExistsInAppState , skoro GetIndexComponents będzie mi zwracała niepoprawne wartości?

bakunet napisał(a):

@Riddle: Ok, dalej nie kumam czemu nie mam testować GetIndexComponents skoro niepoprawna wartość alreadyExistsInAppState wpłynie na prawidłowe funkcjonowanie aplikacji. Co z tego, że przetestuję przypadki true i false dla alreadyExistsInAppState , skoro GetIndexComponents będzie mi zwracała niepoprawne wartości?

No to napisz test, który w given setupuje taki przypadek, w którym GetIndexComponents zwróci błędne wartości, i napisz asercję pod to alreadyExistsInAppState.

@Riddle: Zakładając że if (!alreadyExistsInDb && !_dataDistributor.GetAppStateIsComponentsUpdateInProgress()) to jest jedyna logika w metodzie która woła wymienione wcześniej serwisy, to co jest złego z

(np tylko jeden test pod true, i jeden pod false)

i oczywiście wszystkimi kombinacjami z !_dataDistributor.GetAppStateIsComponentsUpdateInProgress())?

bakunet napisał(a):

@Riddle: Zakładając że if (!alreadyExistsInDb && !_dataDistributor.GetAppStateIsComponentsUpdateInProgress()) to jest jedyna logika w metodzie która woła wymienione wcześniej serwisy, to co jest złego z

(np tylko jeden test pod true, i jeden pod false)

Na to pytanie sam sobie odpowiedziałeś:

• bakunet napisał(a):

  Co z tego, że przetestuję przypadki true i false dla alreadyExistsInAppState , skoro GetIndexComponents będzie mi zwracała niepoprawne
  wartości?

  Musisz przetestować wszystkie przypadki wejściowe, nawet jeśli jest ich więcej niż wyjściowych (czyli w tym wypadku true i false).

bakunet napisał(a):

i oczywiście wszystkimi kombinacjami z !_dataDistributor.GetAppStateIsComponentsUpdateInProgress())?

Nie koniecznie z wszystkimi. Przetestuje je z tyloma kombinacjami, ile jest konieczne żeby się upewnić że kod działa tak jak powinien.

//żeby przetestować poprawność działania SetIndexValue muszę skorzystać z GetIndexValue :/

no to co? to źle?

masz API która wystawia dwie metody - Read/Write, no i skąd jakiś religijny pomysł ze do testu metody Write nie możesz użyć Read?

to jest to publiczne API, z niego będą inni korzystać, to API ma działać. A jako że logika w tym kodzie nie jest skomplikowana, ani nie ma jakichś niepewności (losowość, czas, itd itd) to nie ma jakichś szczególnych powodów aby dobierać się do bebechów czy aby na pewno tam jest to, co ma być, bo zakładasz że metoda Read z jakiegoś powodu zwróciłaby coś innego

sprawdź na kilku wartościach czy następuje zmiana i tyle

return
  _indexComponents.Any(x => x.IndexName == indexName) ?
  _indexComponents.Where(x => x.IndexName == indexName).ToList() :
  new List<CompanyModel>();

a czy tak właściwie to ci się nie upraszcza do

return _indexComponents.Where(x => x.IndexName == indexName).ToList();

MarekR22 napisał(a):

Testy jednostkowe, które sięgają do czegokolwiek prywatnego to testy, które utrudniają utrzymanie kodu.
Celem testów jest sprawdzanie zewnętrznie widocznej funkcjonalności, a nie detali implementacyjnych.
Tak napisane testy potem blokują refactoring kodu i stają się kulą u nogi.

Dokładnie z tych samych powodów rozszerzam to podejście, aby w testach jednostkowych nie testować logiki biznesowej, bo to tak samo głupie jak testowanie prywatnej. Po co test jednostkowy ma przetwarzać model skoro przy zmianie modelu trzeba ten test poprawiać?

W testach jednostkowych testuje rzeczy niezależne od biznesu, wtedy weryfikuje podwaliny na jakich buduje funkcjonalność. Testem wówczas dokumentuje działanie kodu i upewniam się, że podwaliny działają zgodnie z założeniem. Z tego powodu nie martwię się o to czy test będzie problemem w utrzymaniu, ponieważ szanse na zmiany są marginalne. Natomiast konkretne funkcjonalności testuje na możliwie najwyższym poziomie np. w selenium, i zwykle tak bywa, że robię to w innym języku niż tym którego używam do implementacji aplikacji. Oczywiście testy funkcjonalności muszę zmieniać, gdy zmieniają się założenia.

EDIT:

Oczywiście zaraz ktoś powie, że testy funkcjonalności to przecież testy jednostkowe, bo testy jednostkowe nie testują przecież metod, klas, a pojedynczą funkcjonalność, która może wymagać użycia kilku klas czy metod. Jaka by nie była definicja testu jednostkowego to warto, aby test był określony na jak najwyższym poziomie (wspomniane selenium lub chociaż api), aby było jak najmniej styczności z modelami. Wiadomo, że będzie jakaś styczność, ale im wyżej rozpocznie się testowanie tym mniej widocznych jest zależności z modelami do pokrycia wymaganych testów.

Doceniam krytyczne opinie, dzięki. Przetrawię je jak będę miał dłuższą chwilę. Możliwe że tu później wrócę z kotrargumentami / pytaniami :)

masz API która wystawia dwie metody - Read/Write, no i skąd jakiś religijny pomysł ze do testu metody Write nie możesz użyć Read?

@WeiXiao: Istnieje praktyczny powód. Test który używa read do sprawdzenia write jest tym samym co test używający write do sprawdzenia read. Oczywiście przy bardzo prostych implementacjach to nie jest problem, ale przy nietrywialnych fajnie byłoby wiedzieć, czy zawiódł read czy write, a z takiego testu się nie dowiesz. I nie jest to mój wymysł teoretyczny, tylko nie dalej jak miesiąc temu mieliśmy w firmie realny przypadek, gdzie błąd był w "read", a większość ludzi początkowo pomyślała, że błąd był w "write" i że mamy uszkodzone pliki z danymi. Przy czym read i write to w tym przypadku kobyły na kilka tys. linii. ;) Bo wszystkie weryfikatory tychże plików używały tego samego kodu do odczytu. Kodu, który miał błąd.

Dlatego generalnie przy bardziej złożonym kodzie / złożonych strukturach danych jak najbardziej jest sens dorzucania do kodu dodatkowych metod diagnostycznych używanych wyłącznie w testach, tudzież asercji weryfikujących poprawność stanu wewnętrznego. Asercje też są pewną formą testowania, a działają na prywatnych danych. Przykładowo - jeśli mamy strukturę drzewa zrównoważonego, to chcielibyśmy zapewne przetestować, czy faktycznie jest zrównoważone po każdej operacji, chociaż w publicznym API drzewo nie będzie udostępniać metod do oceny stopnia zrównoważenia (skoro obiecuje że zawsze jest zrównoważone), bo taka metoda raczej byłaby bezużyteczna. Dlatego powinniśmy mieć możliwość:

• dostępu do prywatnych bebechów drzewa z testów, albo
• napisania metody diagnostycznej udostępniającej pewne metryki jak np. stopień niezrównoważenia drzewa i użycie jej w testach (to podejście wolę bardziej)

Testy jednostkowe, które sięgają do czegokolwiek prywatnego to testy, które utrudniają utrzymanie kodu.
Celem testów jest sprawdzanie zewnętrznie widocznej funkcjonalności, a nie detali implementacyjnych.

@MarekR22: Z tym się nie mogę zgodzić. Utrudniają modyfikację detali implementacyjnych, ale za to precyzyjniej lokalizują błąd, oraz ułatwiają zrozumienie implementacji (testy są też formą dokumentacji). Jako że kod częściej się wykonuje / czyta niż modyfikuje, zwłaszcza implementacje, to uważam że wartość którą dodają takie testy jest większa niż ich wada. Testy implementacji też można pisać tak, aby zminimalizować ryzyko konieczności ich zmieniania - np. powinny testować niezmienniki / fundamentalne założenia projektowe dla danego modułu a nie detale. Tak jak wyżej napisałem, jeśli fundamentalną cechą drzewa zrównoważonego jest to że jest zrównoważone, to powinno to być przetestowane, nawet jeśli wymaga to dostępu do wewnętrznej struktury drzewa.

BTW: I przestańmy patrzeć na wszystko przez pryzmat Javy/C#. To dział Inżyniera oprogramowania.
Nie wszystko musi być podzielone na klasy, i nie każdy język ma takie głupie ograniczenia że pola mogą być tylko prywatne bądź publiczne i nic pomiędzy.
To co jest publiczne dla modułu X, może być prywatne z punktu widzenia modułu Y. Dlatego to jest kwestia względna. Są po prostu różne poziomy abstrakcji na której działają testy i warto korzystać z wielu, bo to jest pewien trade-off. Testy end-to-end są skrajnym przypadkiem testów testujących tylko to co naprawdę publiczne. Ale one mają znikomą wartość diagnostyczną zwykle, choć mają dużą siłę wykrywania błędów.

Krolik napisał(a):

masz API która wystawia dwie metody - Read/Write, no i skąd jakiś religijny pomysł ze do testu metody Write nie możesz użyć Read?

@WeiXiao: Istnieje praktyczny powód. Test który używa read do sprawdzenia write jest tym samym co test używający write do sprawdzenia read. Oczywiście przy bardzo prostych implementacjach to nie jest problem, ale przy nietrywialnych fajnie byłoby wiedzieć, czy zawiódł read czy write, a z takiego testu się nie dowiesz. I nie jest to mój wymysł teoretyczny, tylko nie dalej jak miesiąc temu mieliśmy w firmie realny przypadek, gdzie błąd był w "read", a większość ludzi początkowo pomyślała, że błąd był w "write" i że mamy uszkodzone pliki z danymi. Przy czym read i write to w tym przypadku kobyły na kilka tys. linii. ;) Bo wszystkie weryfikatory tychże plików używały tego samego kodu do odczytu. Kodu, który miał błąd.

Dlatego generalnie przy bardziej złożonym kodzie / złożonych strukturach danych jak najbardziej jest sens dorzucania do kodu dodatkowych metod diagnostycznych używanych wyłącznie w testach, tudzież asercji weryfikujących poprawność stanu wewnętrznego. Asercje też są pewną formą testowania, a działają na prywatnych danych. Przykładowo - jeśli mamy strukturę drzewa zrównoważonego, to chcielibyśmy zapewne przetestować, czy faktycznie jest zrównoważone po każdej operacji, chociaż w publicznym API drzewo nie będzie udostępniać metod do oceny stopnia zrównoważenia (skoro obiecuje że zawsze jest zrównoważone), bo taka metoda raczej byłaby bezużyteczna. Dlatego powinniśmy mieć możliwość:

• dostępu do prywatnych bebechów drzewa z testów, albo
• napisania metody diagnostycznej udostępniającej pewne metryki jak np. stopień niezrównoważenia drzewa i użycie jej w testach (to podejście wolę bardziej)

Noo, nie zgadzam się, z kilku powodów.

• Po pierwsze, jeśli masz metody write i read, i załóżmy dla uproszczenia że to są jedyne akcje jakie wystawia system, to nie potrzebujesz dwóch testów (osobnego dla read() i osobnego dla write()), tylko wystarczy jeden, np write(x); assert(x, read());. Tzn możesz niby napisać dwa, ale one będą identyczne; skoro jedyna szansa żeby sprawdzić jedną metodę, jest użycie drugiej.

  • Odpowiadając na cytat od @Krolik

    Krolik napisał(a):

    ale przy nietrywialnych fajnie byłoby wiedzieć, czy zawiódł read czy write, a z takiego testu się nie dowiesz.

  • Takie podejście nie jest dobre - takie "wiedzenie" która metoda zawiódła. Bo jeśli masz jeden test który robi read i write, i to są Twoje jedyne access pointy do aplikacji, to nie istnieje obiektywny sposób żeby stwierdzić czy zawiódł read czy write. Kontrakt tych dwóch metod jest taki że read() ma zwrócić to co write() wsadził i tyle, ale to w jaki sposób to robią to powinien być szczegół implementacyjny.

  • Więc z punktu widzenia dobrych testów i TDD; pomysł żeby "wiedzieć która metoda zawiodła" nie specjalnie ma rację bytu. Nie istnieje żaden sensowny testu który usprawiedliwiłby taki koncept, i istnieć nie może.

  • Ciche założenie, jak rozumiem, to np takie założenie że write() i read() mają zapisać coś do bazy, powiedzmy. I wtedy faktycznie, na dwa sposoby taki test może sfailć. Albo może być błąd INSERT (bo np się nie dodaje, albo dodaje pod złym id), albo błąd może być w SELECT (bo np wyciąga po złym id, albo robi WHERE który zwraca pusty result).

  • W takim wypadku musisz sobie odpowiedzieć na jedno ważne (ale to zajefajnie ważne ;) pytanie: Czy baza jest Twoim szczegółem implementacyjnym, czy częścią interfejsu.

    • Jeśli baza jest szczegółem implementacyjnym, to żaden test sprawdzający "która metoda sfailowała" nie może istnieć, i ten INSERT i SELECT należy traktować jako "jeden byt", nawet jeśli jest w dwóch metodach (bo od obu z nich zależy czy operacja się uda czy nie; bez sensu jest próbować je rozdzielać i myśleć o nich jak o "dwóch metodach"). Więc dwie metody write() i read() należy traktować jako jedną funkcjonalność, która albo działa w całości, albo nie działa. Z punktu widzenia testów, powiedzenie "write" nie działa, albo "read" nie działa nie ma sensu; bo ich "nie działanie" widać tylko poprzez tą drugą metodę. Nie możesz wykonać write() i stwierdzić czy działa czy nie działa.
    • Jeśli natomiast baza jest interfejsem, to musisz napisać po każdym teście asercję z bazy która robi selecta i sprawdza dokładnie co w nie jest; tylko wtedy nie możesz nigdy mieć testu który woła read() i write() razem - bo wtedy one już nie są swoją częscią, tylko oba ciągną z bazy.

• Po drugie, wystawianie metryk albo metod diagnozujących, moim zdaniem jest średnie, bo nawet takie metody diagnozujące mogą wystawić złe dane;

  • więc test który przechodzi dla takich metod diagnozujących, nawet jesli przejdzie; to nie koniecznie znaczy to że faktyczne dane używane przez system również są poprawne. Żeby się tego upewnić należy napisać dobry test dla tego systemu; i wtedy takie funkcje diagnozujące są niepotrzebne.
  • Pomijam to ze takie funkcje diagnozujące łamią enkapsulację i utrudniają późniejszą pracę z systemem

Takie podejście nie jest dobre - takie "wiedzenie" która metoda zawiódła. Bo jeśli masz jeden test który robi read i write, i to są Twoje jedyne access pointy do aplikacji, to nie istnieje obiektywny sposób żeby stwierdzić czy zawiódł read czy write. Kontrakt tych dwóch metod jest taki że read() ma zwrócić to co write() wsadził i tyle, ale to w jaki sposób to robią to powinien być szczegół implementacyjny.

Na tym polega różnica między podejściem teoretyka, a praktyka. W teorii akademickiej masz rację. Praktyka tworzenia oprogramowania to coś znacznie więcej niż tylko aby read przeczytał to co zrobił write.
W praktyce takie write może mieć znacznie bardziej rozbudowany kontrakt. Choćby ze względu na kompatybilność wsteczną i dalszy rozwój systemu. Jeżeli ktoś będzie pracował w przyszłości na tym kodzie i w projekcie ma założenia, że np. wewnętrzna struktura zapisywana przez write ma mieć posortowane kolumny wg jakiegoś porządku, to będzie na tej własności polegał pisząc nowy kod. I ta własność powinna być przetestowana. A jak ja spieprzy dodając nowy kod, to ma dostać testami na czerwono. Testami wskazującymi, że naruszył jakiś fundament write.

Więc z punktu widzenia dobrych testów i TDD; pomysł żeby "wiedzieć która metoda zawiodła" nie specjalnie ma rację bytu. Nie istnieje żaden sensowny testu który usprawiedliwiłby taki koncept, i istnieć nie może.

Zalatuje religią i dogmatami.
Jak mi się test wywali, to chcę wiedzieć gdzie poprawiać. Odrzucenie 50% kodu na starcie to duży zysk czasu.

W takim wypadku musisz sobie odpowiedzieć na jedno ważne (ale to zajefajnie ważne ;) pytanie: Czy baza jest Twoim szczegółem implementacyjnym, czy częścią interfejsu.

Nie muszę, bo zarówno jedno jak i drugie warto testować. Z innych powodów, ale warto.
Poza tym jak napisałem wyżej, to nie jest podział czarno-biały. Są wsystkie odcienie szarości pomiędzy. Coś co jest interfejsem dla modułu X, może być tylko szczegółem implementacyjnym dla modułu Y. I co teraz?

Po drugie, wystawianie metryk albo metod diagnozujących, moim zdaniem jest średnie, bo nawet takie metody diagnozujące mogą wystawić złe dane;

Parafrazując: Pisanie testów jest średnie, bo nawet testy mogą zawierać błędy.

Jeśli natomiast baza jest interfejsem, to musisz napisać po każdym teście asercję z bazy która robi selecta i sprawdza dokładnie co w nie jest; tylko wtedy nie możesz nigdy mieć testu który woła read() i write() razem - bo wtedy one już nie są swoją częscią, tylko oba ciągną z bazy.

Też nie. To, że mam testy jednostkowe testujące osobno read i osobno write nie oznacza że nie wolno mi napisać testu integracynego sprawdzającego oba na raz. Zwłaszcza że w takim teście zapewne będę mógł bardziej poszaleć z generacją danych. Znowu - są różne testy i mają różne zastosowania i różne mocne/słabe strony. NIe ma sensu dogmatycznie trzymać się jednego rodzaju testów.

Krolik napisał(a):

Takie podejście nie jest dobre - takie "wiedzenie" która metoda zawiódła. Bo jeśli masz jeden test który robi read i write, i to są Twoje jedyne access pointy do aplikacji, to nie istnieje obiektywny sposób żeby stwierdzić czy zawiódł read czy write. Kontrakt tych dwóch metod jest taki że read() ma zwrócić to co write() wsadził i tyle, ale to w jaki sposób to robią to powinien być szczegół implementacyjny.

Na tym polega różnica między podejściem teoretyka, a praktyka. W teorii akademickiej masz rację. Praktyka tworzenia oprogramowania to coś znacznie więcej niż tylko aby read przeczytał to co zrobił write.
W praktyce takie write może mieć znacznie bardziej rozbudowany kontrakt. Choćby ze względu na kompatybilność wsteczną i dalszy rozwój systemu. Jeżeli ktoś będzie pracował w przyszłości na tym kodzie i w projekcie ma założenia, że np. wewnętrzna struktura zapisywana przez write ma mieć posortowane kolumny wg jakiegoś porządku, to będzie na tej własności polegał. I ta własność powinna być przetestowana.

No okej, Ty sobie to nazywasz podejście "teoretyka", ale ja takie podejście stosuję w swoich komercyjnych projektach od lat. Więc dla mnie to nie jest nic "teoretycznego".

Jeśli write() ma rozbudowany kontrakt, to powinny być testy pod ten kontrakt, i one wtedy mają tą siłę diagnostyczną której potrzebujesz.

Więc z punktu widzenia dobrych testów i TDD; pomysł żeby "wiedzieć która metoda zawiodła" nie specjalnie ma rację bytu. Nie istnieje żaden sensowny testu który usprawiedliwiłby taki koncept, i istnieć nie może.

Zalatuje religią i dogmatami.
Jak mi się test wywali, to chcę wiedzieć gdzie poprawiać. Odrzucenie 50% kodu na starcie to duży zysk czasu.

Rozumiem, że dla Ciebie może tak zalatywać - jeśli nie praktykujesz takiego podejścia. Jeśli masz jakiś projekt publiczny który używa takich metod, to bardzo chętnie mógłbym Ci pokazać jak możnaby dobrze napisać taki test który jest rzetelny, a jednocześnie nie wymaga takich metod diagnostycznych.

No widzę dwa case'y:

• Jeśli read() i write() wiedzą tylko o sobie, to po prostu nie ma sensu test który wykryłby w której metodzie jest błąd. Trochę to przypomina sytuację w której dwie osoby mówią do siebie innym językiem, to nie jesteś w stanie stwierdzić która z tych osób mówi "złym" - chyba, że jest dodatkowe założenie które sugeruje jakim językiem powinni mówić (np angielskim). Tak samo jest z tym read(), write() jeśli one mają działać razem, to kiedy test failuje, nie jesteś w stanie stwierdzić która z nich nie działa - chyba, że dodasz dodatkowe założenie gdzie powinni te dane zapisać (np w bazie).
• Natomiast jeśli read() i write() mają inne kontrakty, również to że kompatybilność wsteczna, ale również jakikolwiek inny; to pod to też należy napisać test - no przecież chcemy to mieć sprawdzone - i wtedy taki test nam załatwi wszystkie potrzebne rzeczy które musimy sprawdzić; i wtedy tych metody diagnostycznych nie potrzebujesz. W gruncie rzeczy wystawienie metody diagnostycznej to jest po prostu złamanie enkapsulacji - jeśli jest wystawiasz to równie dobrze mógłbyś czytać pola prywatne, z testowego punktu widzenia to byłoby to samo. Tylko właściwie jest to gorsze, bo łamiąc enkapsulację przynajmniej przetestujesz to co kod robi (ściśle bo ściśle, testy rigid, tight-coupling, ale przynajmniej przetestujesz). Z takimi metodami diagnozującymi nie wiesz czy testujesz faktyczny kod, czy testujesz tylko te metody diagnozujące, a to co robi kod jest nieprzetestowane.

Krolik napisał(a):

Nie muszę, bo zarówno jedno jak i drugie warto testować. Z innych powodów, ale warto.

Niektórych rzeczy nie warto testować - np szczegółów implementacyjnych.

Poza tym jak napisałem wyżej, to nie jest podział czarno-biały. Są wsystkie odcienie szarości pomiędzy. Coś co jest interfejsem dla modułu X, może być tylko szczegółem implementacyjnym dla modułu Y. I co teraz?

Testy moduły X piszesz pod "to coś" jako interfejs, testy modułu Y piszesz pod "to coś" jako szczegół implementacyjny.

Po drugie, wystawianie metryk albo metod diagnozujących, moim zdaniem jest średnie, bo nawet takie metody diagnozujące mogą wystawić złe dane;

Parafrazując: Pisanie testów jest średnie, bo nawet testy mogą zawierać błędy.

No owszem, testy mogą zawierać błędy; dlatego stosujemy TDD, i najpierw widzimy jak test failuje, potem dopisujemy funkcjonalność, i teraz test przechodzi. Poza tym, nawet jak nie zrobiliśmy testów w TDD; nadal możemy łatwo sprawdzić czy test działa. Jeśli znajdziemy test który sprawdza jakąś funkcjonalnośc, wystarczy specjalnie dodać buga, i sprawdzić czy test sfailuje - jeśli tak, to jest wartościowy. Jeśli nie, to ma buga w sobie albo jest redundant, i można go wyrzucić (zakładając że dodaliśmy tego buga, i widać w aplikacji że ten bug jest).

Natomiast z metodami diagnozujacymi sytuacja jest inna, bo nie jesteś za ich pomocą w stanie stwierdzić czy system działa czy nie - tzn. działa, tylko przy założeniu że faktycznie te metody diagnozujące zwracają 100% danych na który operuje kod; i tego założenia nie jesteś w stanie specjalnie sprawdzić.

Tylko właściwie jest to gorsze, bo łamiąc enkapsulację przynajmniej przetestujesz to co kod robi (ściśle bo ściśle, testy rigid, tight-coupling, ale przynajmniej przetestujesz). Z takimi metodami diagnozującymi nie wiesz czy testujesz faktyczny kod, czy testujesz tylko te metody diagnozujące, a to co robi kod jest nieprzetestowane.

Nie, to jest praktycznie to samo. Różni się lokalizacją kodu testowego. Metoda diagnostyczna jest formalnie częścią testu, natomiast właśnie ze względu na czytelność kodu lepiej ją mieć bliżej danych na których działa. I metoda diagnostyczna powinna być oznaczona jako testowa, w produkcie końcowym jej nie ma.

Poza tym, nawet jak nie zrobiliśmy testów w TDD; nadal możemy łatwo sprawdzić czy test działa. Jeśli znajdziemy test który sprawdza jakąś funkcjonalnośc, wystarczy specjalnie dodać buga, i sprawdzić czy test sfailuje - jeśli tak, to jest wartościowy. Jeśli nie, to ma buga w sobie albo jest redundant, i można go wyrzucić (zakładając że dodaliśmy tego buga, i widać w aplikacji że ten bug jest).

To nie gwarantuje braku bugów. Nadal możesz mieć niepoprawny test działający na niepoprawnym kodzie, który przechodzi na zielono.
Możesz napisać najpierw niepoprawny test, a potem niepoprawny kod, który go przechodzi. Sytuacja identyczna jak z metodami diagnostycznymi. Identyczna, bo kod diagnostyczny i tak stosujesz - są częścią testów. To jest tylko kwestia organizacji kodu, semantycznie niczym się nie różni.

Tak przy okazji, nigdy nie widziałem, aby TDD faktycznie powodowało lepszą jakość kodu, natomiast widziałem wielokrotnie jak prowadziło właśnie do patalogicznie złych designów i przesadnie skomplikowanego kodu najeżonego ifami. Bo zmusza programistę do myślenia o kodzie przez pryzmat przypadków, a nie ogólnego rozwiązania problemu.

Testy moduły X piszesz pod "to coś" jako interfejs, testy modułu Y piszesz pod "to coś" jako szczegół implementacyjny.

Tak, oczywiście z tym się zgadzam. Ale z tego wynika wprost, że mogę mieć coś takiego jak "interfejs / kontrakt wewnętrzny" dla klasy (czyli metody i struktury prywatne) i też mogę pisać pod to testy. Czyli mogę testować klasy/metody prywatne.

I znowu, przestańmy patrzeć przez pryzmat klas / obiektów / interfejsów OOP ala Java. Testowanie jest tematem bardziej ogólnym, a organizacja kodu w klasy i obiekty nie jest jedynym sensownym sposobem organizacji kodu. Metoda prywatna też ma interfejs - zapisany w swoim kontrakcie.

Krolik napisał(a):

Test który używa read do sprawdzenia write jest tym samym co test używający write do sprawdzenia read. Oczywiście przy bardzo prostych implementacjach to nie jest problem, ale przy nietrywialnych fajnie byłoby wiedzieć, czy zawiódł read czy write, a z takiego testu się nie dowiesz. I nie jest to mój wymysł teoretyczny, tylko nie dalej jak miesiąc temu mieliśmy w firmie realny przypadek, gdzie błąd był w "read", a większość ludzi początkowo pomyślała, że błąd był w "write" i że mamy uszkodzone pliki z danymi. Przy czym read i write to w tym przypadku kobyły na kilka tys. linii. ;) Bo wszystkie weryfikatory tychże plików używały tego samego kodu do odczytu. Kodu, który miał błąd.

Z mojego punktu widzenia to jest po prostu nieprawidłowo napisany test. Z tego co zrozumiałem to write tworzy jakiś plik (lub go modyfikuje), a read go odczytuje.

1. Test read powinien polegać na tym, że masz jakiś predefiniowany input.file i sprawdzasz, czy to co read wypluło jest zgodne z tym, co według ciebie wypluć powinno.
2. W przypadku write masz sytuację odwrotną - czyli wrzucasz jakąś strukturę danych do write, tworzy ci się jakiś output.file, i na końcu sprawdzasz, czy ten plik jest zgodny z tym, co być powinno.
3. Natomiast zewnętrzne zależności nie zawsze istnieją. Przykładem jest dowolny obiekt przechowujący stan wyłącznie w pamięci - np. Counter, który ma dwie metody: void increment() i int value(). W takim przypadku testy value oraz increment (czyli odczytu i zapisu) muszą być testowane razem - bo stworzenie obiektu Counteri używanie tylko jednej z tych metod nie ma żadnego sensu - samo increment() nie robi nic, a samo value() będzie zawsze zwracało 0.
4. I tak, dopuszczam możliwość, że istnieją jakieś bardzo wyjątkowe sytuacje, w których monitorowanie prywatnych pól będzie uzasadnione. Ale są to właśnie wyjątkowe sytuacje, a co do zasady powinno się tego unikać.

Krolik napisał(a):

Tylko właściwie jest to gorsze, bo łamiąc enkapsulację przynajmniej przetestujesz to co kod robi (ściśle bo ściśle, testy rigid, tight-coupling, ale przynajmniej przetestujesz). Z takimi metodami diagnozującymi nie wiesz czy testujesz faktyczny kod, czy testujesz tylko te metody diagnozujące, a to co robi kod jest nieprzetestowane.

Nie, to jest praktycznie to samo. Różni się lokalizacją kodu testowego. Metoda diagnostyczna jest formalnie częścią testu, natomiast właśnie ze względu na czytelność kodu lepiej ją mieć bliżej danych na których działa. I metoda diagnostyczna powinna być oznaczona jako testowa, w produkcie końcowym jej nie ma.

To po co ta metoda ma w ogóle być w kodzie produkcyjnym, czemu po prostu nie przenieść jej do testów?

Poza tym, nawet jak nie zrobiliśmy testów w TDD; nadal możemy łatwo sprawdzić czy test działa. Jeśli znajdziemy test który sprawdza jakąś funkcjonalnośc, wystarczy specjalnie dodać buga, i sprawdzić czy test sfailuje - jeśli tak, to jest wartościowy. Jeśli nie, to ma buga w sobie albo jest redundant, i można go wyrzucić (zakładając że dodaliśmy tego buga, i widać w aplikacji że ten bug jest).

To nie gwarantuje braku bugów.

Oczywiście że nie. Żadna metoda nie gwarantuje braku bugów. Ale niektóre metody są lepsze niż inne do ich ograniczenia. Moim zdaniem TDD jest jedną z lepszych (jak nie najlepszą).

Nadal możesz mieć niepoprawny test działający na niepoprawnym kodzie, który przechodzi na zielono.
Możesz napisać najpierw niepoprawny test, a potem niepoprawny kod, który go przechodzi. Sytuacja identyczna jak z metodami diagnostycznymi. Identyczna, bo kod diagnostyczny i tak stosujesz - są częścią testów. To jest tylko kwestia organizacji kodu, semantycznie niczym się nie różni.

Nie sądzę że jesteś w stanie to zrobić. A przynajmniej jest to bardzo trudne.

Weź pod uwagę:

• Piszesz test (zły, ale jeszcze tego nie wiesz)
• Odpalasz test, i on ma nie przejść
• Piszesz mały kawałek kodu produkcyjnego
• Odpalasz zły test, i on teraz przechodzi po dopisaniu tego małego kawałka logiki

Bardzo trudno byłoby napisać zły test który najpierw by sfailował, a potem po dopisaniu odpowiedniej logiki by przeszedł. Właśnie na tym polega cała moc TDD. Owszem, znajdą się takie przypadki pewnie raz na tysiąc, ale to można wtedy bezproblemowo poprawić kolejnym testem.

Jest to możliwe, ale zakłada że programista popełni dwa błedy po sobie - najpierw pisząc test, potem pisząc kod; i to jeszcze takie błędy które nawzajem się enforce'ują (tzn brak kodu == fail, dodanie kodu == pass). To jest niesamowicie rzadki przypadek. I nawet jego wystąpienie nic takiego złego nie robi, bo nawet jakby tam był bug, to na 99% zostanie złapany przez następny test.

Krolik napisał(a):

Sytuacja identyczna jak z metodami diagnostycznymi. Identyczna, bo kod diagnostyczny i tak stosujesz - są częścią testów. To jest tylko kwestia organizacji kodu, semantycznie niczym się nie różni.

No to ponawiam pytanie, czemu nie przenieść tej funkcji do testów? Bo jeśli odpowiedź brzmi "dlatego żeby się dostać do prywatnych pól", to to nie jest semantycznie to samo. Żeby to było to samo, to taka metoda diagnostyczna musiałaby czytać tylko i wyłącznie publiczne pola i metody, i tylko te które są faktyczną częścią publicznego interfejsu żeby to było semantycznie to samo. Jeśli tak jest w istocie to nie mam problemu z takimi funkcjami. Jeśli faktycznie dotykają tylko i wyłącznie publicznych części interfejsu (które tak samo można by łatwo przetestować z testów jak tymi metodami diagnostycznymi), to zwracam honor - i takie funkcje jednak są okej. Wcześniej pomyślałem że te funkcje diagnostyczne dlatego są w kodzie produkcyjnym, żeby czytać pola prywatne i dlatego uznałem że są złe. Tylko jeśli tak jest to nadal nie rozumiem po co trzymać te funkcje przy kodzie, zamiast w testach.

Krolik napisał(a):

Tak przy okazji, nigdy nie widziałem, aby TDD faktycznie powodowało lepszą jakość kodu, natomiast widziałem wielokrotnie jak prowadziło właśnie do patalogicznie złych designów i przesadnie skomplikowanego kodu najeżonego ifami.

To nie widziałeś nigdy TDD.

Z tego co mówisz, widziałeś jedynie projekty w który autor twierdził że praktykował TDD, podczas gdy to nie było to (tylko albo nie do końca to umiał, albo zrobił jakąś swoją fantazję, albo coś jeszcze innego).

Jeśli chcesz zobaczyć dobry przykład, to mogę pokazać swój projekt: https://github.com/t-regx/crlf Możesz przejrzeć historię powstawania, albo popatrzeć całościowo na origin/master na efekt końcowy.

Krolik napisał(a):

Bo zmusza programistę do myślenia o kodzie przez pryzmat przypadków, a nie ogólnego rozwiązania problemu.

No, i to się trochę mija z prawdę, bo w TDD właśnie o to chodzi żeby dotrzeć do ogólnego rozwiązania problemu, takiego który jest general/generic i pasuje do wielu przypadków. Jak powstaje Ci kod najeżony ifami to właśnie nie jest generic, tylko specific, i to nie jest TDD. To jakaś smutna wizja.

Jeśli widziałeś takie przypadki w swojej karierze, to to nie było TDD.