Conversation
There was a problem hiding this comment.
Pull request overview
Adds/adjusts tests and CI/build configuration to improve coverage and analysis for the customer service.
Changes:
- Added new unit tests for
ErrorVm/Constantsand forUserAddressService. - Minor formatting edits in
CustomerServiceTest. - Updated customer runtime Docker base image and SonarQube scan parameters in Jenkins.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| customer/src/test/java/com/yas/customer/viewmodel/ErrorVmAndConstantsTest.java | New tests validating ErrorVm constructor behavior and selected Constants values. |
| customer/src/test/java/com/yas/customer/service/UserAddressServiceTest.java | New unit tests for UserAddressService flows (list/default/create/delete/choose default). |
| customer/src/test/java/com/yas/customer/service/CustomerServiceTest.java | Small formatting changes plus an added file header comment. |
| customer/Dockerfile | Switch customer image base from Temurin 25 to Temurin 21 JRE Alpine. |
| Jenkinsfile | Provide explicit Sonar projectKey/projectName per changed service during scan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class ErrorVmAndConstantsTest { | ||
|
|
||
| @Test | ||
| void errorVm_compactConstructor_shouldInitEmptyFieldErrors() { |
There was a problem hiding this comment.
Test name mentions a "compact constructor", but ErrorVm doesn’t define a compact record constructor (it defines an overload delegating to the canonical constructor). Consider renaming the test to reflect what’s actually being exercised (e.g., the 3-arg constructor initializing fieldErrors).
| void errorVm_compactConstructor_shouldInitEmptyFieldErrors() { | |
| void errorVm_threeArgConstructor_shouldInitEmptyFieldErrors() { |
| @Test | ||
| void constants_shouldExposeExpectedValues() { | ||
| assertThat(Constants.ErrorCode.USER_ADDRESS_NOT_FOUND).isEqualTo("USER_ADDRESS_NOT_FOUND"); | ||
| assertThat(Constants.ErrorCode.UNAUTHENTICATED).contains("LOGIN"); |
There was a problem hiding this comment.
This assertion only checks that UNAUTHENTICATED contains "LOGIN", which makes the test pass even if the constant changes in unintended ways (and the test name says "expected values"). Prefer asserting the full expected value (or at least the exact message contract) for stability.
| assertThat(Constants.ErrorCode.UNAUTHENTICATED).contains("LOGIN"); | |
| assertThat(Constants.ErrorCode.UNAUTHENTICATED).isEqualTo("LOGIN"); |
| setAuthenticationName("user-1"); | ||
| when(userAddressRepository.findAllByUserId("user-1")).thenReturn(List.of(userAddress(1L, "user-1", 100L, true))); | ||
| when(locationService.createAddress(any())).thenReturn(AddressVm.builder().id(102L).build()); | ||
| when(userAddressRepository.save(any())).thenAnswer(invocation -> invocation.getArgument(0)); |
There was a problem hiding this comment.
This line exceeds the 120-character line limit enforced by the repository’s Checkstyle configuration (checkstyle/checkstyle.xml sets LineLength max=120). Please wrap it to avoid Checkstyle failures.
| when(userAddressRepository.save(any())).thenAnswer(invocation -> invocation.getArgument(0)); | |
| when(userAddressRepository.save(any())) | |
| .thenAnswer(invocation -> invocation.getArgument(0)); |
| private static void setAuthenticationName(String username) { | ||
| Authentication authentication = mock(Authentication.class); | ||
| when(authentication.getName()).thenReturn(username); | ||
| SecurityContext context = mock(SecurityContext.class); | ||
| when(context.getAuthentication()).thenReturn(authentication); | ||
| SecurityContextHolder.setContext(context); | ||
| } |
There was a problem hiding this comment.
This test duplicates existing test utility logic for setting up the SecurityContext (see customer/src/test/java/com/yas/customer/util/SecurityContextUtils.java). Consider using that helper here for consistency, and clearing the SecurityContextHolder after each test (or in a teardown) to avoid leaking authentication state across tests.
| @@ -1,3 +1,4 @@ | |||
| // Test | |||
There was a problem hiding this comment.
The added "// Test" comment at the top of the file looks accidental and doesn’t add meaningful documentation. It also deviates from other test files in this module; consider removing it to keep the file header clean.
| // Test |
|
No description provided.