INJIWEB:1609 refactor(ui-test): cleanup xpath selectors and window handling #483
Conversation
Signed-off-by: Lkanath Panda <[email protected]>
WalkthroughThis pull request updates XPath selectors across multiple UI test pages to use more specific element-tag-qualified expressions instead of generic data-testid queries. Additionally, window-switching logic in step definitions is improved to handle multi-window scenarios robustly, and timing-related waits in OIDC login are removed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui-test/src/main/java/pages/HomePage.java (1)
35-383: Excellent refactoring to use specific element tags in XPath selectors.The systematic replacement of generic XPath patterns with element-specific selectors (e.g.,
//button[@data-testid='...'],//p[@data-testid='...']) is a best practice that improves selector performance and clarity.However, address the following brittleness issues:
- Lines 198, 358, 362: Indexed XPath selectors
[1]and[2]are fragile and will break if the DOM structure changes. Consider using more stable selectors or adding fallback logic.- Line 113:
//button[text()='Go To Home']uses text content matching, which is brittle if the button text changes. Align with the data-testid pattern used throughout the file.Verify all locators function correctly by running the UI test suite.
🧹 Nitpick comments (3)
ui-test/src/main/java/stepdefinitions/StepDef.java (1)
1064-1084: Improved window handling logic.The iterator-based approach is more robust than array indexing because it:
- Explicitly identifies a window different from the current one
- Handles the case where a second window might not exist
- Doesn't rely on assumptions about window ordering
♻️ Optional: Extract duplicated window-switching logic
Both
user_open_new_tab(lines 1069-1083) andUserSwitchToAboutInjiTab(lines 1089-1103) contain identical window-switching logic. Consider extracting this into a helper method:private void switchToSecondWindow() { Set<String> allWindowHandles = baseTest.getDriver().getWindowHandles(); String currentWindowHandle = baseTest.getDriver().getWindowHandle(); String secondWindowHandle = null; java.util.Iterator<String> iterator = allWindowHandles.iterator(); while (iterator.hasNext()) { String handle = iterator.next(); if (!handle.equals(currentWindowHandle)) { secondWindowHandle = handle; break; } } if (secondWindowHandle != null) { baseTest.getDriver().switchTo().window(secondWindowHandle); } }Then use it in both methods:
@Then("User open new tab") public void user_open_new_tab() { ((JavascriptExecutor) baseTest.getDriver()).executeScript("window.open('" + baseTest.url + "')"); switchToSecondWindow(); }ui-test/src/main/java/pages/HomePage.java (2)
198-198: Consider making the selector more specific to avoid positional indexing.The indexed XPath
(//div[@data-testid='HeaderTile-Text'])[1]suggests multiple elements share the samedata-testid. Position-based selectors are fragile and can break if the DOM structure changes or elements are reordered.If possible, use a more specific
data-testidvalue that uniquely identifies this element, or add additional context to the XPath (e.g., parent container constraints).
115-115: Usedata-testidattribute instead of text-based selector for consistency.The text-based selector
//button[text()='Go To Home']is fragile and inconsistent with the rest of the file's approach:
- Breaks with text changes or internationalization
- Sensitive to whitespace variations
- All other selectors in this file use
data-testidattributesConsider updating the HTML to add a
data-testidattribute and use it here for consistency and maintainability.♻️ Proposed refactor
If the button has or can have a
data-testidattribute (e.g.,data-testid='Go-To-Home-Button'), use:-return isElementIsVisible(driver, By.xpath("//button[text()='Go To Home']")); +return isElementIsVisible(driver, By.xpath("//button[@data-testid='Go-To-Home-Button']"));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui-test/src/main/java/pages/FaqPage.javaui-test/src/main/java/pages/HomePage.javaui-test/src/main/java/pages/Loginpage.javaui-test/src/main/java/pages/MosipCredentials.javaui-test/src/main/java/pages/SunbirdCredentials.javaui-test/src/main/java/stepdefinitions/StepDef.javaui-test/src/main/java/stepdefinitions/StepDefOIDCLogin.java
💤 Files with no reviewable changes (1)
- ui-test/src/main/java/stepdefinitions/StepDefOIDCLogin.java
🔇 Additional comments (8)
ui-test/src/main/java/pages/SunbirdCredentials.java (1)
30-30: LGTM! XPath selectors improved with specific element tags.The refactoring from wildcard selectors (
//*) to specificdivelements improves selector specificity, performance, and maintainability. These changes align with Selenium best practices.Also applies to: 71-71
ui-test/src/main/java/pages/Loginpage.java (1)
251-251: LGTM! Back arrow selectors correctly target button elements.Replacing the wildcard selector with explicit
buttontags ensures the correct element type is targeted and improves test stability.Also applies to: 279-279
ui-test/src/main/java/stepdefinitions/StepDef.java (1)
1087-1104: Consistent window handling with user_open_new_tab.The logic correctly mirrors the approach in
user_open_new_tab, ensuring consistent window switching behavior across test steps.ui-test/src/main/java/pages/MosipCredentials.java (3)
51-51: LGTM! Specific div selector for credential container.Replacing the wildcard with
divimproves selector precision while maintaining the same functionality for extracting the PDF name.
63-67: LGTM! Button selectors correctly updated.Both button IDs (
login_with_otpandget_otp) now explicitly targetbuttonelements, improving selector specificity and reducing the risk of matching incorrect elements.
75-75: LGTM! Paragraph selector for description text.Using
//pinstead of//*correctly targets the paragraph element containing the download result description.ui-test/src/main/java/pages/FaqPage.java (2)
22-22: LGTM! Semantic HTML tags for FAQ content.Using
pfor content text andh3for title text aligns with proper semantic HTML structure and improves selector reliability.Also applies to: 26-26
30-30: LGTM! Button selectors for FAQ arrow interactions.All arrow-related selectors now explicitly target
buttonelements, which is semantically correct for interactive controls and improves test stability.Also applies to: 34-34, 39-39, 44-44
iamlokanath
changed the title
1 participant
ui-test/src/main/java/stepdefinitions/StepDef.java
instead of if condition, used iterator for getting and storing the window handles in variables and used that
Cleaned up * and used the appropriate path for all the xpath elemtns in all pages.
avoided using the * in xpath and use appropriate tag
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.