refactor(tests): address code review findings

Author: nanotaboada

.coderabbit.yaml

@@ -84,7 +84,7 @@ reviews:
         - Test classes should use JUnit 5 (Jupiter)
         - Verify proper Spring test annotations (@WebMvcTest, @DataJpaTest, etc.)
         - Check use of @MockitoBean (Spring Boot 4.0 style)
-        - Ensure test naming follows method_scenario_outcome pattern (concise)
+        - Ensure test naming follows givenX_whenY_thenZ BDD pattern
         - Validate BDD semantics in JavaDoc comments (Given/When/Then)
         - Validate use of AssertJ for fluent assertions
         - Check @DisplayName for readable test descriptions

.github/copilot-instructions.md

@@ -67,21 +67,21 @@ http://localhost:9001/actuator/health       # Health check
 ## Testing
 
 - **Structure**: `*Tests.java` in `src/test/java/` (mirrors main package structure)
-- **Naming Pattern**: `method_scenario_outcome`
-  - `method`: Method under test (e.g., `post`, `findById`, `create`)
-  - `scenario`: Context (e.g., `playerExists`, `invalidData`, `noMatches`)
-  - `outcome`: Expected result (e.g., `returnsPlayer`, `returnsConflict`, `returnsEmpty`)
+- **Naming Pattern**: `givenX_whenY_thenZ` (BDD Given-When-Then)
+  - `given`: Preconditions/context (e.g., `givenPlayerExists`, `givenInvalidData`, `givenNoMatches`)
+  - `when`: Action being tested (e.g., `whenPost`, `whenFindById`, `whenCreate`)
+  - `then`: Expected outcome (e.g., `thenReturnsPlayer`, `thenReturnsConflict`, `thenReturnsEmpty`)
 - **Examples**:
 
   ```java
   // Controller
-  void post_squadNumberExists_returnsConflict()
+  void givenSquadNumberExists_whenPost_thenReturnsConflict()
 
   // Service
-  void create_noConflict_returnsPlayerDTO()
+  void givenNoConflict_whenCreate_thenReturnsPlayerDTO()
 
   // Repository
-  void findById_playerExists_returnsPlayer()
+  void givenPlayerExists_whenFindById_thenReturnsPlayer()
   ```
 
 - **JavaDoc**: BDD Given/When/Then structure in test comments

README.md

@@ -243,10 +243,10 @@ open target/site/jacoco/index.html
 - **Test Database** - SQLite in-memory (jdbc:sqlite::memory:) for fast, isolated test execution
 - **Mocking** - Mockito with `@MockitoBean` for dependency mocking
 - **Assertions** - AssertJ fluent assertions
-- **Naming Convention** - `method_scenario_outcome` pattern:
-  - `getAll_playersExist_returnsOkWithAllPlayers()`
-  - `post_squadNumberExists_returnsConflict()`
-  - `findById_playerExists_returnsPlayer()`
+- **Naming Convention** - `givenX_whenY_thenZ` BDD pattern:
+  - `givenPlayersExist_whenGetAll_thenReturnsOkWithAllPlayers()`
+  - `givenSquadNumberExists_whenPost_thenReturnsConflict()`
+  - `givenPlayerExists_whenFindById_thenReturnsPlayer()`
 
 **Coverage Targets:**
 

src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java

@@ -4,8 +4,6 @@
 import java.util.Optional;
 
 import org.springframework.data.jpa.repository.JpaRepository;
-import org.springframework.data.jpa.repository.Query;
-import org.springframework.data.repository.query.Param;
 import org.springframework.stereotype.Repository;
 
 import ar.com.nanotaboada.java.samples.spring.boot.models.Player;
@@ -65,6 +63,5 @@ public interface PlayersRepository extends JpaRepository<Player, Long> {
      * @return a list of players whose league name contains the search term (empty
      * list if none found)
      */
-    @Query("SELECT p FROM Player p WHERE LOWER(p.league) LIKE LOWER(CONCAT('%', :league, '%'))")
-    List<Player> findByLeagueContainingIgnoreCase(@Param("league") String league);
+    List<Player> findByLeagueContainingIgnoreCase(String league);
 }

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

