@Riddle
W sumie to spoko pomysł. Jestem ciekawy i chętnie się czegoś dowiem, poniżej wrzucam klasę testową.
O panocku.
@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;
Pomijając kwestię używania / nieużywania mocków - masz tutaj niezły bałagan. Trudno powiedzieć, co kierowało taką, a nie inną kolejnością i poziomami enkapsulacji.
Ale tak, zacząłbym od nieużywania @Mock, @Spy i @InjectMocks, i spróbował ograniczyć liczbę potrzebnych abstrakcji. Prawdopodobnie nie potrzebujesz tego aż tyle. @Spy na mapperze też jest chyba tylko po to, by mieć kontrolę nad kodem, który się i tak wykona. Mappers.getMapper(EventMapper.class) brzmi jak coś w stylu service locatora.
@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();
}
Po co właściwie ten mock na UserPrincipal? Nawet nie mogę znaleźć w tym kodzie, do czego UserPrincipal jest właściwie zależnością. Więc pewnie jast w jakiś ciekawy sposób schowany. Ale jak już go masz i potrzebujesz fejkowej implementacji gettera... może po prostu stwórz sobie prawdziwy obiekt z fejkowymi danymi? A jeśli bardzo chcesz mockować, konfigurowanie tego mocka raz po raz w @BeforeEach raczej nic nie wnosi.
Nie do końca też rozumiem, po co ta dana testowa jest tworzona w setUp, ani dlaczego ta metoda w ogóle jest publiczna. Dana się nie zmienia, może być stałą. Jeśli jest mutowalna i jednak się zmienia, można wydzielić metodę która zwróci nową daną w miejscu użycia, nie trzeba tego robić w setUp. Podobnie tworzenie instancji CompetitionTagService w tej metodzie jest IMO dziwne - co to wnosi do testów?
@Test
public void shouldAddTags() {
Nie ma potrzeby by te metody były publiczne.
when(competitionRepository.findByEventName(competition.getEventName())).thenReturn(
Optional.ofNullable(competition));
Ten setup powtarza się w kilku testach. W dodatku ujawnia, że mocki prawdopodobnie są zbędne - skoro mock instruuje competitionRepository, by wyszukiwało sobie competition po jego właściwości eventName to równie dobrze możesz napisać wprost trywialny test double jako in-memory repository i używać go w testach bez mocków:
class InMemoryCompetitionRepository implements CompetitionRepository {
private final Set<Competition> competitions = new HashSet<>();
@Override
public void add(Competition competition) {
competitions.add(competition);
}
@Override
public Optional<Competition> findByEventTag(String eventTag) {
return competitions.stream()
.filter(competition -> competition.eventTag().equals(eventTag))
.findAny();
}
}
No i w zasadzie pozostałe mocki prawdopodobnie wymagają podobnego nakładu pracy, by mockozy i skomplikowanych setupów pozbyc się całkowicie. Zauważ, że to jest może 20% objętości kodu, jaki poświęcasz w jednej tylko klasie testowej na setup mocków.
Set<String> tags = new HashSet<>(Set.of("someTag", "someNextTag"));
ResponseEntity<?> response = competitionTagService.addCompetitionTag(tags, competition.getEventName(), userPrincipal.getName());
Dlaczego ...Service, czyli zakładam że coś domenowego, zwraca tutaj ResponseEntity i w dodatku z nieokreślonym typem? Potem i tak rzutujesz zawartość. ResponseEntity jest czymś związanym z konkretnym frameworkiem (Spring) i konkretnym rodzajem API, który może być zintegrowany z domenowym serwisem (HTTP). To bardzo mocny coupling to bardzo konkretnych rozwiązań, który uniemożliwia reużywanie i testowanie kodu w pełnej izolacji od założenia, że robimy sobie API HTTP w Springu.
verify(competitionRepository, times(1)).save(competition);
Wartością tego testu jest weryfikacja, że .save(...) zostało zawołane dokładnie raz z określonym parametrem. Czyli zasadniczo niczego to nie testuje, poza pożądanymi wywołaniami. Nie testuje efektu końcowego. Używając in-memory repo lub innego sensowengo test double, mógłbyś po prostu zweryfikować w teście faktyczne rezultaty i nie używać Mockito.verify(...).
EventTagsDto returnedEventTagsDto = (EventTagsDto) response.getBody();
assertEquals(response.getStatusCode(), HttpStatus.CREATED);
assert returnedEventTagsDto != null;
assertEquals(competition.getEventName(), returnedEventTagsDto.getEventName());
assertTrue(returnedEventTagsDto.getTags().containsAll(tags));
}
Jeżeli już używasz jakiejś biblioteki do asercji, skąd nagle ten assert x != null;? Poza tym, na liście asercji null check powinien raczej poprzedzać wszelkie inne weryfikacje. No i możesz przy okazji użyć assertAll, nie pamiętam już czy z JUnitpowych asercji, z AssertJ, czy z obu:
Assertions.assertAll(
() -> assertCośtam(),
() -> assertCośInnego(),
// ...
);
Poza tym, te testy zupełnie mieszają przeróżne poziomy abstrakcji. Dlaczego jeden test weryfikuje wywołania, kody HTTP, zwrotki z jakiegoś mockowego repository i pieron wie co jeszcze?
Dodatkowo, w bibliotekach do asercji najczęściej jest konwencja assertCośtam(expected, actual), tutaj mieszasz i raz actual jest na pierwszym, raz na drugim miejscu. W dodatku asercje są napisane w mocno niskopoziomowy sposób - chyba tylko asercja na status code jest w miarę klarowna, pozostałe to trochę taka makarena.
I jeszcze odnośnie sposobu robienia asercji na zawartościach setów:
assertTrue(returnedEventTagsDto.getTags().containsAll(tags));
Odgadywanie, dlaczego taka asercja się wysypała, to będzie masakra i debugowanie. Opcje widzę dwie:
- biblioteka do asercji która dostarcza asercje na kolekcjach
assertThat(collection).containsAll(expected);
- zrobienie sobie samemu odpowiednika takiej asercji, np. załatwienie tego różnicą na zbiorach dowolną metodą, np. z Guavy
var missing = Sets.difference(expected, actual);
assertTrue(missing.isEmpty(), "Following missing elements are expected: %s".format(missing));
Pozostałych testów już nie będę komentował, bo problemy są zasadniczo takie same jak w pierwszym.
Ja piszę normalnie testy bez tej biblioteki i są super.
