Czy testy są dobrze napisane?

Zacząłem pisać testy do aplikacji. Na tą chwilę napisałem je tylko do jednej metody, ale chce żeby ktoś, kto się zna na tym bardziej ocenił, czy zmierzam w dobrym kierunku z tymi testami. Dodam jeszcze, ze pierwszy raz pisze testy do aplikacji.

Link:
https://github.com/MaciekSm19/Bank

Testy w miarę ok (userPersonalData.peselNumber = "02253000000"; to given a nie when), design kodu bez sensu. Problemy:

• dużo mutowalności, UserPersonalData może być spokojnie immutable
• convertPeselToDateOfBirth drukuje na ekran. Klasa nie powinna sama z siebie drukować na ekran, bo to za dużo coupling. Lepiej zwrócić string
• isValidPesel zwraca boola i drukuje na ekran. A co jak ktoś będzie chciał sprawdzić czy pesel jest dobry bez drukowania na ekran?
• sam pesel powinień być wyciągnięty do osobnej klasy. Istnieje mnóstwo przypadków, gdzie ktoś mógłby trzymać numer pesel lub go walidować, gdzie UserPersonalData nie jest kompletnie przydatne

Na plus:

1. Fajnie ze jest mało kodu w testach
2. Fajnie ze nie ma niepotrzebnych rzeczy
3. Fajnie że jest dużo przypadków testowych
4. Moim zdaniem testy, pod względem tego jak działają są bardzo dobre.

Na minus:

1. Niepotrzebne given/when/then. Funkcja isValidPesel() to jest pure-funkcja, nie ma żadnego stanu, a "given" się używa do stanu, z BDD; "then" się odnosi do finalnego stanu. Jeśli masz pure funkcję, to całość testu powinna być w "when", tylko wtedy jaki jest sens to stosować w ogóle?
2. Dużo powtarzalności w testach, moim zdaniem powinieneś wydzielić helper-funkcję, tak żeby Twoje testy wyglądały tak:
   Kopiuj

    @Test
    void testIsValidPeselIncorrectPeselMonthValueXXICentury() {
        assertInvalid("02203000000");
    }
   
    @Test
    void testIsValidPeselCorrectPeselDayValueXXICentury() {
        assertValid("02253000000");
    }
   

Reszta w porządku. Good job!

@Maciek_SK8 Jeśli mam Ci dać jakąś radę - nie pisz testów sam, i potem pytaj o opinię. Najpierw zapoznaj się z dobrym źródłem do testów, najlepiej kilkoma (daj znać to Ci mogę podesłać kilka), a dopiero potem próbuj sam.

Maciek_SK8 napisał(a):

Zacząłem pisać testy do aplikacji. Na tą chwilę napisałem je tylko do jednej metody, ale chce żeby ktoś, kto się zna na tym bardziej ocenił, czy zmierzam w dobrym kierunku z tymi testami. Dodam jeszcze, ze pierwszy raz pisze testy do aplikacji.

Link:
https://github.com/MaciekSm19/Bank

Nie testujesz sytuacji, kiedy pesel zawiera np. litery czy inne niedozwolone znaki.

No i testujesz po jednym (zawsze kobiecym) peselu w każdym kejsie. Przydałoby się to jakoś zróżnicować i testować po kilka wariantów. Chociaż to nie musi mieć aż takiego znaczenia, zależy od algorytmu. Ale przez analogię. Jeśli masz prostą funkcję addTwoNumbers(), to samo sprawdzenie addTwoNumbers(2, 2) == 4 może przejść nawet przez przypadek (jak np. funkcja pomnoży 2 przez 2 zamiast dodać). Jednak jeśli funkcja dodaje poprawnie liczby dla różnych przypadków, to już jest większe prawdopodobieństwo, że faktycznie robi to, co ma robić.

Tutaj też warto by pomyśleć, co w liczeniu peselu może się rozwalić. Np. widzę, że sprawdzasz w kodzie liczenie dni dla różnych miesięcy, a nie testujesz wcale tych wariantów (np. 32 stycznia, 30 lutego, 31 czerwca itp.), tylko dajesz jeden przykład poprawnego i niepoprawnego dnia.

slsy napisał(a):

Testy w miarę ok (userPersonalData.peselNumber = "02253000000"; to given a nie when), design kodu bez sensu. Problemy:

