Jak poprawnie zarządzać złożonymi zależnościami w testach?

Hej,

mam problem z Mockito podczas wstrzykiwania zależności w testach.
Chodzi o przypadek o przypadek bardziej złożonych zależności
Nie rozumiem dlaczego Mockito nie inicjalizuja poprawnie klas, niektóre są Nullem , choć są oznaczone przez adnotację Mockito

Poniżej przykład:

@RequiredArgsConstructor
@Service
public class CompetitionTagService {

  private final CompetitionRepository competitionRepository;
  private final VerifyMethodsForServices verifyMethodsForServices;
}

@Service
@RequiredArgsConstructor
public class VerifyMethodsForServices {

  private final CompetitionRepository competitionRepository;
  private final FindTeamService findTeamService;
}

Tutaj jak widzimy obie te klasy wykorzystują CompetitionRepository i mam wrażenie, że przez to Mockito się gubi.
Nie wiem tak naprawdę jak mogę to poprawić, tak aby zachować przejrzystość inicjalizacji w testach

Przy takiej konfiguracji:

@Mock
CompetitionRepository competitionRepository;

@Mock
FindTeamService findTeamService;

@InjectMocks
CompetitionTagService competitionTagService;

@InjectMocks
VerifyMethodsForServices verifyMethodsForServices;

Dostaję błąd, że VerifyMethodsForServices jest Nullem (pewnie przez InjectMocks)

Natomiast podczas takiej:

@Mock
CompetitionRepository competitionRepository;

@Mock
FindTeamService findTeamService;

@InjectMocks
CompetitionTagService competitionTagService;

@Mock
VerifyMethodsForServices verifyMethodsForServices;

VerifyMethodsForServices nie widzi CompetitionRepository bo użyłem Mock
Wychodzi takie masło maślane.

Finalnie skończyło się na takie konfiguracji:

@Mock
CompetitionRepository competitionRepository;

@Mock
FindTeamService findTeamService;

CompetitionTagService competitionTagService;

@InjectMocks
VerifyMethodsForServices verifyMethodsForServices;

I po prostu inicjalizuje ręcznie:

competitionTagService = new CompetitionTagService(competitionRepository, verifyMethodsForServices);

Czy da się to jakoś wszystko spiąć annotacjami ?
Czy ręczna inicjalizacja jest po prostu konieczna?

Generalnie nie skupiałbym się w tym przypadku pod względem "architektury" tego typu zależności, choć oczywiście wszelkie rady iw skazówki chętnie przyjmię.
W głównej mierze zależy mi na zrozumieniu mockowania wlaśnie takiego łańcucha zależności.

Za pomoc z góry dzięki.

pitagram napisał(a):

I po prostu inicjalizuje ręcznie:

        competitionTagService = new CompetitionTagService(competitionRepository, verifyMethodsForServices);

No i gratulacje. Nie można było tak od początku?

Czy da się to jakoś wszystko spiąć annotacjami ?

Po co próbujesz na siłę uzyskać nieczytelne adnotacje, skoro, jak sam widzisz, wychodzą z tego same problemy? Nie ma co się bać słówka kluczowego new, tak naprawdę w testach jest to jedyny sensowny sposób tworzenia komponentów.

Znaczy można tak było, ale jakoś tak miałem obawy trochę ze względu na to, że korzystam z Mock, InjectMocks , a tu nagle pojawiam się z operatorem new przy inicjalizacji testu.
Ta rozbieżność jest tak naprawdę jedynym powodem mojej konsternacji na ten temat.

pitagram napisał(a):

Znaczy można tak było, ale jakoś tak miałem obawy trochę ze względu na to, że korzystam z Mock, InjectMocks , a tu nagle pojawiam się z operatorem new przy inicjalizacji testu.
Ta rozbieżność jest tak naprawdę jedynym powodem mojej konsternacji na ten temat.

Zupełnie niepotrzebna ta obawa.

oki, dzięki. Nie byłem pewien, a chciałem dopytać bardziej doświadczonych kolegów.

pitagram napisał(a):

oki, dzięki. Nie byłem pewien, a chciałem dopytać bardziej doświadczonych kolegów.

Ja na Twoim miejscu zastanowiłbym się czy w ogóle używać mockito do testów? Ja piszę normalnie testy bez tej biblioteki i są super.

@Riddle: bez tej biblioteki, ale rozumiem, że nie korzystasz też z zamiennika? Nie dodaje to tak naprawdę pisania dodatkowego kodu, który już masz w tej bibliotece? Przez co tak naprawdę dewelopment się wydłuża?

pitagram napisał(a):

@Riddle: bez tej biblioteki, ale rozumiem, że nie korzystasz też z zamiennika? Nie dodaje to tak naprawdę pisania dodatkowego kodu, który już masz w tej bibliotece? Przez co tak naprawdę dewelopment się wydłuża?

Pokaż Twoje testy z mockami, to ja Ci pokażę jak ja bym to napisał i sam ocenisz.

@Riddle

W sumie to spoko pomysł. Jestem ciekawy i chętnie się czegoś dowiem, poniżej wrzucam klasę testową.

