MOSIP-44431:Masterdata - Update the test case descriptions to improve…#1343
MOSIP-44431:Masterdata - Update the test case descriptions to improve…#1343Famuda1234 wants to merge 2 commits intomosip:developfrom
Conversation
… clarity Signed-off-by: Famuda1234 <famuda.m@cyberpwn.com>
WalkthroughThis PR limits per-language test runs to a single iteration in the test runner, replaces and expands masterdata configuration properties (endpoints, app/client IDs, secrets, toggles, DB and S3 settings), and updates many YAML test case description strings to verification‑focused wording. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… clarity and readability Signed-off-by: Famuda1234 <famuda.m@cyberpwn.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@api-test/src/main/java/io/mosip/testrig/apirig/masterdata/testrunner/MosipTestRunner.java`:
- Around line 104-106: The loop in MosipTestRunner.java that clears and sets
BaseTestCase.languageList uses a hardcoded `i < 1` which will attempt
`localLanguageList.get(0)` even when the source list is empty; modify the loop
to guard against empty lists (e.g., iterate up to Math.min(1,
localLanguageList.size()) or add an explicit check `if
(localLanguageList.isEmpty()) return/continue` before the loop) so that the code
in the block that calls BaseTestCase.languageList.clear() and
BaseTestCase.languageList.add(localLanguageList.get(i)) never executes when
localLanguageList is empty.
In `@api-test/src/main/resources/config/masterdata.properties`:
- Around line 21-42: This file contains hardcoded secrets (keys like
keycloak_Password, postgres-password, audit_password, partner_password, and
mosip_*_client_secret / AuthClientSecret) which must be removed from source;
replace each literal value with a reference to a runtime secret (e.g.,
environment variable names or a single external secrets file path) and update
consumers to read from env or a secret loader instead of masterdata.properties,
add the external secrets file pattern to .gitignore (or ensure env-only usage),
and after changing the file rotate the exposed credentials and purge them from
git history (BFG/git-filter-branch) to eliminate the committed secrets.
In
`@api-test/src/main/resources/masterdata/DeleteUserCenterMapping/DeleteUserCenterMapping.yml`:
- Line 5: The description field in DeleteUserCenterMapping.yml is truncated
("vali"); update the description value for the test case in
DeleteUserCenterMapping to the full sentence, e.g., "Verify that the system
successfully deletes a user-center mapping when all details are valid", by
editing the description entry in the DeleteUserCenterMapping.yml file (look for
the description key in the DeleteUserCenterMapping test definition).
🧹 Nitpick comments (2)
api-test/src/main/resources/masterdata/DeleteUserCenterMapping/DeleteUserCenterMapping.yml (1)
20-20: Consider being more specific about what is invalid.The test specifically validates behavior with an invalid ID (line 26), so the description could be more precise by stating "invalid ID" rather than the generic "invalid details".
✨ Suggested improvement
- description: Verify that the system does not delete a user-center mapping when invalid details are provided + description: Verify that the system does not delete a user-center mapping when an invalid ID is providedapi-test/src/main/resources/masterdata/DeviceSpecification/UpdateDeviceSpecification.yml (1)
462-467: Minor:descriptionfield ordering is inconsistent with other test cases.In all other test cases in this file (and across the PR),
descriptionappears immediately afteruniqueIdentifier. Here it's placed aftertemplateFields(Lines 465-467). Consider reordering for consistency.Suggested reorder
MasterData_UpdateDeviceSpecification_InputParameter_MinDriverversion_AboveMaxLen16: endPoint: /v1/masterdata/devicespecifications uniqueIdentifier: TC_Masterdata_UpdateDeviceSpecification_17 + description: Verify that the system does not update a device specification when the minimum driver version exceeds 16 characters role: admin templateFields: ["name","description"] - description: Verify that the system does not update a device specification when the minimum driver version exceeds 16 characters restMethod: put
| for (int i = 0; i < 1; i++) { | ||
| BaseTestCase.languageList.clear(); | ||
| BaseTestCase.languageList.add(localLanguageList.get(i)); |
There was a problem hiding this comment.
Hardcoding i < 1 removes the empty-list guard the original loop had.
Previously, i < localLanguageList.size() would safely skip the loop body when no languages were configured. Now i < 1 always enters the loop, and localLanguageList.get(0) on Line 106 will throw an IndexOutOfBoundsException if BaseTestCase.getLanguageList() returns an empty list.
If single-language execution is intentional, guard against an empty list or at least use Math.min(1, localLanguageList.size()):
Proposed fix
- for (int i = 0; i < 1; i++) {
+ for (int i = 0; i < Math.min(1, localLanguageList.size()); i++) {📝 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.
| for (int i = 0; i < 1; i++) { | |
| BaseTestCase.languageList.clear(); | |
| BaseTestCase.languageList.add(localLanguageList.get(i)); | |
| for (int i = 0; i < Math.min(1, localLanguageList.size()); i++) { | |
| BaseTestCase.languageList.clear(); | |
| BaseTestCase.languageList.add(localLanguageList.get(i)); |
🤖 Prompt for AI Agents
In
`@api-test/src/main/java/io/mosip/testrig/apirig/masterdata/testrunner/MosipTestRunner.java`
around lines 104 - 106, The loop in MosipTestRunner.java that clears and sets
BaseTestCase.languageList uses a hardcoded `i < 1` which will attempt
`localLanguageList.get(0)` even when the source list is empty; modify the loop
to guard against empty lists (e.g., iterate up to Math.min(1,
localLanguageList.size()) or add an explicit check `if
(localLanguageList.isEmpty()) return/continue` before the loop) so that the code
in the block that calls BaseTestCase.languageList.clear() and
BaseTestCase.languageList.add(localLanguageList.get(i)) never executes when
localLanguageList is empty.
| endPoint: /v1/masterdata/usercentermapping/{id} | ||
| uniqueIdentifier: TC_Masterdata_DeleteUserCenterMapping_01 | ||
| description: Delete User Center Mapping with all valid data | ||
| description: Verify that the system successfully deletes a user-center mapping when all details are vali |
There was a problem hiding this comment.
Complete the truncated description.
The description appears to be cut off, ending with "vali" instead of "valid".
📝 Proposed fix
- description: Verify that the system successfully deletes a user-center mapping when all details are vali
+ description: Verify that the system successfully deletes a user-center mapping when all details are valid📝 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.
| description: Verify that the system successfully deletes a user-center mapping when all details are vali | |
| description: Verify that the system successfully deletes a user-center mapping when all details are valid |
🤖 Prompt for AI Agents
In
`@api-test/src/main/resources/masterdata/DeleteUserCenterMapping/DeleteUserCenterMapping.yml`
at line 5, The description field in DeleteUserCenterMapping.yml is truncated
("vali"); update the description value for the test case in
DeleteUserCenterMapping to the full sentence, e.g., "Verify that the system
successfully deletes a user-center mapping when all details are valid", by
editing the description entry in the DeleteUserCenterMapping.yml file (look for
the description key in the DeleteUserCenterMapping test definition).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-test/src/main/resources/config/masterdata.properties (1)
139-152: 🛠️ Refactor suggestion | 🟠 MajorDocument how to populate empty secrets at runtime.
The hardcoded secrets issue from the previous review has been properly addressed by using empty placeholders. However, there's no documentation explaining how these should be populated at runtime (e.g., via environment variables, external secrets file, or secrets management service).
Adding documentation will help users deploy this correctly without accidentally committing secrets.
📝 Suggested documentation addition
Add a comment block above line 139:
`#----------------------------------` Client Secret Keys -----------------------# `#--` When test rig ran in docker, these values derived from the environment ---# # # IMPORTANT: Do not hardcode secrets in this file. # These values should be provided at runtime via: # - Environment variables (recommended): export mosip_partner_client_secret=<value> # - External secrets file: Specify path via -Dsecrets.file=<path> # - Secrets management service: AWS Secrets Manager, HashiCorp Vault, etc. # # Example using environment variables: # mosip_partner_client_secret = ${mosip_partner_client_secret} #
🤖 Fix all issues with AI agents
In `@api-test/src/main/resources/config/masterdata.properties`:
- Around line 254-260: The properties OTPTimeOut, attempt, ConfigParameters, and
langselect in masterdata.properties appear unused; either remove them to reduce
config clutter or add documentation comments above each explaining intended
purpose, valid values, and where they are consumed (e.g., OTPTimeOut -> OTP
expiry in seconds, attempt -> retry count, ConfigParameters -> comma-separated
keys, langselect -> environment language selector). Update the file by deleting
unused lines or replacing them with commented descriptions and example values,
and if you keep them, add a short comment referencing the related feature or
future ticket/constant name so their purpose is clear (refer to property names
OTPTimeOut, attempt, ConfigParameters, langselect).
🧹 Nitpick comments (3)
api-test/src/main/resources/config/masterdata.properties (3)
111-111: Minor typo in comment."dervied" should be "derived". This typo appears in multiple section headers (lines 111, 124, 140, 167, 190).
✏️ Suggested fix
-#-- When test rig ran in docker, these values dervied from the environment ---# +#-- When test rig ran in docker, these values derived from the environment ---#Apply this fix to all occurrences (lines 111, 124, 140, 167, 190).
189-219: Document required vs. optional DB configuration values.Multiple DB-related properties are empty, including passwords (lines 198, 204, 216) and infrastructure values like
DB_PORT(line 200) andinstallation-domain(line 201). While empty passwords are correct for security, empty infrastructure values may cause runtime failures.Consider adding comments indicating:
- Which properties are required vs. optional
- Default values if applicable
- How these should be provided (environment variables, etc.)
270-271: Clarify the purpose of the commented-out configuration.Line 271 contains a commented-out configuration that appears to be an example for
mosip_components_base_urls. Consider adding a comment to clarify whether this is:
- An example format for users to follow
- Deprecated configuration that should be removed
- A template to be uncommented in certain scenarios
📝 Example clarification
mosip_components_base_urls= -#auditmanager=api-internal.released.mosip.net;idrepository=api-internal.released.mosip.net;partnermanager=api-internal.released.mosip.net;idauthentication=api-internal.released.mosip.net;policymanager=api-internal.released.mosip.net;authmanager=api-internal.released.mosip.net;resident=api-internal.released.mosip.net;preregistration=api-internal.released.mosip.net;masterdata=api-internal.released.mosip.net;idgenerator=api-internal.released.mosip.net; +# Example format (semicolon-separated component=host pairs): +# mosip_components_base_urls=auditmanager=api-internal.example.net;idrepository=api-internal.example.net
| #------------------------- Need to check if these are used or not ------------------------# | ||
| OTPTimeOut = 181 | ||
| attempt = 10 | ||
| ConfigParameters=mosip.kernel.rid.length,mosip.kernel.uin.length,mosip.kernel.sms.country.code,mosip.kernel.sms.number.length,mosip.kernel.otp.default-length,mosip.kernel.otp.expiry-time,mosip.kernel.otp.key-freeze-time,mosip.kernel.otp.validation-attempt-threshold,mosip.kernel.otp.min-key-length,mosip.kernel.otp.max-key-length,mosip.kernel.licensekey.length,mosip.supported-languages | ||
| # supported values are 0 ,1, 2 based on number of env languages | ||
| langselect=0 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for OTPTimeOut usage ==="
rg -i "OTPTimeOut|otp.*time.*out" --type java --type xml --type properties -C 2
echo -e "\n=== Searching for 'attempt' property usage ==="
rg "attempt\s*=" --type java --type xml --type properties -C 2
echo -e "\n=== Searching for ConfigParameters usage ==="
rg "ConfigParameters" --type java --type xml --type properties -C 2
echo -e "\n=== Searching for langselect usage ==="
rg "langselect" --type java --type xml --type properties -C 2Repository: mosip/admin-services
Length of output: 393
🏁 Script executed:
#!/bin/bash
echo "=== Searching for OTPTimeOut usage ==="
rg "OTPTimeOut" -C 2
echo -e "\n=== Searching for 'attempt' property usage ==="
rg "\.attempt\b|getProperty.*attempt" -C 2
echo -e "\n=== Searching for ConfigParameters usage ==="
rg "ConfigParameters" -C 2
echo -e "\n=== Searching for langselect usage ==="
rg "langselect" -C 2
echo -e "\n=== Checking what files reference the masterdata.properties file ==="
rg "masterdata\.properties" -C 2Repository: mosip/admin-services
Length of output: 3789
🏁 Script executed:
#!/bin/bash
echo "=== Searching for property key references in Java code ==="
rg "OTPTimeOut|attempt|ConfigParameters|langselect" --type java
echo -e "\n=== List all files that might load these properties ==="
rg "getProperty|Properties\[|props\.get" --type java -l | head -20Repository: mosip/admin-services
Length of output: 3678
Remove unused properties or document their purpose.
These four properties (OTPTimeOut, attempt, ConfigParameters, langselect) are not referenced anywhere in the codebase. The TODO comment correctly identifies them as potentially unused. Either remove them to reduce configuration clutter or add clear documentation explaining their intended use if they are meant for future functionality.
🤖 Prompt for AI Agents
In `@api-test/src/main/resources/config/masterdata.properties` around lines 254 -
260, The properties OTPTimeOut, attempt, ConfigParameters, and langselect in
masterdata.properties appear unused; either remove them to reduce config clutter
or add documentation comments above each explaining intended purpose, valid
values, and where they are consumed (e.g., OTPTimeOut -> OTP expiry in seconds,
attempt -> retry count, ConfigParameters -> comma-separated keys, langselect ->
environment language selector). Update the file by deleting unused lines or
replacing them with commented descriptions and example values, and if you keep
them, add a short comment referencing the related feature or future
ticket/constant name so their purpose is clear (refer to property names
OTPTimeOut, attempt, ConfigParameters, langselect).
1 participant
… clarity
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores