MOSIP-45061 & MOSIP-45062 - Created new testrig for Keymanger and added few testcases#580
Merged
Merged
Conversation
…ed few testcases Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in>
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughThis PR adds a new ChangesKeyManager API Test Rig
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/GetWithParam.java (2)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant assignment before validation call.
testCaseName = testCaseDTO.getTestCaseName();is immediately overwritten by the next line. Per theisTestCaseValidForExecutioncontract (returnstestCaseNameunmodified), the first assignment is dead code.🧹 Proposed cleanup
- testCaseName = testCaseDTO.getTestCaseName(); - testCaseName = KeyManagerUtil.isTestCaseValidForExecution(testCaseDTO); + testCaseName = KeyManagerUtil.isTestCaseValidForExecution(testCaseDTO);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/GetWithParam.java` around lines 60 - 61, Remove the redundant assignment in GetWithParam where testCaseName is first set from testCaseDTO.getTestCaseName() and then immediately overwritten by KeyManagerUtil.isTestCaseValidForExecution. Keep only the validation call result assignment, and verify the surrounding execution flow in the GetWithParam test script still uses the validated testCaseName correctly.
31-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoilerplate duplicated identically across 5 test-script classes.
setLogLevel,getTestName, thetestcaselistDataProvider, andsetResultTestNameare byte-for-byte identical inGetWithParam,SimplePost,SimplePostForAutoGenId,SimplePostWithCertValidation, andSimplePostWithCsrValidation. Extracting a common abstract base (e.g.,BaseKeyManagerTestScript extends KeyManagerUtil implements ITest) would centralize this logic and reduce future maintenance drift across all five subclasses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/GetWithParam.java` around lines 31 - 90, The boilerplate in GetWithParam, SimplePost, SimplePostForAutoGenId, SimplePostWithCertValidation, and SimplePostWithCsrValidation is duplicated and should be centralized. Extract the shared methods setLogLevel, getTestName, the testcaselist DataProvider, and setResultTestName into a common base such as BaseKeyManagerTestScript extending KeyManagerUtil and implementing ITest, then have each test class inherit it and keep only the unique test logic.api-test/src/main/resources/customize-emailable-report-template.html (2)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing DOCTYPE declaration.
Static analysis flags no
<!DOCTYPE html>declared before content. Cosmetic, but a quick fix for HTML standards compliance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-test/src/main/resources/customize-emailable-report-template.html` around lines 1 - 2, The HTML template is missing a DOCTYPE declaration before the root element. Update the customize-emailable-report-template.html markup so the document starts with the standard HTML5 doctype before the existing html tag, keeping the rest of the template unchanged.Source: Linters/SAST tools
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnclosed
<b>tag reported by HTMLHint.Static analysis reports the
<b>opened at line 61 is never closed, along with missing closing</table></center></body></html>tags. Since this snippet is truncated context, verify the full file properly closes all tags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-test/src/main/resources/customize-emailable-report-template.html` around lines 58 - 61, The HTML snippet in customize-emailable-report-template.html has an unclosed <b> tag and likely missing closing structure for the surrounding table and center/body/html elements. Update the template around the center/table markup to ensure the <b> opened in this section is properly closed and verify the full template closes all tags used by the report layout, especially the table and document wrapper tags.Source: Linters/SAST tools
api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePut.java (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant assignment.
testCaseNameis assigned fromtestCaseDTO.getTestCaseName()and then immediately overwritten byKeyManagerUtil.isTestCaseValidForExecution(testCaseDTO). The first assignment has no effect.♻️ Proposed cleanup
- testCaseName = testCaseDTO.getTestCaseName(); testCaseName = KeyManagerUtil.isTestCaseValidForExecution(testCaseDTO);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePut.java` around lines 60 - 61, Remove the redundant initial assignment to testCaseName in SimplePut so it is not set from testCaseDTO.getTestCaseName() and then immediately overwritten; keep only the assignment that uses KeyManagerUtil.isTestCaseValidForExecution(testCaseDTO), and verify testCaseName is only initialized once in that flow.api-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKey.yml (1)
111-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest identifiers out of sequence.
TC_KeyManager_GenerateMasterKey_08/_09(lines 113, 135) appear before_06/_07(lines 157, 179) in file order. Harmless functionally, but hurts traceability against a test plan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKey.yml` around lines 111 - 197, The GenerateMasterKey negative test cases are out of sequence, which makes the test plan harder to follow. Reorder the scenario entries in GenerateMasterKey.yml so the uniqueIdentifiers in KeyManager_GenerateMasterKey_* appear in ascending order, specifically place KeyManager_GenerateMasterKey_InvalidObjectType_Negative and KeyManager_GenerateMasterKey_EmptyApplicationId_Negative before the Certificate/CSR referenceId cases. Keep the existing scenario contents unchanged and preserve the naming pattern used by KeyManager_GenerateMasterKey_*.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/push-trigger.yml:
- Around line 154-159: The branch guard in the GitHub Actions workflow is
incorrect because the `if` expression on the artifact packaging and upload steps
in `push-trigger.yml` is always true; update the condition to use an `&&` check
so both `master` and `main` are excluded as intended. Adjust the `if:` on the
`find ... zip` step and the “Upload the springboot jars” step to use the same
corrected branch filter expression.
- Around line 97-194: The new jobs under build-maven-apitest-keymanager,
sonar_analysis_apitest_keymanager, build-apitest-keymanager-local, and
build-dockers-apitest-keymanager are inheriting the workflow’s default
GITHUB_TOKEN scope. Add explicit permissions to each job, starting with
contents: read, and only grant broader access where the referenced reusable
workflows or steps in those jobs require it.
- Around line 165-170: The Slack notification step in the workflow still uses
the archived 8398a7/action-slack@v3 and the inconsistent
secrets.SLACK_WEBHOOK_URL secret. Update the notification step in
push-trigger.yml to use the same existing Slack secret pattern as the other
workflows (secrets.SLACK_WEBHOOK) and replace the archived action with a
maintained Slack action, keeping the job.status and fields configuration aligned
with the current step.
- Around line 133-135: Update the workflow to use the current supported major
versions for the checkout and Java setup steps in the
.github/workflows/push-trigger.yml job. Replace the existing actions/checkout
and actions/setup-java references in the relevant step definitions with the
newer majors, and keep the rest of the step configuration unchanged; do not add
or modify persist-credentials since that change is not needed here.
In `@api-test/Dockerfile`:
- Around line 24-45: The install/download step in the Dockerfile needs
hardening: update the apt-get install in the RUN block to use
--no-install-recommends and remove package lists afterward, and in the kubectl
download flow replace curl -LO with curl -fSLo so failures are detected. Add a
SHA256 verification step for the downloaded kubectl binary before the mv to
/usr/local/bin/, using the existing KUBECTL_VERSION-based download logic to
locate the right artifact.
In `@api-test/entrypoint.sh`:
- Line 4: The JVM command in the entrypoint script is passing system properties
after the jar flag, so Java interprets the first -D argument as the jar path and
the process also won’t forward signals cleanly. Update the java invocation in
the entrypoint so all -D flags are placed before -jar, and wrap it with exec so
the JVM receives SIGTERM directly; use the existing MODULES, ENV_USER,
ENV_ENDPOINT, and ENV_TESTLEVEL arguments in the corrected launch command.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testrunner/MosipTestRunner.java`:
- Around line 74-77: The MosipTestRunner setup flow can skip the final
KeyManagerUtil.dbCleanUp() if dbSetup() or startTestRunner() throws, leaving
stale test data behind. Wrap the dbCleanUp(), dbSetup(), and startTestRunner()
sequence in a try/finally in MosipTestRunner so cleanup always runs, and keep
the final cleanup call in the finally block to guarantee teardown regardless of
failures.
- Around line 135-150: Update MosipTestRunner so TestNG failures affect the
process exit code: have startTestRunner() return a status based on whether a
mastertestsuite file was found and whether runner.run() completed successfully,
then make main() call System.exit(non-zero) when no suite is found or the TestNG
execution reports failures instead of always exiting 0. Use the existing
startTestRunner(), main(), and runner.run() flow to propagate the failure state
through the runner logic.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/utils/KeyManagerConfigManager.java`:
- Around line 17-26: The KeyManagerConfigManager bootstrap currently swallows
failures loading keymanager.properties and continues with an empty map, which
hides configuration errors. Update the initialization flow around
getproperties/moduleSpecificPropertiesMap so that a missing or unreadable
properties file causes an immediate fail-fast path instead of calling init with
incomplete data. In the catch block, log the full exception context and rethrow
or terminate using the existing constructor/init flow in KeyManagerConfigManager
so startup stops before any DB or endpoint setup can proceed.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/utils/KeyManagerUtil.java`:
- Around line 70-108: In augmentResponseWithCsrProperties, malformed CSR parsing
currently falls into the broad catch and returns the original JSON with missing
augmentation fields; update this path so invalid parse/crypto cases always set
csrIsValid and csrSignatureValid to false before returning. Apply the same
explicit-false fallback in the corresponding certificate-handling method
referenced by the comment so both certSignRequest and certificate inputs produce
consistent augmented flags even when parsing fails.
In `@api-test/src/main/resources/log4j.properties`:
- Around line 5-6: The FileAppender in log4j.properties is hardcoded to
src/logs/mosip-api-test.log, which won’t work in the shaded JAR/container
runtime. Update log4j.appender.Appender2.File to use the runtime log directory
consistent with mosip.reg.logpath from spring.properties, and keep the change
localized to the Appender2 configuration.
In `@api-test/src/main/resources/spring.properties`:
- Around line 126-136: The request-signing flags are using an incorrect property
key shape with an extra service segment, so the configuration will not be picked
up. Update the affected properties in spring.properties to use the same naming
pattern as the other packet_* settings for upload, sync, and session-key
requests, and verify the corresponding keys referenced by the packet_upload and
packet_sync configuration blocks are the ones being loaded.
- Line 2: The logging configuration key is misspelled, so the DEBUG level will
not apply to the intended package. Update the logger name in spring.properties
from the misspelled io.mosip.registrtaion entry to the correct
io.mosip.registration value so the logging setting matches the actual logger
namespace.
- Around line 32-35: The spring.properties configuration for the api-test app is
committing a plaintext AUTH_SECRET_KEY, which should not be stored in source
control. Update the properties entry used by spring.properties so
AUTH_SECRET_KEY is left empty or resolved from runtime/environment
configuration, while keeping AUTH_CLIENT_ID intact. Make this change in the
existing client auth config block so the secret is no longer hardcoded.
In `@api-test/src/main/resources/testCaseSkippedList.txt`:
- Around line 1-2: The skipped test entry is still commented out and does not
use the required JIRA number;testcase format, so SkipTestCaseHandler will not
recognize MOSIP-37220. Update testCaseSkippedList.txt to add the testcase as a
plain semicolon-delimited entry matching the header format, and remove the
leading comment marker so the parser can read it as an active skipped case. Use
the existing JIRA number;testcase header and the MOSIP-37220 testcase name as
the identifiers to align the entry with the handler’s expected format.
---
Nitpick comments:
In
`@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/GetWithParam.java`:
- Around line 60-61: Remove the redundant assignment in GetWithParam where
testCaseName is first set from testCaseDTO.getTestCaseName() and then
immediately overwritten by KeyManagerUtil.isTestCaseValidForExecution. Keep only
the validation call result assignment, and verify the surrounding execution flow
in the GetWithParam test script still uses the validated testCaseName correctly.
- Around line 31-90: The boilerplate in GetWithParam, SimplePost,
SimplePostForAutoGenId, SimplePostWithCertValidation, and
SimplePostWithCsrValidation is duplicated and should be centralized. Extract the
shared methods setLogLevel, getTestName, the testcaselist DataProvider, and
setResultTestName into a common base such as BaseKeyManagerTestScript extending
KeyManagerUtil and implementing ITest, then have each test class inherit it and
keep only the unique test logic.
In
`@api-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePut.java`:
- Around line 60-61: Remove the redundant initial assignment to testCaseName in
SimplePut so it is not set from testCaseDTO.getTestCaseName() and then
immediately overwritten; keep only the assignment that uses
KeyManagerUtil.isTestCaseValidForExecution(testCaseDTO), and verify testCaseName
is only initialized once in that flow.
In `@api-test/src/main/resources/customize-emailable-report-template.html`:
- Around line 1-2: The HTML template is missing a DOCTYPE declaration before the
root element. Update the customize-emailable-report-template.html markup so the
document starts with the standard HTML5 doctype before the existing html tag,
keeping the rest of the template unchanged.
- Around line 58-61: The HTML snippet in
customize-emailable-report-template.html has an unclosed <b> tag and likely
missing closing structure for the surrounding table and center/body/html
elements. Update the template around the center/table markup to ensure the <b>
opened in this section is properly closed and verify the full template closes
all tags used by the report layout, especially the table and document wrapper
tags.
In
`@api-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKey.yml`:
- Around line 111-197: The GenerateMasterKey negative test cases are out of
sequence, which makes the test plan harder to follow. Reorder the scenario
entries in GenerateMasterKey.yml so the uniqueIdentifiers in
KeyManager_GenerateMasterKey_* appear in ascending order, specifically place
KeyManager_GenerateMasterKey_InvalidObjectType_Negative and
KeyManager_GenerateMasterKey_EmptyApplicationId_Negative before the
Certificate/CSR referenceId cases. Keep the existing scenario contents unchanged
and preserve the naming pattern used by KeyManager_GenerateMasterKey_*.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 075c87e0-a892-490f-b414-9c4b441674a6
📒 Files selected for processing (51)
.github/workflows/push-trigger.yml.gitignore.idea/runConfigurations/KeyManager_API_Tests.xmlapi-test/.vscode/launch.jsonapi-test/Dockerfileapi-test/README.mdapi-test/entrypoint.shapi-test/pom.xmlapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testrunner/MosipTestRunner.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/GetWithParam.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePost.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePostForAutoGenId.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePostWithCertValidation.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePostWithCsrValidation.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/testscripts/SimplePut.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/utils/KeyManagerConfigManager.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/utils/KeyManagerConstants.javaapi-test/src/main/java/io/mosip/testrig/apirig/keymanager/utils/KeyManagerUtil.javaapi-test/src/main/resources/config/Authorization/OtpGeneration/request.jsonapi-test/src/main/resources/config/Authorization/OtpValidation/request.jsonapi-test/src/main/resources/config/Authorization/internalAuthRequest.jsonapi-test/src/main/resources/config/Authorization/request.jsonapi-test/src/main/resources/config/Authorization/zoneMappingRequest.jsonapi-test/src/main/resources/config/application.propertiesapi-test/src/main/resources/config/healthCheckEndpoint.propertiesapi-test/src/main/resources/config/keyManagerDataQueries.txtapi-test/src/main/resources/config/keyManagerDeleteQueries.txtapi-test/src/main/resources/config/keymanager.propertiesapi-test/src/main/resources/customize-emailable-report-template.htmlapi-test/src/main/resources/dbFiles/dbConfig.xmlapi-test/src/main/resources/keymanager/GenerateCSR/GenerateCSR.hbsapi-test/src/main/resources/keymanager/GenerateCSR/GenerateCSR.ymlapi-test/src/main/resources/keymanager/GenerateCSR/GenerateCSRResult.hbsapi-test/src/main/resources/keymanager/GenerateCSRPrereq/GenerateCSRPrereq.ymlapi-test/src/main/resources/keymanager/GenerateCSRPrereq/GenerateCSRPrereqResult.hbsapi-test/src/main/resources/keymanager/GenerateECSignKey/GenerateECSignKey.hbsapi-test/src/main/resources/keymanager/GenerateECSignKey/GenerateECSignKey.ymlapi-test/src/main/resources/keymanager/GenerateECSignKey/GenerateECSignKeyCSRResult.hbsapi-test/src/main/resources/keymanager/GenerateECSignKey/GenerateECSignKeyResult.hbsapi-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKey.hbsapi-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKey.ymlapi-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKeyCSRResult.hbsapi-test/src/main/resources/keymanager/GenerateMasterKey/GenerateMasterKeyResult.hbsapi-test/src/main/resources/keymanager/error.hbsapi-test/src/main/resources/keymanager/responseCode.hbsapi-test/src/main/resources/log4j.propertiesapi-test/src/main/resources/metadata.xmlapi-test/src/main/resources/spring.propertiesapi-test/src/main/resources/testCaseSkippedList.txtapi-test/testNgXmlFiles/keymanagerMasterTestSuite.xmlapi-test/testNgXmlFiles/keymanagerSuite.xml
Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in>
Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in>
Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in>
lsivanand
approved these changes
Jul 10, 2026
lsivanand
approved these changes
Jul 10, 2026
Signed-off-by: Mohanachandran S <mohanachandran.s@technoforte.co.in>
mahammedtaheer
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MOSIP-45061 & MOSIP-45062 - Created new testrig for Keymanger and added few testcases
Summary by CodeRabbit