Add unit tests #111
Add unit tests #111
Conversation
test: move contextLoads test to IT package refactor: convert field injection to constructor injection in JwtAuthenticationHelper
WalkthroughThis update introduces a comprehensive suite of new unit and integration tests for the library management API, covering service, security, notification, and utility components. It also refactors the JWT authentication helper to use constructor-based dependency injection and renames an integration test class for consistency. The changes centralize test data creation and ensure robust test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant TestClass as Test Class
participant Service as Service Under Test
participant Repo as Mocked Repository
participant Util as TestUtil
TestClass->>Util: Generate test data (DTOs, entities)
TestClass->>Repo: Setup mock return values
TestClass->>Service: Call service method with test data
Service->>Repo: Interact with repository (e.g., find, save)
Repo-->>Service: Return mocked data
Service-->>TestClass: Return result
TestClass->>TestClass: Assert result or exception
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @aninagotgit for your contribution! Your pull request has been submitted successfully, and a maintainer will review it soon. We’re excited to have you on board! Remember to star the repo 🌟 to help us grow, and follow @ajaynegi45 to stay in the loop and increase the visibility of your contributions!
|
|
||
| @SpringBootTest | ||
| class LibrarymanApiApplicationTests { | ||
| class LibrarymanApiApplicationIntegrationTest { |
There was a problem hiding this comment.
Moved the test that requires the Spring application context to start up into a folder for Integration tests. (IT)
More integration tests can be added here.
|
|
||
| public JwtAuthenticationHelper(@Value("${jwt.secretKey}") String secret) { | ||
| this.secret = secret; | ||
| } |
There was a problem hiding this comment.
Use constructor injection instead of field injection in order to write tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/test/java/com/libraryman_api/borrowing/BorrowingServiceTest.java (2)
59-59: Consider using mock returns instead of calling real methods in unit tests.The
thenCallRealMethod()calls at lines 59, 86, and in the borrowBook success test (lines 108-112) break unit test isolation by invoking actual implementation code. This can make tests brittle and harder to maintain.Consider mocking these method calls with appropriate return values:
-when(bookService.EntityToDto(any())).thenCallRealMethod(); +when(bookService.EntityToDto(any())).thenReturn(TestUtil.getBookDto());Also applies to: 86-86, 108-112
160-160: Consider extracting date creation logic to improve test readability.The repeated date manipulation using
LocalDate.now().plusDays/minusDayscreates verbose and repetitive code that affects readability.Consider adding helper methods to TestUtil for common date scenarios:
// In TestUtil class public static Date getFutureDueDate() { return Date.from(LocalDate.now().plusDays(1).atStartOfDay(ZoneId.systemDefault()).toInstant()); } public static Date getPastDueDate() { return Date.from(LocalDate.now().minusDays(1).atStartOfDay(ZoneId.systemDefault()).toInstant()); }Also applies to: 190-190, 204-204, 220-220, 236-236, 253-253
src/test/java/com/libraryman_api/security/jwt/JwtAuthenticationHelperTest.java (2)
28-30: Consider using a more secure approach for test secrets.The hardcoded JWT secret in the test setup could be improved for maintainability and security best practices, even in tests.
Consider using a constant or configuration approach:
+ private static final String TEST_JWT_SECRET = "aVeryLongSecretStringThatIsAtLeast64BytesLongAndSecureEnoughForHS512"; + @BeforeEach void setup() { - secret = "aVeryLongSecretStringThatIsAtLeast64BytesLongAndSecureEnoughForHS512"; + secret = TEST_JWT_SECRET; jwtAuthenticationHelper = new JwtAuthenticationHelper(secret); }
37-37: Extract magic number to improve readability.The hardcoded expiration time (100000) should be extracted to a constant for better maintainability.
+ private static final long TOKEN_EXPIRATION_TIME = 100000; + - String token = Jwts.builder().signWith(Keys.hmacShaKeyFor(secret.getBytes()), SignatureAlgorithm.HS256).setSubject(expectedUsername).setIssuedAt(new Date()).setExpiration(new Date(System.currentTimeMillis() + 100000)).compact(); + String token = Jwts.builder().signWith(Keys.hmacShaKeyFor(secret.getBytes()), SignatureAlgorithm.HS256).setSubject(expectedUsername).setIssuedAt(new Date()).setExpiration(new Date(System.currentTimeMillis() + TOKEN_EXPIRATION_TIME)).compact();src/test/java/com/libraryman_api/security/services/SignupServiceTest.java (1)
32-32: Consider mocking PasswordEncoder for better test isolation.Using a real
PasswordEncoderinstance creates a dependency on the actual implementation. Consider mocking it to improve test isolation and performance.+ @Mock + private PasswordEncoder passwordEncoder; - private final PasswordEncoder passwordEncoder = new PasswordEncoder();Then in setup method, you can stub the encoding behavior if needed:
when(passwordEncoder.encode(any())).thenReturn("encodedPassword");src/test/java/com/libraryman_api/email/EmailServiceTest.java (1)
50-54: Consider using Spring's test utilities for property injection.Using reflection to inject properties makes the test more fragile and harder to maintain. Consider using Spring's
@TestPropertySourceorReflectionTestUtils.setField()for cleaner property injection.+import org.springframework.test.util.ReflectionTestUtils; + - private void injectPropertyValue(Object emailService) throws Exception { - Field field = emailService.getClass().getDeclaredField("domainName"); - field.setAccessible(true); - field.set(emailService, "default.io"); - } + private void injectPropertyValue(Object emailService) { + ReflectionTestUtils.setField(emailService, "domainName", "default.io"); + }And update the setup method to remove the Exception handling:
- void setup() throws Exception { + void setup() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/it/java/com/libraryman_api/LibrarymanApiApplicationIntegrationTest.java(1 hunks)src/main/java/com/libraryman_api/security/jwt/JwtAuthenticationHelper.java(1 hunks)src/test/java/com/libraryman_api/TestUtil.java(1 hunks)src/test/java/com/libraryman_api/analytics/AnalyticsServiceTest.java(1 hunks)src/test/java/com/libraryman_api/book/BookServiceTest.java(1 hunks)src/test/java/com/libraryman_api/borrowing/BorrowingServiceTest.java(1 hunks)src/test/java/com/libraryman_api/email/EmailServiceTest.java(1 hunks)src/test/java/com/libraryman_api/member/MemberServiceTest.java(1 hunks)src/test/java/com/libraryman_api/newsletter/NewsletterServiceTest.java(1 hunks)src/test/java/com/libraryman_api/notification/NotificationServiceTest.java(1 hunks)src/test/java/com/libraryman_api/security/jwt/JwtAuthenticationFilterTest.java(1 hunks)src/test/java/com/libraryman_api/security/jwt/JwtAuthenticationHelperTest.java(1 hunks)src/test/java/com/libraryman_api/security/services/CustomUserDetailsServiceTest.java(1 hunks)src/test/java/com/libraryman_api/security/services/LoginServiceTest.java(1 hunks)src/test/java/com/libraryman_api/security/services/SignupServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/test/java/com/libraryman_api/security/services/SignupServiceTest.java (3)
src/test/java/com/libraryman_api/TestUtil.java (1)
TestUtil(23-158)src/main/java/com/libraryman_api/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(11-51)src/test/java/com/libraryman_api/security/services/CustomUserDetailsServiceTest.java (1)
ExtendWith(25-51)
src/test/java/com/libraryman_api/book/BookServiceTest.java (3)
src/test/java/com/libraryman_api/TestUtil.java (2)
TestUtil(23-158)Constant(152-157)src/main/java/com/libraryman_api/exception/InvalidSortFieldException.java (1)
InvalidSortFieldException(11-51)src/main/java/com/libraryman_api/exception/ResourceNotFoundException.java (1)
ResourceNotFoundException(11-51)
🔇 Additional comments (35)
src/main/java/com/libraryman_api/security/jwt/JwtAuthenticationHelper.java (1)
19-23: LGTM! Constructor injection implemented as requested.This refactoring successfully addresses the past review comment by converting from field injection to constructor injection. The
finalkeyword ensures immutability and the constructor parameter approach enables easier unit testing and follows Spring best practices.src/test/java/com/libraryman_api/TestUtil.java (4)
26-28: LGTM! Simple and effective random number generation for tests.The fixed range (100-1000) provides predictable test data while avoiding collisions with the hardcoded constants.
30-150: Excellent test utility design for centralized test data creation.The factory methods provide consistent, reusable test data with good separation between entities and DTOs. Using constants and providing methods for all major domain objects will significantly improve test maintainability across the project.
104-109: LGTM! Date adjustment utility is consistent with codebase.The method correctly uses the Calendar API to adjust dates. While modern projects might prefer LocalDate, this approach is consistent with the Date usage throughout the domain entities.
111-158: Well-designed pagination utilities and constants.The pagination helper methods and centralized constants follow excellent test utility patterns. The
getPageRequestmethod provides consistent pagination parameters, and the constants ensure maintainable test data references across all tests.src/it/java/com/libraryman_api/LibrarymanApiApplicationIntegrationTest.java (1)
7-7: Excellent organizational improvement for integration tests.Renaming the class to
LibrarymanApiApplicationIntegrationTestand moving it to the IT package clearly separates integration tests from unit tests and makes the test purpose explicit. This follows best practices for test organization.src/test/java/com/libraryman_api/analytics/AnalyticsServiceTest.java (4)
34-45: LGTM! Well-structured test for library overview aggregation.The test properly mocks all repository count methods and verifies the service correctly aggregates the data into the expected map structure.
47-55: LGTM! Appropriate test for popular books retrieval.The test correctly verifies that the service properly delegates to the repository and returns the expected popular books data.
57-65: LGTM! Good use of flexible matchers for date-based queries.The test appropriately uses
any()matchers for the date parameters, focusing on verifying the service correctly returns trending data rather than getting bogged down in specific date values.
67-75: LGTM! Straightforward test for member activity reporting.The test correctly verifies that the service properly retrieves and returns member activity report data from the repository.
src/test/java/com/libraryman_api/book/BookServiceTest.java (8)
34-56: Excellent test coverage for getAllBooks functionality.The nested class properly tests both successful book retrieval with pagination and exception handling for invalid sort fields. The exception translation from
PropertyReferenceExceptiontoInvalidSortFieldExceptionis correctly verified.
58-79: LGTM! Proper testing of Optional return pattern.The tests correctly verify both successful book retrieval and empty results, properly using the Optional pattern and realistic test scenarios with random IDs for non-existent records.
81-89: LGTM! Clean test for book addition functionality.The test properly verifies the add book functionality with appropriate mocking and DTO conversion validation.
91-119: Comprehensive testing of book update scenarios.The nested class properly tests both successful updates and exception handling. The success test correctly verifies that existing book data is retrieved, updated with new values, and saved, while the exception test confirms proper error handling for non-existent books.
121-139: LGTM! Appropriate testing of deletion scenarios.The tests correctly verify successful deletion using
assertDoesNotThrowand proper exception handling with meaningful error messages for non-existent books.
141-155: Thorough verification of entity to DTO conversion.The test comprehensively verifies that all fields are correctly mapped from the Book entity to BookDto, ensuring data integrity during conversion.
157-171: Comprehensive verification of DTO to entity conversion.The test thoroughly verifies that all fields are correctly mapped from BookDto to Book entity, ensuring proper bidirectional conversion functionality.
173-181: LGTM! Effective test for book search functionality.The test properly verifies the search functionality by mocking the repository search method and confirming the service returns the expected paginated results.
src/test/java/com/libraryman_api/borrowing/BorrowingServiceTest.java (1)
1-429: Excellent test coverage and organization!The test class demonstrates comprehensive coverage of the BorrowingService with well-structured nested classes, good separation of concerns, and thorough testing of both success and failure scenarios. The use of descriptive test method names and proper exception message assertions enhances maintainability.
src/test/java/com/libraryman_api/member/MemberServiceTest.java (3)
40-45: Good practice using real PasswordEncoder for testing password functionality.Using a real
PasswordEncoderinstance instead of mocking it ensures that password encoding/decoding logic is properly tested, which is critical for security functionality.
156-206: Comprehensive password update testing with excellent scenario coverage.The password update tests cover all critical scenarios: success, member not found, incorrect current password, and same password validation. The test setup properly encodes passwords and validates the behavior correctly.
208-236: Thorough testing of DTO-Entity conversion methods.The conversion method tests verify all field mappings between DTOs and entities, ensuring data integrity during transformations.
src/test/java/com/libraryman_api/security/services/CustomUserDetailsServiceTest.java (1)
35-42: Clean and focused test implementation.The test correctly verifies that the service returns the expected
Membersobject asUserDetailsand properly handles the not-found scenario with appropriate exception throwing.src/test/java/com/libraryman_api/security/services/LoginServiceTest.java (1)
44-69: Comprehensive login service testing with proper exception handling.The test suite effectively covers the main authentication scenarios: successful login with token generation, bad credentials handling, and user not found cases. The exception propagation testing ensures proper error handling behavior.
src/test/java/com/libraryman_api/security/jwt/JwtAuthenticationFilterTest.java (3)
39-42: Excellent test setup with proper SecurityContext management.Clearing the SecurityContext in
@BeforeEachensures test isolation and prevents test interference, which is crucial for security-related testing.
79-90: Sophisticated parameterized testing for JWT exceptions.The use of
@ParameterizedTestwith dynamic class loading to test different JWT exception types is an elegant solution that reduces code duplication while ensuring comprehensive exception handling coverage.
46-144: Exceptionally comprehensive JWT filter testing.This test suite covers all critical authentication scenarios including valid tokens, missing/invalid headers, various JWT exceptions, expired tokens, and pre-existing security contexts. The thorough coverage ensures robust JWT authentication behavior.
src/test/java/com/libraryman_api/security/jwt/JwtAuthenticationHelperTest.java (1)
66-76: Excellent comprehensive test coverage!The test thoroughly validates token generation by verifying claims extraction, subject matching, and presence of issued/expiration dates. The use of TestUtil for consistent test data is also well implemented.
src/test/java/com/libraryman_api/security/services/SignupServiceTest.java (1)
43-84: Excellent test structure and coverage!The nested class organization and comprehensive testing of both success and failure scenarios is well implemented. The use of ArgumentCaptor to verify saved entity properties and the thorough exception testing demonstrate good testing practices.
src/test/java/com/libraryman_api/notification/NotificationServiceTest.java (3)
168-174: Excellent helper method design!The
verifyEmailSenthelper method effectively reduces code duplication and provides consistent verification across all notification tests. The parameter design allows for flexible verification of email components.
131-166: Comprehensive edge case testing in SendDueDateReminders.The nested class provides excellent coverage of the scheduled reminder functionality, including success scenarios, empty results, and missing member handling. This demonstrates thorough testing of the automated reminder system.
70-70: TestUtil provides all referenced methods – no action requiredTestUtil defines the needed static helpers:
- src/test/java/com/libraryman_api/TestUtil.java
• getFine() at line 67
• getBorrowings() at line 94src/test/java/com/libraryman_api/email/EmailServiceTest.java (1)
90-110: Excellent notification status testing!The Send nested class effectively tests both successful email sending (status set to SENT) and failure scenarios (status set to FAILED). This ensures proper notification tracking and error handling.
src/test/java/com/libraryman_api/newsletter/NewsletterServiceTest.java (2)
31-80: Comprehensive subscription testing with good edge case coverage.The Subscribe nested class thoroughly tests all subscription scenarios: re-subscription of inactive users, handling already active subscriptions, new subscriber creation, and email validation. The verification of repository and email service interactions is well implemented.
82-122: Well-structured unsubscription testing.The Unsubscribe nested class effectively covers all unsubscription scenarios including successful unsubscription, invalid tokens, and already unsubscribed users. The test organization and verification patterns are consistent and maintainable.
| Exception exception = assertThrows(ResourceNotFoundException.class, () -> signupService.signup(members)); | ||
|
|
||
| assertEquals("User already Exists", exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify exception type usage - ResourceNotFoundException for user existence checks.
The tests throw ResourceNotFoundException with message "User already Exists" for duplicate users. This seems semantically incorrect - typically ResourceNotFoundException indicates a resource wasn't found, not that it already exists. Consider if a different exception type would be more appropriate.
Let me verify how the actual SignupService handles these cases:
Also applies to: 80-83, 112-115, 123-126, 155-158, 166-169
🏁 Script executed:
#!/bin/bash
# Description: Check how SignupService handles duplicate user scenarios
# Expected: Should show the actual exception types used
ast-grep --pattern $'class SignupService {
$$$
signup($_) {
$$$
}
$$$
}'Length of output: 85
🏁 Script executed:
#!/bin/bash
# Locate the file defining SignupService
rg -l "class\s\+SignupService" -n .
# For each matched file, show the signup method and any exception throws
for file in $(rg -l "class\s\+SignupService" -n .); do
echo "==== Inspecting $file ===="
rg -n "signup" "$file" -C 5
echo
rg -n "throw new" "$file" -C 3
doneLength of output: 76
🏁 Script executed:
#!/bin/bash
# Search for SignupService implementation files anywhere in the repo
echo "Locating SignupService Java files..."
find . -type f -iname "*signupservice*.java" -print
# For each located file, extract the signup method and any thrown exceptions
for file in $(find . -type f -iname "*signupservice*.java"); do
echo
echo "==== Inspecting $file ===="
echo "-- Method signature and body around signup --"
grep -n "signup(" "$file" -C5 || echo " → No signup() signature found here"
echo
echo "-- Exception throws in this class --"
grep -n "throw new" "$file" -C3 || echo " → No throw statements found here"
doneLength of output: 5741
Use a semantically accurate exception for duplicate users
The signup method currently throws ResourceNotFoundException("User already Exists") when a user already exists—this is misleading, since “not found” implies absence, not presence. Please update both the service and its tests to use an exception that clearly conveys a duplicate‐resource error (for example, DuplicateResourceException, ResourceAlreadyExistsException, or a custom UserAlreadyExistsException).
• In src/main/java/com/libraryman_api/security/services/SignupService.java, replace both
throw new ResourceNotFoundException("User already Exists");with something like
throw new UserAlreadyExistsException("User already exists");• In src/test/java/com/libraryman_api/security/services/SignupServiceTest.java, update the assertThrows calls at lines 69–72 and 80–83 to expect the new exception type instead of ResourceNotFoundException.
Example diff snippet:
--- a/src/main/java/com/libraryman_api/security/services/SignupService.java
+++ b/src/main/java/com/libraryman_api/security/services/SignupService.java
@@ -29,7 +29,7 @@ public void signup(Members members) {
if (memberOptId.isPresent()) {
- throw new ResourceNotFoundException("User already Exists");
+ throw new UserAlreadyExistsException("User already exists");
}
if (memberOptUsername.isPresent()) {
- throw new ResourceNotFoundException("User already Exists");
+ throw new UserAlreadyExistsException("User already exists");
}
--- a/src/test/java/com/libraryman_api/security/services/SignupServiceTest.java
+++ b/src/test/java/com/libraryman_api/security/services/SignupServiceTest.java
@@ -69,7 +69,7 @@ void idFound() {
- Exception exception = assertThrows(ResourceNotFoundException.class, () -> signupService.signup(members));
+ Exception exception = assertThrows(UserAlreadyExistsException.class, () -> signupService.signup(members));
assertEquals("User already Exists", exception.getMessage());
}
@@ -80,7 +80,7 @@ void usernameFound() {
- Exception exception = assertThrows(ResourceNotFoundException.class, () -> signupService.signup(members));
+ Exception exception = assertThrows(UserAlreadyExistsException.class, () -> signupService.signup(members));
assertEquals("User already Exists", exception.getMessage());
}🤖 Prompt for AI Agents
In src/test/java/com/libraryman_api/security/services/SignupServiceTest.java
around lines 69 to 72, update the test to expect the new exception type
UserAlreadyExistsException instead of ResourceNotFoundException for duplicate
user signup attempts. Also, ensure the corresponding service file
src/main/java/com/libraryman_api/security/services/SignupService.java replaces
the throw of ResourceNotFoundException with UserAlreadyExistsException to
accurately reflect the error condition.
ajaynegi45
left a comment
ajaynegi45
left a comment
There was a problem hiding this comment.
Hey @aninagotgit 👋
Wow — what an incredible contribution! 🚀 A 2000+ line commit that not only brings in comprehensive unit and integration tests, but also modernizes our codebase. You've clearly put a lot of thought and care into improving both the test structure and overall maintainability. 🙌
It would be awesome if you could take a look at other present issues and push a fix when you get a chance. Let me know if you need help — happy to assist!
Also, if you're enjoying working on this repo, I’d love for you to explore some of the other Java projects I maintain — especially Vayu 🌬️. It’s an evolving project with tons of interesting problems — could definitely use your eye for clean code and thoughtful testing!
Thanks again for this beast of a contribution. Can't wait to get this merged once those last bits are resolved. Keep rocking! 💪
2 participants
test: add unit tests
test: move contextLoads test to IT package
refactor: convert field injection to constructor injection in JwtAuthenticationHelper
Closes #26
Summary by CodeRabbit
Refactor
Style
Tests