MOSIP-43801: Fixed pms ui automation unstable report & test failures. #1603
Conversation
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
…ions Signed-off-by: damodar <[email protected]>
…ions Signed-off-by: damodar <[email protected]>
…ions Signed-off-by: damodar <[email protected]>
…ions Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodar <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
Signed-off-by: damodarguru <[email protected]>
WalkthroughRefactors UI tests/pages to use By locators and explicit waits, adds loader/UI sync helpers and a DateTimeFormatter, hardens WebDriver bootstrap, adjusts TestNG sequencing and test signatures, updates certificate resources, and introduces LogUtil(By) logging overloads. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewSbiDetailsPage.java (1)
128-144: Inconsistent exception handling: missingTimeoutExceptioncatch.Unlike
isSbiCreationDateSameAsBrowserDateFormat, this method does not catchTimeoutException. If the element isn't visible within 20 seconds, the exception will propagate and fail the test instead of returningfalse.🔧 Proposed fix to align exception handling
public boolean isSbiExpirationDateSameAsBrowserDateFormat() { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); - WebElement expiryDateCell = wait - .until(ExpectedConditions.visibilityOfElementLocated(By.id("view_admin_sbi_expiry_date_context"))); - - String browserTime = expiryDateCell.getText().trim(); - - DateTimeFormatter dateFormatter = PmpTestUtil.nonZeroPadderDateFormatter; try { + WebElement expiryDateCell = wait + .until(ExpectedConditions.visibilityOfElementLocated(By.id("view_admin_sbi_expiry_date_context"))); + + String browserTime = expiryDateCell.getText().trim(); + + DateTimeFormatter dateFormatter = PmpTestUtil.nonZeroPadderDateFormatter; LocalDate.parse(browserTime, dateFormatter); + return true; - } catch (DateTimeParseException e) { + } catch (TimeoutException | DateTimeParseException e) { return false; } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/OidcClientAuthPartnerTest.java (1)
395-406: Bug risk: staleoidcClientPageafter re-login (missing assignment from navigation)After
loginAsAuthPartner(), you calldashboardPage.clickOnAuthenticationServicesTitle();but don’t assign it back tooidcClientPagebefore usingoidcClientPage.clickOnCreateOidcClientButton(). This can easily become a stale-element failure (or operate on the wrong page object).Proposed fix
- dashboardPage.clickOnAuthenticationServicesTitle(); - oidcClientPage.clickOnCreateOidcClientButton(); + oidcClientPage = dashboardPage.clickOnAuthenticationServicesTitle(); + oidcClientPage.clickOnCreateOidcClientButton();uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewDeviceDetailsPage.java (1)
272-289: Critical: Using incorrect DateTimeFormatter for date-time parsing.The code uses
PmpTestUtil.nonZeroPadderDateFormatter(pattern"M/d/yyyy") to parse the creation date. However:
- A new
dateTimeFormatterwith pattern"M/d/yyyy, h:mm:ss a"was added toPmpTestUtil(line 60) specifically for parsing date-time values with time components- If the UI displays timestamps with time (e.g., "Created On 1/9/2026, 3:45:23 PM"), the current formatter will fail to parse because it only expects the date portion
- This will cause the method to incorrectly return
falseeven when the date format is validThe fix is to use the appropriate formatter based on the actual format of the UI text.
🐛 Proposed fix to use the correct formatter
If the UI text includes time:
- DateTimeFormatter dateFormatter = PmpTestUtil.nonZeroPadderDateFormatter; + DateTimeFormatter dateTimeFormatterLocal = PmpTestUtil.dateTimeFormatter; try { - LocalDate.parse(dateText, dateFormatter); + LocalDateTime.parse(dateText, dateTimeFormatterLocal); return true; } catch (DateTimeParseException e) { return false; }Note: You'll also need to import
java.time.LocalDateTime.If the UI text does NOT include time, the current code is correct, but please verify the actual UI format and consider adding a comment explaining why
nonZeroPadderDateFormatteris used instead ofdateTimeFormatter.
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java:
- Around line 236-249: The selectDeviceSubType method currently swallows
exceptions via a try-catch that only logs the message; remove the try-catch (or
replace it with explicit handling) so failures propagate or are handled with
full context: locate selectDeviceSubType in AddDevicePage, remove the
surrounding try-catch block around the WebDriverWait/element lookup and
dropdown(subTypeDropdown, value) call (mirroring the fix applied to
selectAddDeviceType), or if you prefer explicit handling, catch specific
exceptions, log a detailed message with the exception (logger.error("... ", e)),
and rethrow or throw a custom RuntimeException so test failures aren't silently
ignored.
- Line 6: Replace the incorrect java.util.NoSuchElementException import with
Selenium's org.openqa.selenium.NoSuchElementException so element-not-found
handling in AddDevicePage uses the correct exception type; update any catch
blocks or references in methods of AddDevicePage that rely on
NoSuchElementException to use the Selenium exception (e.g., constructors,
try/catch around element lookups) to ensure correct exception handling.
- Around line 221-234: The selectAddDeviceType method currently catches all
Exceptions and only logs e.getMessage(), which hides failures; update it to log
the full exception (use logger.error with the exception) and then rethrow so the
test fails fast—e.g., replace the catch block in selectAddDeviceType to catch
Exception e, call logger.error("Failed to select device type: " + value, e), and
then throw e (or wrap and throw a RuntimeException) so the dropdown(...) failure
is not silently swallowed; ensure dropdown(...) remains unchanged.
In @uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java:
- Around line 542-544: The call to isDisplayed(apiKeyView) is redundant because
its boolean return is ignored; remove that call or use its result—either delete
the isDisplayed(apiKeyView) line and keep waitForLoaderToDisappear() followed by
clickOnElement(apiKeyView), or capture and assert its return (e.g., boolean
visible = isDisplayed(apiKeyView); assertTrue(visible)) or wrap
clickOnElement(apiKeyView) in a conditional using isDisplayed; update the
ApiKeyPage method accordingly to reference the existing BasePage methods
(isDisplayed, clickOnElement, waitForLoaderToDisappear) and ensure no dead/no-op
calls remain.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java:
- Line 4: The class imports the wrong NoSuchElementException
(java.util.NoSuchElementException), so the catch blocks in AuthPolicyPage that
expect Selenium's exception (the catches around interactions where
NoSuchElementException is handled) won't catch
org.openqa.selenium.NoSuchElementException; replace the import with
org.openqa.selenium.NoSuchElementException (or fully qualify the exception in
the catch clauses) so Selenium's thrown exception is correctly caught by the
handlers in AuthPolicyPage.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java:
- Around line 278-284: clickOnRequestPolicyButton currently silently no-ops if
neither requestPolicyButton nor policies_request_btn is displayed; change it to
wait for either locator to become clickable (use your existing wait utility or
an explicit WebDriverWait) and then click the one that becomes clickable; if
neither becomes clickable within a reasonable timeout, throw an informative
exception mentioning both requestPolicyButton and policies_request_btn so
callers fail fast with a clear message; keep using
isElementDisplayed/clickOnElement helpers where appropriate but drive their
calls from the wait result rather than an immediate check.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.java:
- Around line 389-398: The clickOnLinkedDevice method currently ignores the
status parameter which can cause the wrong row to be clicked; update the XPath
used to locate addedDevice inside clickOnLinkedDevice to include the status cell
(e.g. append "//td[text()='" + status + "']" into the predicate) so the locator
matches (deviceType, deviceSubType, make, model, status) uniquely, or if status
is not needed remove the status parameter from the method signature and all
callers; adjust references to addedDevice, clickOnElement and any caller code
accordingly.
- Around line 342-351: The XPath in clickOnDeviceThreeDots is fragile because it
climbs out of cells repeatedly and fails if values contain single quotes; change
it to locate a single row (tr) that contains all matching td values (e.g.,
tr[td[normalize-space(.)=deviceType] and td[normalize-space(.)=deviceSubType]
and td[normalize-space(.)=make] and td[normalize-space(.)=model]]) and then find
the button inside that row (e.g.,
.//button[contains(@id,'device_list_action_menu1')]); also ensure each input
value is escaped for XPath literals (add/ reuse an escapeXPathLiteral helper
that handles single quotes via XPath concat or proper quoting) before building
the xpath and then use that stable xpath in the existing WebDriverWait and
clickOnElement call.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java:
- Line 26: The field basePage is declared but never instantiated before use,
causing a NullPointerException when registerAuthPartnerUser() calls
basePage.refreshThePage(); fix by initializing basePage before use (e.g., new
BasePage(...) or via existing page factory) inside registerAuthPartnerUser() or
in the test setup method so refreshThePage() has a non-null instance; update any
constructors or @Before setup to assign basePage and ensure any other uses of
basePage (like in AuthPartnerCreation.registerAuthPartnerUser) reference the
initialized object.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DatasharePolicyTest.java:
- Around line 548-552: The clickOnEditButton() is followed immediately by
refreshThePage(), which can reload before the edit page finishes navigating;
replace that sequence by waiting for the edit page to be present using
isEditPolicyPageTitleDisplayed() with a reasonable timeout after
clickOnEditButton(), only calling refreshThePage() as a fallback if the title
never appears (or remove refresh entirely), then proceed to
enterPolicyName(GlobalConstants.AUTOMATION); this ensures navigation completes
before interacting with the edit page.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java:
- Line 24: The field basePage in class DeactivatePartnerCreation is declared but
never initialized, causing a NullPointerException when calling
basePage.refreshThePage(); initialize basePage before use (e.g., in the
constructor or a setup method) by creating a new BasePage instance or obtaining
it from the existing page factory/context used elsewhere in the test suite;
ensure the initialization occurs before any calls to refreshThePage(), and
update any tearDown/setup logic accordingly so basePage is non-null for the test
lifecycle.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java:
- Line 24: DevicePartnerCreation declares the field basePage but never
initializes it, causing a NullPointerException when basePage.refreshThePage() is
called; initialize basePage (e.g., in the class constructor or the test setup
method like @Before/@BeforeMethod) before any call to refreshThePage, following
the pattern used in DeactivatePartnerCreation, by assigning a new or retrieved
BasePage instance to the basePage field so refreshThePage can be invoked safely.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmDeviceTest.java:
- Line 15: The class-level TestNG annotation on FtmDeviceTest removed the
explicit dependency and should be restored or replaced with a precondition: add
back dependsOnGroups = {"FtmPartnerCreation"} to the @Test(groups =
{"FtmDeviceTest"}) annotation on class FtmDeviceTest to enforce cross-class
ordering, or implement a @BeforeClass method (in FtmDeviceTest) that verifies
the FTM partner/account exists and throws org.testng.SkipException when missing;
reference the class FtmDeviceTest and the @Test annotation or the new
@BeforeClass precondition method to locate where to apply the change.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java:
- Line 16: FtmPartnerCreation has an implicit runtime dependency on
PartnerAdminCreation (the root trust certificate uploads and policy group setup
used by the root trust certificate operations in FtmPartnerCreation) that was
removed; either restore the explicit TestNG dependency by adding back
@Test(dependsOnGroups = "PartnerAdminCreation") on the FtmPartnerCreation test
(so PartnerAdminCreation runs first) or, if it truly no longer depends on that
setup, add a clear Javadoc comment on the FtmPartnerCreation class explaining
why the PartnerAdminCreation setup is no longer required and how the tests
remain safe to run independently.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java:
- Around line 24-25: The field basePage in PolicyAdminAndPartnerCreation is
declared but never initialized, causing a NullPointerException when calling
basePage.refreshThePage(); either initialize basePage before use (e.g., assign
new DashboardPage(...) or obtain it from existing setup) or replace the call
with dashboardPage.refreshThePage() since DashboardPage extends BasePage; update
the code to set basePage = dashboardPage (or construct/obtain a DashboardPage
instance) prior to line where refreshThePage is called to ensure the field is
non-null.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyGroupTest.java:
- Around line 11-12: Removing dependsOnGroups from the PolicyGroupTest class
risks running it without the prerequisite DeactivatePartnerCreation; confirm no
CI or local job runs tests by group selection (e.g., groups={"PolicyGroupTest"})
or else restore an explicit dependency. Either re-add the dependency on
DeactivatePartnerCreation by restoring dependsOnGroups/dependsOnMethods on the
PolicyGroupTest class, or update CI/testng.xml job definitions to always include
the DeactivatePartnerCreation group before PolicyGroupTest; ensure the symbols
PolicyGroupTest, DeactivatePartnerCreation and testng.xml ordering are adjusted
accordingly.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java:
- Line 156: Remove the unnecessary "throws InterruptedException" declaration
from the test method verifySbiAndDeviceAfterDeactivate and the other test
methods in SbiDeviceProviderTest that call page-object methods like
isDeviceStatusRejectedDisplayed(), isDeviceStatusDeactivatedDisplayed(), and
clickOnDeviceThreeDots(); edit each test method signature to delete "throws
InterruptedException" since none of the invoked methods throw
InterruptedException and no blocking calls exist, leaving the rest of the method
body unchanged.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.java:
- Around line 245-248: The code incorrectly spells "kernal" — update all
occurrences to "kernel" and fix casing to match the real file: change the
variable kernalpath to kernelpath, change string literals "kernal.properties"
and "kernal_"+path+".properties" to "Kernel.properties" and
"Kernel_"+path+".properties" respectively, and rename the method
GetKernalFilename() to GetKernelFilename(); apply the same renames/updates in
CommonLibrary.java and ConfigManager.java so all references (variables, method
calls, and file name strings) consistently use
Kernel/kernelpath/GetKernelFilename().
🧹 Nitpick comments (27)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/FtmPage.java (1)
529-539: Good fix for timing issues, but consider consistency.The explicit wait pattern correctly addresses timing-related test failures. The implementation properly waits for element visibility and handles timeouts gracefully.
However, consider these optional improvements:
Extract timeout to constant: The 30-second timeout is hardcoded. Consider extracting it to a class-level constant for easier maintenance and consistency across similar waits.
Framework-wide consistency: This is the only method in the file using explicit waits, while others use
isElementDisplayed(). If timing issues are affecting multiple elements, consider updating the baseisElementDisplayed()method or applying this pattern more broadly.♻️ Optional refactor to extract timeout constant
+ private static final Duration DEFAULT_EXPLICIT_WAIT_TIMEOUT = Duration.ofSeconds(30); + public boolean isPendingForApprovalTextDisplayed() { - WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); + WebDriverWait wait = new WebDriverWait(driver, DEFAULT_EXPLICIT_WAIT_TIMEOUT); try { wait.until(ExpectedConditions.visibilityOf(pendingForApproval)); return true; } catch (TimeoutException e) { return false; } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
259-269: AvoidThread.sleep; add bounds checking for element list.
Thread.sleep(1000)is unreliable and slows tests. Use an explicit wait instead.findElements(...).get(position)can throwIndexOutOfBoundsExceptionif fewer elements exist than expected.Proposed refactor
public void selectAddDeviceTypeWithPosition(String value, int position) throws InterruptedException { - position = position - 1; - Thread.sleep(1000); - WebElement addDeviceTypeSelectDropdown = driver - .findElements(By.xpath("//button[@id='add_device_device_type_dropdown_btn']")).get(position); + WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); + By locator = By.xpath("//button[@id='add_device_device_type_dropdown_btn']"); + wait.until(ExpectedConditions.numberOfElementsToBeMoreThan(locator, position - 1)); + List<WebElement> elements = driver.findElements(locator); + WebElement addDeviceTypeSelectDropdown = elements.get(position - 1); try { - dropdownWithPosition(addDeviceTypeSelectDropdown, value, position + 1); + dropdownWithPosition(addDeviceTypeSelectDropdown, value, position); } catch (IOException e) { logger.info(e.getMessage()); } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (3)
596-610:enterPolicyName: good React-safe clear/type; consider avoiding field shadowing + ensure clickabilityThis is a solid approach for flaky React inputs. Minor maintainability: the local
policyNameBoxshadows the fieldpolicyNameBox; consider renaming the local var (or using the field with a wait on it) to reduce confusion.
817-841:selectPolicyGroupDropdown: prefer clickability/visibility wait over presence before clicking
presenceOfElementLocatedcan return an element that’s not interactable yet. UsingelementToBeClickable(and optionally scrolling) would reduce intermittent click issues; the stale retry is a good addition. Also, the XPath interpolatesvalue—if it can contain quotes, it may break the locator (or select the wrong node).
1253-1266: Clone policy group selection: good scroll + JS fallback; consider narrowing exception + locator escapingCatching
Exceptionis a bit broad—ideally catch click interception/stale cases. Same note on XPath interpolation withvalue(escape if it can contain quotes).uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (1)
343-344: Consider tracking deferred work formally.The comment "required for later" and the commented assertion suggest deferred work. If this verification is needed, consider opening a tracking issue or adding a TODO comment with context.
Do you want me to help generate an issue template to track this deferred device count verification?
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (1)
471-483: Approve with minor suggestion: improve error message specificity.The method rename from
clickOnAPIKeyDisplayedtoclickOnCreateApiKeyimproves naming consistency, and the fallback logic (tryinggenerateApiKeyfirst, thenapiKeyListPageGenerateApiKeyBtn) is good defensive coding. TheRuntimeExceptionis appropriate for fail-fast behavior.💡 Optional: Improve error message specificity
public void clickOnCreateApiKey() { if (isElementDisplayed(generateApiKey)) { clickOnElement(generateApiKey); } else if (isElementDisplayed(apiKeyListPageGenerateApiKeyBtn)) { clickOnElement(apiKeyListPageGenerateApiKeyBtn); } else { - throw new RuntimeException("Create API Key button is not visible on the page"); + throw new RuntimeException("Create API Key button not found. Tried: generateApiKey (id='generate_api_key') and apiKeyListPageGenerateApiKeyBtn (id='generate_api_key_btn')"); } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java (1)
55-56: Remove or re-enable commented assertions.Commented-out assertions reduce test coverage and create maintenance debt. Either:
- Remove them if they're no longer valid
- Uncomment and fix them if they should still be verified
Leaving commented code without explanation is a code smell.
🧹 Consider removing commented assertions
-// assertTrue(partnerCertificatePage.isDashboardFtmChipProviderCardDisplayed(), -// GlobalConstants.isDashboardFtmChipProviderCardDisplayed);and
-// assertTrue(partnerCertificatePage.isPartnerCertificatePageDisplayed(), -// GlobalConstants.isPartnerCertificatePageDisplayed);Also applies to: 141-142
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.java (1)
3-14: Consider centralizingWebDriverWaitcreation inBasePage
Creating new waits per method is fine, but if the suite is standardizing on explicit waits, aBasePage.waitClickable(By, Duration)/waitVisible(By, Duration)helper will reduce duplication and keep timeouts consistent.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
261-269: Placeholder elements: OK, but keep them resilient
The new placeholder locators look fine; just be mindful that placeholder text is often subject to i18n/UI copy changes (more brittle than IDs). If the UI has stable IDs for these placeholders, prefer those.Also applies to: 542-548
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyCreationForAuthPartner.java (1)
255-263: Consider reusing the new retry approach for policy availability
This class’srequestPolicy(String)has no wait/retry; if “policy not yet searchable” is a known flake, consider sharing one robust helper (rather than only fixing it inPartnerPolicyMappingTest).Also applies to: 164-167, 90-92, 133-140
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DatasharePolicyTest.java (1)
885-912: Many repeated “request policy” flows—consider a shared helper (and a wait)
These repeated sequences are prime flake points (open request policy → select partner → select policy → submit). A single helper (with explicit waits) will reduce drift and improve stability.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (2)
75-76: Commented-out assertion may mask test failures.The assertion for
isPartnerCertificateTitleDisplayedis commented out. If this check was removed due to flakiness, consider using an explicit wait instead of removing the validation entirely, to maintain test coverage.
101-107: DuplicatedhandleTermsAndCondition()helper across test classes.This identical helper method is duplicated in
DevicePartnerCreation,PolicyAdminAndPartnerCreation, and potentially other test classes. Consider extracting this to a shared base class or utility to reduce duplication.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (1)
113-119: DuplicatedhandleTermsAndCondition()helper.Same duplication issue as noted in
DeactivatePartnerCreation.java. This pattern is repeated across multiple test classes.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (1)
207-213: DuplicatedhandleTermsAndCondition()helper.This is the fourth occurrence of this identical helper method across test classes. Strongly recommend extracting to a shared utility.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (2)
507-525: LGTM with suggestion: Extract hardcoded timeout values.The wait-based approach with
WebDriverWaitand retry logic for stale elements is a good pattern. Consider extracting the hardcoded timeout values (30 seconds at line 509, 20 seconds at line 529) into constants for consistency and easier maintenance.
1142-1149: Broad exception catch may hide issues.The
catch (Exception e)at line 1146 is overly broad. Consider catching specific exceptions likeTimeoutExceptionto avoid masking unexpected errors during test execution.♻️ Proposed refinement
public boolean isClonePolicyPopupTitleNotDisplayed() { try { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); return wait.until(ExpectedConditions.invisibilityOf(clonePolicyTitle)); - } catch (Exception e) { + } catch (TimeoutException | NoSuchElementException e) { return false; } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (2)
456-462: Silent timeout exception may hide loader issues.The empty catch block swallows the
TimeoutException, which could mask situations where the loader never disappears (e.g., stuck API calls, JavaScript errors). Consider logging a warning or adding a brief debug message to aid troubleshooting.♻️ Suggested improvement
protected void waitForLoaderToDisappear() { try { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); wait.until(ExpectedConditions.invisibilityOfElementLocated(loadingOverlay)); } catch (TimeoutException e) { + // Loader did not disappear within timeout - proceeding anyway + LogUtil.step("Loader still visible after 30s, continuing execution"); } }
433-452: Inconsistent driver reference.
isDisplayedQuickusesDriverManager.getDriver()while the rest of the class uses the instance fieldthis.driver. This inconsistency could lead to subtle bugs if the two references point to different driver instances.♻️ Suggested fix for consistency
protected boolean isDisplayedQuick(By locator) { try { - WebDriverWait wait = new WebDriverWait(DriverManager.getDriver(), QUICK_CHECK_TIMEOUT); + WebDriverWait wait = new WebDriverWait(driver, QUICK_CHECK_TIMEOUT); - return wait.until(driver -> { + return wait.until(d -> { try { - List<WebElement> elements = driver.findElements(locator); + List<WebElement> elements = d.findElements(locator); if (elements.isEmpty()) { return false; } return elements.get(0).isDisplayed(); } catch (StaleElementReferenceException e) { return false; // retry on next poll } }); } catch (TimeoutException e) { return false; } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PartnerCertificatePage.java (1)
602-616: Retry loop lacks delay between attempts.The retry loop for handling
StaleElementReferenceExceptionimmediately retries without any delay. This could result in the same stale state being encountered. Consider adding a small delay or using a more robust wait pattern.♻️ Suggested improvement
public void clickOnFtmChipProviderCard() { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(40)); By card = By.id("dashboard_ftm_chip_provider_card"); - for (int i = 0; i < 2; i++) { - try { - wait.until(ExpectedConditions.elementToBeClickable(card)).click(); - return; - } catch (StaleElementReferenceException e) { - // retry once - } - } - throw new RuntimeException("FTM Chip Provider card not clickable"); + wait.ignoring(StaleElementReferenceException.class) + .until(ExpectedConditions.elementToBeClickable(card)) + .click(); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/BaseClass.java (1)
188-200: Consider adding delay between retry attempts.The retry loop immediately retries without any pause. Adding a brief delay between attempts can help with transient network issues and avoid overwhelming the server.
♻️ Suggested improvement
private void loadUrlWithRetry(String url) { int attempts = 0; while (attempts < 3) { try { driver.get(url); return; } catch (TimeoutException e) { attempts++; + logger.warn("URL load attempt " + attempts + " failed, retrying..."); if (attempts == 3) throw e; + try { + Thread.sleep(1000); // 1 second delay between retries + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + } } } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java (2)
470-513: Duplicate XPath construction across status methods.The methods
isDeviceStatusApprovedDisplayed,isDeviceStatusRejectedDisplayed, andisDeviceStatusDeactivatedDisplayedhave identical implementations. Consider extracting a common helper method.♻️ Refactor to reduce duplication
+private boolean isDeviceStatusDisplayed(String deviceType, String deviceSubType, String make, String model, String status) { + WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); + try { + WebElement deviceStatus = wait.until(ExpectedConditions.visibilityOfElementLocated(By.xpath( + "//td[text()='" + deviceType + "']/.." + "//td[text()='" + deviceSubType + "']/.." + "//td[text()='" + + make + "']/.." + "//td[text()='" + model + "']/.." + "//div[text()='" + status + "']"))); + return isElementDisplayed(deviceStatus); + } catch (TimeoutException e) { + return false; + } +} public boolean isDeviceStatusApprovedDisplayed(String deviceType, String deviceSubType, String make, String model, String status) { - WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); - try { - WebElement deviceStatus = wait.until(ExpectedConditions.visibilityOfElementLocated(By.xpath( - "//td[text()='" + deviceType + "']/.." + "//td[text()='" + deviceSubType + "']/.." + "//td[text()='" - + make + "']/.." + "//td[text()='" + model + "']/.." + "//div[text()='" + status + "']"))); - return isElementDisplayed(deviceStatus); - } catch (TimeoutException e) { - return false; - } + return isDeviceStatusDisplayed(deviceType, deviceSubType, make, model, status); } // Apply same pattern to isDeviceStatusRejectedDisplayed and isDeviceStatusDeactivatedDisplayed
534-554: Duplicate method implementations.
clickOnDeviceThreeDotsAsAdminandclickOnFilteredDeviceEllipsisButtonAsAdminhave identical implementations. Consider having one call the other or consolidating them.♻️ Suggested consolidation
public void clickOnDeviceThreeDotsAsAdmin(String deviceType, String deviceSubType, String make, String model) { - WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); - - WebElement addedDeviceThreeDots = wait.until(ExpectedConditions.elementToBeClickable(By.xpath("//td[text()='" - + deviceType + "']/.." + "//td[text()='" + deviceSubType + "']/.." + "//td[text()='" + make + "']/.." - + "//td[text()='" + model + "']/.." + "//button[contains(@id, 'device_list_action')]"))); - - clickOnElement(addedDeviceThreeDots); + clickOnFilteredDeviceEllipsisButtonAsAdmin(deviceType, deviceSubType, make, model); } public void clickOnFilteredDeviceEllipsisButtonAsAdmin(String deviceType, String deviceSubType, String make, String model) { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); WebElement addedDeviceThreeDots = wait.until(ExpectedConditions.elementToBeClickable(By.xpath("//td[text()='" + deviceType + "']/.." + "//td[text()='" + deviceSubType + "']/.." + "//td[text()='" + make + "']/.." + "//td[text()='" + model + "']/.." + "//button[contains(@id, 'device_list_action')]"))); clickOnElement(addedDeviceThreeDots); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (2)
485-488: Loader wait logic duplicated from BasePage.
waitForDashboardLoaderToDisappear()duplicates the logic inBasePage.waitForLoaderToDisappear()with a different timeout (60s vs 30s). Consider making the timeout configurable in BasePage to avoid duplication.♻️ Suggested consolidation in BasePage
// In BasePage.java protected void waitForLoaderToDisappear(int timeoutSeconds) { try { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(timeoutSeconds)); wait.until(ExpectedConditions.invisibilityOfElementLocated(loadingOverlay)); } catch (TimeoutException e) { // Loader did not disappear within timeout } } protected void waitForLoaderToDisappear() { waitForLoaderToDisappear(30); }Then in DashboardPage:
public void waitForDashboardLoaderToDisappear() { waitForLoaderToDisappear(60); }
348-355: Broad exception handling with WebDriverException.Catching
WebDriverExceptionon line 352 is very broad and may mask unrelated driver issues. Consider catching more specific exceptions.♻️ Suggested narrower exception handling
wait.until((WebDriver d) -> { try { d.findElement(dashboardPartnerCertificateListHeader).click(); return true; - } catch (NoSuchElementException | WebDriverException e) { + } catch (NoSuchElementException | StaleElementReferenceException | ElementClickInterceptedException e) { return false; } });uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (1)
728-734: Consider renamingstatus1andstatus2for clarity.The boolean parameters
status1andstatus2are used to verifyisViewDeviceEnabled()andisDeactivateDeviceEnabled()respectively. More descriptive names would improve readability.♻️ Suggested parameter rename
private void verifyActionMenuOfDevices(String deviceType, String deviceSubType, String make, String model, - String status, boolean status1, boolean status2) throws InterruptedException { + String status, boolean expectViewEnabled, boolean expectDeactivateEnabled) throws InterruptedException { listOfDevicesPage.clickOnDeviceThreeDots(deviceType, deviceSubType, make, model); - assertEquals(listOfDevicesPage.isViewDeviceEnabled(), status1); - assertEquals(listOfDevicesPage.isDeactivateDeviceEnabled(), status2); + assertEquals(listOfDevicesPage.isViewDeviceEnabled(), expectViewEnabled); + assertEquals(listOfDevicesPage.isDeactivateDeviceEnabled(), expectDeactivateEnabled); listOfDevicesPage.clickOnDeviceThreeDots(deviceType, deviceSubType, make, model); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/FtmPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/OidcClientPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PartnerCertificatePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewDeviceDetailsPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewSbiDetailsPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/ApiKeyAuthPartnerTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPolicyTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/CertificateTrustStoreTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DatasharePolicyTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmDeviceTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/OidcClientAuthPartnerTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerAdminCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerDetailsTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyCreationForAuthPartner.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyGroupTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiCreationTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/BaseClass.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.javauitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivateUserClient.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivateUserIntermediateCA.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivateUserRootCA.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivateca.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivatesubca.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserCACert.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserClient.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserInterCACert.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserIntermediateCA.ceruitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserRootCA.ceruitest-pmp-v2/src/main/resources/testngFile/testng.xml
💤 Files with no reviewable changes (4)
- uitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivatesubca.cer
- uitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserCACert.cer
- uitest-pmp-v2/src/main/resources/pmp_uiv2_cert/deactivateca.cer
- uitest-pmp-v2/src/main/resources/pmp_uiv2_cert/policyUserInterCACert.cer
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-10T12:54:52.306Z
Learnt from: damodarguru
Repo: mosip/partner-management-portal PR: 1584
File: uitest-pmp-v2/src/main/resources/config/Kernel.properties:144-146
Timestamp: 2025-12-10T12:54:52.306Z
Learning: In uitest-pmp-v2/src/main/resources/config/Kernel.properties, the properties pmpUiv2Url, apiEnvUser, and apiInternalEndPoint should remain empty in the checked-in configuration file as they are environment-specific credentials and endpoints that are provided at runtime.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.java
📚 Learning: 2025-12-10T12:57:43.001Z
Learnt from: damodarguru
Repo: mosip/partner-management-portal PR: 1584
File: uitest-pmp-v2/src/main/resources/config/Kernel.properties:57-58
Timestamp: 2025-12-10T12:57:43.001Z
Learning: In uitest-pmp-v2/src/main/resources/config/Kernel.properties, the properties db-port and db-server should remain empty in the checked-in configuration file as they are environment-specific credentials that are provided at runtime.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.java
📚 Learning: 2025-11-25T12:41:24.381Z
Learnt from: Sudeep7353
Repo: mosip/partner-management-portal PR: 1574
File: pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js:220-222
Timestamp: 2025-11-25T12:41:24.381Z
Learning: In the file pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js, the `relyingPartyId` field is mandatory and will always have a value from the API, so it does not require a fallback placeholder like `|| '-'`.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/OidcClientAuthPartnerTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/OidcClientPage.java
📚 Learning: 2025-11-25T12:41:40.595Z
Learnt from: Sudeep7353
Repo: mosip/partner-management-portal PR: 1574
File: pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js:332-334
Timestamp: 2025-11-25T12:41:40.595Z
Learning: In the file pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js, the `logoUri` field is mandatory and will always have a value from the API, so it does not require a fallback placeholder like `|| '-'`.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/OidcClientPage.java
🧬 Code graph analysis (16)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/CertificateTrustStoreTest.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java (1)
Test(23-554)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.java (1)
GlobalConstants(3-1215)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(32-477)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-490)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/BaseClass.java (1)
BaseClass(34-202)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewSbiDetailsPage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/OidcClientPage.java (1)
pmp-ui-v2/src/pages/partner/authenticationServices/OidcClientsList.js (1)
createOidcClient(139-141)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.java (1)
GlobalConstants(3-1215)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.java (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (1)
Test(18-215)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
Test(17-109)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java (1)
Test(16-196)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/OidcClientAuthPartnerTest.java (1)
Test(19-733)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (2)
pmp-ui-v2/src/pages/partner/authenticationServices/ApiKeysList.js (1)
generateApiKey(100-102)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyGroupTest.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DatasharePolicyTest.java (1)
Test(20-1010)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
Test(17-109)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmDeviceTest.java (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/ApiKeyAuthPartnerTest.java (1)
Test(17-537)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (1)
Test(18-215)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
Test(17-109)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java (1)
Test(16-196)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/driver/DriverManager.java (1)
DriverManager(5-23)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (1)
Test(17-120)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
Test(18-182)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
Test(17-109)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(32-477)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-490)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/BaseClass.java (1)
BaseClass(34-202)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyCreationForAuthPartner.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/ApiKeyAuthPartnerTest.java (1)
Test(17-537)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (1)
Test(18-215)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PartnerCertificatePage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java
Show resolved
Hide resolved
...st-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyGroupTest.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.java
Outdated
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
250-260: Replace Thread.sleep with explicit wait and avoid mutating the parameter.Two concerns:
- Thread.sleep(1000) is a blocking anti-pattern that makes tests fragile and slow. Replace with an explicit wait condition.
- Mutating the
positionparameter (position = position - 1) reduces code clarity. Consider using a local variable instead.♻️ Proposed refactor
public void selectAddDeviceTypeWithPosition(String value, int position) throws InterruptedException { - position = position - 1; - Thread.sleep(1000); + int index = position - 1; + WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); + wait.until(ExpectedConditions.presenceOfAllElementsLocatedBy( + By.xpath("//button[@id='add_device_device_type_dropdown_btn']"))); + WebElement addDeviceTypeSelectDropdown = driver - .findElements(By.xpath("//button[@id='add_device_device_type_dropdown_btn']")).get(position); + .findElements(By.xpath("//button[@id='add_device_device_type_dropdown_btn']")).get(index); try { - dropdownWithPosition(addDeviceTypeSelectDropdown, value, position + 1); + dropdownWithPosition(addDeviceTypeSelectDropdown, value, index + 1); } catch (IOException e) { logger.info(e.getMessage()); } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
4-16: Fix wrongNoSuchElementExceptionimport—retry loops won't catch Selenium failures.
You importedjava.util.NoSuchElementException, butfindElement(...)throwsorg.openqa.selenium.NoSuchElementException. The catch blocks at lines 259 and 276 will not catch Selenium exceptions, causing them to propagate out of the lambda and break thewait.until()retry mechanism.Proposed fix
-import java.util.NoSuchElementException; import java.util.concurrent.TimeoutException; import org.openqa.selenium.By; import org.openqa.selenium.JavascriptExecutor; +import org.openqa.selenium.NoSuchElementException; import org.openqa.selenium.StaleElementReferenceException; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebDriverException;
🤖 Fix all issues with AI agents
In @uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java:
- Around line 427-446: isDisplayedQuick() currently constructs the WebDriverWait
with DriverManager.getDriver() which can be null and may differ from the page
instance driver; change the wait to use this.driver (WebDriverWait wait = new
WebDriverWait(this.driver, QUICK_CHECK_TIMEOUT)) and inside the wait.until
lambda avoid shadowing the instance field by renaming the lambda parameter
(e.g., drv -> { List<WebElement> elements = drv.findElements(locator); ... }) so
all element lookups use the passed-in drv, while the wait is bound to
this.driver.
- Around line 448-456: The waitForLoaderToDisappear method currently swallows
TimeoutException silently; modify it to log a clear error when the loader
(loadingOverlay) fails to disappear by catching TimeoutException and calling
your page/class logger (or add one) to record a message including the locator
(loadingOverlay), the exception message/stack trace, and a short contextual
message; optionally capture a screenshot or page source for debugging before
rethrowing or failing the test so root cause is visible in logs.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java:
- Around line 596-609: The Ctrl+A chord is not macOS-compatible; update platform
handling so tests work on macOS: either (preferred) replace the select-all
sequence in enterPolicyName (and the two occurrences in DeviceProviderPage) with
a React-safe element.clear() followed by sendKeys(val) to avoid OS-specific
shortcuts, or extend BaseTestCaseFunc.getOSType() to detect macOS distinctly and
use Keys.COMMAND in the chord when running on macOS; apply the chosen approach
to the enterPolicyName method and the two DeviceProviderPage locations.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java:
- Around line 283-305: The clickOnRequestPolicyButton() method fails on
empty-state pages because it only checks request_policy_btn and
policies_request_btn; add the missing show_request_policy locator into the
fallback chain (preferably check isClickable(By.id("show_request_policy"))
first, then request_policy_btn and policies_request_btn) and return/click the
element when found; declare a By showRequestPolicy =
By.id("show_request_policy") (or reuse the existing requestPolicyButton field)
and include it in the WebDriverWait lambda so the method succeeds for empty,
populated, and admin flows.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java:
- Around line 5-6: The method registerDevicePartnerUser declares throws
TimeoutException and the file imports java.util.concurrent.TimeoutException but
the method never throws it; remove the unused import and delete the throws
TimeoutException clause from the registerDevicePartnerUser method signature
(locate the import line for java.util.concurrent.TimeoutException and the method
declaration for registerDevicePartnerUser) so the code compiles with no unused
imports or unnecessary checked exceptions.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java:
- Around line 522-554: The XPath for detecting the "No Data Available" message
in requestPolicy is too strict and doesn't match the actual element text with a
trailing period; update the noDataLabel selector used in requestPolicy
(currently declared as noDataLabel) to a tolerant XPath that uses
contains(normalize-space(.), 'No Data Available') or otherwise matches the
PoliciesPage message (e.g., include or ignore the trailing dot) so the
ExpectedConditions.or check reliably detects the "No Data Available" element and
prevents unnecessary timeouts/retries.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java:
- Around line 5-6: Remove the unused checked TimeoutException import and the
misleading "throws TimeoutException" declaration from the registerNewUser(...)
method (and any other methods in this class that declare
java.util.concurrent.TimeoutException, e.g., the other occurrence around
registerNewUser's siblings); Selenium uses org.openqa.selenium.TimeoutException
(runtime) so delete the import line "import
java.util.concurrent.TimeoutException;" and remove "throws TimeoutException"
from the method signatures to reflect actual exceptions thrown.
🧹 Nitpick comments (12)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
458-469:clickOnElement(By)/enter(By)should reuse your JS-fallback paths;isClickable(By)double-looks up.
Right now the By-based helpers don’t benefit from the existing WebElement-based JS-click fallback, andisClickable(By)callsfindElementtwice.Proposed tweak (reuse WebElement helpers + single lookup)
protected void clickOnElement(By locator) { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); - WebElement element = wait.until(ExpectedConditions.elementToBeClickable(locator)); - element.click(); + WebElement element = wait.until(ExpectedConditions.elementToBeClickable(locator)); + clickOnElement(element); } protected void enter(By locator, String value) { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); WebElement el = wait.until(ExpectedConditions.visibilityOfElementLocated(locator)); - el.clear(); - el.sendKeys(value); + enter(el, value); } protected boolean isClickable(By locator) { try { - return driver.findElement(locator).isDisplayed() && driver.findElement(locator).isEnabled(); + WebElement el = driver.findElement(locator); + return el.isDisplayed() && el.isEnabled(); } catch (Exception e) { return false; } }Also applies to: 471-476
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (2)
213-226: Consider narrowing the exception type.The method throws
Exception, which is overly broad. Consider declaring specific checked exceptions (e.g.,TimeoutException,NoSuchElementException) to help callers handle errors appropriately.♻️ Proposed refactor
-public void selectAddDeviceType(String value) throws Exception { +public void selectAddDeviceType(String value) throws TimeoutException { try { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); WebElement deviceTypeDropdown = wait .until(ExpectedConditions.elementToBeClickable(By.id("add_device_device_type_dropdown"))); dropdown(deviceTypeDropdown, value); } catch (Exception e) { logger.error("Failed to select device type: " + value, e); - throw e; + throw new TimeoutException("Failed to select device type: " + value, e); } }
228-240: Consider narrowing the exception type.Similar to
selectAddDeviceType, this method throwsExceptionwhich is overly broad. Consider declaring specific checked exceptions for better error handling.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
75-80: Remove or document the commented-out assertion.Lines 76-77 contain a commented-out assertion for
isPartnerCertificateTitleDisplayed. If this check is no longer needed, remove the code. If it's temporarily disabled due to a known issue, add a comment explaining why and link to a tracking issue.♻️ Proposed cleanup
handleTermsAndCondition(); basePage.refreshThePage(); -// assertTrue(dashboardPage.isPartnerCertificateTitleDisplayed(), -// GlobalConstants.isPartnerCertificateTitleDisplayed); dashboardPage.clickOnPartnerCertificateTitle();Or, if there's a reason:
handleTermsAndCondition(); basePage.refreshThePage(); +// TODO: Re-enable once issue #XXXX is resolved // assertTrue(dashboardPage.isPartnerCertificateTitleDisplayed(), // GlobalConstants.isPartnerCertificateTitleDisplayed); dashboardPage.clickOnPartnerCertificateTitle();uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
1142-1149: Consider narrowing the exception catch block.Catching all
Exceptiontypes (line 1146) may hide legitimate errors. Consider catching only expected exceptions likeTimeoutExceptionorStaleElementReferenceException.♻️ Proposed refactor
public boolean isClonePolicyPopupTitleNotDisplayed() { try { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); return wait.until(ExpectedConditions.invisibilityOf(clonePolicyTitle)); - } catch (Exception e) { + } catch (TimeoutException | StaleElementReferenceException e) { return false; } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (1)
78-80: Remove or document the commented-out assertion.Lines 79-80 contain a commented-out assertion. If this check is no longer valid after the page refresh, remove the code. Otherwise, add a comment explaining the temporary suppression.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (3)
51-62: Avoid duplicate locators for the same element (partner certificate header).
partnerCertificateTitleanddashboardPartnerCertificateListHeaderpoint to the same id, which is easy to drift later. Consider consolidating to a singleBy.Proposed refactor
@@ - private By partnerCertificateTitle = By.id("dashboard_partner_certificate_list_header"); + private By partnerCertificateListHeader = By.id("dashboard_partner_certificate_list_header"); @@ - private By dashboardPartnerCertificateListHeader = By.id("dashboard_partner_certificate_list_header"); + // reuse partnerCertificateListHeader@@ - WebElement el = d.findElement(partnerCertificateTitle); + WebElement el = d.findElement(partnerCertificateListHeader); el.click();@@ - d.findElement(dashboardPartnerCertificateListHeader).click(); + d.findElement(partnerCertificateListHeader).click(); return true;Also applies to: 105-106
180-193: Harden dynamic XPath building forpolicyGroupValue(quote-safe) + narrow exception handling.
IfpolicyGroupValuecontains'(or other special chars), the XPath can become invalid and fail intermittently. Also, catchingExceptionfor click fallback can hide real driver issues.Suggested direction (quote-safe literal helper)
-By optionLocator = By.xpath("//button[.//span[contains(normalize-space(text()),'" + policyGroupValue + "')]]"); +By optionLocator = By.xpath("//button[.//span[contains(normalize-space(text())," + xpathLiteral(policyGroupValue) + ")]]");
251-265: Centralize and reuse loader waiting + click-with-retry to reduce duplication.
Three methods duplicate the"//*[contains(text(),'Loading')]"locator and have slightly different retry/catch behavior. Consider a sharedprivate static final By LOADING = ...+ aclickWithRetry(By)helper (including optional JS click fallback) to keep behavior consistent.Example small refactor
@@ public class DashboardPage extends BasePage { + private static final By LOADING_TEXT = By.xpath("//*[contains(text(),'Loading')]"); @@ public void waitForDashboardLoaderToDisappear() { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(60)); - wait.until(ExpectedConditions.invisibilityOfElementLocated(By.xpath("//*[contains(text(),'Loading')]"))); + wait.until(ExpectedConditions.invisibilityOfElementLocated(LOADING_TEXT)); }Also applies to: 267-282, 330-342, 471-474
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (3)
195-274: Consider standardizing admin “ellipsis” interaction method to one API.
This method still usesclickOnDeviceThreeDotsAsAdmin(...)for some devices, but switches toclickOnFilteredDeviceEllipsisButtonAsAdmin(...)after filtering. If the new filtered method is the stabilized one, migrating the earlier steps too could reduce behavioral divergence.
521-536: Prefer narrower throws where feasible (avoid blanketthrows Exception).
Usingthrows Exceptionis fine for quick stabilization, but tightening to the real checked exceptions (oftenInterruptedException/TimeoutException/etc.) will make failures easier to reason about and prevent accidental swallowing upstream.Also applies to: 538-557
624-648: Avoid string literals inswitch(status); use constants (or an enum) to prevent typo-based branching.
Right now it matches"Approved","Pending For Approval", etc. It’s easy for a constant/value change to silently fall intodefault.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.java
🚧 Files skipped from review as they are similar to previous changes (2)
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/TestRunner.java
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.java
🧰 Additional context used
🧬 Code graph analysis (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(32-479)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-476)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(32-479)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-476)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/RegisterPage.java (1)
RegisterPage(10-211)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/driver/DriverManager.java (1)
DriverManager(5-23)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.java (1)
GlobalConstants(3-1215)
🔇 Additional comments (22)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (3)
156-156: LGTM!The method signature formatting change is acceptable.
167-175: The status parameter is not redundant and is actively used in the method implementations. BothisDeviceStatusRejectedDisplayed()andisDeviceStatusDeactivatedDisplayed()use the status parameter in their XPath locators to find elements matching the specific status text (//div[text()='" + status + "']). The current usage patterns correctly align method names with matching status constants (e.g.,isDeviceStatusRejectedDisplayedwithGlobalConstants.REJECTED), and the code is appropriate as written.
267-267: Remove unnecessarythrows InterruptedExceptiondeclarations from test methods.Both test methods declare
throws InterruptedException, but none of their called methods propagate this exception. The utility methods that useThread.sleep()internally catch the exception. SinceWebDriverWaitis already being used for explicit waits and doesn't throwInterruptedException, these exception declarations are unnecessary and should be removed.Likely an incorrect or invalid review comment.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
176-182: Good: centralized Terms & Conditions handling.
The helper reduces repeated popup-handling sequences in the test flow.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (1)
114-120: Good: centralized Terms & Conditions handling.
Matches the pattern used in other test classes and reduces duplication inside the main flow.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
387-395: The original review comment is incorrect. WebDriverWait ignoresNotFoundException(parent class ofNoSuchElementException) by default. Thewait.ignoring(StaleElementReferenceException.class)call adds to this default ignore list rather than replacing it. Consequently,NoSuchElementExceptionis already suppressed during the polling loop, and the wait will retry on element-not-found conditions. The code does not flake onNoSuchElementExceptionand the proposed fix is unnecessary.Likely an incorrect or invalid review comment.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
272-286: LGTM - Good use of explicit waits and JS scrolling.The updated methods
enterMakeNameWithPositionandenterModelNameWithPositionuseWebDriverWaitwith dynamic locators andJavascriptExecutorto scroll elements into view. This approach handles dynamic UIs and off-screen elements robustly.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
102-108: LGTM - Good refactor to centralize Terms & Conditions handling.The
handleTermsAndCondition()helper method encapsulates the conditional logic for handling Terms & Conditions popups, improving code reusability and maintainability across test cases.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (3)
817-836: LGTM - Robust stale element handling.The explicit wait with retry logic for
StaleElementReferenceExceptionis a solid pattern for handling dynamic UI updates. TheNoSuchElementExceptionwith a clear message helps with debugging.
1248-1261: LGTM - Good fallback strategy for click interactions.The method uses explicit wait, scroll into view, and falls back to JavaScript click on failure. This layered approach handles various UI states effectively.
1355-1363: Good defensive pattern with explicit timeout handling.Using
WebDriverWaitwith a try-catch forTimeoutExceptionand returningfalseon timeout is a clean way to check element visibility without throwing exceptions. This improves test stability.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (4)
24-26: LGTM - Good refactor from WebElement to By locators.Replacing
@FindByannotatedWebElementfields withBylocators helps avoidStaleElementReferenceExceptionissues by ensuring fresh element lookups on each interaction.
507-525: LGTM - Robust selection with stale element retry.The method uses explicit waits, JavaScript scrolling, and handles
StaleElementReferenceExceptionby retrying the operation. This is a solid pattern for flaky UI interactions.
527-548: LGTM - Good addition of conditional selection method.The new
selectPolicyGroupDropdownIfPresentmethod returnstrueon success andfalseon timeout, allowing callers to handle optional elements gracefully without catching exceptions.
1191-1208: LGTM - Consistent wait-based selection with retry logic.The pattern of using explicit waits with retry logic for stale elements is consistent with other methods in this file and improves test stability.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (2)
156-202: LGTM - Well-structured test for certificate validation.The new test
requestNewPolicyWithoutUploadingCertificatesvalidates that the UI correctly handles policy requests when certificates haven't been uploaded. The intentional use of invalid inputs (spaces for lastName and phone on lines 170, 175) to trigger validation is a good negative-test pattern.
209-215: LGTM - Consistent Terms & Conditions handling.The
handleTermsAndCondition()helper follows the same pattern asDeactivatePartnerCreation.java, promoting consistency and reusability across test cases.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (2)
207-209: Nice move:By+isDisplayed(By)reduces stale element caching.
AssumingBasePage.isDisplayed(By)has a small, bounded wait and handles stale/absence cleanly, this should help stability.
467-469:waitForDashboardReady()is a good “page ready” primitive.
This should make tests less timing-sensitive if callers use it consistently after navigation.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (3)
34-50: Good stabilization: SBI add-device entrypoint + explicit “Add Device page did not load” assert.
The stronger assertion right after clicking the SBI add button should reduce downstream cascade failures.
243-260: +1: granular status checks after actions (Approved/Rejected/Deactivated).
These assertions make the tests verify outcome rather than just click-path completion.Also applies to: 297-300
728-734:verifyActionMenuOfDevices(... ) throws InterruptedException—confirm it’s actually needed.
This method itself doesn’t block/sleep; if downstream APIs no longer throwInterruptedException, drop it to reduce noise across the test class signatures.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java
Outdated
Show resolved
Hide resolved
...st-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
Outdated
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java:
- Around line 89-95: The handleTermsAndCondition() call is flaky because it
checks immediately for a popup; update the implementation to wait explicitly
(e.g., WebDriverWait with a reasonable timeout) for the terms/conditions popup
element to be present/visible and then interact, and ensure it gracefully
returns/continues if the popup never appears; also remove the unnecessary
"throws TimeoutException" declaration from registerNewUser() (and other test
methods that declare it) since Selenium's TimeoutException is unchecked.
🧹 Nitpick comments (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
283-315: HardenclickOnRequestPolicyButton()against stale/intercepted clicks (and reduce TOCTOU).The fallback locator strategy is good, but the current pattern still has a race between
isClickable(...)andfindElement(...).click(), and can flake onStaleElementReferenceException/ click interception in dynamic UIs. Consider leaning onExpectedConditions.elementToBeClickable(By)within the wait and retry the final click once if stale.Proposed adjustment (keeps the same external API)
public void clickOnRequestPolicyButton() { + waitForLoaderToDisappear(); WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); By showRequestPolicyBtn = By.id("show_request_policy"); By requestPolicyBtn = By.id("request_policy_btn"); By policiesRequestBtn = By.id("policies_request_btn"); try { - WebElement button = wait.until(driver -> { - - if (isClickable(showRequestPolicyBtn)) { - return driver.findElement(showRequestPolicyBtn); - } - - if (isClickable(requestPolicyBtn)) { - return driver.findElement(requestPolicyBtn); - } - - if (isClickable(policiesRequestBtn)) { - return driver.findElement(policiesRequestBtn); - } - - return null; - }); - - button.click(); + WebElement button = wait.until(d -> { + try { + WebElement el = ExpectedConditions.elementToBeClickable(showRequestPolicyBtn).apply(d); + if (el != null) return el; + } catch (StaleElementReferenceException ignored) {} + try { + WebElement el = ExpectedConditions.elementToBeClickable(requestPolicyBtn).apply(d); + if (el != null) return el; + } catch (StaleElementReferenceException ignored) {} + try { + WebElement el = ExpectedConditions.elementToBeClickable(policiesRequestBtn).apply(d); + if (el != null) return el; + } catch (StaleElementReferenceException ignored) {} + return null; + }); + + try { + button.click(); + } catch (StaleElementReferenceException e) { + // one retry; DOM may have re-rendered between wait completion and click + wait.until(ExpectedConditions.elementToBeClickable(showRequestPolicyBtn)).click(); + } } catch (TimeoutException e) { throw new RuntimeException("Request Policy button not clickable using any known locator: " + "[show_request_policy, request_policy_btn, policies_request_btn]", e); } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (2)
596-606: Avoid shadowing thepolicyNameBoxfield; reuse BasePage locator helpers.The local
policyNameBoxshadows the field and makes the method harder to maintain. Since you addedBasePage.enter(By, String), this can be simplified.Proposed simplification
public void enterPolicyName(String val) { - - WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(40)); - - WebElement policyNameBox = wait.until( - ExpectedConditions.elementToBeClickable(By.id("policy_name_box")) - ); - - policyNameBox.clear(); - policyNameBox.sendKeys(val); + enter(By.id("policy_name_box"), val); }
814-833: PreferelementToBeClickable(and consider scroll) for the dynamic policy-group option button.
presenceOfElementLocatedcan return an element that still can’t be clicked yet. Also consider scrolling the option into view before clicking to avoid intermittentElementClickInterceptedException.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
445-477: TightenisClickable(By)and consider integrating loader-wait intoclickOnElement(By)/enter(By, ...).
isClickable(By)currently callsdriver.findElement(locator)twice; it’s avoidable and can amplify stale/DOM-race issues.- If callers start using
waitForLoaderToDisappear(), consider invoking it insideclickOnElement(By)/enter(By, ...)for consistent behavior (or document when to call it).loadingOverlayis very broad (//*[contains(text(),'Loading')]); if it matches non-overlay content, waits can become noisy.Proposed tightening for `isClickable(By)` (minimal behavioral change)
protected boolean isClickable(By locator) { try { - return driver.findElement(locator).isDisplayed() && driver.findElement(locator).isEnabled(); - } catch (Exception e) { + List<WebElement> els = driver.findElements(locator); + if (els.isEmpty()) { + return false; + } + WebElement el = els.get(0); + return el.isDisplayed() && el.isEnabled(); + } catch (StaleElementReferenceException e) { return false; } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T06:24:55.376Z
Learnt from: SwethaKrish4
Repo: mosip/partner-management-portal PR: 1567
File: pmp-ui-v2/public/i18n/eng.json:1293-1294
Timestamp: 2025-11-19T06:24:55.376Z
Learning: In pmp-ui-v2/public/i18n/eng.json, the key `mispLicenseKeyExpiryDescription` intentionally has different punctuation in different namespaces: `notificationPopup` uses "for partner <span>" (no colon) while `viewAllNotifications` uses "for partner: <span>" (with colon). This difference is per story requirements and should not be flagged as an inconsistency.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java
🔇 Additional comments (6)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
266-274: New placeholder locators are a useful assertion surface.These additions look straightforward and help make the “clear/cancel” flow assertions less brittle.
Also applies to: 573-579
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java (1)
459-466: Renaming toclickOnRequestPolicyButton()aligns with the page-object consolidation.No concerns with these call-site updates.
Also applies to: 488-503
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
18-18: Confirm the removal ofdependsOnGroupsis intentional.This changes execution ordering; if the test suite relied on prior group side-effects, this can reintroduce instability.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (1)
1246-1258: Good direction on wait-driven clone selection + edit-page title stability.The explicit wait for the clone option and the timeout-safe
isEditPolicyPageTitleDisplayed()are aligned with the PR goal of reducing UI flakiness.Also applies to: 1352-1360, 442-444
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (2)
177-199: Improvement: waiting for the option element before clicking reduces dropdown flakiness.This is a solid stabilization change.
387-395: Wait-basedisDisplayed(...)/ quick checks look good; confirm Selenium “ignored exceptions” behavior on your version.The approach is sound, but exact
WebDriverWaitignore/default behavior can vary across Selenium versions/config. Worth confirming this behaves as expected in your CI grid.Also applies to: 427-443
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java
Show resolved
Hide resolved
...st-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java:
- Around line 479-484: The waitForPageToBeReady method currently hardcodes
By.id("partnerId"), which is not valid for all pages; update
waitForPageToBeReady(WebDriverWait) to accept a configurable ready indicator
(e.g., change signature to waitForPageToBeReady(WebDriverWait wait, By
readyIndicator)) and inside call waitForLoaderToDisappear() then
wait.until(ExpectedConditions.visibilityOfElementLocated(readyIndicator)), and
update callers or add page-specific overrides in subclasses (e.g., PoliciesPage,
DashboardPage) to pass the appropriate By locator or override
waitForPageToBeReady to use a page-specific locator.
- Around line 471-477: isClickable(By) currently checks
driver.findElement(locator).isDisplayed() and .isEnabled() immediately which
causes race conditions; change it to wait for the element to become clickable
using an explicit wait (e.g., WebDriverWait with
ExpectedConditions.elementToBeClickable(locator)) and return true if the wait
succeeds, false on TimeoutException or other caught exceptions; refer to the
isClickable(By) method and use driver and
ExpectedConditions.elementToBeClickable(locator) (or a shared wait helper) with
a sensible timeout instead of immediate driver.findElement calls.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java:
- Around line 283-315: The clickOnRequestPolicyButton method currently polls
three locators with immediate isClickable checks wasting CPU and timeout budget;
change it to use WebDriverWait.until with an explicit ExpectedCondition for each
locator in priority order (show_request_policy, request_policy_btn,
policies_request_btn) or replace the lambda with a single custom
ExpectedCondition that (using driver.findElements) checks presence + displayed +
enabled for any of those IDs and returns the first matching WebElement, and set
a reasonable polling interval on the WebDriverWait to avoid tight loops; then
click the returned element and keep the existing TimeoutException handling.
- Around line 729-732: PoliciesPage currently inherits a waitForPageToBeReady()
that waits for an element with id="partnerId" (not present) causing
TimeoutException; override waitForPageToBeReady() in the PoliciesPage class to
use a WebDriverWait that waits for an actual page element (for example
presenceOfElementLocated(By.id("title_back_icon")) or id="list_of_policies" or
id="show_request_policy"), and ensure waitUntilPageReady() calls this overridden
method so the page readiness check targets a real element on PoliciesPage.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java:
- Around line 139-142: The commented-out assertion after
basePage.refreshThePage() (dashboardPage.isPartnerCertificateTitleDisplayed vs
GlobalConstants.isPartnerCertificateTitleDisplayed) must be resolved: either
remove the commented assertTrue line if the check is no longer required, or
restore it by uncommenting and ensuring the predicate
dashboardPage.isPartnerCertificateTitleDisplayed() returns a stable boolean (add
waits/retries or waitForElement in the DashboardPage) and then call
assertTrue(dashboardPage.isPartnerCertificateTitleDisplayed(),
GlobalConstants.isPartnerCertificateTitleDisplayed); if the failure is due to
flakiness, re-enable the assertion but add a TODO with your tracking ticket
(e.g., // TODO: MOSIP-XXXXX - Re-enable after fixing certificate title timing
issue) and add an explicit wait in the page method to stabilize the check.
🧹 Nitpick comments (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java (2)
522-559: Retry logic with page refresh may hide timing issues.The
requestPolicymethod implements a retry-with-refresh pattern that waits up to 20 seconds, then refreshes and retries 3 times. While this improves test stability, consider:
- Total wait time could be up to 60+ seconds per policy request
- Page refresh may disrupt other operations or lose form state
- The pattern suggests timing/synchronization issues in the application that might need investigation
- Consider using exponential backoff instead of fixed retries
💡 Consider adding exponential backoff
for (int attempt = 1; attempt <= 3; attempt++) { + Duration timeout = Duration.ofSeconds(10 * attempt); // 10s, 20s, 30s + WebDriverWait wait = new WebDriverWait(driver, timeout); - WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); try {
534-540: Verify the XPath selector is resilient to UI changes.The policy row locator uses a complex XPath with class contains and nested text matching. This may be fragile if class names or structure changes.
Consider using a more specific data attribute if available:
By policyRowByName = By.cssSelector("[data-policy-name='" + authPolicyName + "']");uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
5-5: Remove unused import.The
TimeoutExceptionimport is not used anywhere in this file.-import org.openqa.selenium.TimeoutException; - import org.testng.annotations.Test;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
🧰 Additional context used
🧬 Code graph analysis (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(32-487)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-476)
🔇 Additional comments (6)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (3)
176-182: Good refactoring to extract Terms & Conditions handling.The
handleTermsAndCondition()helper method consolidates duplicate code and improves maintainability. The implementation matches the pattern used in other test files.
154-154: Method change is correct and properly implements different certificate handling.The change from
uploadPolicyUserIntermediateCaCert()touploadPolicyUserClientCert()properly reflects the different certificate types needed for different flows: the intermediate CA certificate is used in the admin trust chain setup, while the client certificate is correctly used for policy user registration at line 154.
18-18: Internal test dependencies are properly maintained viadependsOnMethods.The class-level
dependsOnGroupsparameter was removed, but the test execution order within this class remains intact. The three test methods have explicitdependsOnMethodsdependencies:
policiesAdminCreation()(priority 1) runs first and creates test data independentlyuploadTrustCertificate()(priority 2) depends onpoliciesAdminCreationregisterNewUser()(priority 3) depends onuploadTrustCertificateSince the first method creates its own test user without relying on the PartnerAdminCreation group, the removal does not break test prerequisites within this test class.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (3)
445-456: Good addition of loader handling.The
waitForLoaderToDisappear()method properly waits for loading indicators, which is essential for stable UI automation. The timeout warning is helpful for debugging.
427-443: Improved stale element handling in isDisplayedQuick.The updated implementation properly handles
StaleElementReferenceExceptionby catching it within the wait condition and returning false to retry. This is the correct pattern for dealing with dynamic DOM updates.
458-469: Good addition of By-based element interaction helpers.The new
clickOnElement(By)andenter(By, String)overloads provide a cleaner API for direct locator-based interactions with proper waits. These complement the existing WebElement-based methods well.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
Show resolved
Hide resolved
...st-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
182-199: Align dropdown option XPath between try/catch to avoid selecting different elementsThe try uses exact
text()match, while the fallback usescontains(text(), ...)—that can click a different option than intended when multiple similar entries exist.Suggested adjustment (keep the same XPath strategy in both paths)
- WebElement option = driver - .findElement(By.xpath("(//*[contains(text(),'" + value + "')])[" + position + "]")); + WebElement option = driver + .findElement(By.xpath("(//*[text()='" + value + "'])[" + position + "]"));
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java:
- Around line 283-315: The clickOnRequestPolicyButton method nests calls to
isClickable (which itself waits), causing compounded waits and flakiness;
rewrite clickOnRequestPolicyButton to use one WebDriverWait that polls for
element presence/state directly (check element != null, displayed and enabled)
for the three locators (show_request_policy, request_policy_btn,
policies_request_btn) and once found call the BasePage clickOnElement(...)
helper instead of button.click(); remove any internal calls to isClickable
inside the wait so there is only a single timeout and ensure clickOnElement is
used for the JS fallback.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java:
- Around line 54-57: Re-enable the commented assertion for
partnerCertificatePage.isDashboardFtmChipProviderCardDisplayed() instead of
commenting it out; replace the flaky timing by waiting explicitly for the
dashboard chip provider card to appear before asserting (either call a wait
util/ExpectedCondition prior to asserting or update
isDashboardFtmChipProviderCardDisplayed() to use an explicit wait with a
sensible timeout), then assert using
GlobalConstants.isDashboardFtmChipProviderCardDisplayed as the message; keep the
basePage.refreshThePage() but remove the TODO workaround once the explicit wait
is in place.
- Around line 142-143: In FtmPartnerCreation, re-enable the commented assertion
that checks partnerCertificatePage.isPartnerCertificatePageDisplayed (previously
comparing to GlobalConstants.isPartnerCertificatePageDisplayed); instead of
leaving it commented, add an explicit wait for the Partner Certificate page to
be visible (use existing page waiting utilities or WebDriverWait) and then
assert the boolean result from isPartnerCertificatePageDisplayed with the
GlobalConstants message, ensuring the test verifies the page is displayed after
clicking the header.
🧹 Nitpick comments (9)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (3)
5-6: TimeoutException type looks inconsistent; prefer Selenium TimeoutException (or nothrows)Right now the test declares a checked
java.util.concurrent.TimeoutException, while the rest of the UI code typically throws Selenium’s uncheckedorg.openqa.selenium.TimeoutException. If nothing in this method actually throws the checked type, dropping thethrowscleans this up and avoids confusion.Proposed diff (remove checked TimeoutException from signature)
-import java.util.concurrent.TimeoutException; - @@ - public void registerDeactivatePartnerUser() throws TimeoutException { + public void registerDeactivatePartnerUser() {Also applies to: 27-27
73-80: Refresh workaround likely hides a real synchronization issue
basePage.refreshThePage()+ commented assertion will keep flaking if the real problem is “Partner Certificate title not ready/clickable”. Prefer an explicit wait for the title (or a stable ready-indicator) and then re-enable the assertion.
103-109: Deduplicate Terms & Conditions handling across tests
handleTermsAndCondition()is repeated in multiple test classes; consider moving it intoBaseClass(orDashboardPage) to avoid drift and simplify future changes.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
729-731:waitUntilPageReady()may need to wait for any “Request Policy” button variantIf some flows render
request_policy_btn/policies_request_btninstead ofshow_request_policy, this readiness check could be a false-negative. Consider waiting for whichever of the known locators appears.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (2)
137-145: Refresh + commented assertion should be replaced with an explicit waitSame issue as other tests:
refreshThePage()is a brittle sync mechanism. Prefer waiting until Partner Certificate navigation is visible/clickable, then re-enable the assertion.
50-51: CentralizehandleTermsAndCondition()to avoid drift across test classesAlso applies to: 177-183
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (2)
25-48: Use theRegisterPagereturned byclickRegisterButton()(don’t pre-construct + discard return)This is a common source of UI-test flakiness when the click method encapsulates navigation/waits or when the page object expects construction at the correct time.
Proposed diff
public void registerDevicePartnerUser(){ dashboardPage = new DashboardPage(driver); partnerCertificatePage = new PartnerCertificatePage(driver); - registerPage = new RegisterPage(driver); basePage = new BasePage(driver); @@ dashboardPage.clickOnProfileDropdown(); loginpage = dashboardPage.clickOnLogoutButton(); - loginpage.clickRegisterButton(); + registerPage = loginpage.clickRegisterButton();
49-56: Replace refresh workaround with an explicit wait and re-enable the assertionuitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
445-486: Loader locator is very broad; consider making it more specific + avoid duplicating click helpers
//*[contains(text(),'Loading')]can match unrelated text and cause false waits.waitForLoaderToDisappear()logs and continues on timeout; consider at least capturing a screenshot (or escalating) to avoid masking real hangs.clickOnElement(By)duplicatesclick(By)and doesn’t reuse the JS-fallback path fromclickOnElement(WebElement).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-25T12:41:24.381Z
Learnt from: Sudeep7353
Repo: mosip/partner-management-portal PR: 1574
File: pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js:220-222
Timestamp: 2025-11-25T12:41:24.381Z
Learning: In the file pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js, the `relyingPartyId` field is mandatory and will always have a value from the API, so it does not require a fallback placeholder like `|| '-'`.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
🧬 Code graph analysis (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (5)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (1)
Test(15-120)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
Test(18-185)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(32-489)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-476)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/BaseClass.java (1)
BaseClass(34-202)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-476)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/RegisterPage.java (1)
RegisterPage(10-211)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-476)
🔇 Additional comments (6)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java (3)
20-20: LGTM: Improved naming convention.The field rename from
dashboardpagetodashboardPagefollows proper Java camelCase naming conventions and improves code readability.
50-50: Good refactor: centralized Terms & Conditions handling.Extracting the Terms & Conditions flow into
handleTermsAndCondition()eliminates duplication across test classes and improves maintainability. The underlyingisDisplayed()implementation already includes proper explicit waits with a 20-second timeout and exception handling, ensuring robust element detection.
16-16: No issue: dependency removal is justified.The test class does not reference or depend on
PartnerAdminCreation. Both test methods are self-contained—they start from a logged-out state, register new FTM users, and perform all certificate operations within the test flow itself. Removing thedependsOnGroupsannotation is appropriate.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (2)
155-160: Client certificate upload switch looks consistent with the updated cert flow
18-19: This is a new test file—there was no priordependsOnGroupsto remove.The file
PolicyAdminAndPartnerCreation.javawas created in this commit, not modified. The class-level@Test(groups = { "PolicyAdminAndPartnerCreation" })annotation has never containeddependsOnGroups. Additionally, no other test files in the codebase depend on the "PolicyAdminAndPartnerCreation" group, so there is no suite ordering risk.Likely an incorrect or invalid review comment.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
387-395: Stale-tolerant visibility checks are a good stability improvementAlso applies to: 427-443
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java
Outdated
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
43-47: Verify the DateTimeFormatter pattern - minutes before hours is unconventional.The pattern
"mmHHddMMyyyy"places minutes (mm) before hours (HH), which is unusual for timestamps. This will produce formats like "3014091012026" (30 minutes, 14 hours) instead of the more conventional "1430091012026".Consider using
"HHmmddMMyyyy"(hours then minutes) if this is a timestamp, or clarify the intended format if this ordering is intentional.📅 Proposed fix
public static String getDateTime() { - DateTimeFormatter dtf = DateTimeFormatter.ofPattern("mmHHddMMyyyy"); + DateTimeFormatter dtf = DateTimeFormatter.ofPattern("HHmmddMMyyyy"); LocalDateTime now = LocalDateTime.now(); return dtf.format(now); }
🤖 Fix all issues with AI agents
In @uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java:
- Around line 387-395: The WebDriverWait.ignoring(...) call in isDisplayed(By
locator) returns a new WebDriverWait but its result is discarded, so
StaleElementReferenceException is not actually being ignored; update isDisplayed
to capture the returned instance (e.g., assign wait =
wait.ignoring(StaleElementReferenceException.class) or create the WebDriverWait
with .ignoring(...) chained) and then call wait.until(d ->
d.findElement(locator).isDisplayed()) so the ignore behavior takes effect.
- Around line 445-456: The loading locator in BasePage (loadingOverlay) uses an
English text XPath which breaks i18n and is too broad; replace the XPath with a
language-independent ID locator (use By.id("loading_text")) and update any
references in waitForLoaderToDisappear() to use this new loadingOverlay so the
invisibility check targets the stable element id rather than translated text.
🧹 Nitpick comments (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (3)
458-467: Consider improving error handling in the By-locator overload.The fallback JS click finds the element without any wait (line 464), which could throw
NoSuchElementExceptionif the element isn't present. Also, unlike theWebElementoverload, this version doesn't log failures or take screenshots.♻️ Suggested improvements
protected void clickOnElement(By locator) { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); try { WebElement element = wait.until(ExpectedConditions.elementToBeClickable(locator)); element.click(); - } catch (Exception e) { + } catch (ElementClickInterceptedException | TimeoutException e) { + LogUtil.step("Standard click failed, trying JS click for locator: " + locator); - WebElement element = driver.findElement(locator); + WebElement element = wait.until(ExpectedConditions.presenceOfElementLocated(locator)); ((JavascriptExecutor) driver).executeScript("arguments[0].click();", element); } }
476-483: Consider adding a wait to align with the method name.The method name
isClickablesuggests it waits for the clickable state (like otheris*methods in this class that use waits), but it immediately checks the current state without waiting. This inconsistency might lead to false negatives if the element becomes clickable shortly after the check.♻️ Suggested enhancement
protected boolean isClickable(By locator) { try { - WebElement el = driver.findElement(locator); - return el.isDisplayed() && el.isEnabled(); + WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(5)); + wait.until(ExpectedConditions.elementToBeClickable(locator)); + return true; } catch (Exception e) { return false; } }Alternatively, rename to
isCurrentlyClickableif the immediate check is intentional.
485-489: Good pattern for page-ready checks, minor timeout inconsistency.The method correctly combines loader handling and ready indicator checks. However,
waitForLoaderToDisappear()uses a fixed 30-second timeout, which might be shorter than thetimeoutSecondsparameter passed to this method.If the loader timeout should be configurable, consider passing it as a parameter:
protected void waitForPageToBeReady(By readyIndicator, int loaderTimeoutSeconds, int readyTimeoutSeconds) { waitForLoaderToDisappear(loaderTimeoutSeconds); WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(readyTimeoutSeconds)); wait.until(ExpectedConditions.visibilityOfElementLocated(readyIndicator)); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java
🚧 Files skipped from review as they are similar to previous changes (2)
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
🔇 Additional comments (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (3)
182-184: Good addition of explicit waits.Adding
waitForElementVisibleand using the existingclickOnElementmethod improves reliability by ensuring the dropdown element is ready before interaction.
427-443: Good implementation of stale element handling.The refactored
isDisplayedQuickcorrectly usesfindElements(plural) to avoidNoSuchElementExceptionand handlesStaleElementReferenceExceptiongracefully within the lambda. This improves test stability.
469-474: Clean implementation of the By-locator enter method.The method correctly waits for element visibility before clearing and entering text. The simplified approach without JS fallback is appropriate for this helper method.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java
Outdated
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
252-262: RemoveThread.sleep, validateposition, and don’t swallow selection failures.Current code can flake (fixed sleep), can throw
IndexOutOfBoundsException(no bounds check), and can hide failures (catchingIOExceptionand only logging). For UI automation, it’s usually better to fail fast when a selection can’t be performed.Proposed diff
-public void selectAddDeviceTypeWithPosition(String value, int position) throws InterruptedException { - position = position - 1; - Thread.sleep(1000); - WebElement addDeviceTypeSelectDropdown = driver - .findElements(By.xpath("//button[@id='add_device_device_type_dropdown_btn']")).get(position); +public void selectAddDeviceTypeWithPosition(String value, int position) { + if (position < 1) { + throw new IllegalArgumentException("position must be 1-based (>= 1)"); + } + int index = position - 1; + By dropdownBtn = By.id("add_device_device_type_dropdown_btn"); + WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); + wait.until(d -> d.findElements(dropdownBtn).size() > index); + WebElement addDeviceTypeSelectDropdown = driver.findElements(dropdownBtn).get(index); try { - dropdownWithPosition(addDeviceTypeSelectDropdown, value, position + 1); + dropdownWithPosition(addDeviceTypeSelectDropdown, value, position); } catch (IOException e) { - logger.info(e.getMessage()); + logger.error("Failed to select Add Device Type at position " + position + ": " + value, e); + throw new RuntimeException(e); } }
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java:
- Line 8: The locator for deviceTypeDropdown is mismatched: update the By
definition for deviceTypeDropdown to use the same id as the rest of the class
(change to "add_device_device_type_dropdown_btn") so waitForDeviceTypeDropdown()
waits on the correct element; alternatively, if both IDs are valid in DOM, add a
clarifying comment near deviceTypeDropdown and confirm whether
waitForDeviceTypeDropdown() should target the button or the container and adjust
the locator to match the intended element (see deviceTypeDropdown,
addDeviceTypeSelectDropdown and waitForDeviceTypeDropdown).
- Around line 420-437: selectFromCustomDropdown builds an unsafe XPath using the
raw value and should be hardened: escape or encode the input value (handle
single quotes) or construct the XPath using XPath concat() so strings with
apostrophes won't break (keep the By variable name option and still use
WebDriverWait to locate it), and prefer waiting for clickability
(elementToBeClickable) before calling click. Also simplify
waitForDeviceTypeDropdown by replacing the presence/visibility/isEnabled
sequence with a single
wait.until(ExpectedConditions.elementToBeClickable(deviceTypeDropdown)) and
return that resulting WebElement so you avoid returning a stale element.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java:
- Around line 4-5: The DashboardPage class imports the wrong exception types;
replace the java.util.NoSuchElementException and
java.util.concurrent.TimeoutException imports with the Selenium exceptions
(org.openqa.selenium.NoSuchElementException and
org.openqa.selenium.TimeoutException) so caught/throws in DashboardPage resolve
to Selenium types; remove the incorrect imports and update any import statements
at the top of DashboardPage.java (class DashboardPage) so all references to
NoSuchElementException and TimeoutException use the org.openqa.selenium
variants.
- Around line 223-233: The isPartnerCertificateTitleDisplayed() method currently
waits for By.id("dashboard_partner_certificate_list_card") but should target the
header element; change the locator used in that method to
By.id("dashboard_partner_certificate_list_header") (and optionally rename the
local variable from card to header) so the method verifies the h5 title
isDisplayed() consistent with the field definitions and the method name.
- Around line 251-264: In DashboardPage.clickOnPoliciesTitle(), the overlay
selector By.cssSelector("div.fixed.inset-0.z-50") is too generic and matches
many popups; refine the selector used for the blockingOverlay variable to target
the actual blocking overlay shown during Policies navigation (e.g., include the
background opacity class or a unique attribute such as bg-opacity-50 or a data-*
attribute, or use a structural selector that matches the overlay's parent/child
relationship). Update the blockingOverlay declaration and any waits that
reference it so invisibilityOfElementLocated waits only for that specific
overlay before proceeding to locate and click policiesTitle, ensuring no other
popups interfere.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java:
- Line 5: The test class AuthPartnerCreation imports the wrong TimeoutException
(java.util.concurrent.TimeoutException); replace that import with
org.openqa.selenium.TimeoutException and ensure any catch blocks or throws
declarations in AuthPartnerCreation reference
org.openqa.selenium.TimeoutException so the exception type matches Selenium
usage (update import line only and adjust any method signatures or catches that
currently reference the java.util.concurrent type to the Selenium type).
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.java:
- Line 6: The import of TimeoutException is incorrect: replace the
java.util.concurrent.TimeoutException import with
org.openqa.selenium.TimeoutException so any catches/throws in
AuthPartnerWithoutCertificateTest use the Selenium exception type; update the
import statement referencing the TimeoutException symbol accordingly.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java:
- Line 5: The import in DeactivatePartnerCreation.java incorrectly uses
java.util.concurrent.TimeoutException; replace that import with
org.openqa.selenium.TimeoutException so the Selenium-based test code references
the correct TimeoutException class (update the import statement at the top of
DeactivatePartnerCreation.java and remove the java.util.concurrent import to
avoid ambiguity or compile errors).
In @uitest-pmp-v2/src/main/resources/config/kernel.properties:
- Line 76: Replace the hardcoded environment-specific JDBC string in the
kernel.properties entry for audit_url with an empty value or a placeholder and
ensure the real JDBC URL is injected at runtime (via environment variable,
config server or CI secrets); specifically, remove the IP/db value assigned to
the audit_url property and leave it blank or as a clearly annotated placeholder
so no env-specific credentials are committed, and update any deployment/docs to
load the actual URL from environment/secret management.
🧹 Nitpick comments (16)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (3)
13-23: Consider a more robust approach for element description.The
describeElementmethod relies on parsing thetoString()output ofWebElement, which is implementation-dependent and may vary across Selenium versions. While the exception handling provides a fallback, consider these alternatives:
- Extract attributes like tag name, id, class, or name directly using WebElement methods
- Use a combination of
getTagName()andgetAttribute()for a more stable descriptor♻️ Example alternative implementation
-private static String describeElement(WebElement element) { - try { - String desc = element.toString(); - if (desc.contains("->")) { - return desc.substring(desc.indexOf("->") + 2, desc.length() - 1).trim(); - } - return desc; - } catch (Exception e) { - return "Unknown Element"; - } -} +private static String describeElement(WebElement element) { + try { + StringBuilder desc = new StringBuilder(element.getTagName()); + String id = element.getAttribute("id"); + String className = element.getAttribute("class"); + if (id != null && !id.isEmpty()) { + desc.append("#").append(id); + } else if (className != null && !className.isEmpty()) { + desc.append(".").append(className.split(" ")[0]); + } + return desc.toString(); + } catch (Exception e) { + return "Unknown Element"; + } +}
42-46: Standardize Reporter logging using the helper method.The
logToReporterhelper method exists (lines 25-27) but is not consistently used across all logging methods. Methodsaction(String, WebElement),verify(String, WebElement), andverify(String, By)log directly to Reporter, whileaction(String)uses the helper.Standardizing on the helper improves maintainability if the Reporter formatting needs to change in the future.
♻️ Refactor to use helper consistently
public static void action(String message, WebElement element) { String fullMessage = "🔸 ACTION: " + message + ": " + describeElement(element); logger.info(fullMessage); - Reporter.log("<br><span style='color:blue;'>" + fullMessage + "</span>", false); + logToReporter(fullMessage, "blue"); } public static void verify(String message, WebElement element) { String fullMessage = "🔍 VERIFY: " + message + ": " + describeElement(element); logger.info(fullMessage); - Reporter.log("<br><span style='color:darkorange;'>" + fullMessage + "</span>", false); + logToReporter(fullMessage, "darkorange"); } public static void verify(String message, By locator) { String fullMessage = "🔍 VERIFY: " + message + ": " + describeLocator(locator); logger.info(fullMessage); - Reporter.log("<br><span style='color:darkorange;'>" + fullMessage + "</span>", false); + logToReporter(fullMessage, "darkorange"); }Also applies to: 55-59, 62-66
8-11: Apply utility class pattern.Since
LogUtilcontains only static methods and serves as a utility class, consider following the standard utility class pattern:
- Make the class
finalto prevent subclassing- Add a
privateconstructor to prevent instantiationThis is a Java best practice for utility classes.
♻️ Apply utility class pattern
-public class LogUtil { +public final class LogUtil { private static final Logger logger = Logger.getLogger(LogUtil.class); + + private LogUtil() { + throw new UnsupportedOperationException("Utility class"); + } // Helper method to extract meaningful information from WebElement private static String describeElement(WebElement element) {uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (3)
215-228: Avoid shadowingaddDeviceTypeSelectDropdownfield; preferBy.id+ clickable wait.The local
WebElement addDeviceTypeSelectDropdownshadows the@FindByfield, which is easy to misread. AlsoBy.id(...)is simpler than an XPath for an id.Proposed diff
public void selectAddDeviceType(String value) throws Exception { try { - By addDeviceTypeDropdown = By.xpath("//button[@id='add_device_device_type_dropdown_btn']"); - - WebElement addDeviceTypeSelectDropdown = new WebDriverWait(driver, Duration.ofSeconds(20)) - .until(ExpectedConditions.visibilityOfElementLocated(addDeviceTypeDropdown)); - - dropdown(addDeviceTypeSelectDropdown, value); + By addDeviceTypeDropdownBtn = By.id("add_device_device_type_dropdown_btn"); + WebElement deviceTypeDropdownBtn = new WebDriverWait(driver, Duration.ofSeconds(20)) + .until(ExpectedConditions.elementToBeClickable(addDeviceTypeDropdownBtn)); + dropdown(deviceTypeDropdownBtn, value); } catch (Exception e) { logger.error("Failed to select Add Device Type: " + value, e); throw e; } }
230-242: Selection viaelementToBeClickableis aligned with the PR goal; consider reusing a shared wait duration/util.
274-280: Good: explicit wait +scrollIntoViewbefore typing.Optional: consider
elementToBeClickable(not just visible) if you still see occasional “element not interactable” failures.Also applies to: 282-288
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.java (1)
154-154: Remove unnecessary throws declaration.The method declares
throws TimeoutExceptionbut doesn't explicitly throw it. SinceTimeoutExceptionis an unchecked runtime exception in Selenium, declaring it in the signature is unnecessary and can be removed.♻️ Remove the throws clause
-public void userDashboardOfAuthenticationPartner() throws TimeoutException { +public void userDashboardOfAuthenticationPartner() {uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java (1)
54-55: Track the timing workarounds.The page refresh calls and TODO comments indicate underlying timing or synchronization issues with certificate title display. These workarounds should be tracked for resolution once the root cause is identified.
Would you like me to create an issue to track the investigation and removal of these page refresh workarounds?
Also applies to: 140-141
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (1)
3-3: Consider using explicit imports.The wildcard import
org.openqa.selenium.*is generally discouraged in favor of explicit imports for better code clarity and to avoid naming conflicts.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
26-27: Unnecessarythrows TimeoutExceptiondeclaration.The method declares
throws TimeoutExceptionbut no code withinregisterDeactivatePartnerUser()actually throws this exception. The page object methods don't propagate this checked exception.Remove unnecessary throws clause
@Test(priority = 1, description = "This is a test case register new user") -public void registerDeactivatePartnerUser() throws TimeoutException { +public void registerDeactivatePartnerUser() {uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java (1)
7-12: Unused imports.The imports for
Duration,By,TimeoutException,ExpectedConditions, andWebDriverWaitare added but none are used in this test class. These appear to be copy-paste artifacts from other files.Remove unused imports
import static org.testng.Assert.assertTrue; -import java.time.Duration; - -import org.openqa.selenium.By; -import org.openqa.selenium.TimeoutException; -import org.openqa.selenium.support.ui.ExpectedConditions; -import org.openqa.selenium.support.ui.WebDriverWait; import org.testng.annotations.Test;uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (2)
5-5: Unused import.
org.openqa.selenium.TimeoutExceptionis imported but not used anywhere in this class. TheregisterNewUser()method doesn't declarethrows TimeoutException.Remove unused import
-import org.openqa.selenium.TimeoutException; - import org.testng.annotations.Test;
30-30: Consider consolidating BasePage initialization.
basePageis initialized in three separate test methods (policiesAdminCreation,uploadTrustCertificateimplied by line 59, andregisterNewUser). SincebasePageis a class field and tests may run independently, this is functionally correct but could be consolidated using a@BeforeMethodsetup.Also applies to: 59-59, 94-94
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (2)
25-26: Unused field:policiesPage.The
policiesPagefield is declared and initialized inregisterAuthUserWithoutUploadingCertificates()(line 160) but is never used in any test method.Remove unused field and initialization
private RegisterPage registerPage; -private PoliciesPage policiesPage; private BasePage basePage;And in
registerAuthUserWithoutUploadingCertificates():dashboardPage = new DashboardPage(driver); -policiesPage = new PoliciesPage(driver); registerPage = new RegisterPage(driver);
156-157: Unnecessarythrows InterruptedException.The method
registerAuthUserWithoutUploadingCertificates()declaresthrows InterruptedExceptionbut contains no interruptible operations likeThread.sleep().Remove unnecessary throws clause
@Test(priority = 2, description = "Register auth user with out uploading certificates") -public void registerAuthUserWithoutUploadingCertificates() throws InterruptedException { +public void registerAuthUserWithoutUploadingCertificates() {uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
709-711: Hardcoded timeout and locator duplication.The
waitUntilPageReady()method uses a hardcoded 20-second timeout and duplicates the"show_request_policy"ID that's already defined inmiddleRequestPolicyButton. Consider reusing the existing locator and extracting the timeout to a constant.Proposed improvement
public void waitUntilPageReady() { - waitForPageToBeReady(By.id("show_request_policy"), 20); + waitForPageToBeReady(middleRequestPolicyButton, 20); }For the timeout, consider using a constant defined in a configuration class.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
uitest-pmp-v2/pom.xmluitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyCreationForAuthPartner.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.javauitest-pmp-v2/src/main/resources/config/kernel.properties
✅ Files skipped from review due to trivial changes (1)
- uitest-pmp-v2/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyCreationForAuthPartner.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-10T12:54:52.306Z
Learnt from: damodarguru
Repo: mosip/partner-management-portal PR: 1584
File: uitest-pmp-v2/src/main/resources/config/Kernel.properties:144-146
Timestamp: 2025-12-10T12:54:52.306Z
Learning: In uitest-pmp-v2/src/main/resources/config/Kernel.properties, the properties pmpUiv2Url, apiEnvUser, and apiInternalEndPoint should remain empty in the checked-in configuration file as they are environment-specific credentials and endpoints that are provided at runtime.
Applied to files:
uitest-pmp-v2/src/main/resources/config/kernel.properties
📚 Learning: 2025-12-10T12:57:43.001Z
Learnt from: damodarguru
Repo: mosip/partner-management-portal PR: 1584
File: uitest-pmp-v2/src/main/resources/config/Kernel.properties:57-58
Timestamp: 2025-12-10T12:57:43.001Z
Learning: In uitest-pmp-v2/src/main/resources/config/Kernel.properties, the properties db-port and db-server should remain empty in the checked-in configuration file as they are environment-specific credentials that are provided at runtime.
Applied to files:
uitest-pmp-v2/src/main/resources/config/kernel.properties
📚 Learning: 2025-12-10T12:58:09.232Z
Learnt from: damodarguru
Repo: mosip/partner-management-portal PR: 1584
File: uitest-pmp-v2/src/main/resources/config/Kernel.properties:28-28
Timestamp: 2025-12-10T12:58:09.232Z
Learning: In uitest-pmp-v2/src/main/resources/config/Kernel.properties, the property keycloak-external-url should remain empty in the checked-in configuration file as it is an environment-specific credential that is provided at runtime.
Applied to files:
uitest-pmp-v2/src/main/resources/config/kernel.properties
📚 Learning: 2025-11-25T12:41:24.381Z
Learnt from: Sudeep7353
Repo: mosip/partner-management-portal PR: 1574
File: pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js:220-222
Timestamp: 2025-11-25T12:41:24.381Z
Learning: In the file pmp-ui-v2/src/pages/admin/authenticationServices/ViewAdminOidcClientDetails.js, the `relyingPartyId` field is mandatory and will always have a value from the API, so it does not require a fallback placeholder like `|| '-'`.
Applied to files:
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java
🧬 Code graph analysis (5)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (1)
Test(22-736)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(36-515)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-475)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.java (1)
GlobalConstants(3-1215)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(36-515)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/driver/DriverManager.java (1)
DriverManager(5-23)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(36-515)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
DashboardPage(19-475)
🔇 Additional comments (33)
uitest-pmp-v2/src/main/resources/config/kernel.properties (1)
137-137: Verify the intent of togglingreportKnownIssueTestCasestono.This change aligns with the PR objective of fixing test failures and improving report stability. However, confirm that disabling known issue reporting is the intended behavior for this release.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
136-138:isHomeButtonDisplayed()using aBylocator is a good stability move.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (4)
180-193: LGTM: Robust policy group selection.The explicit wait, dynamic locator, scrolling, and JavaScript fallback provide a resilient interaction pattern for handling dynamic dropdown options.
266-281: LGTM: Resilient click with stale element handling.The loader wait combined with retry logic for
StaleElementReferenceExceptionandNoSuchElementExceptionprovides robust handling for dynamic content in single-page applications.
329-341: LGTM: Proper loader wait before interaction.The explicit wait for the loading indicator combined with retry logic ensures the element is ready for interaction before attempting the click.
462-473: LGTM: Useful dashboard readiness helpers.The
waitForDashboardReady()andwaitForDashboardLoaderToDisappear()methods provide clear, reusable abstractions for synchronization, improving test stability.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.java (2)
34-90: LGTM: Test refactored to use login flow.The method rename and switch from registration to login flow better reflect the test's purpose of creating an OIDC client without certificates. The navigation updates are consistent with the updated page objects.
108-109: LGTM: Improved method naming consistency.The changes from
isGenerateAPIKeyDisplayedtoisGenerateApiKeyDisplayedandclickOnAPIKeyDisplayedtoclickOnCreateApiKeyimprove naming consistency and clarity.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (6)
186-188: LGTM: Enhanced dropdown selection reliability.Adding explicit waits for visibility before interacting with the dropdown option improves reliability for dynamic content.
267-283: LGTM: Useful By-based element interaction helpers.These overloaded methods support the migration to By locators and provide consistent, reusable patterns for element interaction checks.
335-339: LGTM: Safe page refresh with null check.Using
DriverManager.getDriver()with a null check prevents potentialNullPointerExceptionand aligns with thread-safe driver management.
411-421: LGTM: Robust visibility check with FluentWait.The FluentWait with exception ignoring and polling provides a resilient pattern for checking element visibility in dynamic single-page applications, handling transient stale element and timing issues gracefully.
453-469: LGTM: Quick visibility check with appropriate timeout.The short 500ms timeout and stale element handling make this method ideal for quick presence checks without blocking test execution. The visibility change to
protectedenables reuse in page object subclasses.
471-514: LGTM: Comprehensive loader and interaction utilities.These helper methods provide a solid foundation for synchronization across page objects:
waitForLoaderToDisappear()centralizes loader handlingclickOnElement(By)andenter(By, String)offer By-based alternatives with fallbackswaitForPageToBeReady()combines common wait patternsThe consistent error handling and logging enhance debuggability.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/FtmPartnerCreation.java (2)
16-20: LGTM: Standardized naming and simplified dependencies.The field rename from
dashboardpagetodashboardPagefollows Java naming conventions, and removingdependsOnGroupsat the class level simplifies test orchestration.
191-197: LGTM: Clean abstraction for terms and conditions flow.The
handleTermsAndCondition()helper eliminates duplication and provides a clear, reusable pattern for this common UI interaction.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (4)
595-605: LGTM: Robust policy name entry.The explicit wait for clickability before entering text is a good pattern. Note that the 40-second timeout is quite long; if this consistently needs such a long wait, consider investigating page load performance.
814-832: LGTM: Robust policy group selection with excellent error handling.The retry logic for stale elements, exception translation to
NoSuchElementExceptionwith context, and logging provide comprehensive error handling and debugging support.
1245-1257: LGTM: Consistent resilient selection pattern.The explicit wait, scroll into view, and JavaScript click fallback provide robust handling for clone policy group selection, consistent with similar methods across the codebase.
441-441: ID-based locator aligns with framework pattern and is appropriate.The
page_titleID is the established locator used consistently across all page objects in the test framework for page title elements (SbiLinkedDevicePage, PartnerAdminPage, MispPartnerPage, AddDevicePage, ViewDeviceDetailsPage). This change from a text-based XPath to an ID-based locator improves stability and follows the existing framework conventions. The ID correctly identifies the page title element in the edit datashare policy page context.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java (1)
103-109: LGTM - Terms and Conditions helper follows consistent pattern.The
handleTermsAndCondition()helper matches the implementation pattern used across other test classes in this PR (e.g.,DeviceCreationTest.java,DeactivatePartnerCreation.java), promoting consistency.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PartnerPolicyMappingTest.java (1)
459-459: LGTM - Method calls updated to match PoliciesPage API change.The calls to
clickOnRequestPolicyButton()correctly align with the renamed method inPoliciesPage.java, which now handles both middle and tabular button detection internally.Also applies to: 488-488, 522-523
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DevicePartnerCreation.java (3)
18-18: Good fix - Variable naming consistency.Renaming
dashboardpagetodashboardPageimproves code consistency by following Java camelCase conventions used throughout the codebase.
24-25: Good cleanup - Removed unnecessary throws declaration.Removing
throws InterruptedExceptionis appropriate since the method no longer usesThread.sleep()or other interruptible operations.
49-56: LGTM - Stabilization pattern applied correctly.The sequence of
handleTermsAndCondition()→basePage.refreshThePage()→ assertion follows the pattern established across this PR to address timing issues with dynamic UI elements. The TODO comment acknowledges the known certificate title timing issue.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/PolicyAdminAndPartnerCreation.java (1)
137-143: LGTM - Stabilization pattern for certificate title timing.The
handleTermsAndCondition()→refreshThePage()→ assertion sequence addresses the known timing issue with the certificate title display, as noted in the TODO comment.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java (2)
169-175: Test data uses whitespace-only values intentionally.The test enters
" "(whitespace) forlastNameandphonefields to validate form error handling. This is intentional negative testing, and line 182 confirms the phone validation warning is expected.
202-208: LGTM - Consistent handleTermsAndCondition helper.The helper method follows the same pattern used across all test classes in this PR.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (5)
20-21: Good refactor - By locators for dynamic button detection.Replacing
@FindByWebElement fields withBylocators enables explicit wait-based detection for buttons that may appear in different UI states (middle vs. tabular view). This is a robust approach for handling dynamic content.
280-291: LGTM - Consolidated button click logic with fail-fast behavior.The unified
clickOnRequestPolicyButton()method:
- Checks for middle button first, then tabular button
- Throws
RuntimeExceptionif neither is foundThis is cleaner than the previous separate methods and provides clear failure messaging.
605-611: LGTM - Visibility check methods for dual-locator pattern.The new
isMiddleRequestPolicyButtonDisplayed()andisTabularRequestPolicyButtonDisplayed()methods properly delegate toisDisplayed(By)for the respective locators, supporting the dual-button detection pattern.
3-6: Potentially unused imports.
Duration,TimeoutException, andWebDriverWaitare imported but don't appear to be used directly in this class. ThewaitUntilPageReady()method delegates towaitForPageToBeReady()which is likely inBasePage.Also applies to: 10-10
345-347: No changes needed. The methodisElementEnabled(By)overload exists in BasePage (lines 267-277), andmiddleRequestPolicyButtonis correctly declared as aBylocator at line 20 of PoliciesPage. The code is correct.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerCreation.java
Show resolved
Hide resolved
...mp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPartnerWithoutCertificateTest.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeactivatePartnerCreation.java
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java:
- Around line 234-245: selectAddDeviceTypeWithPosition may throw
IndexOutOfBoundsException because driver.findElements(...).get(position) assumes
enough elements; before calling .get(position) check the size of the list
returned by findElements (or use an explicit wait that waits until the list size
is > position) and handle the shortage by logging an error or throwing a clear
exception; ensure the method accounts for the earlier position decrement
(position = position - 1) when comparing sizes, and only call
dropdownWithPosition(addDeviceTypeSelectDropdown, value, position + 1) after
verifying the element exists.
In @uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java:
- Around line 537-540: The clickOnApiKeyViewButton method contains an
unnecessary call to isDisplayed(apiKeyView) whose return value is ignored;
remove that line and keep waitForLoaderToDisappear() followed by
clickOnElement(apiKeyView) since clickOnElement() already calls
waitForElementClickable() and ensures visibility/clickability. Ensure no other
logic depends on the removed isDisplayed(apiKeyView) call.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java:
- Around line 1111-1114: The method isClonePolicyPopupTitleNotDisplayed()
currently negates isElementDisplayed() and uses a fragile XPath; replace it to
explicitly wait for invisibility using
ExpectedConditions.invisibilityOfElementLocated with the stable id locator used
by the annotated field (id "clone_policy_popup_title") so it mirrors
isClonePolicyPopupTitleDisplayed(); update isClonePolicyPopupTitleNotDisplayed()
to use a WebDriverWait (or the existing wait helper) against
By.id("clone_policy_popup_title") and return the invisibility result instead of
negating visibility.
- Around line 1023-1026: The isPolicyStatusDeactivatedDisplayed() method
currently constructs an inline XPath and should instead use the existing
PageFactory WebElement field deactivateStatus (defined with @FindBy(xpath =
"//div[text()='Deactivated']")); update isPolicyStatusDeactivatedDisplayed() to
call isElementDisplayed(deactivateStatus) (mirroring
isPolicyStatusActivatedDisplayed() which uses activateStatus) so the locator is
consistent and scoped to the existing field.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java:
- Around line 174-185: The dynamic XPath in selectPolicyGroupDropdown uses raw
policyGroupValue (risking broken XPaths for values with quotes) and it catches a
broad Exception; fix by escaping policyGroupValue for XPath (add a helper like
escapeForXPath or build using concat/parameterized pattern) and use that escaped
string when creating optionLocator, and replace the broad catch with specific
catches (e.g., ElementClickInterceptedException and WebDriverException) so you
only fallback to JavaScript click on those known failures and rethrow or fail
for other exceptions.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java:
- Around line 458-486: The three methods isDeviceStatusApprovedDisplayed,
isDeviceStatusRejectedDisplayed, and isDeviceStatusDeactivatedDisplayed are
identical; consolidate them into a single method (e.g., isDeviceStatusDisplayed)
that accepts deviceType, deviceSubType, make, model, and status, builds the same
XPath and calls isElementDisplayed, then remove the three duplicates (or make
them delegate to the new isDeviceStatusDisplayed for backward compatibility) and
update any callers/tests to use the unified method.
- Around line 1003-1012: The method clickOnDeviceThreeDotsInAdmin creates a
local WebDriverWait instead of using the BasePage helpers; replace the explicit
WebDriverWait/ExpectedConditions usage with the BasePage wait helper (e.g.,
waitForElementToBeVisible or a new isClickable helper) to obtain the same target
element and then call clickOnElement(addedDeviceThreeDots); update
clickOnDeviceThreeDotsInAdmin to build the same XPath, call the BasePage wait
helper to wait for the element to be ready (or add an isClickable method in
BasePage if elementToBeClickable behavior is required), and then use
clickOnElement to perform the click so the wait pattern matches
clickOnDeviceThreeDots and clickOnDeviceThreeDotsAsAdmin.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.java:
- Around line 696-701: The current selectors rejectedRow and firstCellInRow are
too broad and may target the wrong "Rejected" row; update these By xpaths to
filter by the specific sbiVersion (same approach used in
isRejectedStatusDisplayedInAdminPage()) so you locate the row that contains the
given sbiVersion and its "Rejected" status before scrolling and clicking; ensure
sbiVersion is passed/available in this method, replace the two By definitions
with xpaths that include contains(normalize-space(.), sbiVersion), keep using
waitForElementToBeVisible(row) and clickOnElement(firstCellInRow) with the new
locators.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java:
- Around line 341-343: isPoliciesEmptyTableEnabled() only checks
middleRequestPolicyButton and should mirror clickOnRequestPolicyButton() logic;
update isPoliciesEmptyTableEnabled() to first check which button is displayed
(use the same isElementDisplayed(...) check as in clickOnRequestPolicyButton())
and then return isElementEnabled(middleRequestPolicyButton) if displayed,
otherwise return isElementEnabled(tabularPoliciesRequestButton); reference the
methods isElementDisplayed(...) and isElementEnabled(...) and the fields
middleRequestPolicyButton and tabularPoliciesRequestButton when making the
change.
In @uitest-pmp-v2/src/main/resources/testngFile/testng.xml:
- Around line 8-14: The DeactivatePartnerCreation test class
(io.mosip.testrig.pmpuiv2.testcase.DeactivatePartnerCreation) in the creation
sequence can invalidate shared data for later tests; either remove/move this
class out of the creation phase in testng.xml and place it into a dedicated
teardown/cleanup suite, or modify DeactivatePartnerCreation to operate only on a
disposable partner/account/policy that it creates and deletes within the same
test so it cannot affect other tests.
🧹 Nitpick comments (26)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/FtmPage.java (1)
526-527: Consider completing the migration pattern and removing shadowed field.The local variable
pendingForApprovalshadows the instance field declared at line 195-196. While this refactoring to useBylocators aligns with the PR's goal of improving test stability, consider:
Remove the unused field: The instance field at lines 195-196 appears to be superseded by this local declaration and should be removed to avoid confusion.
Pattern inconsistency: This is the only method in the class using a local
Bylocator, while all other similar methods (e.g.,isFilterResetButtonDisplayed()at line 530) still use the WebElement fields directly.Is the migration to
Bylocators ongoing? If so, should this pattern be consistently applied to other similar methods in this file?♻️ Proposed cleanup
If the migration is complete for this element, remove the unused field at lines 195-196:
- @FindBy(xpath = "//*[contains(text(), 'Pending For Approval')]") - private WebElement pendingForApproval; -uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewDeviceDetailsPage.java (2)
3-3: Consider removing unused import.The
Durationimport appears unused in this file. If it's not required for BasePage methods or future changes, consider removing it to keep imports clean.
271-284: Consider locale-safe date extraction.The string replacement on line 275 uses a hard-coded English text
"Created On"which may fail if the UI is localized to other languages. If internationalization is a concern, consider using a more robust extraction method such as substring manipulation based on known format positions or a regex pattern.♻️ Example alternative approach
If the format is always
"Created On <date>", you could use:-String dateText = browserTime.replace("Created On", "").trim(); +String dateText = browserTime.substring(browserTime.indexOf(' ', browserTime.indexOf("On")) + 1).trim();Or if you want to be more defensive:
-String dateText = browserTime.replace("Created On", "").trim(); +String dateText = browserTime.replaceFirst("^.*?\\s+(\\d)", "$1").trim();However, if the UI is guaranteed to be English-only, the current implementation is acceptable.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PartnerCertificatePage.java (1)
596-612: Address duplicate locator and empty catch block.
- The local
By card(line 598) duplicates the class fielddashboardFtmChipProviderCardDashboard(line 581). Reuse the existing field.- The empty catch block silently swallows exceptions. Add a comment or logging to clarify intent.
♻️ Suggested refactor
public void clickOnFtmChipProviderCard() { - - By card = By.id("dashboard_ftm_chip_provider_card"); - waitForUiToBeUnblocked(); for (int i = 0; i < 2; i++) { try { - WebElement cardElement = waitForElementToBeVisible(card); + WebElement cardElement = waitForElementToBeVisible(dashboardFtmChipProviderCardDashboard); clickOnElement(cardElement); return; } catch (StaleElementReferenceException e) { + // Retry on stale element - UI may have refreshed } } throw new RuntimeException("FTM Chip Provider card not clickable"); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (3)
13-23: String parsing is fragile but well-handled.The
describeElementmethod relies on parsingWebElement.toString()output, which depends on Selenium's internal format. While the try-catch fallback provides resilience, consider that future Selenium versions might change the toString() format.
42-59: Consider extracting common logging logic.The
actionmethod overloads forWebElementandByhave nearly identical implementations. While the current approach is clear and maintainable, you could optionally extract the common logging pattern to reduce duplication.♻️ Optional refactor to reduce duplication
+private static void logAction(String message, String descriptor) { + String fullMessage = "🔸 ACTION: " + message + ": " + descriptor; + logger.info(fullMessage); + Reporter.log("<br><span style='color:blue;'>" + fullMessage + "</span>", false); +} + public static void action(String message, WebElement element) { - String fullMessage = "🔸 ACTION: " + message + ": " + describeElement(element); - logger.info(fullMessage); - Reporter.log("<br><span style='color:blue;'>" + fullMessage + "</span>", false); + logAction(message, describeElement(element)); } public static void action(String message, By locator) { - String fullMessage = "🔸 ACTION: " + message + ": " + describeLocator(locator); - logger.info(fullMessage); - Reporter.log("<br><span style='color:blue;'>" + fullMessage + "</span>", false); + logAction(message, describeLocator(locator)); }A similar pattern could be applied to the
verifymethods.
62-73: Same duplication pattern as action methods.The
verifymethod overloads exhibit the same duplication pattern as theactionmethods. The same refactoring approach mentioned in the previous comment could be applied here.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DeviceProviderPage.java (1)
235-241: Consider applying wait pattern to existing date getters for consistency.The new validation methods (lines 398-520) use
waitForElementToBeVisible()before accessing date values, but the existing getter methodsgetCreatedDateValue()andgetExpiredDateValue()access the WebElements directly without explicit waits. This inconsistency could lead to flakiness if these methods are called before the elements are fully loaded.♻️ Suggested enhancement for consistency
public String getCreatedDateValue() { - return getTextFromAttribute(createdDate, GlobalConstants.VALUE); + WebElement element = waitForElementToBeVisible(By.id("sbi_created_date_calender")); + return getTextFromAttribute(element, GlobalConstants.VALUE); } public String getExpiredDateValue() { - return getTextFromAttribute(expiryDate, GlobalConstants.VALUE); + WebElement element = waitForElementToBeVisible(By.id("sbi_expiry_date_calender")); + return getTextFromAttribute(element, GlobalConstants.VALUE); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (1)
1162-1174: Improved robustness with dynamic row selection.The refactoring improves test stability:
- Uses dynamic row locator
starts-with(@id,'api_key_list_item')instead of hardcoded row ID- Adds explicit wait with
waitForElementToBeVisible()before accessing cells- Properly handles multiple rows by selecting the first available one
This makes the test more resilient to data variations.
Optional: Consider using semantic column selection instead of hardcoded index.
Line 1165 uses
./td[6](hardcoded column index). While functional, this could break if the table structure changes. Consider using a more semantic approach:// Option 1: Use column header text or data attribute if available WebElement dateCell = firstRow.findElement(By.xpath("./td[@data-column='creationDate']")); // Option 2: Define column index as a constant for maintainability private static final int CREATION_DATE_COLUMN_INDEX = 6; WebElement dateCell = firstRow.findElement(By.xpath("./td[" + CREATION_DATE_COLUMN_INDEX + "]"));This is a minor maintainability improvement and can be deferred.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java (2)
4-4: Inconsistent wait pattern - prefer BasePage helper.The imports
Duration,WebDriverWait, andExpectedConditionsare only used inclickOnDeviceThreeDotsInAdmin(line 1005), while other refactored methods use thewaitForElementToBeVisiblehelper fromBasePage. For consistency and to avoid code duplication, consider refactoringclickOnDeviceThreeDotsInAdminto use the same helper pattern.[scratchpad_end] -->♻️ Refactor clickOnDeviceThreeDotsInAdmin to use BasePage helper
After this refactor, the imports at lines 4, 17-18 can be removed if not used elsewhere.
See comment on lines 1004-1012 for the specific refactoring suggestion.
Also applies to: 17-18
446-456: Remove outdated comments.The comments at lines 452 and 522 stating "replace local WebDriverWait with existing helper" are misleading since the code already uses
waitForElementToBeVisiblehelper. These comments should be removed.♻️ Remove misleading comments
public void clickOnDevice(String deviceType, String deviceSubType, String make, String model, String status) { By deviceLocator = By.xpath("//tr[" + "td[normalize-space()='" + deviceType + "'] and " + "td[normalize-space()='" + deviceSubType + "'] and " + "td[normalize-space()='" + make + "'] and " + "td[normalize-space()='" + model + "']" + "]"); -// replace local WebDriverWait with existing helper WebElement addedDevice = waitForElementToBeVisible(deviceLocator); clickOnElement(addedDevice); }Similarly for lines 515-526:
public void clickOnFilteredDeviceEllipsisButtonAsAdmin(String deviceType, String deviceSubType, String make, String model) { By deviceEllipsisLocator = By.xpath("//tr[" + "td[normalize-space()='" + deviceType + "'] and " + "td[normalize-space()='" + deviceSubType + "'] and " + "td[normalize-space()='" + make + "'] and " + "td[normalize-space()='" + model + "']" + "]//button[contains(@id,'device_list_action')]"); -// replace local WebDriverWait with existing helper WebElement addedDeviceThreeDots = waitForElementToBeVisible(deviceEllipsisLocator); clickOnElement(addedDeviceThreeDots); }Also applies to: 515-526
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
247-255: LGTM! Consider aligning with selectAddDeviceTypeWithPosition approach.This implementation is more robust than
selectAddDeviceTypeWithPositionbecause it uses the position directly in the XPath predicate, avoiding potentialIndexOutOfBoundsException.Consider refactoring
selectAddDeviceTypeWithPosition(lines 234-245) to use the same XPath-based position approach for consistency.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (2)
156-156: Consider more specific exception type.The method signature
throws Exceptionis very broad. Consider declaring specific exception types (e.g.,InterruptedException,IOException) to make the method contract clearer and help callers handle exceptions appropriately.♻️ Use specific exception types
- public void verifySbiAndDeviceAfterDeactivate() throws Exception { + public void verifySbiAndDeviceAfterDeactivate() throws InterruptedException {Or if multiple exception types are needed:
- public void verifySbiAndDeviceAfterDeactivate() throws Exception { + public void verifySbiAndDeviceAfterDeactivate() throws InterruptedException, IOException {
692-700: Unused helper method - consider removing or uncommenting caller.The
fillDeviceDetailsForRejectVerificationmethod is only called from commented-out code (lines 200-201). Consider either:
- Uncommenting the caller if this functionality is needed, or
- Removing this method to avoid dead code
Also, as with
verifySbiAndDeviceAfterDeactivate, consider using more specific exception types instead ofthrows Exception.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.java (5)
3-3: Avoid wildcard imports for better code clarity.The wildcard import
import org.openqa.selenium.*;reduces code readability and can lead to naming conflicts. Prefer explicit imports for the specific classes used.📝 Suggested fix
-import org.openqa.selenium.*; +import org.openqa.selenium.By; +import org.openqa.selenium.JavascriptExecutor; +import org.openqa.selenium.NoSuchElementException; +import org.openqa.selenium.StaleElementReferenceException; +import org.openqa.selenium.TimeoutException; +import org.openqa.selenium.WebDriver; +import org.openqa.selenium.WebElement;
591-596: Consider extracting the By locator as a class field.The
By.id("policy_name_box")locator is created inline inenterPolicyName. If this element is accessed multiple times, consider extracting it as a private field to improve reusability and maintainability.♻️ Proposed refactor
At the class level:
private By policyNameBoxLocator = By.id("policy_name_box");Then update the method:
public void enterPolicyName(String val) { - By policyNameBoxLocator = By.id("policy_name_box"); WebElement policyNameBox = waitForElementToBeVisible(policyNameBoxLocator); policyNameBox.clear(); policyNameBox.sendKeys(val); }
806-818: Enhance exception message with more context.The exception thrown at line 817 provides the value but could be more helpful by including information about available options or suggesting next steps.
💡 Suggested improvement
} catch (TimeoutException e) { logger.warn("Policy group not found: " + value); - throw new NoSuchElementException("Failed to select policy group: " + value, e); + throw new NoSuchElementException( + "Failed to select policy group: '" + value + "'. " + + "Verify that the policy group exists and is visible in the dropdown.", e); }
1234-1243: Consider consolidating duplicate retry patterns.Both
selectPolicyGroupDropdown(lines 806-818) andselectValidPolicyGroupForClone(lines 1234-1243) implement similar click-with-fallback patterns. Consider extracting this into a reusable helper method in BasePage to reduce duplication.♻️ Example helper method
In BasePage:
protected void clickWithJsFallback(By locator) { WebElement element = waitForElementToBeVisible(locator); ((JavascriptExecutor) driver).executeScript( "arguments[0].scrollIntoView({block:'center'});", element); try { element.click(); } catch (Exception e) { ((JavascriptExecutor) driver).executeScript("arguments[0].click();", element); } }Then simplify both methods to use this helper.
1338-1340: Extract inline By locator to a class field.Creating the
By.id("page_title")locator inline inisEditPolicyPageTitleDisplayedis inefficient if this method is called multiple times. This locator should be a private field for better performance and maintainability.♻️ Proposed refactor
At the class level:
private By editPolicyPageTitleLocator = By.id("page_title");Update the method:
public boolean isEditPolicyPageTitleDisplayed() { - By editPolicyPageTitleLocator = By.id("page_title"); return isElementDisplayed(editPolicyPageTitleLocator); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
705-708: Consider making the timeout configurable.The
waitUntilPageReadymethod uses a hardcoded 20-second timeout. For consistency and flexibility, consider accepting the timeout as a parameter or using a constant.♻️ Proposed refactor
-public void waitUntilPageReady() { - waitForPageToBeReady(By.id("show_request_policy"), 20); -} +public void waitUntilPageReady() { + waitUntilPageReady(20); +} + +public void waitUntilPageReady(int timeoutSeconds) { + waitForPageToBeReady(By.id("show_request_policy"), timeoutSeconds); +}uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (3)
528-535: Missing exception handling in isClickable.The
isClickable(By locator)method callsdriver.findElement(locator)which can throwNoSuchElementExceptionif the element is not present in the DOM. While the catch block will handle this, consider explicitly ignoring this exception for clarity.♻️ Suggested improvement
protected boolean isClickable(By locator) { try { WebElement el = driver.findElement(locator); return el.isDisplayed() && el.isEnabled(); - } catch (Exception e) { + } catch (NoSuchElementException | StaleElementReferenceException e) { return false; } }
543-552: Broad CSS selector may cause false positives.The
waitForUiToBeUnblocked()method uses a broad CSS selector that matches any element with classes containing "loading", "spinner", or "overlay". This could inadvertently match legitimate UI elements that use these common class names but aren't actually blocking overlays.Consider making the blocking overlay selector more specific or configurable based on the actual overlay structure in the application. You might also want to add a comment documenting the expected overlay patterns.
💡 Suggested improvement
protected void waitForUiToBeUnblocked() { WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(20)); + // Matches common blocking overlay patterns - update if app uses different overlay structure By blockingOverlay = By.cssSelector("div.fixed.inset-0.z-50, " + "div[role='dialog'], " + "div[class*='loading'], " + "div[class*='spinner'], " + "div[class*='overlay']"); try { wait.until(ExpectedConditions.invisibilityOfElementLocated(blockingOverlay)); } catch (TimeoutException e) { LogUtil.step("UI overlay still present after wait, proceeding cautiously"); } }
554-558: Exact text matching may be fragile.
selectFromCustomDropdownusesnormalize-space()=for exact text matching. This is brittle if the dropdown options have minor whitespace variations or text changes. Consider whethercontains()would be more appropriate, or document that exact matches are required.If exact matching is required, add a comment documenting this constraint. Otherwise, consider a more flexible approach:
♻️ Alternative implementation
protected void selectFromCustomDropdown(String value) { // First try exact match By exactOption = By.xpath("//li[normalize-space()='" + value + "']"); try { waitForElementToBeVisible(exactOption); clickOnElement(exactOption); } catch (TimeoutException e) { // Fallback to contains match By containsOption = By.xpath("//li[contains(normalize-space(), '" + value + "')]"); waitForElementToBeVisible(containsOption); clickOnElement(containsOption); } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.java (1)
298-308: Consider extracting common XPath pattern builder.Multiple methods (e.g.,
isRejectedStatusDisplayed,getDeviceDetails,isViewDeviceButtonEnabled,isDeactivatedStatusDisplayed) construct similar XPath patterns withnormalize-space()and ancestor navigation. Consider extracting a helper method to build these patterns consistently and reduce duplication.♻️ Example helper method
private By buildSbiVersionLocator(String sbiVersion, String descendantXpath) { return By.xpath("//p[normalize-space()='" + sbiVersion + "']" + descendantXpath); } // Usage: public boolean isRejectedStatusDisplayed(String sbiVersion) { By rejectedStatusLocator = buildSbiVersionLocator(sbiVersion, "/..//div[normalize-space()='Rejected' " + "and contains(@class,'bg-[#FAD6D1]') " + "and contains(@class,'text-[#5E1515]')]"); return isElementDisplayed(rejectedStatusLocator); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
510-523:selectPolicyGroupDropdownIfPresent: consider handling common transient WebDriver failures (stale/intercepted) if the intent is “best effort”.
Right now onlyTimeoutExceptionreturnsfalse; a stale/intercepted click will still fail the test even though the element is “present”.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
429-432:waitForDashboardLoaderToDisappear: the “contains(text(),'Loading')” locator is very broad—consider a more specific loader selector.
This may wait on unrelated “Loading” text elsewhere (or localized variants), reintroducing flakiness. If the app has a stable loader id/data-testid/class, prefer that; also consider calling this insidewaitForDashboardReady()to centralize readiness gating.Also applies to: 425-427
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DatasharePolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DeviceProviderPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/FtmPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PartnerCertificatePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewDeviceDetailsPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewSbiDetailsPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.javauitest-pmp-v2/src/main/resources/config/kernel.propertiesuitest-pmp-v2/src/main/resources/testngFile/testng.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- uitest-pmp-v2/src/main/resources/config/kernel.properties
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/SbiLinkedDevicePage.java
🧰 Additional context used
🧬 Code graph analysis (4)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (1)
LogUtil(8-98)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DeviceProviderPage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewDeviceDetailsPage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/fw/util/PmpTestUtil.java (1)
PmpTestUtil(32-175)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (1)
LogUtil(8-98)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/driver/DriverManager.java (1)
DriverManager(5-23)
🔇 Additional comments (36)
uitest-pmp-v2/src/main/resources/testngFile/testng.xml (2)
4-18: Confirm TestNG ordering/parallel semantics match the intent (Step 1 strictly before Step 2, while Step 2 runs in parallel).With
<suite ... preserve-order="true" parallel="false">(Line 4) plus<test ... parallel="classes" thread-count="4">(Line 17), please confirm in CI that:
PartnerCreationSequencealways completes beforePartnerTestsParallelstarts.- Classes under
PartnerTestsParallelactually execute concurrently (not silently serialized due to suite-level settings).
16-34: Parallel class execution: watch for shared-state collisions (accounts, policies, report artifacts, WebDriver/session reuse).Since
PartnerTestsParallelruns classes concurrently (Line 17), please verify each listed testcase is thread-safe and uses isolated test data and isolated output artifacts (e.g., screenshots/downloads/reports) to avoid intermittent failures and unstable reporting.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/FtmPage.java (1)
3-3: LGTM! Import supports the By locator refactoring.The import is correctly added to support the use of
Bylocators in the refactored method.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewDeviceDetailsPage.java (1)
272-273: Good stability improvement with explicit wait.The refactoring from direct element lookup to using
By.idlocator withwaitForElementToBeVisiblesignificantly improves test stability by handling timing issues when the element is not immediately available. This aligns well with the PR objective to fix unstable test failures.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PartnerCertificatePage.java (5)
3-6: LGTM!The new imports are correctly added and used within the file -
LocalDateTimefor date parsing andStaleElementReferenceExceptionfor the retry mechanism.
36-37: LGTM!The migration from
@FindByWebElement toBylocator aligns with the broader refactoring pattern and enables better integration with explicit waits inBasePage.
591-594: LGTM!Good addition of
waitForLoaderToDisappear()to ensure UI synchronization before checking element visibility.
1090-1110: LGTM!Good use of the shared
PmpTestUtil.dateTimeFormatterconstant for consistent date-time parsing across validation methods.
1244-1274: All certificate resource files are present.The upload methods reference certificate files that all exist in the
pmp_uiv2_certresources directory. The code follows the established pattern and has no issues.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (4)
1-11: LGTM! Clean class setup.The imports are appropriate for the logging functionality, and the Logger is properly declared as a static final field.
25-27: LGTM! Clean helper method.The
logToReportermethod provides a simple, reusable way to add colored output to TestNG reports.
30-39: LGTM! Well-structured logging methods.The logging methods (
section,step,input,success,error,endSection) provide clear, consistent formatting with helpful visual cues through emojis and structured messages.Also applies to: 80-97
75-77: LGTM! Simple and effective.The
describeLocatormethod appropriately delegates toBy.toString(), which provides a clear description of the locator strategy.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (4)
467-469: LGTM! Naming convention improved.Method renamed from
isGenerateAPIKeyDisplayed()toisGenerateApiKeyDisplayed()to follow proper camelCase convention.
471-479: Excellent defensive programming with fallback logic.The refactored method:
- Uses a more descriptive name:
clickOnCreateApiKey()vs.clickOnAPIKeyDisplayed()- Implements fallback logic to handle the "Create API Key" button in different page states
- Provides clear error messaging when neither button variant is found
This improves test robustness by handling UI variations gracefully.
448-460: Verify that dynamic XPath construction receives only controlled policy name inputs.The refactoring adds robust explicit waits, error handling, and boolean return values. However, the dynamic XPath construction
"//button[.//span[normalize-space()='" + value + "']]"could fail or misbehave if thevalueparameter contains single quotes or special XPath characters. Inspect all callers to this method to confirm that policy name values always come from controlled test data sources and cannot contain problematic characters.
6-6: LGTM! Refactoring to By-based locators improves test stability.The changes align with best practices for Selenium Page Object Model:
- Added
TimeoutExceptionimport for explicit wait handling- Corrected field naming from
generateAPIKeytogenerateApiKey(proper camelCase)- Refactored
apiKeyViewfrom@FindByWebElement toBylocator for more flexible element handlingBasePage correctly provides overloads for both
clickOnElement(By locator)andisDisplayed(By locator), so the usage on lines 539-540 is properly supported.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java (2)
430-435: LGTM!The refactor to use a By locator with
isElementDisplayedimproves robustness, and thenormalize-space()XPath function handles whitespace variations effectively.
488-500: LGTM!The explicit wait with
TimeoutExceptionhandling is a good pattern for optional elements. Returningnullwhen the element is not found allows callers to handle the absence gracefully.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (3)
7-7: LGTM!The
JavascriptExecutorimport andhomeButtonByfield additions align with the PR's goal of using By locators and improving element interaction reliability through scrolling.Also applies to: 125-126
210-224: LGTM!The refactor to use By locators with explicit waits (
waitForUiToBeUnblockedandwaitForElementToBeVisible) improves test stability by ensuring UI readiness before interaction.
257-269: LGTM!The combination of explicit waits, JavaScript scrolling to bring elements into the viewport, and centralized
enter()helper creates a robust interaction pattern that should reduce flakiness.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (2)
168-176: Status checks align with ListOfDevicesPage changes.The updated method calls (
isDeviceStatusRejectedDisplayed,isDeviceStatusDeactivatedDisplayed) align with the new methods introduced inListOfDevicesPage.java. However, if the suggestion to consolidate those duplicate methods is accepted, these calls should be updated to use the consolidatedisDeviceStatusDisplayedmethod.
267-267: LGTM!Adding
throws InterruptedExceptionto the method signatures makes exception handling explicit and aligns with the updatedverifyActionMenuOfDevicesInAdminmethod.Also applies to: 363-363
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java (1)
276-287: Good defensive programming with dual button support.The conditional logic to check both
middleRequestPolicyButtonandtabularRequestPolicyButtonprovides good resilience for different UI states. Consider adding a log statement before throwing the exception to aid debugging.💡 Optional enhancement
} else { + logger.warn("Request Policy button not found in either middle or tabular view"); throw new RuntimeException("Request Policy button not found in middle or tabular view"); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (4)
79-102: Excellent addition of By-based click method with fallback.The new
clickOnElement(By locator)method provides good resilience with JavaScript fallback for difficult-to-click elements. The exception handling and logging are appropriate.
374-378: Verify the null-safety pattern in refreshThePage.The null check on line 375 protects against NPE, but if
DriverManager.getDriver()returns null, the method silently does nothing. Consider whether this silent failure is intentional or if it should log a warning or throw an exception.Based on learnings or project patterns, confirm whether silent failure is the expected behavior when the driver is null, or if this should be treated as an error condition.
450-460: Robust FluentWait implementation for dynamic content.The updated
isDisplayed(By locator)method usesFluentWaitwith proper handling ofStaleElementReferenceExceptionandNoSuchElementException. This is an excellent pattern for dealing with dynamic UI elements that may refresh or be temporarily unavailable.
510-519: Good loader synchronization pattern.
waitForLoaderToDisappear()is essential for test stability when dealing with loading overlays. The timeout exception handling with a warning is appropriate, allowing tests to continue rather than fail hard.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ViewSbiDetailsPage.java (2)
107-118: Improved robustness with explicit waits.The refactored
isSbiCreationDateSameAsBrowserDateFormatnow uses an explicit wait viawaitForElementToBeVisible(creationDateCell)before accessing the element's text. This prevents potential race conditions where the element might not be fully loaded. The addition ofTimeoutExceptionhandling is appropriate.
120-131: Consistent date validation pattern.
isSbiExpirationDateSameAsBrowserDateFormatfollows the same pattern as the creation date method, which is good for maintainability. Both methods now properly handle dynamic content loading.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.java (3)
330-345: Excellent stale element retry pattern.The
clickOnThreeDotsOfSbiListAsAdminmethod implements a robust retry mechanism with up to 3 attempts to handleStaleElementReferenceException. The use ofLogUtil.stepfor logging retry attempts is helpful for debugging. This is a best practice for dealing with dynamic DOM updates.
367-373: Method rename improves clarity.Renaming
clickOnAddDeviceButtontoclickOnAddDeviceButtonForSbibetter indicates that this method is specific to SBI-related device addition, avoiding potential confusion with other device-addition flows.
659-669: Consistent date validation with improved error handling.The updated
isSbiCreationDateSameAsBrowserDateFormatnow catchesTimeoutExceptionin addition toDateTimeParseException, which properly handles cases where the element doesn't appear within the wait period. The use ofwaitForElementToBeVisiblealigns with the broader refactoring pattern.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
45-56: Good shift toBy+ explicit waits for dynamic dashboard elements.
This should reduce stale element references vs long-lived@FindByfields.Also applies to: 199-222, 236-248, 296-300
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
16-18: By-based API overloads exist and are accessible. BasePage provides protected methods that acceptBylocators:clickOnElement(By)(line 79),enter(By, String)(line 521),isDisplayed(By)(line 450), andwaitForElementToBeVisible(By)(line 324). Since AuthPolicyPage is in the same package, these protected methods are accessible and the code will compile correctly. No API compatibility issues exist.Likely an incorrect or invalid review comment.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DeviceProviderPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfSbiPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java:
- Around line 236-257: The method selectAddDeviceTypeWithPosition currently
declares throws InterruptedException but only propagates IOException from
dropdownWithPosition—change the signature to throw IOException instead of
InterruptedException; also simplify position handling by treating the incoming
position as 1-based, compute a single zeroBasedIndex = position - 1, use
zeroBasedIndex for bounds check and for dropdowns.get(zeroBasedIndex), throw the
NoSuchElementException referencing the original 1-based position, and call
dropdownWithPosition(addDeviceTypeSelectDropdown, value, position) (i.e., pass
the original 1-based position) so there is one clear conversion point and no
back-and-forth adjustments.
In @uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java:
- Around line 448-460: The XPath construction in selectPolicyNameDropdown
currently injects the raw value into policyNameOption, causing failures if value
contains a single quote; update selectPolicyNameDropdown to build a quote-safe
XPath (e.g., use XPath concat() or split the string and join with concat) when
creating policyNameOption instead of naively concatenating value, ensure the
same safe value is used with generatePolicyNameSearchInputBox and
waitForElementToBeVisible, and keep the existing TimeoutException handling and
return semantics.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java:
- Around line 519-532: The XPath in selectPolicyGroupDropdownIfPresent embeds
policyGroupValue directly and can break if the value contains quotes; change the
construction of optionLocator in selectPolicyGroupDropdownIfPresent to use the
existing escapeXPathText helper (same approach as selectPolicyGroupDropdown) to
properly escape policyGroupValue before concatenating it into the XPath, then
use the escaped string when creating optionLocator so the XPath remains valid
for values with single/double quotes.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java:
- Around line 433-436: The waitForDashboardLoaderToDisappear method uses a
brittle XPath and an overly long 60s timeout; change the timeout to a more
reasonable 20–30 seconds and replace the generic XPath
"//*[contains(text(),'Loading')]" with a more specific locator that targets the
actual loader element (e.g., By.id("loader"),
By.cssSelector(".dashboard-loader"), or a unique data-* attribute) so the wait
uses wait.until(ExpectedConditions.invisibilityOfElementLocated(<new locator>));
if no stable attribute exists, add one in the loader component and use that
unique identifier.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java:
- Around line 437-444: The XPath constructed in clickOnDeviceThreeDots
concatenates raw parameters and can break or be vulnerable; update the method to
call escapeXPathText on deviceType, deviceSubType, make, and model before
building the XPath (same approach used in isDeviceDisplayed), then use the
escaped values when composing deviceThreeDotsLocator and locating the element
(refer to method clickOnDeviceThreeDots and helper escapeXPathText).
- Around line 446-456: The XPath built in clickOnDevice uses raw parameters and
can break or be vulnerable when values contain quotes; update clickOnDevice to
call the existing escapeXPathText helper for deviceType, deviceSubType, make,
and model before concatenating them into deviceLocator, then use the escaped
values to build the By.xpath string so waitForElementToBeVisible(deviceLocator)
and clickOnElement(addedDevice) receive a safe locator.
- Around line 488-500: The XPath built in getDeviceStatusClassValue uses raw
inputs and can break or be injected; wrap each parameter—deviceType,
deviceSubType, make, model, and status—with escapeXPathText when constructing
deviceStatusLocator so the string concatenation uses escaped values (e.g., use
escapeXPathText(deviceType) etc. in the XPath expression) and keep the rest of
the method (waitForElementToBeVisible, getTextFromAttribute, returning null on
TimeoutException) unchanged.
- Around line 430-435: The XPath is built by concatenating raw parameters in
isDeviceDisplayed, which breaks when values contain single quotes; fix by
passing deviceType, deviceSubType, make, and model through the existing
escapeXPathText helper (same approach used in clickOnDeviceThreeDotsInAdmin) and
use the escaped values when constructing rowLocator so the XPath stays valid and
injection-safe.
🧹 Nitpick comments (8)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (1)
127-128: Remove unused WebElement field to eliminate duplication.The
homeButtonBylocator duplicates thehomeButtonWebElement field (line 19-20). SinceisHomeButtonDisplayed()now useshomeButtonBy, the@FindByannotated field appears unused and should be removed.♻️ Proposed fix
Remove the unused field at line 19-20:
- @FindBy(id = "sub_title_home_btn") - private WebElement homeButton; -uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (1)
1161-1173: Improved row locator strategy.Using
starts-with(@id,'api_key_list_item')is more resilient than a fixed row index. The date parsing validation approach is clean.Consider adding a comment noting the column index dependency for maintainability:
WebElement firstRow = waitForElementToBeVisible(firstRowLocator); +// Column 6 contains Creation Date - update if table structure changes WebElement dateCell = firstRow.findElement(By.xpath("./td[6]"));uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (2)
176-189: Consider narrowing the exception catch block.The method catches the generic
WebDriverException(line 185), which may mask specific exceptions likeElementClickInterceptedExceptionorStaleElementReferenceException. Consider catchingElementClickInterceptedExceptionspecifically, or log the exception type to aid debugging when fallback is triggered.♻️ Proposed refinement
public void selectPolicyGroupDropdown(String policyGroupValue) { clickOnElement(selectPolicyGroupDropdown); enter(SearchBox, policyGroupValue); By optionLocator = By.xpath("//button[contains(@id,'policy_group_selector_option_button')" + " and .//span[contains(normalize-space(.), " + escapeXPathText(policyGroupValue) + ")]]"); WebElement option = waitForElementToBeVisible(optionLocator); ((JavascriptExecutor) driver).executeScript("arguments[0].scrollIntoView({block:'center'});", option); try { option.click(); - } catch (WebDriverException e) { + } catch (ElementClickInterceptedException e) { ((JavascriptExecutor) driver).executeScript("arguments[0].click();", option); } }
240-245: Consider adding standard click with JavaScript fallback.This method directly uses JavaScript click (line 243), whereas other methods like
selectPolicyGroupDropdown(lines 184-187) attempt a standard click first with a JavaScript fallback. For consistency and to respect browser event handling, consider trying a standard click first.♻️ Proposed refinement
public PoliciesPage clickOnPoliciesTitle() { waitForUiToBeUnblocked(); WebElement policiesCard = waitForElementToBeVisible(policiesTitle); - ((JavascriptExecutor) driver).executeScript("arguments[0].click();", policiesCard); + clickOnElement(policiesCard); return new PoliciesPage(driver); }Note:
BasePage.clickOnElementalready implements the try-standard-then-JS-fallback pattern.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (2)
205-227: Remove redundant JavaScript fallback logic.Lines 216-220 implement a JavaScript fallback that is already handled by the
clickOnElementmethod called on line 212. TheclickOnElementmethod (lines 79-102) already includes try/catch with JavaScript fallback, making this additional try/catch block redundant.♻️ Proposed simplification
public void dropdownWithPosition(WebElement element, String value, int position) throws IOException { LogUtil.action("Selecting dropdown position " + position + " for element: ", element); - try { - waitForElementVisible(element); - clickOnElement(element); - WebElement dropdown = driver.findElement(By.xpath("(//*[text()='" + value + "'])[" + position + "]")); - waitForElementVisible(dropdown); - clickOnElement(dropdown); - } catch (Exception e) { - LogUtil.step("Dropdown position select failed, trying JS click: " + element.toString()); - try { - ((JavascriptExecutor) driver).executeScript("arguments[0].click();", element); - WebElement option = driver - .findElement(By.xpath("(//*[contains(text(),'" + value + "')])[" + position + "]")); - ((JavascriptExecutor) driver).executeScript("arguments[0].click();", option); - } catch (Exception jsEx) { - LogUtil.error("Dropdown text select failed: " + element.toString()); - takeScreenshot(); - throw new RuntimeException("Failed to select dropdown value '" + value + "' at position " + position, - jsEx); - } - } + waitForElementVisible(element); + clickOnElement(element); + WebElement dropdown = driver.findElement(By.xpath("(//*[text()='" + value + "'])[" + position + "]")); + waitForElementVisible(dropdown); + clickOnElement(dropdown); }
373-378: Clarify the use of DriverManager.getDriver() instead of this.driver.The method retrieves the driver via
DriverManager.getDriver()(line 374) instead of using the instance fieldthis.driverset in the constructor (line 42). This introduces inconsistency with other methods in the class that usethis.driverdirectly.If
DriverManageris used for thread-local access in parallel test execution, this pattern should be applied consistently across all methods. Otherwise, consider usingthis.driverdirectly with a null check.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
1119-1128: Consider reusing the existing WebElement locator.Line 1120 creates a new
Bylocator inline, but line 358 already definesclonePolicyTitleas a@FindBy(id = "clone_policy_popup_title")WebElement. While the current approach works, defining aByconstant alongside the@FindByfield would improve maintainability and reduce duplication.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java (1)
1003-1012: Good use of XPath escaping, but consider using the existing helper.Lines 1006-1009 correctly use
escapeXPathTextfor all parameters, which prevents XPath injection. However, lines 1005-1010 create a localWebDriverWaitinstead of using the existingwaitForElementClickable(By locator)helper defined inBasePage(lines 313-317).♻️ Proposed simplification
public void clickOnDeviceThreeDotsInAdmin(String deviceType, String deviceSubType, String make, String model) { - WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(30)); By threeDotsLocator = By.xpath("//tr[" + "td[normalize-space()=" + escapeXPathText(deviceType) + "] and " + "td[normalize-space()=" + escapeXPathText(deviceSubType) + "] and " + "td[normalize-space()=" + escapeXPathText(make) + "] and " + "td[normalize-space()=" + escapeXPathText(model) + "]" + "]//button[contains(@id,'device_list_action_menu')]"); - WebElement threeDots = wait.until(ExpectedConditions.elementToBeClickable(threeDotsLocator)); + WebElement threeDots = waitForElementClickable(threeDotsLocator); clickOnElement(threeDots); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/PoliciesPage.java
🧰 Additional context used
🧬 Code graph analysis (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.java (1)
GlobalConstants(3-1215)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (2)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (1)
LogUtil(8-98)uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/driver/DriverManager.java (1)
DriverManager(5-23)
🔇 Additional comments (17)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (5)
212-218: LGTM!The refactoring to use By locators and explicit waits improves test stability and aligns with the PR's objectives.
220-226: LGTM!Consistent refactoring pattern that improves reliability through explicit waits and standardized dropdown interaction.
259-267: LGTM!The position handling here is clearer than in
selectAddDeviceTypeWithPosition- using the position directly in the XPath and passing it unchanged todropdownWithPositionis straightforward.
269-274: LGTM!The combination of explicit wait and scroll-into-view improves interaction reliability, particularly for elements that may be off-screen.
276-281: LGTM!Consistent implementation with
enterMakeNameWithPosition, following the same wait-and-scroll pattern for improved stability.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (3)
36-36: LGTM on field refactoring.The rename to
generateApiKeyaligns with the JavaScript API naming convention, and the migration ofapiKeyViewto aBylocator enables proper explicit wait handling.Also applies to: 77-77
471-479: Good consolidation of API key button click logic.The conditional fallback approach handles both the generate page and list page button locations gracefully. The exception provides a clear message for debugging test failures.
537-540: Good addition of loader synchronization.Adding
waitForLoaderToDisappear()before interacting with the view button should reduce flaky test failures caused by UI loading states. TheBasePage.clickOnElement(By)overload is available and correctly accepts theapiKeyViewlocator.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (2)
203-205: LGTM!The migration to
By-based element checks improves resilience against stale element references.
429-431: LGTM!The
waitForDashboardReadymethod provides a robust way to synchronize with the dashboard by waiting for either of the key dashboard cards to appear, improving test reliability.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (6)
79-102: LGTM!The
By-based overload ofclickOnElementfollows the same robust pattern as theWebElementversion, with proper wait, fallback to JavaScript click, and error handling. This enhances consistency and flexibility across the page objects.
313-317: LGTM!The
By-basedwaitForElementClickablemethod properly handles stale element references and provides a consistent wait pattern for locator-based interactions.
450-460: LGTM!The enhanced
isDisplayedmethod withFluentWaitprovides better resilience against timing issues and stale elements. The visibility change toprotectedis appropriate for a base class utility method.
510-519: Consider verifying the loading overlay locator specificity.The
loadingOverlaylocator usesBy.id("loading_text")(line 510), which assumes a specific ID is consistently used for loading indicators across the application. Verify that this ID is:
- Unique and not reused for other UI elements
- Present on all loading overlays throughout the application
If different pages use different loading indicators, consider parameterizing this method or adding page-specific overrides.
543-553: LGTM!The
waitForUiToBeUnblockedmethod provides robust synchronization for modal overlays. The CSS selector (lines 545-546) covers multiple overlay patterns, though it may require maintenance if overlay styling changes.
563-581: LGTM!The
escapeXPathTextmethod properly handles all quote scenarios for XPath string literals, preventing injection issues and syntax errors when dynamic values contain special characters.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
503-517: LGTM!The refactored
selectPolicyGroupDropdownmethod uses robust dynamic XPath construction with proper escaping, explicit waits, and scroll-into-view before clicking, improving reliability for dynamic dropdown interactions.
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java
Outdated
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java
Show resolved
Hide resolved
| public void clickOnDeviceThreeDots(String deviceType, String deviceSubType, String make, String model) { | ||
| WebElement addedDeviceThreeDots = driver.findElement( | ||
| By.xpath("//td[text()='" + deviceType + "']/..//td[text()='" + deviceSubType + "']/..//td[text()='" | ||
| + make + "']/..//td[text()='" + model + "']/..//button[contains(@id, 'device_list_action')]")); | ||
|
|
||
| By deviceThreeDotsLocator = By.xpath("//tr[" + "td[normalize-space()='" + deviceType + "'] and " | ||
| + "td[normalize-space()='" + deviceSubType + "'] and " + "td[normalize-space()='" + make + "'] and " | ||
| + "td[normalize-space()='" + model + "']" + "]//button[contains(@id,'device_list_action')]"); | ||
| WebElement addedDeviceThreeDots = waitForElementToBeVisible(deviceThreeDotsLocator); | ||
| clickOnElement(addedDeviceThreeDots); | ||
| } |
There was a problem hiding this comment.
Add XPath escaping to prevent injection and syntax errors.
Similar to isDeviceDisplayed, this method builds an XPath by directly concatenating parameters (lines 439-441) without escaping. Apply escapeXPathText to all four parameter values to prevent XPath syntax errors.
🤖 Prompt for AI Agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java
around lines 437 - 444, The XPath constructed in clickOnDeviceThreeDots
concatenates raw parameters and can break or be vulnerable; update the method to
call escapeXPathText on deviceType, deviceSubType, make, and model before
building the XPath (same approach used in isDeviceDisplayed), then use the
escaped values when composing deviceThreeDotsLocator and locating the element
(refer to method clickOnDeviceThreeDots and helper escapeXPathText).
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java
Show resolved
Hide resolved
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.java
Show resolved
Hide resolved
Signed-off-by: damodarguru <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java:
- Around line 1173-1181: The XPath in selectValidPolicyGroupForClone currently
interpolates raw value (risking breakage with quotes/special chars); update the
method to pass value through the existing escapeXPathText(...) helper before
building optionLocator and use that escaped string in the XPath construction so
optionLocator (and subsequent waitForElementToBeVisible/clickOnElement calls)
work reliably for values containing quotes or special characters.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPolicyTest.java:
- Around line 637-639: The assertion uses isClonePolicyPopupTitleNotDisplayed()
but the failure message constant is
GlobalConstants.isClonePolicyPopupTitleDisplayed, creating a mismatch; either
replace the message with the matching
GlobalConstants.isClonePolicyPopupTitleNotDisplayed (or another appropriately
named "NotDisplayed" constant) or change the assertion to
isClonePolicyPopupTitleDisplayed() to reflect the intended check—update the
single line asserting the clone popup so the boolean method and the
GlobalConstants message refer to the same expected visibility.
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java:
- Around line 156-176: In verifySbiAndDeviceAfterDeactivate the calls to
isDeviceStatusRejectedDisplayed and isDeviceStatusDeactivatedDisplayed currently
discard their boolean results; wrap each call with an assertion (e.g.,
Assert.assertTrue or assertTrue) so the test fails when the UI check returns
false; update the three invocations of
listOfDevicesPage.isDeviceStatusRejectedDisplayed(...) and
listOfDevicesPage.isDeviceStatusDeactivatedDisplayed(...) to be asserted using
the test framework's assertTrue.
🧹 Nitpick comments (13)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiCreationTest.java (1)
10-11: BasePage introduction looks fine; consider scoping it locally if not reused.
basePageis only used insidecreateSbiDevice()right now—making it a local variable would reduce shared mutable state in the test class.Also applies to: 24-32
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
503-535: Good move to escaped dynamic XPath + wait; butIfPresentshould clean up UI state on failure.
If the option isn’t found, consider closing the dropdown (ESC / click outside) before returningfalse, otherwise subsequent steps can become flaky.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (2)
267-275: Avoid broadthrows InterruptedExceptionon tests/helpers unless you truly propagate it.
If this was added only to satisfy upstream signatures, consider containing the wait logic inside page objects/utilities instead.Also applies to: 633-640
343-345: Consider removing commented-out test logic / “required for later” blocks (or convert to TODO + issue).
Keeping dead code paths in tests tends to rot quickly and makes failures harder to reason about.Also applies to: 692-700
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPolicyTest.java (1)
176-190: Avoid hard-coded policy group name in tests.
Prefer aGlobalConstantsentry instead of"automationui policy group"so the test doesn’t break on copy or rename.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (2)
473-481: PreferIllegalStateException(and include context) over genericRuntimeExceptionfor missing CTA.
This will make failures easier to triage (page/role mismatch vs actual bug).
1163-1175: Consider handling “no rows found” gracefully inisApiKeyCreationDateSameAsBrowserDateFormat().
Right now a missing row likely surfaces as a timeout exception rather than returningfalse, which can obscure the actual cause.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.java (4)
19-20: Consider consolidating element references.The
homeButtonWebElement field (lines 19-20) and thehomeButtonByBy locator (line 127) both reference the same element (id="sub_title_home_btn"). SinceisHomeButtonDisplayed()now uses the By locator, consider removing the unused WebElement field to eliminate duplication and prevent confusion.♻️ Consolidate to use only the By locator
- @FindBy(id = "sub_title_home_btn") - private WebElement homeButton; -Also applies to: 127-127, 133-135
212-226: Refine exception handling to use specific exception types.The method declares
throws Exception, which is too broad. Consider throwing more specific exceptions likeTimeoutExceptionorNoSuchElementException, or create a custom exception type. This helps callers handle different failure scenarios appropriately.♻️ Use specific exception types
- public void selectAddDeviceType(String value) throws Exception { + public void selectAddDeviceType(String value) throws TimeoutException, NoSuchElementException { try { waitForUiToBeUnblocked(); By addDeviceTypeDropdown = By.id("add_device_device_type_dropdown_btn"); WebElement deviceTypeDropdown = waitForElementClickable(addDeviceTypeDropdown); dropdown(deviceTypeDropdown, value); - } catch (Exception e) { + } catch (TimeoutException | NoSuchElementException e) { logger.error("Failed to select Add Device Type: " + value, e); throw e; } }
228-242: Refine exception handling to use specific exception types.Similar to
selectAddDeviceType(), this method declaresthrows Exception, which is too broad. Consider throwing more specific exceptions for better error handling.
252-268: Simplify position handling to avoid confusion.The method converts the position parameter from 1-based to 0-based (line 253), uses it for array indexing, then converts it back to 1-based when calling
dropdownWithPosition()(line 264). This back-and-forth conversion is unnecessary and error-prone. Consider either:
- Keeping the position 1-based throughout and adjusting only the array access
- Documenting clearly whether methods expect 0-based or 1-based indexing
♻️ Simplify position conversion
public void selectAddDeviceTypeWithPosition(String value, int position) { - position = position - 1; By addDeviceTypeDropdown = By.id("add_device_device_type_dropdown_btn"); waitForElementToBeVisible(addDeviceTypeDropdown); List<WebElement> dropdowns = driver .findElements(By.xpath("//button[@id='add_device_device_type_dropdown_btn']")); - if (dropdowns.size() <= position) { - throw new NoSuchElementException("Add Device Type dropdown not found at position: " + (position + 1)); + if (dropdowns.size() < position) { + throw new NoSuchElementException("Add Device Type dropdown not found at position: " + position); } - WebElement addDeviceTypeSelectDropdown = dropdowns.get(position); + WebElement addDeviceTypeSelectDropdown = dropdowns.get(position - 1); try { - dropdownWithPosition(addDeviceTypeSelectDropdown, value, position + 1); + dropdownWithPosition(addDeviceTypeSelectDropdown, value, position); } catch (IOException e) { logger.info(e.getMessage()); } }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
240-245: Consider trying standard click before JavaScript click.This method always uses JavaScript click (line 243), while other methods in the file (e.g.,
selectPolicyGroupDropdownat lines 183-188) try a standard click first and only fall back to JavaScript click on failure. For consistency and to prefer native interactions, consider tryingelement.click()first.♻️ Add standard click attempt with JS fallback
public PoliciesPage clickOnPoliciesTitle() { waitForUiToBeUnblocked(); WebElement policiesCard = waitForElementToBeVisible(policiesTitle); - ((JavascriptExecutor) driver).executeScript("arguments[0].click();", policiesCard); + try { + policiesCard.click(); + } catch (WebDriverException e) { + ((JavascriptExecutor) driver).executeScript("arguments[0].click();", policiesCard); + } return new PoliciesPage(driver); }uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (1)
34-34: Reconsider exception handling in test methods.Test methods now declare
throws Exceptionorthrows InterruptedException. While TestNG handles thrown exceptions by marking the test as failed, this approach can obscure the actual failure cause and make debugging harder. Consider:
- Handling checked exceptions within the test method with appropriate assertions
- Allowing only unchecked exceptions to propagate naturally
- Using TestNG's
expectedExceptionsattribute if testing exception scenariosThis improves test clarity and makes failures easier to diagnose.
Also applies to: 195-195, 277-277, 309-309
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AddDevicePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPolicyTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiCreationTest.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java
🧰 Additional context used
🧬 Code graph analysis (3)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/GlobalConstants.java (1)
GlobalConstants(3-1215)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/utility/LogUtil.java (1)
LogUtil(8-98)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiCreationTest.java (1)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (1)
BasePage(35-620)
🔇 Additional comments (12)
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPolicyTest.java (1)
545-546: BasePage-based navigation update looks consistent; confirm it’s the intended “back” semantics.
navigateBackDefaultButton()vsnavigateBack()can diverge (UI back vs browser back). Make sure this matches the page’s expected behavior in these flows.Also applies to: 557-557, 613-614
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ApiKeyPage.java (2)
448-462: Good: escaped XPath + boolean return for “option exists” selection.
This is a nice pattern for stabilizing dropdown selection without hard failures.
6-7: No changes required—TimeoutException wiring is correct.The
waitForElementToBeVisible(By)method in BasePage (line 324–327) usesWebDriverWait.until(), which throwsorg.openqa.selenium.TimeoutExceptiondirectly without wrapping. The catch block inselectPolicyNameDropdown(line 458) correctly catches this exception, and the import is accurate. No verification issue exists.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiCreationTest.java (1)
155-161: EnsurenavigateBack()reliably triggers and waits for the navigation-alert UI before asserting.
Right afterbasePage.navigateBack()you assert the navigation alert (Line 156). IfnavigateBack()doesn’t include UI sync, this can reintroduce flakiness.#!/bin/bash # Verify BasePage.navigateBack() implementation (sync/waits) and any alert handling. rg -n --hidden --glob '!.git/**' "void\\s+navigateBack\\s*\\(" -n rg -n --hidden --glob '!.git/**' "navigateBack\\(" uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.javauitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java (1)
519-535: TimeoutException assumptions are correct.Both
selectPolicyGroupDropdownIfPresentandisClonePolicyPopupTitleNotDisplayedcorrectly rely onTimeoutException. ThewaitForElementToBeVisible(By)method at line 324-327 ofBasePage.javausesWebDriverWait.until(ExpectedConditions.visibilityOfElementLocated(locator)), which throwsTimeoutExceptionwhen the element is not visible within the timeout period. Both methods properly catch this exception, and no other exceptions bypass the catch blocks.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/DashboardPage.java (1)
433-439: Verify loader selector specificity.The CSS selector on lines 435-436 is quite broad, combining multiple patterns:
"div.fixed.inset-0.z-50, div[class*='loading'], div[class*='spinner']". This may inadvertently match elements that aren't loaders. Consider making the selector more specific to the actual dashboard loader element to avoid false positives.uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/DeviceCreationTest.java (1)
243-245: Good refactoring to use explicit status check methods.The change from generic
isDeviceStatusDisplayed(...)to specific status methods likeisDeviceStatusApprovedDisplayed(...),isDeviceStatusRejectedDisplayed(...), andisDeviceStatusDeactivatedDisplayed(...)improves type safety and code clarity. This eliminates potential bugs from incorrect status string comparisons.Also applies to: 257-259, 297-299
uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/BasePage.java (5)
79-102: Well-implemented By-based click method.The new
clickOnElement(By locator)method follows the same resilient pattern as the WebElement-based version, with explicit waits and JavaScript click fallback. This consistency improves code maintainability.
373-395: Robust page refresh implementation.The enhanced
refreshThePage()method adds excellent defensive programming:
- Null and session validation before attempting refresh
- UI synchronization waits before and after the action
- Specific exception handling with meaningful error messages
This significantly improves test stability.
487-497: Excellent use of FluentWait for resilience.The refactored
isDisplayed(By locator)method uses FluentWait with:
- Automatic retry on
StaleElementReferenceExceptionandNoSuchElementException- Configurable polling interval
- Clean timeout handling
This pattern significantly improves resilience against timing issues in dynamic UIs.
580-590: Verify overlay selector specificity.The blocking overlay selector on lines 582-583 uses a broad pattern:
"div.fixed.inset-0.z-50.bg-opacity-35, div.fixed.inset-0.z-50.bg-black". While this may intentionally cover multiple overlay types, ensure it doesn't inadvertently match non-blocking UI elements. Consider documenting which specific overlays this targets or making the selector more specific if possible.
600-618: Well-implemented XPath escaping utility.The
escapeXPathText()method correctly handles all quote scenarios:
- Strings without quotes → wrap in single quotes
- Strings with single quotes only → wrap in double quotes
- Strings with both quote types → use XPath
concat()This prevents XPath injection vulnerabilities and ensures special characters are handled correctly.
| public void selectValidPolicyGroupForClone(String value) { | ||
| clickOnElement(clonePolicyGroupDropdown); | ||
| clickOnElement(clonePolicyGroupDropdownSearchInput); | ||
| enter(clonePolicyGroupDropdownSearchInput, value); | ||
| WebElement policyGroupOption = driver.findElement(By.xpath( | ||
| "//span[@id='policy_group_selector_option_name_1' and normalize-space(text())='" + value + "']")); | ||
| clickOnElement(policyGroupOption); | ||
| By optionLocator = By.xpath("//button[contains(@id,'policy_group_selector_option_button')" | ||
| + " and .//span[contains(@id,'policy_group_selector_option_name')" | ||
| + " and contains(normalize-space(.),'" + value + "')]]"); | ||
| WebElement option = waitForElementToBeVisible(optionLocator); | ||
| clickOnElement(option); | ||
| } |
There was a problem hiding this comment.
Fix XPath escaping in selectValidPolicyGroupForClone (currently raw string interpolation).
This can break when value contains quotes/special characters and is inconsistent with the rest of the file’s escapeXPathText(...) usage.
Proposed fix
public void selectValidPolicyGroupForClone(String value) {
clickOnElement(clonePolicyGroupDropdown);
enter(clonePolicyGroupDropdownSearchInput, value);
- By optionLocator = By.xpath("//button[contains(@id,'policy_group_selector_option_button')"
- + " and .//span[contains(@id,'policy_group_selector_option_name')"
- + " and contains(normalize-space(.),'" + value + "')]]");
+ String escapedValue = escapeXPathText(value);
+ By optionLocator = By.xpath("//button[contains(@id,'policy_group_selector_option_button')"
+ + " and .//span[contains(@id,'policy_group_selector_option_name')"
+ + " and contains(normalize-space(.)," + escapedValue + ")]]");
WebElement option = waitForElementToBeVisible(optionLocator);
clickOnElement(option);
}🤖 Prompt for AI Agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/AuthPolicyPage.java
around lines 1173 - 1181, The XPath in selectValidPolicyGroupForClone currently
interpolates raw value (risking breakage with quotes/special chars); update the
method to pass value through the existing escapeXPathText(...) helper before
building optionLocator and use that escaped string in the XPath construction so
optionLocator (and subsequent waitForElementToBeVisible/clickOnElement calls)
work reliably for values containing quotes or special characters.
| assertTrue(authPolicyPage.isClonePolicyPopupTitleNotDisplayed(), | ||
| GlobalConstants.isClonePolicyPopupTitleDisplayed); | ||
|
|
There was a problem hiding this comment.
Double-check clone popup assertion intent (NotDisplayed vs Displayed).
You now assert isClonePolicyPopupTitleNotDisplayed() but still pass GlobalConstants.isClonePolicyPopupTitleDisplayed as the message (slightly confusing for future debugging).
🤖 Prompt for AI Agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/AuthPolicyTest.java
around lines 637 - 639, The assertion uses isClonePolicyPopupTitleNotDisplayed()
but the failure message constant is
GlobalConstants.isClonePolicyPopupTitleDisplayed, creating a mismatch; either
replace the message with the matching
GlobalConstants.isClonePolicyPopupTitleNotDisplayed (or another appropriately
named "NotDisplayed" constant) or change the assertion to
isClonePolicyPopupTitleDisplayed() to reflect the intended check—update the
single line asserting the clone popup so the boolean method and the
GlobalConstants message refer to the same expected visibility.
| public void verifySbiAndDeviceAfterDeactivate() throws Exception { | ||
|
|
||
| dashboardpage = new DashboardPage(driver); | ||
| listOfSbiPage = new ListOfSbiPage(driver); | ||
| listOfDevicesPage = new ListOfDevicesPage(driver); | ||
| addDevicePage = new AddDevicePage(driver); | ||
| loginAsDeviceProvider(); | ||
|
|
||
| deviceProviderPage = dashboardpage.clickOnDeviceProviderServicesTitle(); | ||
|
|
||
| listOfSbiPage.clickOnViewDeviceButton(GlobalConstants.ALPHANUMERIC); | ||
|
|
||
| listOfDevicesPage.isDeviceStatusDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE, | ||
| listOfDevicesPage.isDeviceStatusRejectedDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE, | ||
| GlobalConstants.AUTOMATION_DEACTIVATING_5, GlobalConstants.AUTOMATION_DEACTIVATING_5, | ||
| GlobalConstants.REJECTED); | ||
| listOfDevicesPage.isDeviceStatusDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE, | ||
| listOfDevicesPage.isDeviceStatusDeactivatedDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE, | ||
| GlobalConstants.AUTOMATION_DEACTIVATING_4, GlobalConstants.AUTOMATION_DEACTIVATING_4, | ||
| GlobalConstants.DEACTIVATED); | ||
| listOfDevicesPage.isDeviceStatusDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE, | ||
| listOfDevicesPage.isDeviceStatusRejectedDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE, | ||
| GlobalConstants.AUTOMATION_DEACTIVATING_3, GlobalConstants.AUTOMATION_DEACTIVATING_3, | ||
| GlobalConstants.REJECTED); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "isDeviceStatusRejectedDisplayed\s*\(" uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javaRepository: mosip/partner-management-portal
Length of output: 194
🏁 Script executed:
rg -n "isDeviceStatusDeactivatedDisplayed\s*\(" uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javaRepository: mosip/partner-management-portal
Length of output: 183
🏁 Script executed:
sed -n '468,476p' uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javaRepository: mosip/partner-management-portal
Length of output: 561
🏁 Script executed:
sed -n '478,490p' uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/pages/ListOfDevicesPage.javaRepository: mosip/partner-management-portal
Length of output: 790
Assert the device status check results.
Both isDeviceStatusRejectedDisplayed() and isDeviceStatusDeactivatedDisplayed() return boolean without internal assertions. The test at lines 168–176 calls these methods but discards their return values, so failures silently pass. Wrap these calls with assertTrue() to ensure the test actually verifies the UI state:
assertTrue(listOfDevicesPage.isDeviceStatusRejectedDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE,
GlobalConstants.AUTOMATION_DEACTIVATING_5, GlobalConstants.AUTOMATION_DEACTIVATING_5,
GlobalConstants.REJECTED));
assertTrue(listOfDevicesPage.isDeviceStatusDeactivatedDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE,
GlobalConstants.AUTOMATION_DEACTIVATING_4, GlobalConstants.AUTOMATION_DEACTIVATING_4,
GlobalConstants.DEACTIVATED));
assertTrue(listOfDevicesPage.isDeviceStatusRejectedDisplayed(GlobalConstants.FACE, GlobalConstants.FULL_FACE,
GlobalConstants.AUTOMATION_DEACTIVATING_3, GlobalConstants.AUTOMATION_DEACTIVATING_3,
GlobalConstants.REJECTED));🤖 Prompt for AI Agents
In
@uitest-pmp-v2/src/main/java/io/mosip/testrig/pmpuiv2/testcase/SbiDeviceProviderTest.java
around lines 156 - 176, In verifySbiAndDeviceAfterDeactivate the calls to
isDeviceStatusRejectedDisplayed and isDeviceStatusDeactivatedDisplayed currently
discard their boolean results; wrap each call with an assertion (e.g.,
Assert.assertTrue or assertTrue) so the test fails when the UI check returns
false; update the three invocations of
listOfDevicesPage.isDeviceStatusRejectedDisplayed(...) and
listOfDevicesPage.isDeviceStatusDeactivatedDisplayed(...) to be asserted using
the test framework's assertTrue.
1 participant
MOSIP-43801: Fixed pms ui automation unstable report & test failures.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.