refactor(tests): address code review findings and improve clarity

Documentation updates:
- Update test naming convention to givenX_whenY_thenZ BDD pattern
- Update PlayersRepository JavaDoc to reflect derived query methods
- Update assertion examples to BDD then() style

Code improvements:
- Remove redundant @query annotation from findByLeagueContainingIgnoreCase
- Remove unused PlayersRepository mock from controller tests
- Fix Integer comparisons to use .equals() instead of == operator
- Remove redundant size assertions covered by recursive comparison
- Replace magic string "Messi" with explicit variable
- Rename misleading "expected" variables to descriptive names
  (nonExistentId, leagueName, nonExistentSquadNumber, etc.)
- Add content assertion to retrieveAll service test

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

@@ -43,7 +43,7 @@ http://localhost:9001/actuator/health       # Health check
 - **Validation**: Bean Validation annotations in DTOs (`@NotNull`, `@Min`, etc.)
 - **Caching**: Spring `@Cacheable` on service layer (1-hour TTL)
 - **DTO Pattern**: ModelMapper for entity ↔ DTO transformations
-- **Repository**: Spring Data JPA with derived queries and custom JPQL
+- **Repository**: Spring Data JPA with derived queries (prefer over custom JPQL)
 
 ## Code Conventions
 
@@ -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
@@ -97,7 +97,7 @@ http://localhost:9001/actuator/health       # Health check
   ```
 
 - **Annotations**: `@SpringBootTest`, `@AutoConfigureMockMvc`, `@Test`
-- **Assertions**: AssertJ (fluent, e.g., `assertThat(result).isNotNull()`)
+- **Assertions**: AssertJ BDD style (e.g., `then(result).isNotNull()`)
 - **Mocking**: Mockito for service layer tests
 - **Database**: Tests use in-memory SQLite (auto-cleared after each test)
 - **Coverage**: Target high coverage (JaCoCo reports in `target/site/jacoco/`)

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;
@@ -26,8 +24,8 @@
  *
  * <h3>Query Strategies:</h3>
  * <ul>
- * <li><b>@Query:</b> Explicit JPQL for complex searches (findByLeagueContainingIgnoreCase)</li>
- * <li><b>Method Names:</b> Spring Data derives queries from method names (Query Creation)</li>
+ * <li><b>Derived Queries:</b> Spring Data derives queries from method names (findBySquadNumber,
+ * findByLeagueContainingIgnoreCase)</li>
  * </ul>
  *
  * @see Player
@@ -57,14 +55,13 @@ public interface PlayersRepository extends JpaRepository<Player, Long> {
     /**
      * Finds players by league name using case-insensitive wildcard matching.
      * <p>
-     * This method uses a custom JPQL query with LIKE operator for partial matches.
+     * This method uses Spring Data's derived query mechanism to perform partial matching.
      * For example, searching for "Premier" will match "Premier League".
      * </p>
      *
      * @param league the league name to search for (partial matches allowed)
      * @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;
 
@@ -188,7 +184,6 @@ void givenPlayersExist_whenGetAll_thenReturnsOkWithAllPlayers()
         then(response.getContentType()).contains("application/json");
         verify(playersServiceMock, times(1)).retrieveAll();
         then(response.getStatus()).isEqualTo(HttpStatus.OK.value());
-        then(actual).hasSize(26);
         then(actual).usingRecursiveComparison().isEqualTo(expected);
     }
 
@@ -257,7 +252,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
@@ -335,7 +330,6 @@ void givenPlayersExist_whenSearchByLeague_thenReturnsOk()
         then(response.getContentType()).contains("application/json");
         verify(playersServiceMock, times(1)).searchByLeague(any());
         then(response.getStatus()).isEqualTo(HttpStatus.OK.value());
-        then(actual).hasSize(7);
         then(actual).usingRecursiveComparison().isEqualTo(expected);
     }
 

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

@@ -50,27 +50,27 @@ void givenPlayerExists_whenFindById_thenReturnsPlayer() {
     @Test
     void givenPlayerDoesNotExist_whenFindById_thenReturnsEmpty() {
         // Given
-        Long expected = 999L;
+        Long nonExistentId = 999L;
         // When
-        Optional<Player> actual = repository.findById(expected);
+        Optional<Player> actual = repository.findById(nonExistentId);
         // Then
         then(actual).isEmpty();
     }
 
     /**
-     * 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
      */
     @Test
     void givenPlayersExist_whenFindByLeague_thenReturnsList() {
         // Given
-        String expected = "Premier";
+        String leagueName = "Premier";
         // When
-        List<Player> actual = repository.findByLeagueContainingIgnoreCase(expected);
+        List<Player> actual = repository.findByLeagueContainingIgnoreCase(leagueName);
         // Then
         then(actual).isNotEmpty()
-                .allMatch(player -> player.getLeague().toLowerCase().contains(expected.toLowerCase()));
+                .allMatch(player -> player.getLeague().toLowerCase().contains(leagueName.toLowerCase()));
     }
 
     /**
@@ -81,30 +81,29 @@ void givenPlayersExist_whenFindByLeague_thenReturnsList() {
     @Test
     void givenNoPlayersExist_whenFindByLeague_thenReturnsEmptyList() {
         // Given
-        String expected = "Expected League";
+        String nonExistentLeague = "Nonexistent League";
         // When
-        List<Player> actual = repository.findByLeagueContainingIgnoreCase(expected);
+        List<Player> actual = repository.findByLeagueContainingIgnoreCase(nonExistentLeague);
         // Then
         then(actual).isEmpty();
     }
 
     /**
-     * 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 messiSquadNumber = 10; // Pre-seeded from dml.sql
+        String expectedLastName = "Messi";
         // When
-        Optional<Player> actual = repository.findBySquadNumber(expected.getSquadNumber());
+        Optional<Player> actual = repository.findBySquadNumber(messiSquadNumber);
         // Then
         then(actual).isPresent();
-        then(actual.get()).usingRecursiveComparison().isEqualTo(expected);
+        then(actual.get().getSquadNumber()).isEqualTo(messiSquadNumber);
+        then(actual.get().getLastName()).isEqualTo(expectedLastName);
     }
 
     /**
@@ -115,9 +114,9 @@ void givenPlayerExists_whenFindBySquadNumber_thenReturnsPlayer() {
     @Test
     void givenPlayerDoesNotExist_whenFindBySquadNumber_thenReturnsEmpty() {
         // Given
-        Integer expected = 99;
+        Integer nonExistentSquadNumber = 99;
         // When
-        Optional<Player> actual = repository.findBySquadNumber(expected);
+        Optional<Player> actual = repository.findBySquadNumber(nonExistentSquadNumber);
         // Then
         then(actual).isEmpty();
     }

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
@@ -159,7 +159,7 @@ void givenAllPlayersExist_whenRetrieveAll_thenReturns26Players() {
         List<PlayerDTO> actual = playersService.retrieveAll();
         // Then
         verify(playersRepositoryMock, times(1)).findAll();
-        then(actual).hasSize(26);
+        then(actual).usingRecursiveComparison().isEqualTo(dtos);
     }
 
     /**
@@ -216,11 +216,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