[MOSIP-32144] Code coverage of IDA #1675
Conversation
|
Warning Rate limit exceeded@tarique-azeez has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 27 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (15)
WalkthroughThis pull request expands test coverage across the authentication module by adding 13 new unit test classes and extending 2 existing test files. Tests validate Spring configurations, filter behaviors, exception handling, validators, and helpers using reflection injection, mocking, and assertion techniques. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
authentication/authentication-common/src/test/java/childauthFilter/ChildAuthFilterImplTest.java (2)
5-5: Consider using specific imports instead of wildcard.Wildcard imports can reduce clarity and make it harder to identify which classes are in use. Prefer explicit imports for better maintainability.
122-122: Consider consistent List creation throughout the file.The newer tests use
List.of()(lines 122, 134, 144) while earlier tests useArrays.asList()(lines 59, 72, 79, 87). For consistency, consider using the same approach throughout the file.Also applies to: 134-134, 144-144
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/KafkaProducerConfigTest.java (1)
14-14: Consider removing unused@RunWith(MockitoJUnitRunner.class).The
MockitoJUnitRunneris declared but no Mockito annotations (@Mock,@InjectMocks, etc.) are used in this test class. You can simplify by removing the runner annotation.🔎 Proposed fix
-@RunWith(MockitoJUnitRunner.class) public class KafkaProducerConfigTest {authentication/authentication-service/src/test/java/io/mosip/authentication/service/kyc/validator/IdentityKeyBindingRequestValidatorUnitTest.java (1)
35-51: Consider adding negative test cases for completeness.The tests validate the happy path scenarios but miss the error paths:
validateIdentityKeyBindingPublicKeywith missingnorekeysvalidateIdentityKeyBindingAuthFactorTypewith blank/empty valueThese would strengthen the validation coverage.
🔎 Suggested additional tests
@Test public void validateIdentityKeyBindingPublicKeyAddsErrorWhenModulusMissing() { Errors errors = errors(); Map<String, Object> jwk = new HashMap<>(); jwk.put("e", "AQAB"); // missing "n" ReflectionTestUtils.invokeMethod(validator, "validateIdentityKeyBindingPublicKey", jwk, errors); assertTrue(errors.hasErrors()); } @Test public void validateIdentityKeyBindingAuthFactorTypeAddsErrorWhenBlank() { Errors errors = errors(); ReflectionTestUtils.invokeMethod(validator, "validateIdentityKeyBindingAuthFactorType", "", errors); assertTrue(errors.hasErrors()); }authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/helper/IdentityAttributesForMatchTypeHelperTest.java (1)
126-150: UnusedidMappingmock variables.In both
getIdentityAttributesForMatchType_dynamic_returnsConfiguredListWhenPresent(line 133) andgetIdentityAttributesForMatchType_dynamic_returnsIdNameWhenNotConfigured(line 146), theidMappingmock is created but never used. Either remove these unused variables or wire them appropriately.🔎 Proposed fix
@Test public void getIdentityAttributesForMatchType_dynamic_returnsConfiguredListWhenPresent() { when(idMappingConfig.getDynamicAttributes()).thenReturn(Map.of("residenceStatus", List.of("residenceStatus"))); MatchType matchType = mock(MatchType.class); when(matchType.isDynamic()).thenReturn(true); - IdMapping idMapping = mock(IdMapping.class); - List<String> result = helper.getIdentityAttributesForMatchType(matchType, "residenceStatus"); assertEquals(List.of("residenceStatus"), result); } @Test public void getIdentityAttributesForMatchType_dynamic_returnsIdNameWhenNotConfigured() { when(idMappingConfig.getDynamicAttributes()).thenReturn(Map.of()); MatchType matchType = mock(MatchType.class); when(matchType.isDynamic()).thenReturn(true); - IdMapping idMapping = mock(IdMapping.class); - List<String> result = helper.getIdentityAttributesForMatchType(matchType, "newAttribute"); assertEquals(List.of("newAttribute"), result); }authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.java (3)
33-46: Duplicate test methods.
testGetSystemSupportedLanguageCodesandtestGetSystemSupportedLanguageCodesActualLogicare functionally identical—both invoke the same overridden method on the anonymous subclass and perform the same assertions. The second test's name suggests it tests "actual logic," but it still exercises the mocked implementation.Consider removing the duplicate or, if the intent is to test the real
LangComparatorConfigbehavior, create a separate instance without the override (though that would require proper Spring context or environment setup for the actual property injection).Also applies to: 60-74
35-39: Unnecessary reflection for public method.
getSystemSupportedLanguageCodes()is a public method that can be invoked directly onconfig. UsingReflectionUtilsadds complexity without benefit.🔎 Simplified approach
@Test public void testGetSystemSupportedLanguageCodes() { - Method method = ReflectionUtils.findMethod(LangComparatorConfig.class, "getSystemSupportedLanguageCodes"); - ReflectionUtils.makeAccessible(method); - - @SuppressWarnings("unchecked") - List<String> result = (List<String>) ReflectionUtils.invokeMethod(method, config); + List<String> result = config.getSystemSupportedLanguageCodes(); assertNotNull(result); assertEquals(3, result.size());
10-11: UnnecessaryMockitoJUnitRunner.No
@Mockor@Spyannotations are used in this test class. The runner can be removed to reduce overhead.Also applies to: 16-16
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/CacheConfigTest.java (2)
17-25: Use try-with-resources to ensure context cleanup.If
context.getBean()throws,context.close()won't be called. Using try-with-resources ensures proper cleanup.🔎 Proposed fix
@Test public void testCacheManagerBeanCreation() { - // Load only this configuration - AnnotationConfigApplicationContext context = - new AnnotationConfigApplicationContext(CacheConfig.class); - - ConcurrentMapCacheManager cacheManager = - context.getBean(ConcurrentMapCacheManager.class); - - assertNotNull(cacheManager); - - context.close(); + try (AnnotationConfigApplicationContext context = + new AnnotationConfigApplicationContext(CacheConfig.class)) { + ConcurrentMapCacheManager cacheManager = + context.getBean(ConcurrentMapCacheManager.class); + assertNotNull(cacheManager); + } }
7-7:SpringRunneris unnecessary here.Since you're manually creating
AnnotationConfigApplicationContext, the@RunWith(SpringRunner.class)annotation provides no benefit. Consider removing it.Also applies to: 11-11
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/DefaultInternalFilterTest.java (1)
22-41: Limited assertion value in these tests.These three tests call methods with empty inputs and assert nothing meaningful—they only verify no exception is thrown. While this provides code coverage, consider adding assertions on return values or observable side effects, or rename them to indicate they're smoke/no-throw tests (e.g.,
testDecipherAndValidateRequest_noException).authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java (1)
69-71: Trailing blank lines.Remove the extra blank lines at the end of the file.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
authentication/authentication-common/src/test/java/childauthFilter/ChildAuthFilterImplTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/CacheConfigTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/DemoAuthConfigTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/IdAuthConfigTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/KafkaProducerConfigTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/SwaggerConfigTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/TrailingSlashRedirectFilterTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/exception/IDAuthExceptionHandlerTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/DefaultInternalFilterTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/ExternalAuthFilterTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/helper/IdentityAttributesForMatchTypeHelperTest.javaauthentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.javaauthentication/authentication-otp-service/src/test/java/io/mosip/authentication/otp/service/filter/OTPFilterTest.javaauthentication/authentication-service/src/test/java/io/mosip/authentication/service/kyc/validator/IdentityKeyBindingRequestValidatorUnitTest.java
🧰 Additional context used
🧬 Code graph analysis (5)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java (2)
authentication/authentication-filter-api/src/main/java/io/mosip/authentication/authfilter/exception/IdAuthenticationFilterException.java (1)
IdAuthenticationFilterException(9-63)authentication/authentication-common/src/main/java/io/mosip/authentication/common/service/factory/MosipAuthFilterFactory.java (1)
MosipAuthFilterFactory(31-133)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.java (1)
authentication/authentication-core/src/main/java/io/mosip/authentication/core/util/LanguageComparator.java (1)
LanguageComparator(11-33)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/IdAuthConfigTest.java (3)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/KafkaProducerConfigTest.java (1)
RunWith(14-47)authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.java (1)
RunWith(16-75)authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/SwaggerConfigTest.java (1)
RunWith(16-93)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/DefaultInternalFilterTest.java (1)
authentication/authentication-core/src/main/java/io/mosip/authentication/core/constant/IdAuthConfigKeyConstants.java (1)
IdAuthConfigKeyConstants(11-180)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/KafkaProducerConfigTest.java (2)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/IdAuthConfigTest.java (1)
RunWith(16-80)authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/SwaggerConfigTest.java (1)
RunWith(16-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-apitest-auth / maven-build
🔇 Additional comments (18)
authentication/authentication-common/src/test/java/childauthFilter/ChildAuthFilterImplTest.java (2)
91-115: LGTM! Helper methods improve test readability.The builder methods effectively reduce duplication and make the test setup clearer. The structure is clean and reusable across multiple test cases.
117-147: LGTM! Comprehensive test coverage for child authentication restrictions.The three new test methods effectively validate:
- Denial of restricted auth factors (OTP) for children
- Dynamic configuration changes for additional restrictions (demo)
- Permissive behavior for empty auth requests
The test logic correctly exercises age-based filtering with appropriate assertions.
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/KafkaProducerConfigTest.java (1)
32-46: LGTM!The tests correctly validate that
producerFactory()andkafkaTemplate()return non-null instances. This is consistent with the testing pattern used in other configuration tests in this PR (e.g.,SwaggerConfigTest,IdAuthConfigTest).authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/DemoAuthConfigTest.java (2)
27-46: LGTM!Good coverage of error paths for SDK instance creation. The tests correctly validate
ClassNotFoundExceptionfor invalid class names and null return when no default constructor exists.
54-63: Unable to verify the specific code location in DemoAuthConfigTest.java — repository access failed. However, the technical claim aboutList.of()is accurate.Java's
List.of()indeed throwsNullPointerExceptionimmediately during list creation if any element is null, per the official Javadoc. The code snippet shown (lines 54-63) does not display theList.of((String) null)pattern being reviewed. A similar pattern was found in the codebase atIdentityAttributesForMatchTypeHelperTest.javaline 60. If this pattern exists in DemoAuthConfigTest.java, the suggestion to useArrays.asList((String) null)orCollections.singletonList(null)is valid for testing null handling in the mapping function.authentication/authentication-otp-service/src/test/java/io/mosip/authentication/otp/service/filter/OTPFilterTest.java (1)
79-98: LGTM!The new tests properly validate both the positive case (OTP request allowed when policy type is "otp-request") and the negative case (exception thrown when policy type doesn't match). Good use of
assertDoesNotThrowandassertThrowsfor clear intent.authentication/authentication-service/src/test/java/io/mosip/authentication/service/kyc/validator/IdentityKeyBindingRequestValidatorUnitTest.java (1)
28-33: LGTM!Good test for validating that null
IdentityKeyBindingDTOinput properly triggers an error.authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/exception/IDAuthExceptionHandlerTest.java (2)
507-529: LGTM!The tests for
InvalidFormatExceptionhandling witheventTypeandexpiryTimestampscenarios properly validate the exception handling paths. The assertions verify the response is non-null and returns HTTP 200.
556-562: LGTM!Good test coverage for the edge case where
buildExceptionResponsereceives an empty error list, validating that it returns null as expected.authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/IdAuthConfigTest.java (1)
61-79: LGTM!The tests for
messageSource(),threadPoolTaskScheduler(),afterburnerModule(), andgetRestRequestBuilder()appropriately validate that these configuration methods return non-null instances.authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/helper/IdentityAttributesForMatchTypeHelperTest.java (1)
65-77: LGTM!Good test for verifying that the helper correctly recurses into dynamic ID mappings and returns the expanded list from
getDynamicAttributes().authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/ExternalAuthFilterTest.java (2)
18-24: LGTM!The test correctly validates that
fetchId()concatenates the attribute with "auth". The mock request is appropriately unused iffetchIdonly uses the attribute parameter.
26-49: LGTM!Good coverage of the filter's boolean flag methods. The tests clearly document the expected behavior of each method.
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/TrailingSlashRedirectFilterTest.java (1)
34-77: Good test coverage for filter scenarios.The tests cover the key branches: trailing slash removal with URI/URL verification, passthrough when no trailing slash, and the
ObjectWithMetadatabypass. The use ofArgumentCaptorto verify the wrapped request is appropriate.authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/SwaggerConfigTest.java (1)
21-92: Comprehensive tests for Swagger configuration.The setup properly injects dependencies via reflection, and the test methods thoroughly validate the
OpenAPIandGroupedOpenApibean configurations including info, license, servers, and grouped paths.authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/DefaultInternalFilterTest.java (2)
43-60: Good test for field removal logic.This test meaningfully validates the
removeNullOrEmptyFieldsInResponsebehavior, checking that null fields, empty lists, and nested maps are handled correctly.
62-69: The test correctly validates the implementation. ThefetchId()method at line 93 inDefaultInternalFilter.javadoes concatenateattributedirectly withOTP_INTERNAL_ID_SUFFIXwithout a separator, producing"attributeotp.internal". This matches the test assertion exactly.authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java (1)
50-58: Good test for filter invocation.The test correctly verifies that all enabled filters are retrieved from the factory and that each filter's
validatemethod is invoked once.
...ion-common/src/test/java/io/mosip/authentication/common/service/config/IdAuthConfigTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/io/mosip/authentication/common/service/exception/IDAuthExceptionHandlerTest.java
Outdated
Show resolved
Hide resolved
.../io/mosip/authentication/common/service/helper/IdentityAttributesForMatchTypeHelperTest.java
Show resolved
Hide resolved
...src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java
Show resolved
Hide resolved
tarique-azeez
force-pushed
the
develop
branch
5 times, most recently
from
18e72f5 to
b00a75f
Compare
Signed-off-by: tarique-azeez <mdtarique2703@gmail.com>
Signed-off-by: tarique-azeez <mdtarique2703@gmail.com>
Signed-off-by: tarique-azeez <mdtarique2703@gmail.com>
tarique-azeez
force-pushed
the
develop
branch
from
b00a75f to
0744923
Compare
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.