• dużo mutowalności, UserPersonalData może być spokojnie immutable
• convertPeselToDateOfBirth drukuje na ekran. Klasa nie powinna sama z siebie drukować na ekran, bo to za dużo coupling. Lepiej zwrócić string
• isValidPesel zwraca boola i drukuje na ekran. A co jak ktoś będzie chciał sprawdzić czy pesel jest dobry bez drukowania na ekran?
• sam pesel powinień być wyciągnięty do osobnej klasy. Istnieje mnóstwo przypadków, gdzie ktoś mógłby trzymać numer pesel lub go walidować, gdzie UserPersonalData nie jest kompletnie przydatne

Funkcje zaraz pójdą do poprawy

Riddle napisał(a):

Na plus:

1. Fajnie ze jest mało kodu w testach
2. Fajnie ze nie ma niepotrzebnych rzeczy
3. Fajnie że jest dużo przypadków testowych
4. Moim zdaniem testy, pod względem tego jak działają są bardzo dobre.

Dzięki!

Na minus:

1. Niepotrzebne given/when/then. Funkcja isValidPesel() to jest pure-funkcja, nie ma żadnego stanu, a "given" się używa do stanu, z BDD; "then" się odnosi do finalnego stanu. Jeśli masz pure funkcję, to całość testu powinna być w "when", tylko wtedy jaki jest sens to stosować w ogóle?
2. Dużo powtarzalności w testach, moim zdaniem powinieneś wydzielić helper-funkcję, tak żeby Twoje testy wyglądały tak:
   Kopiuj

    @Test
    void testIsValidPeselIncorrectPeselMonthValueXXICentury() {
        assertInvalid("02203000000");
    }
   
    @Test
    void testIsValidPeselCorrectPeselDayValueXXICentury() {
        assertValid("02253000000");
    }
   

Ok, zaraz spróbuje zrobić cos takiego

Reszta w porządku. Good job!

@Maciek_SK8 Jeśli mam Ci dać jakąś radę - nie pisz testów sam, i potem pytaj o opinię. Najpierw zapoznaj się z dobrym źródłem do testów, najlepiej kilkoma (daj znać to Ci mogę podesłać kilka), a dopiero potem próbuj sam.

Ogólnie, żeby mieć jakikolwiek punkt zaczepienia z testami poczytałem trochę o nich stąd:
https://javastart.pl/baza-wiedzy/testowanie-jednostkowe/junit

Wysyłałeś mi pod innym postem link do filmu, gdzie tłumaczone jest TDD, ale kiedy mam już napisany kod to nie wykorzystam TDD (chyba, ze się mylę). Byłbym wdzięczny jeśli byś coś podesłał

LukeJL napisał(a):

Maciek_SK8 napisał(a):

Zacząłem pisać testy do aplikacji. Na tą chwilę napisałem je tylko do jednej metody, ale chce żeby ktoś, kto się zna na tym bardziej ocenił, czy zmierzam w dobrym kierunku z tymi testami. Dodam jeszcze, ze pierwszy raz pisze testy do aplikacji.

Link:
https://github.com/MaciekSm19/Bank

Nie testujesz sytuacji, kiedy pesel zawiera np. litery czy inne niedozwolone znaki.

O tym zapomniałem. Dzięki, ze mówisz

No i testujesz po jednym (zawsze kobiecym) peselu w każdym kejsie. Przydałoby się to jakoś zróżnicować i testować po kilka wariantów. Chociaż to nie musi mieć aż takiego znaczenia, zależy od algorytmu. Ale przez analogię. Jeśli masz prostą funkcję addTwoNumbers(), to samo sprawdzenie addTwoNumbers(2, 2) == 4 może przejść nawet przez przypadek (jak np. funkcja pomnoży 2 przez 2 zamiast dodać). Jednak jeśli funkcja dodaje poprawnie liczby dla różnych przypadków, to już jest większe prawdopodobieństwo, że faktycznie robi to, co ma robić.

Ogólnie ta funkcja miała sprawdzać poprawność peselu pod kątem daty urodzenia (następne cyfry mogą być obojętne), żebym nie musiał za każdym razem wpisywać poprawnego peselu, tylko taki, z którego można przeczytać datę urodzenia

Tutaj też warto by pomyśleć, co w liczeniu peselu może się rozwalić. Np. widzę, że sprawdzasz w kodzie liczenie dni dla różnych miesięcy, a nie testujesz wcale tych wariantów (np. 32 stycznia, 30 lutego, 31 czerwca itp.), tylko dajesz jeden przykład poprawnego i niepoprawnego dnia.

Zaraz dodam więcej przykładów

Maciek_SK8 napisał(a):

