-
Notifications
You must be signed in to change notification settings - Fork 151
MOSIP-43936: Prefixed the run context for running the testrig at same time in multiple instances. #1604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
WalkthroughThe changes introduce context-driven parameterization throughout the test suite, replacing hardcoded ESIGNET module references with dynamic values derived from BaseTestCase.runContext. This affects test initialization, email generation, module matching logic, and database cleanup queries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/SimplePostForAutoGenIdForUrlEncoded.java (1)
151-155: Case sensitivity change:equalsIgnoreCase→contains.The switch from
equalsIgnoreCase("esignet")tocontains("esignet")introduces case sensitivity. IfBaseTestCase.currentModulecould contain uppercase variants (e.g.,"ESIGNET"or"prefix_ESIGNET"), this check will fail.Verify that
currentModuleis always set with lowercase "esignet" (e.g., derived fromrunContext + "esignet") or consider usingtoLowerCase().contains("esignet")for robustness.🔎 Suggested case-insensitive check
if (testCaseDTO.getEndPoint().contains("$GETENDPOINTFROMWELLKNOWN$") - && BaseTestCase.currentModule.contains("esignet")) { + && BaseTestCase.currentModule.toLowerCase().contains("esignet")) {api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/AddIdentity.java (1)
167-167: Verify thatrunContextcontains only email-safe characters.The inclusion of
BaseTestCase.runContextin the email address properly supports concurrent test execution. Ensure that therunContextvalue is validated to contain only email-safe characters.Note: Minor code style observation - there's inconsistent spacing in the concatenation (missing space before
+ "_" + BaseTestCase.runContext), though this doesn't affect functionality.🔎 Optional: Improve code formatting consistency
-String email = testCaseName + "_" + generateRandomAlphaNumericString(3) + generateRandomAlphaNumericString(3)+ "_" + BaseTestCase.runContext + "@mosip.net"; +String email = testCaseName + "_" + generateRandomAlphaNumericString(3) + generateRandomAlphaNumericString(3) + "_" + BaseTestCase.runContext + "@mosip.net";api-test/src/main/java/io/mosip/testrig/apirig/esignet/testrunner/MosipTestRunner.java (1)
220-221: Consider adding a separator for consistency with other module name concatenations.The concatenation
BaseTestCase.runContext + GlobalConstants.ESIGNETcreates a module name without a separator. While the codebase uses.contains(GlobalConstants.ESIGNET)for module checks (which works correctly with this pattern), other concatenations in the same codebase use underscores as separators (e.g., in AddIdentity:testCaseName + "_" + runContext). Adding a separator likeBaseTestCase.runContext + "_" + GlobalConstants.ESIGNETwould improve consistency and readability without affecting functionality.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api-test/pom.xmlapi-test/src/main/java/io/mosip/testrig/apirig/esignet/testrunner/MosipTestRunner.javaapi-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/AddIdentity.javaapi-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/SimplePostForAutoGenIdForUrlEncoded.javaapi-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.javaapi-test/src/main/resources/config/idaCertDataDeleteQueries.txtapi-test/src/main/resources/config/keyManagerCertDataDeleteQueries.txtapi-test/src/main/resources/config/masterDataCertDataDeleteQueries.txtapi-test/src/main/resources/config/pmsDataDeleteQueries.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-02T11:19:44.545Z
Learnt from: prathmeshj12
Repo: mosip/esignet PR: 1560
File: api-test/src/main/resources/esignet/LinkedConsent/LinkedAuthenticationConsentWla/LinkedAuthenticationConsentWla.yml:512-566
Timestamp: 2025-12-02T11:19:44.545Z
Learning: In the esignet test suite file LinkedAuthenticationConsentWla.yml, negative test cases can include successful sub-operation responses (sendOtpResp and validateOtpResp) alongside the expected error response. This composite structure validates that OTP operations can succeed while the main authentication fails, allowing comprehensive testing of the authentication flow.
Applied to files:
api-test/src/main/java/io/mosip/testrig/apirig/esignet/testrunner/MosipTestRunner.javaapi-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java
📚 Learning: 2025-11-28T05:36:42.921Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:127-130
Timestamp: 2025-11-28T05:36:42.921Z
Learning: In the MOSIP esignet codebase, the `mosip.esignet.discovery.issuer-id` property is configured as `${mosip.esignet.domain.url}${server.servlet.path}`, where `server.servlet.path` typically equals `/v1/esignet`. Therefore, the `discoveryIssuerId` field already contains the `/v1/esignet` prefix, and appending `/oauth/token` for the non-V2 token endpoint correctly produces the full path `/v1/esignet/oauth/token`.
Applied to files:
api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/SimplePostForAutoGenIdForUrlEncoded.java
📚 Learning: 2025-11-28T06:02:21.845Z
Learnt from: sacrana0
Repo: mosip/esignet PR: 1546
File: oidc-service-impl/src/main/java/io/mosip/esignet/services/OAuthServiceImpl.java:198-198
Timestamp: 2025-11-28T06:02:21.845Z
Learning: In the MOSIP esignet codebase, the properties `pushed_authorization_request_endpoint`, `token_endpoint` (from `mosip.esignet.oauth.key-values`), and `issuer` (from `mosip.esignet.discovery.key-values`) are permanently defined in application default properties and are guaranteed to be non-null. Therefore, null checks for these configuration values in OAuthServiceImpl are not required.
Applied to files:
api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/SimplePostForAutoGenIdForUrlEncoded.java
🔇 Additional comments (10)
api-test/src/main/resources/config/masterDataCertDataDeleteQueries.txt (1)
3-8: Parameterization is consistent with other query files.The changes align with the context-driven approach across all delete query config files. The same placeholder substitution verification noted for
idaCertDataDeleteQueries.txtapplies here.api-test/src/main/resources/config/pmsDataDeleteQueries.txt (1)
3-10: Parameterization forcr_byfield is consistent with context-driven approach.The
${currentModule}-111997pattern correctly prefixes the module context to the creator identifier, enabling proper teardown when running multiple test instances concurrently.api-test/src/main/resources/config/keyManagerCertDataDeleteQueries.txt (1)
3-14: Comprehensive parameterization for key manager cert stores.The changes correctly parameterize both
ca_cert_storeandpartner_cert_storetables with the${currentModule}placeholder, maintaining consistency with other delete query configurations.api-test/src/main/java/io/mosip/testrig/apirig/esignet/utils/EsignetUtil.java (3)
231-240: Verify the restructured condition logic is intentional.The condition grouping has changed. Previously, the
endpoint.contains(...)check may have been part of a different logical structure. Now, specific test case names must match AND the endpoint must match for the skip to trigger.Ensure this restructuring doesn't inadvertently allow test cases that should be skipped when running against the IDA authenticator.
2242-2244: Usingcontains()is appropriate for prefixed module contexts.The change from
equals(GlobalConstants.ESIGNET)tocontains(GlobalConstants.ESIGNET)correctly supports prefixed module names (e.g.,prefix_esignet) as described in the PR objective. This enables the OTP handling logic to trigger for any module containing "esignet" in its name.
2342-2344: Consistent with thecontains()approach inpostRequestWithCookieAndAuthHeader.This change mirrors the logic at line 2242, ensuring both methods handle prefixed module contexts consistently for OTP handling.
api-test/pom.xml (1)
71-71: Verify why the dependency version was bumped and address the version inconsistency.The version bump to
1.3.6-SNAPSHOTcreates an inconsistency:api-test/pom.xmluses1.3.6-SNAPSHOTwhileui-test/pom.xmluses1.3.3(stable release). Additionally,BaseTestCase.runContextandinitializePMSDetails()are already in use inMosipTestRunner.java, so it's unclear what new functionality the version bump provides. Confirm that this version bump is intentional and either align versions across modules or document the reason for the divergence.api-test/src/main/resources/config/idaCertDataDeleteQueries.txt (1)
3-8: Verify${currentModule}placeholder substitution in DBManager.The queries use
${currentModule}as a placeholder. Ensure thatDBManager.executeDBQueries()performs this substitution before database execution. Without proper substitution, these queries will match literal${currentModule}strings rather than the dynamic module context. Since DBManager is from an external library (apitest-commons), confirm its implementation handles variable substitution correctly.api-test/src/main/java/io/mosip/testrig/apirig/esignet/testrunner/MosipTestRunner.java (1)
222-222: LGTM! PMS details initialization added.The call to
initializePMSDetails()is appropriately placed after module context setup and aligns with the PR's objective to support concurrent test execution with context isolation.api-test/src/main/java/io/mosip/testrig/apirig/esignet/testscripts/AddIdentity.java (1)
101-101: Verify thatrunContextcontains only email-safe characters.The inclusion of
BaseTestCase.runContextin the email address is appropriate for test isolation. However, ensure that therunContextvalue is validated to contain only characters allowed in the local-part of email addresses (alphanumeric, dots, hyphens, underscores, etc.) to prevent runtime failures.
Signed-off-by: SradhaMohanty5899 <mohantysradha10@gmail.com>
MOSIP-43936: Prefixed the run context for running the testrig at same time in multiple instances.
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.