@@ -32,7 +32,6 @@
 
 import ar.com.nanotaboada.java.samples.spring.boot.controllers.PlayersController;
 import ar.com.nanotaboada.java.samples.spring.boot.models.PlayerDTO;
-import ar.com.nanotaboada.java.samples.spring.boot.repositories.PlayersRepository;
 import ar.com.nanotaboada.java.samples.spring.boot.services.PlayersService;
 import ar.com.nanotaboada.java.samples.spring.boot.test.PlayerDTOFakes;
 
@@ -50,9 +49,6 @@ class PlayersControllerTests {
     @MockitoBean
     private PlayersService playersServiceMock;
 
-    @MockitoBean
-    private PlayersRepository playersRepositoryMock;
-
     @Autowired
     private ObjectMapper objectMapper;
 
@@ -257,7 +253,7 @@ void givenPlayerExists_whenGetBySquadNumber_thenReturnsOk()
         // Given
         Integer squadNumber = 10;
         PlayerDTO expected = PlayerDTOFakes.createAll().stream()
-                .filter(player -> player.getSquadNumber() == squadNumber)
+                .filter(player -> squadNumber.equals(player.getSquadNumber()))
                 .findFirst()
                 .orElseThrow();
         Mockito

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

@@ -58,7 +58,7 @@ void givenPlayerDoesNotExist_whenFindById_thenReturnsEmpty() {
     }
 
     /**
-     * Given players exist in a specific league
+     * Given players exist in a specific league (pre-seeded from dml.sql)
      * When searching by league name
      * Then a list of matching players is returned
      */
@@ -89,22 +89,20 @@ void givenNoPlayersExist_whenFindByLeague_thenReturnsEmptyList() {
     }
 
     /**
-     * Given a player with a specific squad number exists in the database
+     * Given a player with a specific squad number exists (pre-seeded from dml.sql)
      * When querying by that squad number
      * Then the player record is returned
      */
     @Test
     void givenPlayerExists_whenFindBySquadNumber_thenReturnsPlayer() {
         // Given
-        Player expected = PlayerFakes.createAll().stream()
-                .filter(p -> p.getId().equals(10L))
-                .findFirst()
-                .orElseThrow();
+        Integer expectedSquadNumber = 10; // Messi's squad number from pre-seeded data
         // When
-        Optional<Player> actual = repository.findBySquadNumber(expected.getSquadNumber());
+        Optional<Player> actual = repository.findBySquadNumber(expectedSquadNumber);
         // Then
         then(actual).isPresent();
-        then(actual.get()).usingRecursiveComparison().isEqualTo(expected);
+        then(actual.get().getSquadNumber()).isEqualTo(expectedSquadNumber);
+        then(actual.get().getLastName()).isEqualTo("Messi");
     }
 
     /**

src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java

@@ -89,7 +89,7 @@ void givenPlayerAlreadyExists_whenCreate_thenReturnsNull() {
         PlayerDTO dto = PlayerDTOFakes.createOneValid();
         Integer expectedSquadNumber = 10;
         Player existingPlayer = PlayerFakes.createAll().stream()
-                .filter(player -> player.getSquadNumber() == expectedSquadNumber)
+                .filter(player -> expectedSquadNumber.equals(player.getSquadNumber()))
                 .findFirst()
                 .orElseThrow();
         Mockito
@@ -160,6 +160,7 @@ void givenAllPlayersExist_whenRetrieveAll_thenReturns26Players() {
         // Then
         verify(playersRepositoryMock, times(1)).findAll();
         then(actual).hasSize(26);
+        then(actual).usingRecursiveComparison().isEqualTo(dtos);
     }
 
     /**
@@ -216,11 +217,11 @@ void givenPlayerExists_whenRetrieveBySquadNumber_thenReturnsPlayerDTO() {
         // Given
         Integer squadNumber = 10;
         Player entity = PlayerFakes.createAll().stream()
-                .filter(player -> player.getSquadNumber() == squadNumber)
+                .filter(player -> squadNumber.equals(player.getSquadNumber()))
                 .findFirst()
                 .orElseThrow();
         PlayerDTO expected = PlayerDTOFakes.createAll().stream()
-                .filter(player -> player.getSquadNumber() == squadNumber)
+                .filter(player -> squadNumber.equals(player.getSquadNumber()))
                 .findFirst()
                 .orElseThrow();
         Mockito