Ogólnie, żeby mieć jakikolwiek punkt zaczepienia z testami poczytałem trochę o nich stąd:
https://javastart.pl/baza-wiedzy/testowanie-jednostkowe/junit

Przykro mi, ale to nie jest dobre źródło Nie polecam Ci korzystać z niego jeśli chodzi o testy.

Maciek_SK8 napisał(a):

Wysyłałeś mi pod innym postem link do filmu, gdzie tłumaczone jest TDD, ale kiedy mam już napisany kod to nie wykorzystam TDD (chyba, ze się mylę).

To prawda.

Ale możesz usunąć kod, i napisać go z TDD od nowa jeśli chcesz. Ja bym tak zrobił.

Maciek_SK8 napisał(a):

Byłbym wdzięczny jeśli byś coś podesłał

No ogólnie testy stworzone później niż kod będą często gorszej jakości niż te napisane wcześniej, niestety Głównie dlatego że my, jako ludzie, jeśli kod już jest, mamy tendencję do pisania testów pod szczegóły implementacyjne, nie zachowanie.

Czasami napiszesz test który da Ci feedback, że design jest zły, i wtedy należałoby go zmienić - łatwiej to zrobić jak kodu jeszcze nie ma.

Maciek_SK8 napisał(a):

Ogólnie ta funkcja miała sprawdzać poprawność peselu pod kątem daty urodzenia (następne cyfry mogą być obojętne), żebym nie musiał za każdym razem wpisywać poprawnego peselu, tylko taki, z którego można przeczytać datę urodzenia

Tutaj też warto by pomyśleć, co w liczeniu peselu może się rozwalić. Np. widzę, że sprawdzasz w kodzie liczenie dni dla różnych miesięcy, a nie testujesz wcale tych wariantów (np. 32 stycznia, 30 lutego, 31 czerwca itp.), tylko dajesz jeden przykład poprawnego i niepoprawnego dnia.

Zaraz dodam więcej przykładów

@LukeJL myślał że chcesz walidować PESEL, i słusznie zauważył że testów jest mało. Ale Ty nie chcesz jak rozumiem walidować PESEL, tylko chcesz wyciągnąć datę urodzenia; więc moim zdaniem przypadki testowe które masz są w 100% wystarczające.

Gdybyś chciał dodatkowo walidować PESEL (bo zakładasz że użytkownik może podać zły), to wtedy - owszem - warto posłuchać rady @LukeJL i dopisać przypadki testowe pod walidację. Ale jeśli chcesz tylko wyciągnąć datę urodzenia, to nie musisz nic dopisywać, moim zdaniem.

Riddle napisał(a):

Maciek_SK8 napisał(a):

Ogólnie, żeby mieć jakikolwiek punkt zaczepienia z testami poczytałem trochę o nich stąd:
https://javastart.pl/baza-wiedzy/testowanie-jednostkowe/junit

Przykro mi, ale to nie jest dobre źródło Nie polecam Ci korzystać z niego jeśli chodzi o testy.

Ok dzięki

Maciek_SK8 napisał(a):

Wysyłałeś mi pod innym postem link do filmu, gdzie tłumaczone jest TDD, ale kiedy mam już napisany kod to nie wykorzystam TDD (chyba, ze się mylę).

To prawda.

Ale możesz usunąć kod, i napisać go z TDD od nowa jeśli chcesz. Ja bym tak zrobił.

W tym przypadku nie chce już mi się pisać tego od nowa, ale w następnych projektach już będę stosował TDD

Maciek_SK8 napisał(a):

Byłbym wdzięczny jeśli byś coś podesłał

No ogólnie testy stworzone później niż kod będą często gorszej jakości niż te napisane wcześniej, niestety Głównie dlatego że my, jako ludzie, jeśli kod już jest, mamy tendencję do pisania testów pod szczegóły implementacyjne, nie zachowanie.

Czasami napiszesz test który da Ci feedback, że design jest zły, i wtedy należałoby go zmienić - łatwiej to zrobić jak kodu jeszcze nie ma.

Rozumiem o co chodzi

Maciek_SK8 napisał(a):

Ogólnie ta funkcja miała sprawdzać poprawność peselu pod kątem daty urodzenia (następne cyfry mogą być obojętne), żebym nie musiał za każdym razem wpisywać poprawnego peselu, tylko taki, z którego można przeczytać datę urodzenia