@ExtendWith(MockitoExtension.class)
public class CompetitionTagServiceTest {

    @Mock
    CompetitionRepository competitionRepository;

    CompetitionTagService competitionTagService;

    @InjectMocks
    VerifyMethodsForServices verifyMethodsForServices;


    @Mock
    private UserPrincipal userPrincipal;

    @Spy
    EventMapper eventMapper = Mappers.getMapper(EventMapper.class);

    private static final String userName = "someUserName";

    Competition competition;
    Tag competitionTag;

    @BeforeEach
    public void setUp() {

        competitionTagService = new CompetitionTagService(competitionRepository, verifyMethodsForServices);

        when(userPrincipal.getName()).thenReturn(userName);

        competition = Competition.builder()
                .id(1L)
                .eventOwner(userName)
                .eventName("zawody1")
                .eventStartDate(Timestamp.valueOf("2020-05-01 12:30:00").toLocalDateTime())
                .eventEndDate(Timestamp.valueOf("2020-05-02 12:30:00").toLocalDateTime())
                .city("Gdynia")
                .maxAmountOfTeams(10)
                .tags(Sets.newHashSet())
                .build();

        competitionTag = Tag.builder().tag("Tag").id(1L).competitions(Sets.newHashSet()).build();
    }

    @Test
    public void shouldAddTags() {

        when(competitionRepository.findByEventName(competition.getEventName())).thenReturn(
            Optional.ofNullable(competition));

        Set<String> tags = new HashSet<>(Set.of("someTag", "someNextTag"));
        ResponseEntity<?> response = competitionTagService.addCompetitionTag(tags, competition.getEventName(), userPrincipal.getName());

        verify(competitionRepository, times(1)).save(competition);

        EventTagsDto returnedEventTagsDto = (EventTagsDto) response.getBody();

        assertEquals(response.getStatusCode(), HttpStatus.CREATED);
        assert returnedEventTagsDto != null;
        assertEquals(competition.getEventName(), returnedEventTagsDto.getEventName());
        assertTrue(returnedEventTagsDto.getTags().containsAll(tags));
    }

    @Test
    public void shouldUpdateTag() {

        when(competitionRepository.findByEventName(competition.getEventName())).thenReturn(
            Optional.ofNullable(competition));

        when(competitionRepository.save(competition)).thenReturn(competition);

        String competitionTag = "updatedTag";
        ResponseEntity<?> response = competitionTagService.updateCompetitionTag(competitionTag, competition.getEventName(), userPrincipal.getName());

        verify(competitionRepository, times(1)).save(competition);

        EventTagsDto returnedEventTagsDto = (EventTagsDto) response.getBody();

        assertEquals(response.getStatusCode(), HttpStatus.CREATED);
        assert returnedEventTagsDto != null;
        assertEquals(competition.getEventName(), returnedEventTagsDto.getEventName());
        assertTrue(returnedEventTagsDto.getTags().contains(competitionTag));

    }

    @Test
    public void shouldThrowExceptionCompetitionNotExistsWhenAddTag() {

        Set<String> tags = Set.of("someTag");

        ResponseStatusException exception = assertThrows(
            ResponseStatusException.class,
            () ->  competitionTagService.addCompetitionTag(tags, competition.getEventName(), userPrincipal.getName()),
            "Expected doThing() to throw, but it didn't");

        verify(competitionRepository, times(1)).findByEventName(competition.getEventName());
        assertEquals(HttpStatus.NOT_FOUND, exception.getStatusCode());
        assertEquals("Competition not exists, Name: "+ competition.getEventName(), exception.getReason());
    }


    @Test
    public void shouldThrowExceptionCompetitionNotExistsWhenUpdateTag() {

        String competitionTag = "updatedTag";

        ResponseStatusException exception = assertThrows(
            ResponseStatusException.class,
            () ->  competitionTagService.updateCompetitionTag(competitionTag, competition.getEventName(), userPrincipal.getName()),
            "Expected doThing() to throw, but it didn't");

        verify(competitionRepository, times(1)).findByEventName(competition.getEventName());
        assertEquals(HttpStatus.NOT_FOUND, exception.getStatusCode());
        assertEquals("Competition not exists, Name: "+ competition.getEventName(), exception.getReason());
    }

}

pitagram napisał(a):

@Riddle: bez tej biblioteki, ale rozumiem, że nie korzystasz też z zamiennika? Nie dodaje to tak naprawdę pisania dodatkowego kodu, który już masz w tej bibliotece? Przez co tak naprawdę dewelopment się wydłuża?

Odpowiadając na Twoje pytanie: nie, i to z conajmniej 4ech powodów:

• Po pierwsze - nie mierz czasu pracy na podstawie tego ile piszesz. Zmierz kiedyś stoperem ile czasu piszesz, ile czytasz kod, ile analizujesz, ile szukasz rozwiązań, ile debugujesz. Pisanie to będzie 10%. Ważniejsze jest umiejętne rozumowanie nad kodem.
• Po drugie - biblioteki "do mocków" z reguły mają podobną ilość kodu jak pisanie samych klas - ilośc informacji w kodzie jest podobna, zwłaszcza jak dodamy stubowanie tego jak metody mają działać i te wszystkie verify() i inne takie.
• Po trzecie - kod z mockito bardzo ciężko się czyta (przynajmniej mi), przez to że bardzo trudno nim wyrazić intencje. Kod z mockito mówi jaka mogłaby być przekładowa implementacja, ale nie mówi nic o intencji.

