Jak przetestować metody w void metodzie przez Mockito?

Cześć wszystkim ! Nie mogę skumać jak przetestować void metodę. Z tego co rozumiem mogę korzystać z doXXX.when() metody oraz verify, natomiast co jeżeli chcę pobrać
obiekt (var user = userService.getByActivationToken(token)) do testowej metody, żeby sprawdzić jego pola? Jak bym mógł dostać obiekt Usera

Metoda z serwisa

@Override
    public void confirmPasswordAndRegistration(String token) {
        if (token == null) throw new PasswordException("Token is null, password and registration are not confirmed");
        var user = userService.getByActivationToken(token);
        user.setActivationToken(null);
        user.setEnabled(true);
        userService.edit(user);
    }

Klasa testowa

@ExtendWith(SpringExtension.class)
class PasswordServiceImplTest {
    private final String token = "fb17fe76-6e4f-4a01-902a-10b79ffc78c7";
    @Mock
    private PasswordMailSender passwordMailSender;
    @Mock
    private UserService userService;
    @Mock
    private BCryptPasswordEncoder passwordEncoder;
    @InjectMocks
    private PasswordServiceImpl passwordService;
    private User mockedUser;

    @BeforeEach
    void setUp() {
        mockedUser = UserMocked.createMockedUser();
    }

    @Test
    @DisplayName("Confirm password and registration test is successful")
    void confirmPasswordAndRegistrationIsSuccessful() {

        doReturn(mockedUser).when(userService).getByActivationToken(token);

        passwordService.confirmPasswordAndRegistration(token);
**        // Dalej chcę pobrać mockedUser i przetesować jego pola i potem przez verify, sprawdzić  ile razy zostało wywołano metody wewnątrz public void confirmPasswordAndRegistration(String token).**

    }
}

1. Ten kod jest bez sensu, bo wysypie się jak ktoś poda zły token
2. Czemu nie zwrócisz usera z tej metody i wtedy nie będzie void i nie trzeba będzie cudować.

Jest rozwiązanie dla voidów - czytałeś dokumentacje? To jest jedno z pierwszych pytań każdego, kto zaczyna z Mockito.

EDIT: przeczytałem Twój post do końca :) popieram @Shalom - testuj to integracyjnie, black boksowo

Ale po co? Nie ma innego rozwiązania dla voidów?

Ja bym tego nie testował jednostkowo w ogóle, tylko integracyjnie, tzn wykonał kod, a potem sprawdził czy w bazie zapisały się takie informacje jakie oczekujemy i robił asercje na tym.

public void confirmPasswordAndRegistration(String token)

Mi ta metoda w ogóle nie pasuje... Ona robi kilka rzeczy :/ W ogóle ta metoda to ma nazwę nieadekwatną do zawartości czy źle patrzę?

Na chłopski rozum, dlaczego uważasz, że metoda confirmPasswordAndRegistration nie powinna niczego zwracać? Skoro jej zadaniem jest coś confirmnąć, to na logikę powinna zwracać jakąś informację - udało się lub coś poszło źle.

Poza tym, to wygląda trochę bez sensu

• metoda mówi, że robi coś z hasłem, coś tam sobie potwierdza
• ale przyjmuje "token"
• po czym resetuje token użytkownika
• nie daje żadnej zwrotki o tym, czy udało się to "coś" potwierdzić, najwyżej wali wyjątkiem, wymuszając na programiście użycie w stylu Exception Driven Development

Proponuję nie mockować nic, tylko jeśli ta klasa ma coś robić na userze, a zakładam że user jest zapisany do bazy; to proponuję wywołać tą metodę w teście a potem zobaczyć w bazie czy dane się zapisały takie jakie powinny.

Pamiętaj że w testach nie chodzi o to żeby odkliknąć coverage, albo żeby w testach napisać instrukcje jak kod ma działać (przepisać kod do testów). Testy mają sprawdzić czy klasa robi to co robi. Innymi słowy, mają zapewnić, żeby kod stał się odporny na refactory przy zachowaniu kontraktu.

Twój pomysł z mockito w żaden sposób nie daje klasie miejsca na refactor, bo jak tylko zmienisz implementacje, to test zacznie failować; mimo że klasa robiłaby to co miała robić. Test na bazie nie ma tej wady.

Masz trzy opcje, od najlepszej do najmniej sensownej:

1. Tak jak pisał @Shalom - zwróć User i sprawdź co tam chcesz.
2. Po wywołaniu metody passwordService.confirmPasswordAndRegistration(token); wyciągnij User przez userService i wtedy sprawdź, czy obiekt jest OK.
3. W końcu - do mocka userService.getByActivationToken(token) załaduj Spy/Mocka Usera i po wykonaniu wszystkiego sprawdź czy odpowiednie metody z odpowiednimi parametrami się wykonały.