Tutaj też warto by pomyśleć, co w liczeniu peselu może się rozwalić. Np. widzę, że sprawdzasz w kodzie liczenie dni dla różnych miesięcy, a nie testujesz wcale tych wariantów (np. 32 stycznia, 30 lutego, 31 czerwca itp.), tylko dajesz jeden przykład poprawnego i niepoprawnego dnia.

Zaraz dodam więcej przykładów

@LukeJL myślał że chcesz walidować PESEL, i słusznie zauważył że testów jest mało. Ale Ty nie chcesz jak rozumiem walidować PESEL, tylko chcesz wyciągnąć datę urodzenia; więc moim zdaniem przypadki testowe które masz są w 100% wystarczające.

Gdybyś chciał dodatkowo walidować PESEL (bo zakładasz że użytkownik może podać zły), to wtedy - owszem - warto posłuchać rady @LukeJL i dopisać przypadki testowe pod walidację. Ale jeśli chcesz tylko wyciągnąć datę urodzenia, to nie musisz nic dopisywać, moim zdaniem.

A no to w takim razie nie będę dodawał więcej przykładów tylko poprawie te testy, żeby były czytelniejsze

I tak jak @LukeJL napisał, przetestuj też niedozwolone znaki, pusty string, nulla...

lion137 napisał(a):

I tak jak @LukeJL napisał, przetestuj też niedozwolone znaki, pusty string, nulla...

Nie ucz człowieka testować "na zaś".

Jak będzie chciał obsłużyć te case'y to obsłuży, jak nie to nie.

Riddle napisał(a):

lion137 napisał(a):

I tak jak @LukeJL napisał, przetestuj też niedozwolone znaki, pusty string, nulla...

Nie ucz człowieka testować "na zaś".

Jak będzie chciał obsłużyć te case'y to obsłuży, jak nie to nie.

To akurat musze dodać, żeby program działał poprawnie

Testy są dobrze napisane, jeżeli przemielisz pół projektu, zapuścisz testy, i jak przechodzą to masz pewność że system działa jak powinien i nie musisz już testować ręcznie na UI / swaggerze / konsoli :D

Poprawiam teraz te testy (dodałem helper metodę i więcej przykładów) i nie wiem czemu, kiedy chce sprawdzić poprawność długości i wpisze pesel długości mniejszej niż 6 znaków to pokazuje mi błąd (kiedy ma 6 znaków lub więcej wszystko działa bez problemu):

java.lang.StringIndexOutOfBoundsException: Index 5 out of bounds for length 5

Tutaj metoda testująca:

@Test
void testIsValidPeselIncorrectPeselLengthXXICentury() {
    assertInvalid("00000");
    assertInvalid("");
    assertInvalid("12");
}

Maciek_SK8 napisał(a):

Poprawiam teraz te testy (dodałem helper metodę i więcej przykładów) i nie wiem czemu, kiedy chce sprawdzić poprawność długości i wpisze pesel długości mniejszej niż 6 znaków to pokazuje mi błąd (kiedy ma 6 znaków lub więcej wszystko działa bez problemu):

Test jest w porządku.

Wyjątek poleciał z implementacji, widocznie próbujesz się odnieść do szóstego znaku w stringu który nie ma sześciu znaków.

Riddle napisał(a):

Maciek_SK8 napisał(a):

Poprawiam teraz te testy (dodałem helper metodę i więcej przykładów) i nie wiem czemu, kiedy chce sprawdzić poprawność długości i wpisze pesel długości mniejszej niż 6 znaków to pokazuje mi błąd (kiedy ma 6 znaków lub więcej wszystko działa bez problemu):

Test jest w porządku.

Wyjątek poleciał z implementacji, widocznie próbujesz się odnieść do szóstego znaku w stringu który nie ma sześciu znaków.

Dobra juz wszystko rozumiem, w funkcji do walidacji sprawdzam 6 pierwszych znaków i to dlatego tak wyszło. Dzięki!

Riddle napisał(a):

lion137 napisał(a):

I tak jak @LukeJL napisał, przetestuj też niedozwolone znaki, pusty string, nulla...

Nie ucz człowieka testować "na zaś".

Jak będzie chciał obsłużyć te case'y to obsłuży, jak nie to nie.

Przecież musi przetestowac wszystkie corner - case'y to norma.

lion137 napisał(a):

Przecież musi przetestowac wszystkie corner - case'y to norma.

Nie koniecznie. Wszystko zależy od use-case'u.

Jeśli dostaje dane z niepewnego źródła, to powinien. Jeśli dane są pewne, to nie ma potrzeby tego robić.

