Skip to content

Commit aeece8e

Browse files
nanotaboadananotaboada
committed
refactor(tests,service): add logging and improve test robustness (#248)
- Add SLF4J logging to PlayersService (debug/info/warn levels) - Remove redundant setId(null) in PlayersRepositoryTests - Fix controller tests to assert Content-Type instead of overriding it - Use id variable consistently in getById test - Fix put_invalidPlayer test ID alignment (use DTO.getId()) - Add @TestConfiguration for ObjectMapper bean injection in tests - Use unnamed pattern for unused DataIntegrityViolationException
1 parent d0d1d04 commit aeece8e

File tree

5 files changed

+72
-30
lines changed

5 files changed

+72
-30
lines changed

‎README.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,10 @@ open target/site/jacoco/index.html
243243
- **Test Database** - SQLite in-memory (jdbc:sqlite::memory:) for fast, isolated test execution
244244
- **Mocking** - Mockito with `@MockitoBean` for dependency mocking
245245
- **Assertions** - AssertJ fluent assertions
246-
- **Naming Convention** - BDD style: `givenMethod_whenDependency[Method]_thenResult`
246+
- **Naming Convention** - `method_scenario_outcome` pattern:
247+
- `getAll_playersExist_returnsOkWithAllPlayers()`
248+
- `post_squadNumberExists_returnsConflict()`
249+
- `findById_playerExists_returnsPlayer()`
247250

248251
**Coverage Targets:**
249252

‎src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
* <li><b>GET</b> {@code /players/search/league/{league}} - Search players by league name</li>
4242
* <li><b>GET</b> {@code /players/search/squadnumber/{number}} - Search player by squad number</li>
4343
* <li><b>POST</b> {@code /players} - Create a new player</li>
44-
* <li><b>PUT</b> {@code /players} - Update an existing player</li>
44+
* <li><b>PUT</b> {@code /players/{id}} - Update an existing player</li>
4545
* <li><b>DELETE</b> {@code /players/{id}} - Delete a player by ID</li>
4646
* </ul>
4747
*

‎src/main/java/ar/com/nanotaboada/java/samples/spring/boot/services/PlayersService.java‎

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,15 @@
55
import org.modelmapper.ModelMapper;
66
import org.springframework.cache.annotation.CacheEvict;
77
import org.springframework.cache.annotation.Cacheable;
8+
import org.springframework.dao.DataIntegrityViolationException;
89
import org.springframework.stereotype.Service;
910
import org.springframework.transaction.annotation.Transactional;
1011

1112
import ar.com.nanotaboada.java.samples.spring.boot.models.Player;
1213
import ar.com.nanotaboada.java.samples.spring.boot.models.PlayerDTO;
1314
import ar.com.nanotaboada.java.samples.spring.boot.repositories.PlayersRepository;
1415
import lombok.RequiredArgsConstructor;
16+
import lombok.extern.slf4j.Slf4j;
1517

1618
/**
1719
* Service layer for managing Player business logic.
@@ -46,6 +48,7 @@
4648
* @see org.modelmapper.ModelMapper
4749
* @since 4.0.2025
4850
*/
51+
@Slf4j
4952
@Service
5053
@RequiredArgsConstructor
5154
public class PlayersService {
@@ -66,8 +69,10 @@ public class PlayersService {
6669
* ID. The result is automatically cached using the player's ID as the cache key.
6770
* </p>
6871
* <p>
69-
* <b>Conflict Detection:</b> Before creating, checks if a player with the same squad number already exists.
70-
* Squad numbers are unique identifiers (jersey numbers).
72+
* <b>Conflict Detection:</b> Checks if a player with the same squad number already exists (optimization).
73+
* Squad numbers are unique identifiers (jersey numbers). Database constraint ensures uniqueness even under
74+
* concurrent operations. If a race condition occurs between check and save, DataIntegrityViolationException
75+
* is caught and null is returned to indicate conflict.
7176
* </p>
7277
*
7378
* @param playerDTO the player data to create (must not be null)
@@ -77,13 +82,26 @@ public class PlayersService {
7782
@Transactional
7883
@CacheEvict(value = "players", allEntries = true)
7984
public PlayerDTO create(PlayerDTO playerDTO) {
80-
// Check if squad number already exists
85+
log.debug("Creating new player with squad number: {}", playerDTO.getSquadNumber());
86+
87+
// Check if squad number already exists (optimization to avoid unnecessary DB write)
8188
if (playersRepository.findBySquadNumber(playerDTO.getSquadNumber()).isPresent()) {
89+
log.warn("Cannot create player - squad number {} already exists", playerDTO.getSquadNumber());
8290
return null; // Conflict: squad number already taken
8391
}
84-
Player player = mapFrom(playerDTO);
85-
Player savedPlayer = playersRepository.save(player);
86-
return mapFrom(savedPlayer);
92+
93+
try {
94+
Player player = mapFrom(playerDTO);
95+
Player savedPlayer = playersRepository.save(player);
96+
PlayerDTO result = mapFrom(savedPlayer);
97+
log.info("Player created successfully - ID: {}, Squad Number: {}", result.getId(), result.getSquadNumber());
98+
return result;
99+
} catch (DataIntegrityViolationException _) {
100+
// Handle race condition: concurrent request created player with same squad number
101+
// between our check and save operation
102+
log.warn("Cannot create player - squad number {} already exists (race condition)", playerDTO.getSquadNumber());
103+
return null;
104+
}
87105
}
88106

89107
/*
@@ -188,11 +206,15 @@ public PlayerDTO searchBySquadNumber(Integer squadNumber) {
188206
@Transactional
189207
@CacheEvict(value = "players", allEntries = true)
190208
public boolean update(PlayerDTO playerDTO) {
209+
log.debug("Updating player with ID: {}", playerDTO.getId());
210+
191211
if (playerDTO.getId() != null && playersRepository.existsById(playerDTO.getId())) {
192212
Player player = mapFrom(playerDTO);
193213
playersRepository.save(player);
214+
log.info("Player updated successfully - ID: {}", playerDTO.getId());
194215
return true;
195216
} else {
217+
log.warn("Cannot update player - ID {} not found", playerDTO.getId());
196218
return false;
197219
}
198220
}
@@ -217,10 +239,14 @@ public boolean update(PlayerDTO playerDTO) {
217239
@Transactional
218240
@CacheEvict(value = "players", allEntries = true)
219241
public boolean delete(Long id) {
242+
log.debug("Deleting player with ID: {}", id);
243+
220244
if (playersRepository.existsById(id)) {
221245
playersRepository.deleteById(id);
246+
log.info("Player deleted successfully - ID: {}", id);
222247
return true;
223248
} else {
249+
log.warn("Cannot delete player - ID {} not found", id);
224250
return false;
225251
}
226252
}

‎src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java‎

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@
1414
import org.mockito.Mockito;
1515
import org.springframework.beans.factory.annotation.Autowired;
1616
import org.springframework.boot.cache.test.autoconfigure.AutoConfigureCache;
17+
import org.springframework.boot.test.autoconfigure.json.AutoConfigureJsonTesters;
18+
import org.springframework.boot.test.context.TestConfiguration;
1719
import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest;
20+
import org.springframework.context.annotation.Bean;
1821
import org.springframework.http.HttpHeaders;
1922
import org.springframework.http.HttpStatus;
2023
import org.springframework.http.MediaType;
@@ -36,6 +39,7 @@
3639
@DisplayName("HTTP Methods on Controller")
3740
@WebMvcTest(PlayersController.class)
3841
@AutoConfigureCache
42+
@AutoConfigureJsonTesters
3943
class PlayersControllerTests {
4044

4145
private static final String PATH = "/players";
@@ -49,6 +53,17 @@ class PlayersControllerTests {
4953
@MockitoBean
5054
private PlayersRepository playersRepositoryMock;
5155

56+
@Autowired
57+
private ObjectMapper objectMapper;
58+
59+
@TestConfiguration
60+
static class ObjectMapperTestConfig {
61+
@Bean
62+
public ObjectMapper objectMapper() {
63+
return new ObjectMapper().findAndRegisterModules();
64+
}
65+
}
66+
5267
/*
5368
* -------------------------------------------------------------------------
5469
* HTTP POST
@@ -67,7 +82,7 @@ void post_validPlayer_returnsCreated()
6782
PlayerDTO playerDTO = PlayerDTOFakes.createOneValid();
6883
PlayerDTO savedPlayer = PlayerDTOFakes.createOneValid();
6984
savedPlayer.setId(19L); // Simulating auto-generated ID
70-
String body = new ObjectMapper().writeValueAsString(playerDTO);
85+
String body = objectMapper.writeValueAsString(playerDTO);
7186
Mockito
7287
.when(playersServiceMock.create(any(PlayerDTO.class)))
7388
.thenReturn(savedPlayer);
@@ -80,7 +95,6 @@ void post_validPlayer_returnsCreated()
8095
.perform(request)
8196
.andReturn()
8297
.getResponse();
83-
response.setContentType("application/json;charset=UTF-8");
8498
// Assert
8599
verify(playersServiceMock, times(1)).create(any(PlayerDTO.class));
86100
assertThat(response.getStatus()).isEqualTo(HttpStatus.CREATED.value());
@@ -98,7 +112,7 @@ void post_invalidPlayer_returnsBadRequest()
98112
throws Exception {
99113
// Arrange
100114
PlayerDTO playerDTO = PlayerDTOFakes.createOneInvalid();
101-
String body = new ObjectMapper().writeValueAsString(playerDTO);
115+
String body = objectMapper.writeValueAsString(playerDTO);
102116
MockHttpServletRequestBuilder request = MockMvcRequestBuilders
103117
.post(PATH)
104118
.content(body)
@@ -123,7 +137,7 @@ void post_squadNumberExists_returnsConflict()
123137
throws Exception {
124138
// Arrange
125139
PlayerDTO playerDTO = PlayerDTOFakes.createOneValid();
126-
String body = new ObjectMapper().writeValueAsString(playerDTO);
140+
String body = objectMapper.writeValueAsString(playerDTO);
127141
Mockito
128142
.when(playersServiceMock.create(any(PlayerDTO.class)))
129143
.thenReturn(null); // Conflict: squad number already exists
@@ -159,7 +173,7 @@ void getById_playerExists_returnsOkWithPlayer()
159173
PlayerDTO playerDTO = PlayerDTOFakes.createOneForUpdate();
160174
Long id = 1L;
161175
Mockito
162-
.when(playersServiceMock.retrieveById(1L))
176+
.when(playersServiceMock.retrieveById(id))
163177
.thenReturn(playerDTO);
164178
MockHttpServletRequestBuilder request = MockMvcRequestBuilders
165179
.get(PATH + "/{id}", id);
@@ -168,11 +182,11 @@ void getById_playerExists_returnsOkWithPlayer()
168182
.perform(request)
169183
.andReturn()
170184
.getResponse();
171-
response.setContentType("application/json;charset=UTF-8");
172185
String content = response.getContentAsString();
173-
PlayerDTO result = new ObjectMapper().readValue(content, PlayerDTO.class);
186+
PlayerDTO result = objectMapper.readValue(content, PlayerDTO.class);
174187
// Assert
175-
verify(playersServiceMock, times(1)).retrieveById(1L);
188+
assertThat(response.getContentType()).contains("application/json");
189+
verify(playersServiceMock, times(1)).retrieveById(id);
176190
assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
177191
assertThat(result).usingRecursiveComparison().isEqualTo(playerDTO);
178192
}
@@ -222,11 +236,11 @@ void getAll_playersExist_returnsOkWithAllPlayers()
222236
.perform(request)
223237
.andReturn()
224238
.getResponse();
225-
response.setContentType("application/json;charset=UTF-8");
226239
String content = response.getContentAsString();
227-
List<PlayerDTO> result = new ObjectMapper().readValue(content, new TypeReference<List<PlayerDTO>>() {
240+
List<PlayerDTO> result = objectMapper.readValue(content, new TypeReference<List<PlayerDTO>>() {
228241
});
229242
// Assert
243+
assertThat(response.getContentType()).contains("application/json");
230244
verify(playersServiceMock, times(1)).retrieveAll();
231245
assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
232246
assertThat(result).hasSize(26);
@@ -255,11 +269,11 @@ void searchByLeague_matchingPlayersExist_returnsOkWithList()
255269
.perform(request)
256270
.andReturn()
257271
.getResponse();
258-
response.setContentType("application/json;charset=UTF-8");
259272
String content = response.getContentAsString();
260-
List<PlayerDTO> result = new ObjectMapper().readValue(content, new TypeReference<List<PlayerDTO>>() {
273+
List<PlayerDTO> result = objectMapper.readValue(content, new TypeReference<List<PlayerDTO>>() {
261274
});
262275
// Assert
276+
assertThat(response.getContentType()).contains("application/json");
263277
verify(playersServiceMock, times(1)).searchByLeague(any());
264278
assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
265279
assertThat(result).hasSize(7);
@@ -285,11 +299,11 @@ void searchByLeague_noMatches_returnsOkWithEmptyList()
285299
.perform(request)
286300
.andReturn()
287301
.getResponse();
288-
response.setContentType("application/json;charset=UTF-8");
289302
String content = response.getContentAsString();
290-
List<PlayerDTO> result = new ObjectMapper().readValue(content, new TypeReference<List<PlayerDTO>>() {
303+
List<PlayerDTO> result = objectMapper.readValue(content, new TypeReference<List<PlayerDTO>>() {
291304
});
292305
// Assert
306+
assertThat(response.getContentType()).contains("application/json");
293307
verify(playersServiceMock, times(1)).searchByLeague(any());
294308
assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
295309
assertThat(result).isEmpty();
@@ -315,10 +329,10 @@ void searchBySquadNumber_playerExists_returnsOkWithPlayer()
315329
.perform(request)
316330
.andReturn()
317331
.getResponse();
318-
response.setContentType("application/json;charset=UTF-8");
319332
String content = response.getContentAsString();
320-
PlayerDTO result = new ObjectMapper().readValue(content, PlayerDTO.class);
333+
PlayerDTO result = objectMapper.readValue(content, PlayerDTO.class);
321334
// Assert
335+
assertThat(response.getContentType()).contains("application/json");
322336
verify(playersServiceMock, times(1)).searchBySquadNumber(10);
323337
assertThat(response.getStatus()).isEqualTo(HttpStatus.OK.value());
324338
assertThat(result).usingRecursiveComparison().isEqualTo(playerDTO);
@@ -367,7 +381,7 @@ void put_playerExists_returnsNoContent()
367381
PlayerDTO playerDTO = PlayerDTOFakes.createOneValid();
368382
playerDTO.setId(1L); // Set ID for update operation
369383
Long id = playerDTO.getId();
370-
String body = new ObjectMapper().writeValueAsString(playerDTO);
384+
String body = objectMapper.writeValueAsString(playerDTO);
371385
Mockito
372386
.when(playersServiceMock.update(any(PlayerDTO.class)))
373387
.thenReturn(true);
@@ -397,7 +411,7 @@ void put_playerNotFound_returnsNotFound()
397411
PlayerDTO playerDTO = PlayerDTOFakes.createOneValid();
398412
playerDTO.setId(999L); // Set ID for update operation
399413
Long id = playerDTO.getId();
400-
String body = new ObjectMapper().writeValueAsString(playerDTO);
414+
String body = objectMapper.writeValueAsString(playerDTO);
401415
Mockito
402416
.when(playersServiceMock.update(any(PlayerDTO.class)))
403417
.thenReturn(false);
@@ -425,8 +439,8 @@ void put_invalidPlayer_returnsBadRequest()
425439
throws Exception {
426440
// Arrange
427441
PlayerDTO playerDTO = PlayerDTOFakes.createOneInvalid();
428-
Long id = 1L;
429-
String body = new ObjectMapper().writeValueAsString(playerDTO);
442+
Long id = playerDTO.getId();
443+
String body = objectMapper.writeValueAsString(playerDTO);
430444
MockHttpServletRequestBuilder request = MockMvcRequestBuilders
431445
.put(PATH + "/{id}", id)
432446
.content(body)
@@ -453,7 +467,7 @@ void put_idMismatch_returnsBadRequest()
453467
PlayerDTO playerDTO = PlayerDTOFakes.createOneValid();
454468
playerDTO.setId(999L); // Body has different ID
455469
Long pathId = 1L; // Path has different ID
456-
String body = new ObjectMapper().writeValueAsString(playerDTO);
470+
String body = objectMapper.writeValueAsString(playerDTO);
457471
MockHttpServletRequestBuilder request = MockMvcRequestBuilders
458472
.put(PATH + "/{id}", pathId)
459473
.content(body)

‎src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ class PlayersRepositoryTests {
3535
void findById_playerExists_returnsPlayer() {
3636
// Arrange
3737
Player leandro = PlayerFakes.createOneValid();
38-
leandro.setId(null);
3938
Player saved = repository.save(leandro);
4039
// Act
4140
Optional<Player> actual = repository.findById(saved.getId());

0 commit comments

Comments
 (0)