Można też przejść na testy integracyjne.

wartek01 napisał(a):

Masz trzy opcje, od najlepszej do najmniej sensownej:

1. Tak jak pisał @Shalom - zwróć User i sprawdź co tam chcesz.

Ma zmienić interfejs funkcji, po to żeby ją przetestować? Innymi słowy, napisać nietestowalną funkcję, i zmienić ją żeby była testowalna? I to jest posortowane "od najlepszej"? Potem patrzysz na kod produkcyjny, widzisz że return-value nigdy nie jest użyte, i myślisz "po co ta funkcja zwraca tego usera w sumie...?", i potem odkrywasz że do testów? Wtf?

TomRiddle napisał(a):

Ma zmienić interfejs funkcji, po to żeby ją przetestować? Innymi słowy, napisać nietestowalną funkcję, i zmienić ją żeby była testowalna? I to jest posortowane "od najlepszej"? Potem patrzysz na kod produkcyjny, widzisz że return-value nigdy nie jest użyte, i myślisz "po co ta funkcja zwraca tego usera w sumie...?", i potem odkrywasz że do testów? Wtf?

No ze zwracaniem User po nic to może niefortunnie, ale generalnie lepsze to niż void a potem w testach ścianka asercji

inOrder.verify(foo).isCalledOnce().withArguments(....)

(DSL zmyślony)

superdurszlak napisał(a):

TomRiddle napisał(a):

Ma zmienić interfejs funkcji, po to żeby ją przetestować? Innymi słowy, napisać nietestowalną funkcję, i zmienić ją żeby była testowalna? I to jest posortowane "od najlepszej"? Potem patrzysz na kod produkcyjny, widzisz że return-value nigdy nie jest użyte, i myślisz "po co ta funkcja zwraca tego usera w sumie...?", i potem odkrywasz że do testów? Wtf?

No ze zwracaniem User po nic to może niefortunnie, ale generalnie lepsze to niż void a potem w testach ścianka asercji

Kopiuj

inOrder.verify(foo).isCalledOnce().withArguments(....)

Oj polemizowałbym. Zmiana interfejsu pod testy to moim zdaniem zły pomysł, bo to super znak żę funkcja jest nietestowalna.

Ona te pomysły są średnie IMO.

TomRiddle napisał(a):

Oj polemizowałbym. Zmiana interfejsu pod testy to moim zdaniem zły pomysł, bo to super znak żę funkcja jest nietestowalna.

No tak, pytanie co należy w takim razie robić z nietestowalnymi funkcjami (metodami, komponentami...)? Nawet dokumentacja tych cudaków w stylu spy, asercji na kolejność wywołań wspomina, żeby używać tego z legacy kodem, który jest zaspawany na cztery spusty i nie można inaczej. Jak się pisze na bieżąco nowy kod i już potrzebuje takich wynalazków, to nie jest dobrze.

Zostawianie ich nietestowalnymi po wsze czasy, jak już raz ktoś tak napisał może jest spoko, jak się jest w language committee - nie męczysz się z taką decyzją sam, tylko zostawiasz to innym. Ale jak wracasz do tego kodu raz po raz i za każdym razem zachodzisz w głowę, jak testować nietestowalne - już niekoniecznie :P

TomRiddle napisał(a):

Ma zmienić interfejs funkcji, po to żeby ją przetestować? Innymi słowy, napisać nietestowalną funkcję, i zmienić ją żeby była testowalna? I to jest posortowane "od najlepszej"?

Tak. O ile jestem w stanie sobie wyobrazić nietestowalną funkcję, której nietestowalność ma sens - o tyle w tym konkretnym przypadku nie ma powodu, żeby nie zwracać Usera.

Potem patrzysz na kod produkcyjny, widzisz że return-value nigdy nie jest użyte, i myślisz "po co ta funkcja zwraca tego usera w sumie...?", i potem odkrywasz że do testów?

Nie odkrywasz, że to "do testów" tylko że taki jest kontrakt metody, który jest przetestowany.

wartek01 napisał(a):

TomRiddle napisał(a):

Ma zmienić interfejs funkcji, po to żeby ją przetestować? Innymi słowy, napisać nietestowalną funkcję, i zmienić ją żeby była testowalna? I to jest posortowane "od najlepszej"?

Tak. O ile jestem w stanie sobie wyobrazić nietestowalną funkcję, której nietestowalność ma sens - o tyle w tym konkretnym przypadku nie ma powodu, żeby nie zwracać Usera.