No i po czwarte i najważniejsze - kod powstały z takim agresywnym mockowaniem bardzo często ma zły design i jest przywiązany do implementacji, zwłaszcza jak używamy wspomnianych już verify()'ów.

Co do Twojego kodu, jasno widać że te testy powstały po kodzie, przez co są przywiązane do implementacji tak jak mówiłem. Dobrze napisane testy mogłyby wyglądać jakoś tak:

public class CompetitionTagServiceTest {
    CompetitionTagService tags;

    @BeforeEach
    public void setUp() {
        competitionTagService = new CompetitionTagService(competitionRepository, verifyMethodsForServices);
    }

    @Test
    public void shouldAddManyTags() {
        existingCompetition("zawody1");
        tags.addCompetitionTag(hashSet("someTag", "someNextTag")), "zawody1", "someUserName");
        assertTagExists("someTag");
        assertTagExists("someNextTag");
    }

    @Test
    public void shouldUpdateTag() {
        existingCompetition("zawody1");
        tags.updateCompetitionTag("updatedTag", "zawody1", "user");
        assertTagUpdated("updatedTag");
    }

    @Test
    public void addingMissingTags_throwsException() {
        ResponseStatusException exception = assertThrows(
            ResponseStatusException.class,
            () -> tags.addCompetitionTag("someTag", "zawody1", "someUserName"));

        assertEquals(HttpStatus.NOT_FOUND, exception.getStatusCode());
        assertEquals("Competition not exists, Name: zawody1", exception.getReason());
    }

    @Test
    public void updatingMissingTags_throwsException() {
        ResponseStatusException exception = assertThrows(
            ResponseStatusException.class,
            () -> tags.updateCompetitionTag("updatedTag", "zawody1", "someUserName"));

        assertEquals(HttpStatus.NOT_FOUND, exception.getStatusCode());
        assertEquals("Competition not exists, Name: zawody1", exception.getReason());
    }
}

Metody assertTagExists(), hashSet() oraz existingCompetition() to metody pomocnicze które musisz zaimplementować sam, tak żeby testy pozostały czyste i wyrażające intencje.

Widziałem że niektóre Twoje testy jednocześnie testują odpowiedź i działanie (np. jednocześnie sprawdzasz success-code oraz wynik działania). To jest ogólnie słabe, i powinieneś mieć dwa testy pod to - jeden na odpowiedź update'a (status code), i osobny na działanie update'a (czy tag jest zaktualizowany).

@Riddle: dzięki bardzo za wyjaśnienie. Chyba wszystkie rozumiem, prócz tego co napisałeś W ostatnim akapicie. Jeśli rozdzielę do jednego testu weryfikowania status code, a do drugiego, czy tag został zaaktualizowany to finalnie nie doprowadzę do sytuacji, gdzie testy będą tak naprawdę uruchamiany dwukrotnie przez co zwiększy się czas uruchamiania testów oraz uzyskam trochę "zabrudzenie" kodu z racji tego, że rozdzielam to na dwie osobne metody, choć wszystko mógłbym sprawdzić w jednej? Czy się mylę i nie do końca zrozumiałem, co miałeś na myśli

pitagram napisał(a):

@Riddle: dzięki bardzo za wyjaśnienie. Chyba wszystkie rozumiem, prócz tego co napisałeś W ostatnim akapicie. Jeśli rozdzielę do jednego testu weryfikowania status code, a do drugiego, czy tag został zaaktualizowany to finalnie nie doprowadzę do sytuacji, gdzie testy będą tak naprawdę uruchamiany dwukrotnie przez co zwiększy się czas uruchamiania testów oraz uzyskam trochę "zabrudzenie" kodu z racji tego, że rozdzielam to na dwie osobne metody, choć wszystko mógłbym sprawdzić w jednej? Czy się mylę i nie do końca zrozumiałem, co miałeś na myśli

Owszem, kod uruchomi się dwa razy, ale to żaden problem.

Pamiętaj po co są testy - po to żebyś mógł dowoli refaktorować i usprawniać kod, i użyć testów żeby sprawdzić czy Twój kod nadal działa tak jak powinien. Jeśli coś zepsujesz (dodasz buga), jakiś test powinien nie przejść.

Mając dwa osobne testy:

• Jeśli zepsujesz odpowiedź HTTP, ale logika działa wtedy nie przejdzie jeden test, a drugi tak (wiesz gdzie szukać błędu).
• Jeśli zepujesz logikę, ale odpowiedź działa, wtedy pierwszy test przejdzie, drugi nie (też wiesz gdzie szukać błędu)
• Jeśli zepsujesz obie te rzeczy, wtedy oba nie przejdą (na prawdę coś bardzo zepsułeś)