[MOSIP-43456] Fixed test cases auth_DemoAuth_with_age_below_calculated_value_Neg #1682
Conversation
WalkthroughThis pull request adds comprehensive test coverage for multiple configuration classes, service filters, validators, and helpers across authentication modules. It also modifies the DemoMatcherUtil behavior to require exact equality instead of less-than-equal matching. The changes are primarily test additions with one behavior modification in production code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
tarique-azeez
force-pushed
the
MOSIP-43456
branch
from
c70ee78 to
7aa0547
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>
Signed-off-by: tarique-azeez <mdtarique2703@gmail.com>
Signed-off-by: tarique-azeez <mdtarique2703@gmail.com>
tarique-azeez
force-pushed
the
MOSIP-43456
branch
from
ba7f0bb to
75ac241
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
authentication/authentication-core/src/test/java/io/mosip/authentication/core/util/MatcherUtilTest.java (1)
114-127: Missing test case forreqInfo < entityInfoscenario.The test was changed from
(18, 20)to(20, 20), but there's no test verifying the new behavior wherereqInfo < entityInforeturns0. Add a test case to explicitly document that the "less than" case is no longer a match:Suggested test to add
/** * Assert do less than equal match - reqInfo less than entityInfo now returns 0 */ `@Test` public void TestLessThanDoesNotMatch() { int value = demoMatcherUtil.doLessThanEqualToMatch(18, 20); assertEquals(0, value); }authentication/authentication-core/src/main/java/io/mosip/authentication/core/util/DemoMatcherUtil.java (1)
58-71: Method name and Javadoc do not match implementation.The method
doLessThanEqualToMatchperforms only exact equality matching (reqInfo == entityInfo), but the method name and Javadoc claim "Less than or equal to match" semantics. The implementation change was likely intentional for use in theAgeMatchingStrategy.EXACTstrategy, but the misleading name creates a maintenance hazard—future developers reading the signature without the implementation will expect<=behavior.Rename the method to
doExactMatch(int, int)and update the Javadoc, or if the current name is retained, clarify in the Javadoc that it performs equality matching only.
🤖 Fix all issues with AI agents
In
`@authentication/authentication-common/src/test/java/childauthFilter/ChildAuthFilterImplTest.java`:
- Around line 91-115: The buildDemoRequest method in ChildAuthFilterImplTest
currently hardcodes IdentityDTO.dob to "2019-01-01", which can cause age/source
mismatches against the identity map used by ChildAuthFilterImpl; update
buildDemoRequest to either omit setting dob or set it to the same test DOB
constant used by the identity map (e.g., reference a shared
TEST_DOB/TestIdentity constant), or accept a DOB parameter so tests can pass the
canonical value; change call sites in the test to use the shared DOB to keep
demographics consistent with ChildAuthFilterImpl's age derivation.
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/CacheConfigTest.java`:
- Around line 14-25: The test method testCacheManagerBeanCreation currently
creates an AnnotationConfigApplicationContext named context and calls
context.close() manually; change it to use try-with-resources by instantiating
the AnnotationConfigApplicationContext in a try(...) block so the context is
automatically closed even if assertions fail, then obtain the
ConcurrentMapCacheManager bean inside that block and remove the explicit
context.close() call.
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.java`:
- Around line 60-74: The test method
testGetSystemSupportedLanguageCodesActualLogic is a duplicate of
testGetSystemSupportedLanguageCodes and currently invokes
getSystemSupportedLanguageCodes on the overridden test fixture config instead of
the real implementation; either delete the duplicate test method or change it to
instantiate a real LangComparatorConfig (instead of reusing the test's
overridden config variable) and supply/mocking the real configuration source so
that invoking getSystemSupportedLanguageCodes exercises production logic and
asserts the expected language list.
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/exception/IDAuthExceptionHandlerTest.java`:
- Around line 5-6: The test class IDAuthExceptionHandlerTest imports JUnit5
assertions but uses the JUnit4 runner; replace the static imports of
assertNotNull and assertNull with the JUnit4 equivalents (import
org.junit.Assert.assertNotNull and org.junit.Assert.assertNull or import
org.junit.Assert.*) so the test uses org.junit.Assert assertions consistently
with the rest of the module and the class’s JUnit4 annotations/runners.
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java`:
- Around line 60-68: The test uses `@Test`(expected =
IdAuthenticationFilterException.class) which causes the exception to
short-circuit the method so the verification of filter2 never runs; change the
test to capture the exception (use try/catch or JUnit's assertThrows) around the
call to validator.validateAuthFilters(authRequestDTO, identityData, properties)
so execution continues and then call verify(filter2, never()). Keep the setup
lines that mock mosipAuthFilterFactory.getEnabledAuthFilters() and doThrow on
filter1 as-is, and assert the caught exception is an
IdAuthenticationFilterException to preserve the original expectation.
♻️ Duplicate comments (1)
authentication/authentication-common/src/test/java/childauthFilter/ChildAuthFilterImplTest.java (1)
127-137: Follow the helper signature update from the earlier comment.
If you adopt thebuildDemoRequest(String demoDob)change, this call should passdob.
🧹 Nitpick comments (12)
authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/ExternalAuthFilterTest.java (2)
3-4: Consider using JUnit 5 for consistency with other test files in this PR.This file uses JUnit 4 annotations (
org.junit.Before,org.junit.Test) whileOTPFilterTest.javauses JUnit 5 (Jupiter). For consistency across the test suite, consider migrating to JUnit 5.♻️ Suggested migration to JUnit 5
-import org.junit.Before; -import org.junit.Test; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test;And update the setup method:
- `@Before` + `@BeforeEach` public void setUp() {
18-24: Mock is created but unused—either add stubbing or simplify.The
ResettableStreamHttpServletRequestmock has no behavior setup. IffetchId()doesn't use the request parameter internally, passingnullwould be simpler and clearer. If it does use the request, consider adding appropriate stubbing to make the test more robust.♻️ Simplified alternative if request is unused
`@Test` public void testFetchId() { - ResettableStreamHttpServletRequest request = mock(ResettableStreamHttpServletRequest.class); String attribute = "attribute"; - String id = filter.fetchId(request, attribute); + String id = filter.fetchId(null, attribute); assertEquals("attributeauth", id); }authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/TrailingSlashRedirectFilterTest.java (1)
34-54: Tighten URL assertion to guarantee trailing-slash removal.
contains("/test")would still pass if the URL remains/test/. Consider asserting the URL ends with/testto make the test stricter.🛠️ Suggested tweak
- assertTrue(url.toString().contains("/test")); - assertTrue(url.toString().startsWith("http://localhost:8080")); + String urlStr = url.toString(); + assertTrue(urlStr.startsWith("http://localhost:8080/")); + assertTrue(urlStr.endsWith("/test"));authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.java (3)
10-11: Unnecessary use of MockitoJUnitRunner.No mocks (
@Mock,@InjectMocks) are used in this test class. Consider removing the runner annotation or switching to a plain JUnit runner.♻️ Proposed fix
-import org.junit.runner.RunWith; -import org.mockito.junit.MockitoJUnitRunner;-@RunWith(MockitoJUnitRunner.class) public class LangComparatorConfigTest {Also applies to: 16-16
33-46: Unnecessary reflection for public method.Since
getSystemSupportedLanguageCodes()is public (as evidenced by the override in setup), reflection is not needed. Direct invocation is cleaner and easier to maintain.♻️ Proposed fix
`@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()); assertTrue(result.contains("en")); assertTrue(result.contains("fr")); assertTrue(result.contains("hi")); }
49-58: Consider direct method invocation and adding behavioral assertions.Similar to above, reflection is unnecessary. Additionally, consider verifying the comparator's behavior rather than just non-null existence.
♻️ Proposed fix with behavioral test
`@Test` public void testGetLanguageComparator() { - Method method = ReflectionUtils.findMethod(LangComparatorConfig.class, "getLanguageComparator"); - ReflectionUtils.makeAccessible(method); - - LanguageComparator comparator = (LanguageComparator) ReflectionUtils.invokeMethod(method, config); - - // just check object creation (we don't need getLanguageCodes) + LanguageComparator comparator = config.getLanguageComparator(); + assertNotNull(comparator); + // Verify comparator sorts based on language priority + assertTrue(comparator.compare("en", "fr") < 0); // "en" comes before "fr" + assertTrue(comparator.compare("hi", "en") > 0); // "hi" comes after "en" }authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/filter/DefaultInternalFilterTest.java (1)
22-41: Coverage-only tests with no behavioral assertions.These tests invoke methods without verifying outcomes. If these methods are truly no-ops in
DefaultInternalFilter, this is acceptable for coverage. Consider adding a brief comment at class level documenting that certain methods are intentionally no-ops in this filter implementation.authentication/authentication-service/src/test/java/io/mosip/authentication/service/kyc/validator/IdentityKeyBindingRequestValidatorUnitTest.java (2)
35-43: Consider adding negative test cases for public key validation.This test only covers the happy path. Consider adding tests for error scenarios:
- Missing modulus (
"n")- Missing exponent (
"e")- Empty JWK map
nullvalues for modulus/exponent💡 Example negative test cases
`@Test` public void validateIdentityKeyBindingPublicKeyAddsErrorWhenModulusMissing() { Errors errors = errors(); Map<String, Object> jwk = new HashMap<>(); jwk.put("e", "AQAB"); ReflectionTestUtils.invokeMethod(validator, "validateIdentityKeyBindingPublicKey", jwk, errors); assertTrue(errors.hasErrors()); } `@Test` public void validateIdentityKeyBindingPublicKeyAddsErrorWhenExponentMissing() { Errors errors = errors(); Map<String, Object> jwk = new HashMap<>(); jwk.put("n", "modulusValue"); ReflectionTestUtils.invokeMethod(validator, "validateIdentityKeyBindingPublicKey", jwk, errors); assertTrue(errors.hasErrors()); }
46-51: Consider adding a test for blank or empty auth factor type.For comprehensive coverage, add a negative test verifying that blank/empty auth factor types produce validation errors.
💡 Example negative test case
`@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/validator/AuthFiltersValidatorTest.java (1)
50-58: Test name claims “in order” but order isn’t asserted.
If order matters, useInOrder; otherwise rename the test to avoid a misleading guarantee.♻️ Example using InOrder
- verify(filter1, times(1)).validate(authRequestDTO, identityData, properties); - verify(filter2, times(1)).validate(authRequestDTO, identityData, properties); + InOrder inOrder = inOrder(filter1, filter2); + inOrder.verify(filter1, times(1)).validate(authRequestDTO, identityData, properties); + inOrder.verify(filter2, times(1)).validate(authRequestDTO, identityData, properties);authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/SwaggerConfigTest.java (1)
21-61: UseReflectionTestUtilsfor cleaner private-field injection.The project includes
spring-boot-starter-test, which providesReflectionTestUtils. This utility is simpler, less brittle, and consistent with Spring testing conventions compared to manual reflection.♻️ Suggested change
- // Inject private field using reflection - Field field = SwaggerConfig.class.getDeclaredField("openApiProperties"); - field.setAccessible(true); - field.set(config, openApiProperties); + // Inject private field via Spring test utility + ReflectionTestUtils.setField(config, "openApiProperties", openApiProperties);-import java.lang.reflect.Field; +import org.springframework.test.util.ReflectionTestUtils;authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/IdAuthConfigTest.java (1)
24-38: PreferReflectionTestUtilsfor private-field injection.Using Spring's test utility keeps this concise and avoids direct reflective access boilerplate.
♻️ Suggested change
- // 🔑 inject `@Autowired` field manually - java.lang.reflect.Field field = - IdAuthConfig.class.getDeclaredField("environment"); - field.setAccessible(true); - field.set(config, environment); + // 🔑 inject `@Autowired` field via Spring test utility + ReflectionTestUtils.setField(config, "environment", environment);+import org.springframework.test.util.ReflectionTestUtils;
| private AuthRequestDTO buildOtpRequest() { | ||
| AuthRequestDTO authRequest = new AuthRequestDTO(); | ||
| RequestDTO request = new RequestDTO(); | ||
| request.setOtp("123456"); | ||
| authRequest.setRequest(request); | ||
| return authRequest; | ||
| } | ||
|
|
||
| private AuthRequestDTO buildDemoRequest() { | ||
| AuthRequestDTO authRequest = new AuthRequestDTO(); | ||
| RequestDTO request = new RequestDTO(); | ||
|
|
||
| IdentityDTO identity = new IdentityDTO(); | ||
| identity.setDob("2019-01-01"); // any demo attribute | ||
|
|
||
| request.setDemographics(identity); | ||
| authRequest.setRequest(request); | ||
| return authRequest; | ||
| } | ||
|
|
||
| private AuthRequestDTO buildEmptyAuthRequest() { | ||
| AuthRequestDTO authRequest = new AuthRequestDTO(); | ||
| authRequest.setRequest(new RequestDTO()); | ||
| return authRequest; | ||
| } |
There was a problem hiding this comment.
Avoid hard‑coded demo DOB to prevent age‑source mismatches.
If ChildAuthFilterImpl ever derives age from request demographics, the fixed "2019-01-01" can make the test non‑representative or time‑sensitive. Prefer passing the same DOB used in the identity map (or omit DOB and use another demo field).
🔧 Suggested tweak
- private AuthRequestDTO buildDemoRequest() {
+ private AuthRequestDTO buildDemoRequest(String demoDob) {
AuthRequestDTO authRequest = new AuthRequestDTO();
RequestDTO request = new RequestDTO();
IdentityDTO identity = new IdentityDTO();
- identity.setDob("2019-01-01"); // any demo attribute
+ identity.setDob(demoDob);
request.setDemographics(identity);
authRequest.setRequest(request);
return authRequest;
}- filter.validate(buildDemoRequest(), data, new HashMap<>());
+ filter.validate(buildDemoRequest(dob), data, new HashMap<>());🤖 Prompt for AI Agents
In
`@authentication/authentication-common/src/test/java/childauthFilter/ChildAuthFilterImplTest.java`
around lines 91 - 115, The buildDemoRequest method in ChildAuthFilterImplTest
currently hardcodes IdentityDTO.dob to "2019-01-01", which can cause age/source
mismatches against the identity map used by ChildAuthFilterImpl; update
buildDemoRequest to either omit setting dob or set it to the same test DOB
constant used by the identity map (e.g., reference a shared
TEST_DOB/TestIdentity constant), or accept a DOB parameter so tests can pass the
canonical value; change call sites in the test to use the shared DOB to keep
demographics consistent with ChildAuthFilterImpl's age derivation.
| @Test | ||
| public void testCacheManagerBeanCreation() { | ||
| // Load only this configuration | ||
| AnnotationConfigApplicationContext context = | ||
| new AnnotationConfigApplicationContext(CacheConfig.class); | ||
|
|
||
| ConcurrentMapCacheManager cacheManager = | ||
| context.getBean(ConcurrentMapCacheManager.class); | ||
|
|
||
| assertNotNull(cacheManager); | ||
|
|
||
| context.close(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "AnnotationConfigApplicationContext" -C 3Repository: mosip/id-authentication
Length of output: 190
🌐 Web query:
AnnotationConfigApplicationContext AutoCloseable Spring Framework
💡 Result:
AnnotationConfigApplicationContext is AutoCloseable because it implements Closeable, AutoCloseable (inherited through Spring’s ConfigurableApplicationContext/GenericApplicationContext hierarchy). (docs.spring.io)
That means you can safely use it with try-with-resources (or call close() manually) to shut down the Spring container and release resources / run bean destruction callbacks:
try (var ctx = new AnnotationConfigApplicationContext(AppConfig.class)) {
MyService s = ctx.getBean(MyService.class);
}
// ctx.close() called automaticallyAnnotationConfigApplicationContext is the standalone container variant that accepts @Configuration / @Component / JSR-330 annotated classes (or package scanning). (docs.spring.io)
Citations:
- 1: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/context/annotation/AnnotationConfigApplicationContext.html?utm_source=openai
- 2: https://docs.spring.io/spring-framework/reference/core/beans/java/instantiating-container.html?utm_source=openai
Close AnnotationConfigApplicationContext via try-with-resources.
The context must be closed to release Spring container resources and trigger bean destruction callbacks. Using try-with-resources guarantees cleanup even if an assertion fails, whereas manual close() won't execute if an exception is thrown before reaching it.
Suggested change
- 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);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void testCacheManagerBeanCreation() { | |
| // Load only this configuration | |
| AnnotationConfigApplicationContext context = | |
| new AnnotationConfigApplicationContext(CacheConfig.class); | |
| ConcurrentMapCacheManager cacheManager = | |
| context.getBean(ConcurrentMapCacheManager.class); | |
| assertNotNull(cacheManager); | |
| context.close(); | |
| `@Test` | |
| public void testCacheManagerBeanCreation() { | |
| // Load only this configuration | |
| try (AnnotationConfigApplicationContext context = | |
| new AnnotationConfigApplicationContext(CacheConfig.class)) { | |
| ConcurrentMapCacheManager cacheManager = | |
| context.getBean(ConcurrentMapCacheManager.class); | |
| assertNotNull(cacheManager); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/CacheConfigTest.java`
around lines 14 - 25, The test method testCacheManagerBeanCreation currently
creates an AnnotationConfigApplicationContext named context and calls
context.close() manually; change it to use try-with-resources by instantiating
the AnnotationConfigApplicationContext in a try(...) block so the context is
automatically closed even if assertions fail, then obtain the
ConcurrentMapCacheManager bean inside that block and remove the explicit
context.close() call.
| @Test | ||
| public void testGetSystemSupportedLanguageCodesActualLogic() throws Exception { | ||
| // use ReflectionUtils to invoke the method | ||
| Method method = ReflectionUtils.findMethod(LangComparatorConfig.class, "getSystemSupportedLanguageCodes"); | ||
| ReflectionUtils.makeAccessible(method); | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| List<String> result = (List<String>) ReflectionUtils.invokeMethod(method, config); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals(3, result.size()); | ||
| assertTrue(result.contains("en")); | ||
| assertTrue(result.contains("fr")); | ||
| assertTrue(result.contains("hi")); | ||
| } |
There was a problem hiding this comment.
Duplicate test – does not test actual production logic.
testGetSystemSupportedLanguageCodesActualLogic is identical to testGetSystemSupportedLanguageCodes and uses the same overridden config instance. Despite its name, it does not exercise the real implementation.
Either remove this duplicate or, if the intent is to test production logic, create a separate instance without the override and provide necessary dependencies (e.g., mock the configuration source).
♻️ Proposed fix – remove duplicate
- `@Test`
- public void testGetSystemSupportedLanguageCodesActualLogic() throws Exception {
- // use ReflectionUtils to invoke the method
- Method method = ReflectionUtils.findMethod(LangComparatorConfig.class, "getSystemSupportedLanguageCodes");
- ReflectionUtils.makeAccessible(method);
-
- `@SuppressWarnings`("unchecked")
- List<String> result = (List<String>) ReflectionUtils.invokeMethod(method, config);
-
- assertNotNull(result);
- assertEquals(3, result.size());
- assertTrue(result.contains("en"));
- assertTrue(result.contains("fr"));
- assertTrue(result.contains("hi"));
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void testGetSystemSupportedLanguageCodesActualLogic() throws Exception { | |
| // use ReflectionUtils to invoke the method | |
| Method method = ReflectionUtils.findMethod(LangComparatorConfig.class, "getSystemSupportedLanguageCodes"); | |
| ReflectionUtils.makeAccessible(method); | |
| @SuppressWarnings("unchecked") | |
| List<String> result = (List<String>) ReflectionUtils.invokeMethod(method, config); | |
| assertNotNull(result); | |
| assertEquals(3, result.size()); | |
| assertTrue(result.contains("en")); | |
| assertTrue(result.contains("fr")); | |
| assertTrue(result.contains("hi")); | |
| } |
🤖 Prompt for AI Agents
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/config/LangComparatorConfigTest.java`
around lines 60 - 74, The test method
testGetSystemSupportedLanguageCodesActualLogic is a duplicate of
testGetSystemSupportedLanguageCodes and currently invokes
getSystemSupportedLanguageCodes on the overridden test fixture config instead of
the real implementation; either delete the duplicate test method or change it to
instantiate a real LangComparatorConfig (instead of reusing the test's
overridden config variable) and supply/mocking the real configuration source so
that invoking getSystemSupportedLanguageCodes exercises production logic and
asserts the expected language list.
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find build configuration files
echo "=== Looking for build configuration files ==="
fd -a -t f '(pom\.xml|build\.gradle|build\.gradle\.kts|gradle\.properties)' | head -20Repository: mosip/id-authentication
Length of output: 749
🏁 Script executed:
#!/bin/bash
# Check the authentication-common pom.xml for junit dependencies
echo "=== Checking authentication-common/pom.xml for junit dependencies ==="
rg -n 'junit' /home/jailuser/git/authentication/authentication-common/pom.xml -i -A 3Repository: mosip/id-authentication
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Read the full pom.xml to see all junit dependencies
echo "=== Full junit/jupiter dependencies in authentication-common/pom.xml ==="
cat /home/jailuser/git/authentication/authentication-common/pom.xml | rg -B 2 -A 3 'junit|jupiter' -iRepository: mosip/id-authentication
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Get a broader view of the pom.xml dependencies section
echo "=== All junit-related artifacts in authentication-common/pom.xml ==="
sed -n '/<dependencies>/,/<\/dependencies>/p' /home/jailuser/git/authentication/authentication-common/pom.xml | grep -i 'junit\|jupiter' -B 1 -A 2Repository: mosip/id-authentication
Length of output: 262
🏁 Script executed:
#!/bin/bash
# Check parent pom and all junit artifacts more comprehensively
echo "=== Parent pom reference in authentication-common/pom.xml ==="
head -20 /home/jailuser/git/authentication/authentication-common/pom.xml | grep -i parent -A 3
echo -e "\n=== All dependency artifacts in authentication-common/pom.xml ==="
sed -n '/<dependencies>/,/<\/dependencies>/p' /home/jailuser/git/authentication/authentication-common/pom.xml | grep '<artifactId>' | head -30Repository: mosip/id-authentication
Length of output: 1832
🏁 Script executed:
#!/bin/bash
# Check parent pom for junit dependencies
echo "=== Parent pom junit/jupiter dependencies ==="
rg -n 'junit|jupiter' /home/jailuser/git/authentication/pom.xml -i -A 2 -B 1
echo -e "\n=== Now check the actual test file ==="
cat -n /home/jailuser/git/authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/exception/IDAuthExceptionHandlerTest.java | head -30Repository: mosip/id-authentication
Length of output: 2088
🏁 Script executed:
#!/bin/bash
# Verify junit-vintage-engine dependency chain
echo "=== Checking junit-vintage-engine version and transitive deps ==="
cat /home/jailuser/git/authentication/pom.xml | sed -n '175,250p'
echo -e "\n=== Check if there are other test files in the project to see patterns ==="
find /home/jailuser/git/authentication/authentication-common/src/test -name "*Test.java" -type f | head -5 | xargs -I {} bash -c 'echo "=== {} ===" && head -30 {}'Repository: mosip/id-authentication
Length of output: 9130
Use JUnit4 assertions for consistency with the rest of the codebase.
Lines 5-6 import JUnit5 assertions while the test uses JUnit4 runner and annotations. All other test files in this module use only org.junit.Assert. Switch to JUnit4 assertions to maintain consistent test patterns across the project.
Suggested change
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;🤖 Prompt for AI Agents
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/exception/IDAuthExceptionHandlerTest.java`
around lines 5 - 6, The test class IDAuthExceptionHandlerTest imports JUnit5
assertions but uses the JUnit4 runner; replace the static imports of
assertNotNull and assertNull with the JUnit4 equivalents (import
org.junit.Assert.assertNotNull and org.junit.Assert.assertNull or import
org.junit.Assert.*) so the test uses org.junit.Assert assertions consistently
with the rest of the module and the class’s JUnit4 annotations/runners.
| @Test(expected = IdAuthenticationFilterException.class) | ||
| public void validateAuthFilters_stopsWhenAFilterThrows() throws Exception { | ||
| when(mosipAuthFilterFactory.getEnabledAuthFilters()).thenReturn(List.of(filter1, filter2)); | ||
| doThrow(new IdAuthenticationFilterException("E", "m")).when(filter1).validate(authRequestDTO, identityData, properties); | ||
|
|
||
| validator.validateAuthFilters(authRequestDTO, identityData, properties); | ||
|
|
||
| verify(filter2, never()).validate(authRequestDTO, identityData, properties); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd authentication && find . -name "AuthFiltersValidatorTest.java" -type fRepository: mosip/id-authentication
Length of output: 183
🏁 Script executed:
cat -n authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java | head -100Repository: mosip/id-authentication
Length of output: 213
🏁 Script executed:
find . -name "AuthFiltersValidatorTest.java" -type fRepository: mosip/id-authentication
Length of output: 198
🏁 Script executed:
cat -n ./authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java | head -100Repository: mosip/id-authentication
Length of output: 3033
🏁 Script executed:
find . -name "AuthFiltersValidator.java" -type f | grep -v TestRepository: mosip/id-authentication
Length of output: 194
🏁 Script executed:
cat -n ./authentication/authentication-common/src/main/java/io/mosip/authentication/common/service/validator/AuthFiltersValidator.javaRepository: mosip/id-authentication
Length of output: 2159
The verify() call after the exception is unreachable with @Test(expected=...).
When @Test(expected = IdAuthenticationFilterException.class) catches the matching exception on line 65, JUnit exits the test method immediately, so the verify(filter2, never()) assertion on line 67 never executes. Use try/catch (or assertThrows) to ensure the assertion runs and verifies that filter2 was not called.
🐛 Suggested fix (no import changes)
- `@Test`(expected = IdAuthenticationFilterException.class)
- public void validateAuthFilters_stopsWhenAFilterThrows() throws Exception {
+ `@Test`
+ public void validateAuthFilters_stopsWhenAFilterThrows() throws Exception {
when(mosipAuthFilterFactory.getEnabledAuthFilters()).thenReturn(List.of(filter1, filter2));
doThrow(new IdAuthenticationFilterException("E", "m")).when(filter1).validate(authRequestDTO, identityData, properties);
- validator.validateAuthFilters(authRequestDTO, identityData, properties);
+ try {
+ validator.validateAuthFilters(authRequestDTO, identityData, properties);
+ org.junit.Assert.fail("Expected IdAuthenticationFilterException");
+ } catch (IdAuthenticationFilterException expected) {
+ // expected
+ }
verify(filter2, never()).validate(authRequestDTO, identityData, properties);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test(expected = IdAuthenticationFilterException.class) | |
| public void validateAuthFilters_stopsWhenAFilterThrows() throws Exception { | |
| when(mosipAuthFilterFactory.getEnabledAuthFilters()).thenReturn(List.of(filter1, filter2)); | |
| doThrow(new IdAuthenticationFilterException("E", "m")).when(filter1).validate(authRequestDTO, identityData, properties); | |
| validator.validateAuthFilters(authRequestDTO, identityData, properties); | |
| verify(filter2, never()).validate(authRequestDTO, identityData, properties); | |
| } | |
| `@Test` | |
| public void validateAuthFilters_stopsWhenAFilterThrows() throws Exception { | |
| when(mosipAuthFilterFactory.getEnabledAuthFilters()).thenReturn(List.of(filter1, filter2)); | |
| doThrow(new IdAuthenticationFilterException("E", "m")).when(filter1).validate(authRequestDTO, identityData, properties); | |
| try { | |
| validator.validateAuthFilters(authRequestDTO, identityData, properties); | |
| org.junit.Assert.fail("Expected IdAuthenticationFilterException"); | |
| } catch (IdAuthenticationFilterException expected) { | |
| // expected | |
| } | |
| verify(filter2, never()).validate(authRequestDTO, identityData, properties); | |
| } |
🤖 Prompt for AI Agents
In
`@authentication/authentication-common/src/test/java/io/mosip/authentication/common/service/validator/AuthFiltersValidatorTest.java`
around lines 60 - 68, The test uses `@Test`(expected =
IdAuthenticationFilterException.class) which causes the exception to
short-circuit the method so the verification of filter2 never runs; change the
test to capture the exception (use try/catch or JUnit's assertThrows) around the
call to validator.validateAuthFilters(authRequestDTO, identityData, properties)
so execution continues and then call verify(filter2, never()). Keep the setup
lines that mock mosipAuthFilterFactory.getEnabledAuthFilters() and doThrow on
filter1 as-is, and assert the caught exception is an
IdAuthenticationFilterException to preserve the original expectation.
1 participant
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.