Potem patrzysz na kod produkcyjny, widzisz że return-value nigdy nie jest użyte, i myślisz "po co ta funkcja zwraca tego usera w sumie...?", i potem odkrywasz że do testów?

Nie odkrywasz, że to "do testów" tylko że taki jest kontrakt metody, który jest przetestowany.

Nie odkrywasz, że to "do testów" tylko że taki jest kontrakt metody, który jest przetestowany.

Tak, odkrywasz że tylko taki kontrakt jest przetestowany, ergo odkrywasz że inny kontrakt, ten który jest używany w innym miejscu w kodzie nie jest przetestowany.

To mi wygląda jak próba zracjonalizowania sobie złego designu.

Po drugie, taki design jest średni; bo otestujesz w ten sposób tylko to co funkcja zwróci, nie otestujesz nic co ta funkcja zrobi w środku.

superdurszlak napisał(a):

TomRiddle napisał(a):

Oj polemizowałbym. Zmiana interfejsu pod testy to moim zdaniem zły pomysł, bo to super znak żę funkcja jest nietestowalna.

No tak, pytanie co należy w takim razie robić z nietestowalnymi funkcjami (metodami, komponentami...)? Nawet dokumentacja tych cudaków w stylu spy, asercji na kolejność wywołań wspomina, żeby używać tego z legacy kodem, który jest zaspawany na cztery spusty i nie można inaczej. Jak się pisze na bieżąco nowy kod i już potrzebuje takich wynalazków, to nie jest dobrze.

Zostawianie ich nietestowalnymi po wsze czasy, jak już raz ktoś tak napisał może jest spoko, jak się jest w language committee - nie męczysz się z taką decyzją sam, tylko zostawiasz to innym. Ale jak wracasz do tego kodu raz po raz i za każdym razem zachodzisz w głowę, jak testować nietestowalne - już niekoniecznie :P

Powiedzmy, że mamy funkcje jak tą z postu autora wątki. Ma klasę, która dostaje UserService, i klasa robi coś na userze, a potem zapisuje go w bazie przez UserService i nic nie zwraca.

Oto co można zrobić:

1. Sprawdzić czy odpowiedni user jest w bazie. Ma to taką zaletę że można całkowicie zmienić implementację klasy, można użyć dowolnej metody z UserService, można użyć ORM'a albo czegoś zupełnie innego, a i tak test zawsze przetestuje to samo: obecność zmienionego usera w bazie, czyli dokładnie to co ta funkcja miała robić.
2. Przekazać testową implementację UserService, np TestUserService implements UserService, która mogłaby wyglądać jakoś tak:

class TestUserService implements UserService {
    User savedUser;
    void saveUser(User user) {
      savedUser = user;
    }
}

i potem w teście odczytać sobie tego savedUsera, i zobaczyć czy jest zapisany tak jak miał być. Ma to tą zaletę że nie rusza bazy, i testuje kontrakt pomiędzy klasą testowaną a UserService. Rozumiem że dla większości userów to może być szok! Jak to, testowa implementacja?! To się robi mockami przecież! Jeśli chcesz mieć drabinki when()ów i then()ów, to możesz sobie to zrobić mockami, ja wolę robić testowe implementacje serwisów, jak się komuś nie podobają oba podejścia, to może sobie to odczytać z bazy.

Przypominam! Warto robić wszystko co się uważa za dobre, pod warunkiem że się to robi z głową. Nie polecam follować schematów, tylko po to żeby followować. Spodziewanym wynikiem każdego z nas jest to żeby mieć dobrze otestowany kod bez zbędnych przeszkadzajek.

Myślałem że trzeba testować każdą klasę. Jak w takim zrozumieć co trzeba testować, a co nie?

Testuje się funkcje systemu/programu/aplikacji a nie klasy. Jak przetestujesz wszystkie klasy osobno to dostaniesz betonowe testy które uniemożliwiają jakikolwiek refaktoring

TomRiddle napisał(a):

Po drugie, taki design jest średni; bo otestujesz w ten sposób tylko to co funkcja zwróci, nie otestujesz nic co ta funkcja zrobi w środku.

Tyle, że testowanie tak bardzo bezpośrednio tego kontraktu z zależnością wydaje się mocno średnie. Jeśli B jest odrębną jednostką, dlaczego jej kontrakt nie może być testowany osobno? Jeśli A i B tworzą jedną jednostkę, to po co sprawdzać internalsy jednostki, które mogą się zmieniać, o ile kontrakt zostaje zachowany?

Jeśli tutaj A (jakiś magiczny walidator) i B (CRUDowy serwis do zapisu/odczytu użytkowników) traktujemy jako osobne jednostki, to w unit testach interesuje mnie, czy A nie będzie mieć false positive / false negative zależnie od wejścia (np. popsuty token). Nie powinno mnie obchodzić, czy na pewno woła on serwis B, który na pewno coś sobie zapisze w bazie / w pamięci etc. - nieważne, czy to będzie mock, spy, testowa implementacja itd.

