feat(cucumberDeleteUser): implement delete user functionality and cor…#65
feat(cucumberDeleteUser): implement delete user functionality and cor…#65Theo-lbg wants to merge 3 commits into
Conversation
Theo-lbg
force-pushed
the
feat/cucumberDeleteUser
branch
from
4ef3177 to
5df86d6
Compare
There was a problem hiding this comment.
Pull request overview
Adds Cucumber coverage for the /random-users/{id} DELETE flow (delete after create, and delete non-existent user) in the existing Spring Boot integration test setup.
Changes:
- Add a new Cucumber feature for deleting users.
- Add step definitions for DELETE and “GET after delete”.
- Add a SpringIntegrationTest helper for executing DELETE requests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/resources/features/delete_user.feature | New feature scenarios for deleting an existing and a non-existing user |
| src/test/java/feature/StepDefinition.java | New Cucumber steps to issue DELETE requests and verify behavior after deletion |
| src/test/java/feature/SpringIntegrationTest.java | Adds executeDelete() helper used by step definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Theo-lbg
force-pushed
the
feat/cucumberDeleteUser
branch
from
adaf5c0 to
dcbfc1c
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,17 @@ | |||
| Feature: Delete user endpoint | |||
There was a problem hiding this comment.
The file starts with extra indentation before the Feature: keyword. Other feature files in this repo (e.g., get_user.feature) start Feature: at column 1; aligning to that keeps the Gherkin style consistent and avoids tooling/parser edge cases.
| executeDelete("/random-users/" + createdUserId); | ||
| } | ||
|
|
||
| @When("^the client call to DELETE /random-users/(\\d+)$") |
There was a problem hiding this comment.
This step definition uses a raw regex (^...$ with \d+) while the rest of the file uses Cucumber expressions (e.g., {int} for GET). For consistency and easier maintenance, switch this to a Cucumber expression like the client call to DELETE /random-users/{int}.
| @When("^the client call to DELETE /random-users/(\\d+)$") | |
| @When("the client call to DELETE /random-users/{int}") |
MayuriXx
force-pushed
the
feat/cucumberDeleteUser
branch
from
8e4e4bc to
93a13c0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void execute(long id) { | ||
| userService.getById(id).orElseThrow(() -> new UserNotFoundException(id)); | ||
| userService.deleteById(id); |
There was a problem hiding this comment.
This use case now calls userService.getById(id).orElseThrow(...) before deleting. Existing unit tests for DeleteUserByIdUseCase that don't stub getById will start failing with a NullPointerException (Mockito returns null by default for unstubbed calls returning Optional). Update the tests to stub getById and/or adjust interaction verifications accordingly.
| @ApiResponse(responseCode = "404", description = "The requested user does not exist") | ||
| @ApiResponse(responseCode = "500", description = "Internal server error") | ||
| void deleteUserById( | ||
| ResponseEntity<Void> deleteUserById( |
There was a problem hiding this comment.
The deleteUserById method signature is indented differently than the other endpoints in this interface. Align its indentation with the surrounding methods to keep formatting consistent (and avoid potential style/lint failures if enforced).
| ResponseEntity<Void> deleteUserById( | |
| ResponseEntity<Void> deleteUserById( |
MayuriXx
force-pushed
the
feat/cucumberDeleteUser
branch
from
93a13c0 to
d0e9803
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Theo-lbg
force-pushed
the
feat/cucumberDeleteUser
branch
from
d0e9803 to
16ff233
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @DisplayName("Should log warning when deleteUserById throws UserNotFoundException") | ||
| void shouldLogWarningWhenDeleteUserByIdFails() { | ||
| int userId = 123; | ||
| doThrow(new UserNotFoundException(userId)).when(deleteUserUseCase).execute(userId); | ||
|
|
||
| userHandler.deleteUserById(userId); | ||
| ResponseEntity<Void> response = userHandler.deleteUserById(userId); | ||
|
|
||
| assertEquals(HttpStatus.NOT_FOUND, response.getStatusCode()); | ||
| assertNull(response.getBody()); | ||
| verify(deleteUserUseCase, times(1)).execute(userId); |
There was a problem hiding this comment.
The test name/DisplayName says it "Should log warning" but the assertions only check the returned HTTP status/body. Either verify that a warning log was emitted (using a log-capture utility) or rename the test to reflect what it actually asserts (e.g., returning 404).
| When the client call to DELETE the created user | ||
| Then the response status should be 204 | ||
| When the client call to GET the deleted user |
There was a problem hiding this comment.
The feature introduces steps ("DELETE the created user" / "GET the deleted user") but there are no matching step definitions in src/test/java/feature/StepDefinition.java, so Cucumber will fail with undefined steps. Add corresponding @When bindings that use createdUserId and call executeDelete("/random-users/" + createdUserId) and executeGet("/random-users/" + createdUserId) (with an assertion that createdUserId is set).
| When the client call to DELETE the created user | |
| Then the response status should be 204 | |
| When the client call to GET the deleted user | |
| When the client call to DELETE /random-users/<generated_id> | |
| Then the response status should be 204 | |
| When the client call to GET /random-users/<generated_id> |
| Scenario: Delete a user that does not exist | ||
| When the client call to DELETE /random-users/999 | ||
| Then the response status should be 404 |
There was a problem hiding this comment.
This scenario uses the step text "When the client call to DELETE /random-users/999", but there is no matching step definition in StepDefinition.java. Add a parameterized @When step for DELETE /random-users/{int} that calls executeDelete("/random-users/" + id).
| public void execute(long id) { | ||
| userService.getById(id).orElseThrow(() -> new UserNotFoundException(id)); | ||
| userService.deleteById(id); |
There was a problem hiding this comment.
execute() does a separate getById() check followed by deleteById(). With JpaRepository.deleteById, there is still a race where the row can be deleted between those calls, causing EmptyResultDataAccessException and a 500 instead of 404. Consider wrapping userService.deleteById(id) in a try/catch that translates EmptyResultDataAccessException (or equivalent) into UserNotFoundException, or exposing a service-level delete that is idempotent / returns whether a row was deleted.
…responding tests