test: adopt BDD Given-When-Then pattern across all tests#266
Merged
nanotaboada merged 7 commits intomasterfrom Feb 18, 2026
Merged
test: adopt BDD Given-When-Then pattern across all tests#266nanotaboada merged 7 commits intomasterfrom
nanotaboada merged 7 commits intomasterfrom
Conversation
- Rename all test methods to givenX_whenY_thenZ() format - Replace AssertJ assertThat() with BDDAssertions then() - Update code comments from Arrange/Act/Assert to Given/When/Then - Apply semantic variable naming (expected/actual for assertions, dto/entity for setup) - Eliminate magic strings/numbers from assertions - Update all JavaDocs to abstract business descriptions - Ensure comprehensive object comparisons using usingRecursiveComparison() - Reorder test methods to match controller/service method order - Refactor searchBySquadNumber to retrieveBySquadNumber (RESTful GET pattern)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GET /players (retrieveAll), moves squad-number lookup to GET /players/squadnumber/{squadNumber} returning a single PlayerDTO or 404, replaces a JPQL league query with a derived repository method, and converts tests to BDD Given/When/Then style with assertion changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant Cache
participant Repository
participant DB
Client->>Controller: GET /players/squadnumber/{squadNumber}
Controller->>Service: retrieveBySquadNumber(squadNumber)
Service->>Cache: check "squad-#{squadNumber}"
alt cache hit
Cache-->>Service: cached PlayerDTO
else cache miss
Service->>Repository: findBySquadNumber(squadNumber)
Repository->>DB: query by squad_number
DB-->>Repository: row / empty
Repository-->>Service: Player entity / empty
Service->>Cache: store "squad-#{squadNumber}"
end
Service-->>Controller: PlayerDTO / null
Controller-->>Client: 200 + PlayerDTO / 404 Not Found
sequenceDiagram
participant Client
participant Controller
participant Service
participant Cache
participant Repository
participant DB
Client->>Controller: GET /players
Controller->>Service: retrieveAll()
Service->>Cache: check "players"
alt cache hit
Cache-->>Service: cached List<PlayerDTO>
else cache miss
Service->>Repository: findAll()
Repository->>DB: select * from players
DB-->>Repository: rows
Repository-->>Service: List<Player>
Service->>Cache: store "players"
end
Service-->>Controller: List<PlayerDTO>
Controller-->>Client: 200 + List<PlayerDTO>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #266 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 28 28
===========================================
Files 2 2
Lines 77 77
Branches 8 8
===========================================
Hits 77 77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/copilot-instructions.md:
- Line 36: Replace the old test naming example
`post_squadNumberExists_returnsConflict` with the BDD Given-When-Then pattern
used elsewhere in this file; update the example to follow `givenX_whenY_thenZ`
(e.g., `givenSquad_whenPostAndNumberExists_thenReturnsConflict`) so it matches
the documented pattern referenced by `givenX_whenY_thenZ`.
In `@AGENTS.md`:
- Line 65: The test name getAll_playersExist_returnsOkWithAllPlayers uses the
old method_scenario_outcome pattern; update the test method name to follow the
BDD Given-When-Then pattern (e.g.,
givenPlayersExist_whenGetAll_thenReturnsOkWithAllPlayers) and update any
references (test runner command example and/or test invocation like ./mvnw test
-Dtest=PlayersControllerTests#getAll_playersExist_returnsOkWithAllPlayers) to
use the new method name so the example and test code are consistent with the
PR's BDD naming convention.
🧹 Nitpick comments (6)
src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java (1)
68-69: The explicit@Queryis redundant with the derived query method name.Spring Data JPA automatically generates the equivalent query from the method name
findByLeagueContainingIgnoreCase. The explicit@Queryduplicates that behavior and introduces a maintenance burden — if the entity field name changes, you'd need to update both the method name and the JPQL string.That said, if this is a deliberate choice for readability/documentation, it's perfectly functional as-is.
♻️ Optionally remove the redundant `@Query`
- `@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/repositories/PlayersRepositoryTests.java (1)
66-74: Same pre-seeded data dependency as the squad-number test.This test queries
findByLeagueContainingIgnoreCase("Premier")without saving any players first, relying on pre-seeded data. This is consistent with the squad-number test (Line 97) but inconsistent with thefindByIdtests (Lines 35, 51) that use explicit saves.src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java (2)
258-262:==comparison onIntegerobjects — safe here but fragile.Line 260 uses
player.getSquadNumber() == squadNumberto compareIntegerobjects. This works for values in the [-128, 127] range due to Integer caching, but is a subtle trap if the value ever exceeds that range. Prefer.equals()or unbox explicitly.♻️ Use .equals() for Integer comparison
- .filter(player -> player.getSquadNumber() == squadNumber) + .filter(player -> squadNumber.equals(player.getSquadNumber()))
53-54: Remove unused@MockitoBeanforPlayersRepository.The
playersRepositoryMockfield is never referenced in any test.@WebMvcTestcontroller tests should only mock the service layer.♻️ Remove unused mock
- `@MockitoBean` - private PlayersRepository playersRepositoryMock; -src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java (2)
215-239:==onIntegerobjects appears in multiple filter lambdas across test files.Lines 219 and 223 use
player.getSquadNumber() == squadNumberwithIntegerobjects. Same pattern appears inPlayersControllerTests.java(Line 260) and earlier in this file (Line 92). While safe for small values due to Integer caching, using.equals()is the idiomatic and safe approach.♻️ Use .equals() consistently
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();Apply the same fix at Line 92 in this file.
144-163:retrieveAlltest — adequate coverage.The loop-based mock setup (Lines 153–157) correctly maps each entity to its DTO counterpart. Consider adding
usingRecursiveComparison().isEqualTo(dtos)on the result for content validation beyond size, matching the thoroughness of other tests in this file.♻️ Add content assertion
// Then verify(playersRepositoryMock, times(1)).findAll(); then(actual).hasSize(26); + then(actual).usingRecursiveComparison().isEqualTo(dtos);
nanotaboada
force-pushed
the
test/bdd-given-when-then-pattern
branch
from
61d9aad to
1f915ff
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/copilot-instructions.md (1)
72-72: Consider specifying BDDAssertions for consistency.Line 72 mentions "AssertJ for fluent assertions" generically, while line 109 specifically requires
then()fromAssertJ BDDAssertions. For consistency and clarity, consider mentioning BDDAssertions explicitly here.📝 Proposed clarification
-- **Unit tests**: JUnit 5 with AssertJ for fluent assertions +- **Unit tests**: JUnit 5 with AssertJ BDDAssertions (`then()` for assertions)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java (1)
25-29:⚠️ Potential issue | 🟡 MinorStale JavaDoc – both class-level and method-level descriptions still reference
@Query/JPQLThe class-level javadoc (Line 27) still lists
"@Query: Explicit JPQL for complex searches (findByLeagueContainingIgnoreCase)", and the method-level javadoc (Line 58) still reads"This method uses a custom JPQL query with LIKE operator for partial matches". Both are now incorrect: the method is a standard derived query and no JPQL annotation exists.📝 Suggested correction
- * <li><b>@Query:</b> Explicit JPQL for complex searches (findByLeagueContainingIgnoreCase)</li> + * <li><b>Derived Queries:</b> findBySquadNumber, findByLeagueContainingIgnoreCase (method name conventions)</li>- * This method uses a custom JPQL query with LIKE operator for partial matches. - * For example, searching for "Premier" will match "Premier League". + * This is a derived query method – Spring Data JPA generates the query automatically + * from the method name. For example, searching for "Premier" will match "Premier League".Also applies to: 55-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java` around lines 25 - 29, Update the stale Javadoc in PlayersRepository: remove references to "@Query" and "JPQL" at the class-level and in the method-level JavaDoc for findByLeagueContainingIgnoreCase, and replace them with accurate descriptions stating that the repository uses Spring Data derived query methods (e.g., findByLeagueContainingIgnoreCase performs a case-insensitive partial match derived from the method name) so the docs reflect the actual implementation.src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java (1)
30-31:⚠️ Potential issue | 🔴 CriticalUpdate Jackson imports to Jackson 3 package names for Spring Boot 4.0 compatibility
Lines 30–31 import
com.fasterxml.jackson.core.type.TypeReferenceandcom.fasterxml.jackson.databind.ObjectMapperfrom Jackson 2 packages (com.fasterxml.jackson.*). Spring Boot 4.0 defaults to Jackson 3, which relocates databind and core classes to thetools.jackson.*package namespace (Jackson 3 usestools.jackson.databind.json.JsonMapperandtools.jackson.databind.ObjectMapper). Since the pom.xml declares Spring Boot 4.0.0 without explicit Jackson 2 retention, these imports will fail to compile.The
ObjectMapperTestConfiginner class (lines 55–61) also instantiatesnew ObjectMapper()using the Jackson 2 API, which must be updated to use the Jackson 3 equivalent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java` around lines 30 - 31, Update the Jackson 2 imports and usages to Jackson 3 names: replace imports of com.fasterxml.jackson.core.type.TypeReference and com.fasterxml.jackson.databind.ObjectMapper with the Jackson 3 equivalents (tools.jackson.core.type.TypeReference or tools.jackson.databind.type.TypeFactory as needed and tools.jackson.databind.ObjectMapper / tools.jackson.databind.json.JsonMapper), and modify the ObjectMapperTestConfig inner class to instantiate the Jackson 3 mapper (use new JsonMapper() or JsonMapper.builder().build() instead of new ObjectMapper()). Ensure any TypeReference usages reference the proper tools.jackson package names or adjust to use JsonMapper.readValue with explicit TypeFactory if necessary so compilation against Spring Boot 4/Jackson 3 succeeds.
🧹 Nitpick comments (4)
src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java (1)
162-163:hasSize(26)is both a magic number and redundant
then(actual).hasSize(26)on Line 162 is immediately followed bythen(actual).usingRecursiveComparison().isEqualTo(dtos)on Line 163. The recursive equality check already validates size. Using the magic literal26is also fragile — if the test data grows, this breaks whiledtos.size()would not.♻️ Suggested fix
- then(actual).hasSize(26); - then(actual).usingRecursiveComparison().isEqualTo(dtos); + then(actual).usingRecursiveComparison().isEqualTo(dtos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java` around lines 162 - 163, The assertion then(actual).hasSize(26) in PlayersServiceTests is a magic-number, redundant with then(actual).usingRecursiveComparison().isEqualTo(dtos); either remove the hasSize call or make it non-fragile by comparing against dtos.size() instead; update the test to delete the hard-coded 26 or replace it with then(actual).hasSize(dtos.size()) so the size check stays consistent with the dtos collection used in the equality assertion.src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java (2)
104-105: Magic string"Messi"in assertion contradicts the PR's stated goal of eliminating magic stringsLine 105 asserts
then(actual.get().getLastName()).isEqualTo("Messi")— a hardcoded value. The expected last name can be derived from the already-availablePlayerFakesdata to stay consistent and resilient to fixture changes.♻️ Suggested fix
Integer expectedSquadNumber = 10; // Messi's squad number from pre-seeded data + String expectedLastName = PlayerFakes.createAll().stream() + .filter(p -> expectedSquadNumber.equals(p.getSquadNumber())) + .map(Player::getLastName) + .findFirst() + .orElseThrow(); // When Optional<Player> actual = repository.findBySquadNumber(expectedSquadNumber); // Then then(actual).isPresent(); then(actual.get().getSquadNumber()).isEqualTo(expectedSquadNumber); - then(actual.get().getLastName()).isEqualTo("Messi"); + then(actual.get().getLastName()).isEqualTo(expectedLastName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java` around lines 104 - 105, The test in PlayersRepositoryTests uses a hardcoded magic string in the assertion then(actual.get().getLastName()).isEqualTo("Messi"); — replace the literal with the canonical expected value from your fixtures (PlayerFakes) so the test follows the PR goal of removing magic strings; for example, obtain the expected last name from PlayerFakes (e.g., PlayerFakes.getExpectedPlayer(...).getLastName() or a PlayerFakes.MESSI_LAST_NAME constant) and use that in the assertion alongside the existing expectedSquadNumber check.
53-53:expectedused as a name for input/query parameters, not expected outputsOn lines 53, 84, and 116, variables named
expectedhold query inputs (Long expected = 999L;,String expected = "Expected League";,Integer expected = 99;). In a Given/When/Then context,expectedconventionally refers to an anticipated result. Using it for inputs is misleading and conflicts with the semantic naming introduced elsewhere in this PR.♻️ Suggested rename
- Long expected = 999L; + Long nonExistentId = 999L; // When - Optional<Player> actual = repository.findById(expected); + Optional<Player> actual = repository.findById(nonExistentId);- String expected = "Expected League"; + String nonExistentLeague = "Expected League"; // When - List<Player> actual = repository.findByLeagueContainingIgnoreCase(expected); + List<Player> actual = repository.findByLeagueContainingIgnoreCase(nonExistentLeague);- Integer expected = 99; + Integer nonExistentSquadNumber = 99; // When - Optional<Player> actual = repository.findBySquadNumber(expected); + Optional<Player> actual = repository.findBySquadNumber(nonExistentSquadNumber);Also applies to: 84-84, 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java` at line 53, Rename the misleading local variables named expected (e.g., Long expected = 999L;, String expected = "Expected League";, Integer expected = 99;) to names that reflect they are query/input parameters (for example playerId, leagueName, scoreThreshold or similar) and update all uses in PlayersRepositoryTests (including assertions and repository calls) so the Given/When/Then semantics are clear and 'expected' is reserved for asserted results.src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java (1)
187-188: RedundanthasSize()checks with magic numbers
- Lines 187–188:
then(actual).hasSize(26)is immediately followed bythen(actual).usingRecursiveComparison().isEqualTo(expected), making the size check redundant. The magic literal26will also need manual updating if the fixture changes.- Line 334:
then(actual).hasSize(7)uses a magic number that is implicitly derived from the "Premier" league filter applied toexpected. Usingexpected.size()is self-documenting and resilient.♻️ Suggested fixes
- then(actual).hasSize(26); then(actual).usingRecursiveComparison().isEqualTo(expected);- then(actual).hasSize(7); + then(actual).hasSize(expected.size()); then(actual).usingRecursiveComparison().isEqualTo(expected);Also applies to: 334-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java` around lines 187 - 188, Remove the brittle magic-number size assertions and use the expected collection size or drop the redundant check: replace then(actual).hasSize(26) (in PlayersControllerTests) with then(actual).hasSize(expected.size()) or remove it entirely since then(actual).usingRecursiveComparison().isEqualTo(expected) already validates size; do the same for then(actual).hasSize(7) (replace with expected.size() or remove).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Line 75: Test methods (e.g., givenValidPlayer_whenPost_thenReturnsCreated)
violate the required test naming convention; rename each test method to follow
method_scenario_outcome (for example
createPlayer_withValidPlayer_returnsCreated) and apply the same refactor to all
other test methods listed (lines referenced methods) so names reflect the
invoked method, the specific scenario, and the expected outcome (keep method
body unchanged, only rename the test identifiers).
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java`:
- Line 35: Rename the test methods to follow the method_scenario_outcome
convention instead of the BDD givenX_whenY_thenZ names; specifically, rename
givenPlayerExists_whenFindById_thenReturnsPlayer to
findById_playerExists_returnsPlayer and apply the same transformation to the
other test methods in this class (the methods currently named with the
givenX_whenY_thenZ pattern), updating any references/imports or `@Test`
annotations as needed so method names compile and reflect the required naming
convention.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java`:
- Line 55: Rename all test methods in PlayersServiceTests from the
givenX_whenY_thenZ pattern to the project's required method_scenario_outcome
pattern; locate each offending test method (e.g., the method currently named
"givenNoExistingPlayer_whenCreate_thenReturnsPlayerDTO" in class
PlayersServiceTests and the other methods flagged at lines
87,112,145,172,196,216,248,274,307,333,358,380,405,424) and change their names
to a concise method_scenario_outcome form (for example
"create_noExistingPlayer_returnsPlayerDTO"), keeping the existing test bodies
and updating any references/annotations if necessary to compile.
---
Outside diff comments:
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java`:
- Around line 25-29: Update the stale Javadoc in PlayersRepository: remove
references to "@Query" and "JPQL" at the class-level and in the method-level
JavaDoc for findByLeagueContainingIgnoreCase, and replace them with accurate
descriptions stating that the repository uses Spring Data derived query methods
(e.g., findByLeagueContainingIgnoreCase performs a case-insensitive partial
match derived from the method name) so the docs reflect the actual
implementation.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Around line 30-31: Update the Jackson 2 imports and usages to Jackson 3 names:
replace imports of com.fasterxml.jackson.core.type.TypeReference and
com.fasterxml.jackson.databind.ObjectMapper with the Jackson 3 equivalents
(tools.jackson.core.type.TypeReference or
tools.jackson.databind.type.TypeFactory as needed and
tools.jackson.databind.ObjectMapper / tools.jackson.databind.json.JsonMapper),
and modify the ObjectMapperTestConfig inner class to instantiate the Jackson 3
mapper (use new JsonMapper() or JsonMapper.builder().build() instead of new
ObjectMapper()). Ensure any TypeReference usages reference the proper
tools.jackson package names or adjust to use JsonMapper.readValue with explicit
TypeFactory if necessary so compilation against Spring Boot 4/Jackson 3
succeeds.
---
Nitpick comments:
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Around line 187-188: Remove the brittle magic-number size assertions and use
the expected collection size or drop the redundant check: replace
then(actual).hasSize(26) (in PlayersControllerTests) with
then(actual).hasSize(expected.size()) or remove it entirely since
then(actual).usingRecursiveComparison().isEqualTo(expected) already validates
size; do the same for then(actual).hasSize(7) (replace with expected.size() or
remove).
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java`:
- Around line 104-105: The test in PlayersRepositoryTests uses a hardcoded magic
string in the assertion then(actual.get().getLastName()).isEqualTo("Messi"); —
replace the literal with the canonical expected value from your fixtures
(PlayerFakes) so the test follows the PR goal of removing magic strings; for
example, obtain the expected last name from PlayerFakes (e.g.,
PlayerFakes.getExpectedPlayer(...).getLastName() or a
PlayerFakes.MESSI_LAST_NAME constant) and use that in the assertion alongside
the existing expectedSquadNumber check.
- Line 53: Rename the misleading local variables named expected (e.g., Long
expected = 999L;, String expected = "Expected League";, Integer expected = 99;)
to names that reflect they are query/input parameters (for example playerId,
leagueName, scoreThreshold or similar) and update all uses in
PlayersRepositoryTests (including assertions and repository calls) so the
Given/When/Then semantics are clear and 'expected' is reserved for asserted
results.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java`:
- Around line 162-163: The assertion then(actual).hasSize(26) in
PlayersServiceTests is a magic-number, redundant with
then(actual).usingRecursiveComparison().isEqualTo(dtos); either remove the
hasSize call or make it non-fragile by comparing against dtos.size() instead;
update the test to delete the hard-coded 26 or replace it with
then(actual).hasSize(dtos.size()) so the size check stays consistent with the
dtos collection used in the equality assertion.
...ava/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java
Show resolved
Hide resolved
...va/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java
Show resolved
Hide resolved
...test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java
Show resolved
Hide resolved
nanotaboada
force-pushed
the
test/bdd-given-when-then-pattern
branch
from
bc5ca73 to
e9ca527
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
171-177:⚠️ Potential issue | 🟡 MinorUpdate line 174 to match the actual endpoint path in PlayersController.
The README documents
GET /players/search/squadnumber/{squadNumber}, but the controller implementsGET /players/squadnumber/{squadNumber}(line 163 in PlayersController.java). Remove the/searchsegment to match the implementation.Suggested correction
- `GET /players/search/squadnumber/{squadNumber}` - Get player by squad number + `GET /players/squadnumber/{squadNumber}` - Get player by squad number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 171 - 177, The README entry for the squad number endpoint is incorrect; update the documented path to match PlayersController's implemented route by replacing the README line that reads `GET /players/search/squadnumber/{squadNumber}` with `GET /players/squadnumber/{squadNumber}` so the documentation aligns with the PlayersController endpoint.
🧹 Nitpick comments (2)
README.md (1)
246-249: LGTM — minor naming nit on example 3.Examples 1 and 2 are clean and well-aligned with the BDD convention. Example 3 (
whenFindById) uses JPA repository-style naming; given that the PR adopts RESTful verb prefixes elsewhere (e.g.,retrieveBySquadNumber),whenGetByIdorwhenRetrieveByIdwould be more consistent.✏️ Optional nit
- - `givenPlayerExists_whenFindById_thenReturnsPlayer()` + - `givenPlayerExists_whenRetrieveById_thenReturnsPlayer()`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 246 - 249, Update the README BDD naming example to use RESTful verb consistency: replace the third example name givenPlayerExists_whenFindById_thenReturnsPlayer() with either givenPlayerExists_whenGetById_thenReturnsPlayer() or givenPlayerExists_whenRetrieveById_thenReturnsPlayer() so it matches the RESTful verb style used elsewhere (e.g., retrieveBySquadNumber); ensure any accompanying examples or references in the README use the same chosen verb.src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java (1)
86-104: UnusedexpectedSquadNumbervariable creates slight confusion.The local
expectedSquadNumber = 10on Line 90 is only used to pick an arbitrary existing player fromcreateAll(). Consider inlining the filter value or renaming the variable to something likeexistingSquadNumberto better convey intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java` around lines 86 - 104, The test method givenPlayerAlreadyExists_whenCreate_thenReturnsNull uses a confusing local variable expectedSquadNumber that’s only used to pick an existing player; either inline the literal (replace expectedSquadNumber with 10 in the filter) or rename the variable to existingSquadNumber to convey intent; update references in the test (PlayerFakes.createAll() filter and any comments) so the purpose is clear and no unused/ambiguous variable remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Line 100: Update the Assertions example to use the BDD style by replacing the
`assertThat(...)` usage with `BDDAssertions.then(...)` (or the shorter
`then(...)` import) so the guidance matches the PR convention; locate the
example line containing `assertThat(result).isNotNull()` and change it to
`BDDAssertions.then(result).isNotNull()` (or `then(result).isNotNull()` if
`then` is statically imported) and ensure any surrounding text mentions the BDD
`then()` style instead of `assertThat()`.
- Line 46: The "custom JPQL" entry in .github/copilot-instructions.md is now
stale because the PR replaced the JPQL query with a derived repository method;
update the file by removing or replacing the "custom JPQL" entry so it no longer
suggests JPQL for this repository and instead references the derived query
approach (look for the literal "custom JPQL" entry and update that section to
mention the derived Spring Data JPA method).
---
Outside diff comments:
In `@README.md`:
- Around line 171-177: The README entry for the squad number endpoint is
incorrect; update the documented path to match PlayersController's implemented
route by replacing the README line that reads `GET
/players/search/squadnumber/{squadNumber}` with `GET
/players/squadnumber/{squadNumber}` so the documentation aligns with the
PlayersController endpoint.
---
Duplicate comments:
In @.github/copilot-instructions.md:
- Around line 89-97: The JavaDoc example uses the deprecated
method_scenario_outcome naming (void post_squadNumberExists_returnsConflict()),
so update that example to the mandated givenX_whenY_thenZ pattern by renaming
the test method to a descriptive given/when/then form (e.g.,
givenPlayerWithSquadNumber5_whenPostPlayers_thenConflict or
givenPlayerWithSquadNumber5_whenPostPlayer_thenReturnsConflict) and update the
JavaDoc snippet accordingly to reflect the standardized naming convention.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Line 75: Multiple test methods (e.g.,
givenValidPlayer_whenPost_thenReturnsCreated) use the givenX_whenY_thenZ naming
while the project standard is method_scenario_outcome; rename these test methods
to follow method_scenario_outcome (for example rename
givenValidPlayer_whenPost_thenReturnsCreated to
createPlayer_validRequest_returnsCreated), apply the same pattern to the other
test methods flagged (all methods matching the given*_when*_then* pattern),
update any usages or IDE-run configurations that reference those method names,
and keep test annotations and assertions unchanged.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java`:
- Line 35: Rename the test methods to use the project's method_scenario_outcome
naming convention instead of givenX_whenY_thenZ; for example change
givenPlayerExists_whenFindById_thenReturnsPlayer to
findById_existingPlayer_returnsPlayer, and similarly rename the other tests at
the indicated locations (lines referenced) to follow the same pattern (e.g.,
findAll_noPlayers_returnsEmpty, save_newPlayer_persistsPlayer,
delete_existingPlayer_removesPlayer, update_existingPlayer_updatesFields).
Update any direct references to these test method names if present (e.g., in
test suites or annotations).
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java`:
- Line 55: Rename the test methods in PlayersServiceTests to use the project's
canonical method_scenario_outcome pattern (instead of givenX_whenY_thenZ); for
example rename givenNoExistingPlayer_whenCreate_thenReturnsPlayerDTO to
create_noExistingPlayer_returnsPlayerDTO, and apply the same transformation to
the other test methods mentioned (the ones at the other flagged locations) so
all test method names in PlayersServiceTests follow method_scenario_outcome
consistently.
---
Nitpick comments:
In `@README.md`:
- Around line 246-249: Update the README BDD naming example to use RESTful verb
consistency: replace the third example name
givenPlayerExists_whenFindById_thenReturnsPlayer() with either
givenPlayerExists_whenGetById_thenReturnsPlayer() or
givenPlayerExists_whenRetrieveById_thenReturnsPlayer() so it matches the RESTful
verb style used elsewhere (e.g., retrieveBySquadNumber); ensure any accompanying
examples or references in the README use the same chosen verb.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java`:
- Around line 86-104: The test method
givenPlayerAlreadyExists_whenCreate_thenReturnsNull uses a confusing local
variable expectedSquadNumber that’s only used to pick an existing player; either
inline the literal (replace expectedSquadNumber with 10 in the filter) or rename
the variable to existingSquadNumber to convey intent; update references in the
test (PlayerFakes.createAll() filter and any comments) so the purpose is clear
and no unused/ambiguous variable remains.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java (1)
19-23:⚠️ Potential issue | 🟡 MinorStale Javadoc bullet: "Custom Query Methods" no longer applies
After removing the
@Query-annotated JPQL method, the "Custom Query Methods" bullet at line 21 is inaccurate —findByLeagueContainingIgnoreCaseis now a derived query, not a JPQL custom query. The description already lists both derived methods correctly in the "Query Strategies" block (lines 27–28); the "Custom Query Methods" entry in "Provided Methods" should be updated to reflect the derived query approach.📝 Proposed Javadoc fix
- * <li><b>Custom Query Methods:</b> League search with case-insensitive wildcard matching</li> - * <li><b>Derived Queries:</b> findBySquadNumber (method name conventions)</li> + * <li><b>Derived Queries:</b> findBySquadNumber, findByLeagueContainingIgnoreCase (method name conventions)</li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java` around lines 19 - 23, Update the Javadoc in PlayersRepository to remove or reword the stale "Custom Query Methods" bullet: replace the reference to a JPQL custom query with a note that findByLeagueContainingIgnoreCase is a derived query (or simply list derived queries), ensuring the "Provided Methods" section correctly reflects that findByLeagueContainingIgnoreCase and findBySquadNumber are derived query methods rather than `@Query` JPQL methods; adjust the bullet text near the list that mentions "Custom Query Methods" so it aligns with the "Query Strategies" block and the actual repository methods.
🧹 Nitpick comments (1)
src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java (1)
274-275: Redundant assertion on Line 275
then(actual.getSquadNumber()).isEqualTo(squadNumber)at line 275 is already implied bythen(actual).usingRecursiveComparison().isEqualTo(expected)on line 274, sinceexpectedwas filtered on the samesquadNumber. The second assertion adds no new coverage.🧹 Proposed cleanup
then(actual).usingRecursiveComparison().isEqualTo(expected); - then(actual.getSquadNumber()).isEqualTo(squadNumber);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java` around lines 274 - 275, The assertion then(actual.getSquadNumber()).isEqualTo(squadNumber) in PlayersControllerTests is redundant because then(actual).usingRecursiveComparison().isEqualTo(expected) already verifies squadNumber (expected was built with that value); remove the redundant line to clean up the test and rely on usingRecursiveComparison() for the full object equality check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java`:
- Around line 273-298: In
PlayersServiceTests.givenPlayersExist_whenSearchByLeague_thenReturns7Players:
replace the hard-coded assertion .hasSize(7) with .hasSize(expected.size()) and
replace the partial checks (size + league contains) with a full object
comparison using then(actual).usingRecursiveComparison().isEqualTo(expected);
keep the existing Mockito setup for
playersRepositoryMock.findByLeagueContainingIgnoreCase and modelMapperMock.map
and the verify(playersRepositoryMock, times(1)) call unchanged so the test still
mocks mapping per-entity and verifies repository interaction.
---
Outside diff comments:
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/repositories/PlayersRepository.java`:
- Around line 19-23: Update the Javadoc in PlayersRepository to remove or reword
the stale "Custom Query Methods" bullet: replace the reference to a JPQL custom
query with a note that findByLeagueContainingIgnoreCase is a derived query (or
simply list derived queries), ensuring the "Provided Methods" section correctly
reflects that findByLeagueContainingIgnoreCase and findBySquadNumber are derived
query methods rather than `@Query` JPQL methods; adjust the bullet text near the
list that mentions "Custom Query Methods" so it aligns with the "Query
Strategies" block and the actual repository methods.
---
Duplicate comments:
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Line 75: Rename tests in PlayersControllerTests from the BDD-style names to
the required method_scenario_outcome pattern; for example change
givenValidPlayer_whenPost_thenReturnsCreated to post_validPlayer_returnsCreated
(and similarly rename other methods in PlayersControllerTests to follow
method_scenario_outcome, e.g., get_missingPlayer_returnsNotFound,
put_existingPlayer_returnsOk) and update any references/usages of these method
names in the test file.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/repositories/PlayersRepositoryTests.java`:
- Line 35: Rename the test methods in PlayersRepositoryTests from the BDD-style
names (e.g., givenPlayerExists_whenFindById_thenReturnsPlayer,
givenPlayerDoesNotExist_whenFindById_thenReturnsEmpty,
givenAllPlayers_whenFindAll_thenReturnsList,
givenPlayerSaved_whenSave_thenPersists,
givenDuplicatePlayer_whenSave_thenThrows,
givenPlayerExists_whenDelete_thenRemoves) to the required
method_scenario_outcome pattern; update each method name to a concise form like
findById_playerExists_returnsPlayer, findById_playerNotFound_returnsEmpty,
findAll_multiplePlayers_returnsList, save_newPlayer_persists,
save_duplicatePlayer_throws, delete_playerExists_removes (keeping the `@Test`
annotation and method body unchanged) and ensure any references to these method
names in the test class (e.g., utility calls or display names) are updated
accordingly.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java`:
- Line 55: Rename test methods in PlayersServiceTests to follow the
method_scenario_outcome pattern instead of the givenX_whenY_thenZ style; for
example rename givenNoExistingPlayer_whenCreate_thenReturnsPlayerDTO to
create_noExistingPlayer_returnsPlayerDTO (and apply the same transformation to
all other test methods in this class), updating any references/usages of those
test method names so the test runner and IDE still recognize them.
---
Nitpick comments:
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Around line 274-275: The assertion
then(actual.getSquadNumber()).isEqualTo(squadNumber) in PlayersControllerTests
is redundant because then(actual).usingRecursiveComparison().isEqualTo(expected)
already verifies squadNumber (expected was built with that value); remove the
redundant line to clean up the test and rely on usingRecursiveComparison() for
the full object equality check.
...test/java/ar/com/nanotaboada/java/samples/spring/boot/test/services/PlayersServiceTests.java
Show resolved
Hide resolved
nanotaboada
force-pushed
the
test/bdd-given-when-then-pattern
branch
from
71357f7 to
112fc9f
Compare
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
nanotaboada
force-pushed
the
test/bdd-given-when-then-pattern
branch
from
112fc9f to
5b6c1a6
Compare
- Replace hard-coded hasSize(7) with hasSize(expected.size()) - Replace partial assertions with full recursive comparison - Improves test robustness in league search test
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is
Summary by CodeRabbit
New Features
API Improvements
Performance
Behavioral / Repository
Tests
Documentation