W teście integracyjnym chciałbym przetestować ten kawałek aplikacji w całości, bez podstawiania jakichkolwiek protez - i wtedy na pewno będzie mnie interesować, czy po wykonaniu operacji X zmieniającej stan, ten stan ulegnie zmianie. Ale wtedy będę dobijał się o te informacje do publicznego interfejsu tego kawałka - czy to publiczne metody, czy API endpointy etc. Nawet wtedy raczej nie powinienem musieć odwoływać się do sprawdzania testowej implementacji zależności, czy podziało się w niej to, co trzeba.

TomRiddle napisał(a):

Tak, odkrywasz że tylko taki kontrakt jest przetestowany, ergo odkrywasz że inny kontrakt, ten który jest używany w innym miejscu w kodzie nie jest przetestowany.

Nie mam pojęcia skąd ci się to wzięło. Jeśli dorzucisz zwracanie usera to kontrakt powinien być w tym miejscu przetestowany.

To mi wygląda jak próba zracjonalizowania sobie złego designu.

Nie ma znaczenia jak według ciebie to wygląda.

Po drugie, taki design jest średni; bo otestujesz w ten sposób tylko to co funkcja zwróci, nie otestujesz nic co ta funkcja zrobi w środku.

A teraz wracamy do pierwszego postu, gdzie autor wątku pyta się jak sprawdzić czy pola zostały ustawione poprawnie. Przy czym właśnie o to chodzi, że nie powinno cię obchodzić co funkcja zrobi w środku poza tym co zwróci.
W przypadku gdy testujesz bebechy funkcji fiksujesz się na to, że funkcja ma robić to co chcesz zamiast działać tak jak chcesz.

W przypadku tej konkretnej metody masz dwie rzeczy, które cię interesują:

1. Czy pola zostały poprawnie ustawione
2. Czy dane zostały poprawnie zapisane do bazy danych

Pkt. 1 możesz rozwiązać przez dorzucenie zwracanego obiektu.
Pkt. 2 w czystym Mockito jest nierozwiązywalny, możesz jedynie sprawdzić czy wywołano zapis.

wartek01 napisał(a):

TomRiddle napisał(a):

Tak, odkrywasz że tylko taki kontrakt jest przetestowany, ergo odkrywasz że inny kontrakt, ten który jest używany w innym miejscu w kodzie nie jest przetestowany.

Nie mam pojęcia skąd ci się to wzięło. Jeśli dorzucisz zwracanie usera to kontrakt powinien być w tym miejscu przetestowany.

To mi wygląda jak próba zracjonalizowania sobie złego designu.

Nie ma znaczenia jak według ciebie to wygląda.

Po drugie, taki design jest średni; bo otestujesz w ten sposób tylko to co funkcja zwróci, nie otestujesz nic co ta funkcja zrobi w środku.

A teraz wracamy do pierwszego postu, gdzie autor wątku pyta się jak sprawdzić czy pola zostały ustawione poprawnie. Przy czym właśnie o to chodzi, że nie powinno cię obchodzić co funkcja zrobi w środku poza tym co zwróci.
W przypadku gdy testujesz bebechy funkcji fiksujesz się na to, że funkcja ma robić to co chcesz zamiast działać tak jak chcesz.

W przypadku tej konkretnej metody masz dwie rzeczy, które cię interesują:

1. Czy pola zostały poprawnie ustawione
2. Czy dane zostały poprawnie zapisane do bazy danych

Pkt. 1 możesz rozwiązać przez dorzucenie zwracanego obiektu.
Pkt. 2 w czystym Mockito jest nierozwiązywalny, możesz jedynie sprawdzić czy wywołano zapis.

Dobrze, ja dziękuję za rozmowę, widzę że rozmawiam z osobą która nie ma zamiaru zmienić swojego zdania. Życzę szczęścia w dalszych rozterkach programistycznych, i z całego szczęścia życzę Ci żebyś nie trafił nigdy na klasę której nie będzie się dało w ten sposób przetestować (Np funkcja która zapisuje coś do pliku, albo kopiuje coś pomiędzy plikami? ;> Albo funkcja która wyprintowuje coś na ekran, jak w aplikacji konsolowej). Chętnie Ci pomogę wtedy, jeśli założysz temat na forum.

Kończę rozmowę, dziękuję za poświęcony czas; jeśli kiedyś będziesz chciał poszukać innych/lepszych metod testowania, to zapraszam na forum, chętnie pomogę.