Nie wiem, jak tam testujesz, ale standardem jest, przynajmniej tam gdzie widziałem na żywo i w teorii, pokrycie jak najwiekszej ilości przypadków; a już specjalnie, jak jest jakiś string input, sprawdzeni pustego, itp.

lion137 napisał(a):

Nie wiem, jak tam testujesz, ale standardem jest, przynajmniej tam gdzie widziałem na żywo i w teorii, pokrycie jak najwiekszej ilości przypadków; a już specjalnie, jak jest jakiś string input, sprawdzeni pustego, itp.

Po prostu obsługuj to co chcesz obsługiwać, i nie obsługuj czegoś czego nie chcesz. Simple.

Dopisałem testy, które sprawdza, czy pesel nie jest pusty i czy nie zawiera znaków innych niż cyfry. Chciałem napisać testy do innych klas, ale pobieram w nich wartości podane w konsoli za pomocą klasy Scanner. Jak testować takie klasy?

Maciek_SK8 napisał(a):

Dopisałem testy, które sprawdza, czy pesel nie jest pusty i czy nie zawiera znaków innych niż cyfry. Chciałem napisać testy do innych klas, ale pobieram w nich wartości podane w konsoli za pomocą klasy Scanner. Jak testować takie klasy?

To że trudno przetestować jakąś klasę, to bardzo często jest sygnał dla Ciebie o tym że design klasy jest słaby (gdyby był dobry, testowanie byłoby łatwe).

Design klasy dlatego jest słaby (i testowanie jest trudne), przez ciasne powiązanie Twojej logiki do Scanner i do System.in, należałoby złamać to powiązanie, albo wstrzykując System.in do klasy, albo chowając Scanner w osobnej klasie (np. ConsoleInput), i ją wstrzyknąć do testowanej klasy.

Całego problemu możnaby uninknąć pisząć testy najpierw, a potem kod.

Riddle napisał(a):

Maciek_SK8 napisał(a):

Dopisałem testy, które sprawdza, czy pesel nie jest pusty i czy nie zawiera znaków innych niż cyfry. Chciałem napisać testy do innych klas, ale pobieram w nich wartości podane w konsoli za pomocą klasy Scanner. Jak testować takie klasy?

To że trudno przetestować jakąś klasę, to bardzo często jest sygnał dla Ciebie o tym że design klasy jest słaby (gdyby był dobry, testowanie byłoby łatwe).

Design klasy dlatego jest słaby (i testowanie jest trudne), przez ciasne powiązanie Twojej logiki do Scanner i do System.in, należałoby złamać to powiązanie, albo wstrzykując System.in do klasy, albo chowając Scanner w osobnej klasie (np. ConsoleInput), i ją wstrzyknąć do testowanej klasy.

Ok zrobię tak

Całego problemu możnaby uninknąć pisząć testy najpierw, a potem kod.

Następne programy będę pisał w ten sposób tylko nie wiem, jak zacząć stosować TDD. Film, który mi kiedyś poleciłeś ( ) jest mam wrażenie dla osób na bardziej zaawansowanym poziomie niż ja . Sam autor filmu mówi, ze pomija podstawowe rzeczy i jeśli się nie rozumie, nie warto go oglądać.

Spokojnie, na przykładzie tego peselu, planujesz sobie, że public API (w tym przypadku funkcja) pobiera stringa i zwraca boolean, czy pesel. I jeszcze nie mając implenetacji, czy jakąś dumb, piszesz test, który pewnie sfailuje i już. Przez samo to, że tak zacząłeś nie przyjdzie ci do głowy wmieszanie IO do tej metody, bo jak tu sprawdzić w teście.

Maciek_SK8 napisał(a):

Następne programy będę pisał w ten sposób tylko nie wiem, jak zacząć stosować TDD. Film, który mi kiedyś poleciłeś [...] jest mam wrażenie dla osób na bardziej zaawansowanym poziomie niż ja . Sam autor filmu mówi, ze pomija podstawowe rzeczy i jeśli się nie rozumie, nie warto go oglądać.

Celna uwaga. Najlepiej będzie jeśli przeczytasz krótką książkę "TDD By Example" napisaną przez autora TDD, Kent Beck. Nie jest długa.

Jeśli nie chcesz czytać, to możesz zapoznać się z tym filmem, który podlinkowałem. Robert Martin (autor książki "Clean Code") prezentuje tam przykład tworzenia kolekcji stos wykorzystując TDD.

Tylko że ten filmik musisz obejrzeć cały, najlepiej kilka razy.