MOSIP-44172: Inji Web - Enable the parallel execution for the inji we… #498
Conversation
…b ui automation Signed-off-by: maheswaras <[email protected]>
WalkthroughThis pull request refactors the test framework to support configurable timeouts, mobile view testing, and concurrent test execution. It introduces ThreadLocal-based WebDriver management, feature-level locking for parallel execution, mobile-specific UI interactions, and updates test infrastructure to handle mobile viewport scenarios with enhanced timeout strategies. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant BasePage as BasePage<br/>(Config)
participant WebDriver as WebDriver
participant Element as UI Element
Test->>BasePage: Load explicit_timeout<br/>from ConfigManager
BasePage->>BasePage: Validate & set default<br/>(30 seconds)
Test->>WebDriver: Find element with<br/>configurable timeout
WebDriver->>Element: Locate element
alt Element not visible (mobile)
WebDriver->>WebDriver: Call safeScrollForMobileView()
loop maxAttempts (default 3)
WebDriver->>Element: Scroll into view
WebDriver->>Element: Check visibility
end
end
Element-->>WebDriver: Element located & visible
WebDriver-->>Test: Perform action (click/getText)
sequenceDiagram
participant Client as Test Thread
participant Runner as Runner<br/>(Parallel)
participant FeatureLock as Feature Locks<br/>(ConcurrentHashMap)
participant TestNG as TestNG Engine
Client->>TestNG: Execute Scenario (via `@DataProvider`)
TestNG->>Runner: runScenario(Pickle)
Runner->>Runner: acquireFeatureLockIfRequired()
activate Runner
Runner->>FeatureLock: Get/Create lock for feature
FeatureLock-->>Runner: Feature-level ReentrantLock
Runner->>FeatureLock: Lock.acquire()
Runner->>Client: Execute test steps
Client-->>Runner: Steps complete
Runner->>FeatureLock: Release feature lock
deactivate Runner
Runner-->>TestNG: Scenario result
TestNG-->>Client: Test complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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: 11
🤖 Fix all issues with AI agents
In `@ui-test/src/main/java/pages/HomePage.java`:
- Around line 346-348: The locator in clickOnHamburgerMenu uses a fragile XPath
"(//div[`@role`='button'])[1]" which can select the wrong button; update
clickOnHamburgerMenu to call clickOnElement with a stable locator (e.g.,
By.cssSelector or By.xpath that targets the known data-testid/hamburger id used
elsewhere) instead of the generic index-based XPath, keeping the method
signature and still using driver and clickOnElement for the actual click.
- Around line 147-153: The Back button locator in isBackButtonDisplayed is too
generic (By.xpath("//*[`@class`='cursor-pointer']")) and can produce false
positives; update isBackButtonDisplayed to use a more specific locator (prefer a
unique data-testid or a tighter CSS/XPath) that targets the actual back control
(e.g., data-testid="back-button" or a container-specific selector) so the method
reliably finds the intended element; keep method name isBackButtonDisplayed and
only change the By locator passed to isElementIsVisible.
In `@ui-test/src/main/java/pages/Loginpage.java`:
- Around line 69-75: In enterConfirmPasscode, the NoSuchElementException message
contains a replacement character ("�"); update the exception text in the
NoSuchElementException thrown in enterConfirmPasscode(String) to use a readable
normalized character (e.g., an apostrophe or plain ASCII apostrophe) or rephrase
the message to "passcode input field not found - cannot enter passcode." so it
no longer contains the replacement glyph.
- Around line 42-47: In Loginpage.enterPasscode, the error message contains a
replacement character; update the thrown NoSuchElementException message to
remove or replace the “�” with a proper ASCII character (e.g., an apostrophe or
hyphen) so the log reads something like "passcode input field not found - cannot
enter passcode." Locate enterPasscode in Loginpage and modify the string passed
to NoSuchElementException (the call site referencing isElementIsVisible and
BasePage.explicit_timeout) accordingly.
- Around line 252-262: The three mobile menu click methods
(clickOnProfileOptionMobileView, clickOnLogoutOptionMobileView,
clickOnFaqOptionMobileView) use unscoped XPaths that may match non-menu
elements; update their locators to scope the searches inside the hamburger/menu
container (e.g., find the menu root element first – the hamburger panel or nav
container – and then use a relative locator like ".//div[text()='Profile']" /
".//div[text()='Logout']" / ".//div[text()='FAQ']" or an XPath that prefixes the
menu container such as "//div[@<hamburger-identifier>]//div[text()=...]" so the
clicks only target items inside the mobile menu).
In `@ui-test/src/main/java/runnerfiles/Runner.java`:
- Around line 310-317: The updateFeaturesPath() method currently hard-codes the
non-Windows features path to /home/mosip; change it to set a project-relative
path when running from the IDE by consulting getRunType() (e.g., when
getRunType() == RunType.IDE use "src/test/resources/featurefiles/" or similar
relative path) and retain any absolute/CI-specific path only for non-IDE runs;
update the System.setProperty("cucumber.features", ...) logic in
updateFeaturesPath() to branch on getRunType() instead of OS alone so IDE
execution on Linux/macOS uses the project-relative feature folder.
- Around line 208-243: The parsed threadCount in scenarios() is never applied;
update startTestRunner() to read the same
ConfigManager.getproperty("threadCount") (or expose the parsed value) and call
testng.setDataProviderThreadCount(threadCount) (or set it on the XmlSuite via
setDataProviderThreadCount) before invoking runner.run(); this ensures the
DataProvider parallel thread pool uses the configured threadCount rather than
TestNG's default.
In `@ui-test/src/main/java/stepdefinitions/StepDefOIDCLogin.java`:
- Around line 970-973: The assertion in the method
user_verify_the_wallet_permanently_locked is inverted: it currently asserts the
permanent-lock message is NOT displayed. Update the assertion in
StepDefOIDCLogin.user_verify_the_wallet_permanently_locked to assert that
loginpage.isPermLockMsgDisplayed() is true (e.g., remove the negation) and keep
the existing failure message "Permanent lock message is not displayed" so the
step correctly fails when the lock message is missing.
- Around line 427-440: The mobile menu click steps (methods
clickOnProfileOptionMobileView, clickOnLogoutOptionMobileView,
clickOnFaqOptionMobileView used in StepDefOIDCLogin) are using generic text
XPaths that can match non-menu elements; update each method's locator to first
find the hamburger/menu container element (the same container used elsewhere)
and then find the descendant menu item by text (scope like
containerElement.findElement(By.xpath(".//div[text()='Profile']"))), ensuring
clicks are performed on the menu's descendant elements only; modify the three
methods' locators accordingly and keep the rest of the step definitions
unchanged.
In `@ui-test/src/main/java/utils/BaseTest.java`:
- Around line 48-54: The file contains duplicate import declarations for
java.util.Base64, org.openqa.selenium.Dimension,
io.mosip.testrig.apirig.utils.ConfigManager, and org.slf4j.Logger/LoggerFactory;
remove the redundant import lines so each of the symbols Dimension,
ConfigManager, Logger, and LoggerFactory is imported only once (keep the
original/earliest import and delete the later duplicate imports in
BaseTest.java).
In `@ui-test/src/test/resources/featurefiles/downloadMosipCredentials.feature`:
- Around line 97-124: The scenario outline title "Mosip Natonal Id by e-Signet"
is a duplicate; rename it to a distinct, descriptive name that includes the
mobile context (e.g., "Mosip National Id by e-Signet (Mobile)") and correct the
typo "Natonal"→"National" so the tag `@mobileview` clearly differentiates this
outline from the desktop one and avoids ambiguity in test reports and reruns.
🧹 Nitpick comments (20)
ui-test/src/main/resources/config/injiweb.properties (1)
8-11: Add brief unit annotations for new config keys.These values look fine, but a short inline note for units/intent will prevent accidental misconfiguration later.
♻️ Suggested clarification
+# dimensions=<width>,<height> in px for mobile viewports dimensions=390,844 +# maxAttempts = retry attempts for flaky actions maxAttempts=3 +# threadCount = parallel test threads threadCount=3 +# explicit_timeout in seconds for explicit waits explicit_timeout=30ui-test/src/main/java/pages/FaqPage.java (1)
30-38: Rename the down-arrow list variable for clarity.The variable name implies up arrows but holds down-arrow elements, which is confusing when reading test failures.
♻️ Suggested rename
- public int getDownArrowCount() { - List<WebElement> upArrowElements = driver.findElements(By.xpath("//*[`@data-testid`='Faq-Item-DownArrow']")); - return upArrowElements.size(); - } + public int getDownArrowCount() { + List<WebElement> downArrowElements = driver.findElements(By.xpath("//*[`@data-testid`='Faq-Item-DownArrow']")); + return downArrowElements.size(); + }ui-test/src/main/java/utils/ExtentReportManager.java (1)
16-17: GuardcreateTestagainst uninitialized reports and ensure ThreadLocal cleanup.In parallel runs, if any thread calls
createTestbeforeinitReport, this can NPE. Also ensureremoveTest()is called in teardown to prevent ThreadLocal retention when threads are reused.♻️ Suggested guard
public static synchronized void createTest(String testName) { + if (extent == null) { + initReport(); + } ExtentTest test = extent.createTest(testName); testThread.set(test); }Also applies to: 86-103
ui-test/src/test/resources/featurefiles/homePage_mobileview.feature (2)
33-33: Typo in parameter name: "Vailidty" should be "Validity".The parameter name has a typo that appears in multiple places (lines 33, 59, 72). Additionally, since there's only one example in the Examples table, a regular
Scenariowould be simpler than aScenario Outline.✏️ Suggested fix
- Then user enter validity for data share content "<Vailidty>" + Then user enter validity for data share content "<Validity>" ... Examples: - | Vailidty | + | Validity | | 3 |Also applies to: 43-45
26-27: Typo: "cridentials" should be "credentials".This typo appears in multiple step definitions throughout the file (lines 26-27, 52-53). Consider fixing for consistency and clarity.
ui-test/src/main/java/stepdefinitions/StepDefSunbirdCredentials.java (2)
208-220: Consider replacingThread.sleepwith explicit wait.While reducing from 5000ms to 2000ms is an improvement, using
Thread.sleepin test automation can cause flaky tests. Consider using an explicit wait condition (e.g., waiting for a specific element state) for more reliable synchronization.
284-302: PDF file not cleaned up after test.The PDF file is written to the current working directory but never deleted. This could accumulate files over multiple test runs.
🧹 Suggested fix to cleanup PDF file
`@Then`("User verify pdf is downloaded for Insurance") public String user_verify_pdf_is_downloaded_for_insurance() throws IOException { String pdfName = sunbirdCredentials.pdfNameInsurance; String base64EncodedFile = (String) baseTest.getJse().executeScript( "browserstack_executor: {\"action\": \"getFileContent\", \"arguments\": {\"fileName\": \"" + pdfName + "\"}}"); byte[] data = Base64.getDecoder().decode(base64EncodedFile); - try (OutputStream os = new FileOutputStream(pdfName)) { - os.write(data); - } - - File pdfFile = new File(System.getProperty("user.dir") + "/" + pdfName); + File pdfFile = new File(System.getProperty("user.dir") + "/" + pdfName); + try (OutputStream os = new FileOutputStream(pdfFile)) { + os.write(data); + } + try (PDDocument document = PDDocument.load(pdfFile)) { PDFTextStripper stripper = new PDFTextStripper(); return stripper.getText(document); + } finally { + if (pdfFile.exists()) { + pdfFile.delete(); + } } }ui-test/src/test/resources/featurefiles/login_oidc.feature (1)
143-156: Inconsistent indentation on new steps.The newly added steps (lines 143-156) have an extra leading space compared to other steps in the scenario. This doesn't affect functionality but reduces readability.
✏️ Fix indentation
- Then user click on back arrow button verify userhome page - Then user click on stored credentials button + Then user click on back arrow button verify userhome page + Then user click on stored credentials button Then user verifies cards are in horizontal order - Then user verify menu option on downloaded cards - Then user click menu option on downloaded cards + Then user verify menu option on downloaded cards + Then user click menu option on downloaded cardsui-test/src/main/java/base/BasePage.java (1)
46-69: Review scroll retry behavior and fix encoding issue.
- Line 68 contains "??" which appears to be a corrupted emoji character - consider using a plain text marker like "[ERROR]".
- The method silently continues execution even when the element is not visible after all retry attempts. This could mask test failures.
🔧 Suggested improvement
- logger.error("?? Element {} not visible after {} attempts.", locator, maxAttempts); + logger.error("[SCROLL_FAILED] Element {} not visible after {} attempts.", locator, maxAttempts);Consider whether this method should throw an exception or return a boolean to indicate success/failure, allowing callers to handle the failure appropriately.
ui-test/src/main/java/stepdefinitions/StepDef.java (1)
2055-2073: Extract magic number 30 to a named constant.The hardcoded value
30for alignment tolerance should be extracted to a named constant for better readability and maintainability.✏️ Suggested improvement
+ private static final int ALIGNMENT_TOLERANCE_PIXELS = 30; + `@Then`("User verify search and title are appeare in the same line") public void user_verify_search_title_appeare_diff_lines() { try { - assertTrue((homePage.getXCoordinateForSearch() - homePage.getXCoordinateForCredentialHeading()) <= 30, + assertTrue(Math.abs(homePage.getXCoordinateForSearch() - homePage.getXCoordinateForCredentialHeading()) <= ALIGNMENT_TOLERANCE_PIXELS, "Search and title are NOT aligned in the same line as expected.");Also note: The current check only validates if search is within 30px to the right of the heading. Consider using
Math.abs()to check alignment regardless of which element is further left.ui-test/src/main/java/utils/BaseTest.java (3)
57-60: Consolidate duplicate logger instances.The class declares two logger instances:
LOGGER(line 59) andlogger(line 86). Use only one to avoid confusion.🔧 Suggested fix
public class BaseTest { private static final ThreadLocal<WebDriver> driverThreadLocal = new ThreadLocal<>(); private static final ThreadLocal<JavascriptExecutor> jseThreadLocal = new ThreadLocal<>(); - private static final Logger LOGGER = LoggerFactory.getLogger(BaseTest.class); + private static final Logger logger = LoggerFactory.getLogger(BaseTest.class); ... - private static final Logger logger = LoggerFactory.getLogger(BaseTest.class);Then update all
LOGGER.references to uselogger.consistently.Also applies to: 86-86
152-168: Add validation for dimension parsing.The dimension parsing lacks error handling for:
- Invalid number format (non-numeric values)
- Negative or zero dimensions
🛡️ Suggested improvement
if (isMobileView()) { String dim = ConfigManager.getproperty("dimensions"); if (dim != null && dim.contains(",")) { String[] parts = dim.split(","); - int width = Integer.parseInt(parts[0].trim()); - int height = Integer.parseInt(parts[1].trim()); - getDriver().manage().window().setSize(new Dimension(width, height)); - logger.info("📱 Running in Mobile View ({}, {}) for scenario: {}", width, height, scenario.getName()); + try { + int width = Integer.parseInt(parts[0].trim()); + int height = Integer.parseInt(parts[1].trim()); + if (width > 0 && height > 0) { + getDriver().manage().window().setSize(new Dimension(width, height)); + logger.info("📱 Running in Mobile View ({}, {}) for scenario: {}", width, height, scenario.getName()); + } else { + logger.warn("⚠️ Invalid dimensions (must be positive), defaulting to maximize"); + getDriver().manage().window().maximize(); + } + } catch (NumberFormatException e) { + logger.warn("⚠️ Failed to parse dimensions '{}', defaulting to maximize", dim); + getDriver().manage().window().maximize(); + } } else {
281-285: Remove outdated comment.Line 283 contains a comment
// <-- add this method in ExtentReportManagerbut theremoveTest()method already exists inExtentReportManager(as seen in the relevant code snippets at line 104).🧹 Remove stale comment
ExtentReportManager.flushReport(); - ExtentReportManager.getTest(); // optional: just for reference - ExtentReportManager.removeTest(); // <-- add this method in ExtentReportManager + ExtentReportManager.removeTest(); clearSkip(); clearMobileView();ui-test/src/main/java/stepdefinitions/StepDefOIDCLogin.java (2)
96-101: Remove commented-out waits to keep steps clean.
Commented code adds noise and makes intent unclear. If the waits are no longer needed, remove them; if still required, switch to an explicit wait tied to the configurable timeout.🧹 Proposed cleanup
- // BasePage.waitForSeconds(driver, 10); driver.manage().addCookie(myCookie); - // BasePage.waitForSeconds(driver, 10);
236-239: Prefer a scoped locator for the mobile “Add Cards” button.
Index-based XPath(…)[2]is fragile in responsive layouts and can click the wrong element when DOM order changes. Consider scoping to a mobile container or a dedicateddata-testid.ui-test/src/main/java/pages/Loginpage.java (2)
463-466: Use the configurable timeout instead of a hard-coded 10s.
This keeps behavior consistent with other mobile waits.⏱️ Suggested change
- return isElementIsVisible(driver, By.xpath(xpath), 10); + return isElementIsVisible(driver, By.xpath(xpath), BasePage.explicit_timeout);
494-536: Consider reducing index-based locators for card actions.
Index selectors like(…)[1]or(…)[2]are brittle across layout changes. If possible, parameterize the card index or use a more specificdata-testid/container scope.ui-test/src/main/java/pages/HomePage.java (3)
30-42: Use a locale-stable lowercase conversion.
toLowerCase()without a locale can behave unexpectedly (e.g., Turkish locale). UseLocale.ROOT.🌐 Suggested change
+import java.util.Locale; ... - if (title.toLowerCase().contains("inji web")) { + if (title.toLowerCase(Locale.ROOT).contains("inji web")) {
130-132: Prefer a config-driven timeout instead of a hard-coded 60s.
This keeps behavior consistent across environments and reduces magic numbers.⏱️ Suggested change
- return isElementIsVisible(driver, By.xpath("//div[starts-with(`@data-testid`, 'ItemBox-Outer-Container-0-')]"), - 60); + return isElementIsVisible(driver, By.xpath("//div[starts-with(`@data-testid`, 'ItemBox-Outer-Container-0-')]"), + BasePage.explicit_timeout * 2);
179-181: Avoid index-based language selection.
(//button[@type='button'])[3]is brittle. Prefer adata-testidor select by language label to keep tests stable.
ui-test/src/test/resources/featurefiles/downloadMosipCredentials.feature
Show resolved
Hide resolved
…b ui automation Signed-off-by: maheswaras <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ui-test/src/main/java/runnerfiles/Runner.java`:
- Line 190: There's a typo in the TestNG output property key: update the
property set in Runner (the System.getProperties().setProperty call that
currently uses "testng.outpur.dir") to the correct key "testng.output.dir" so
TestNG recognizes the output directory; locate the setProperty invocation in the
Runner class and correct the string literal only.
In `@ui-test/testNgXmlFiles/TestNg.xml`:
- Line 3: Update the TestNG suite declaration to include
data-provider-thread-count="5" so the suite's DataProvider thread pool matches
the method-level thread-count; specifically modify the <suite name="Cucumber
Test Suite" ...> element (used alongside Runner.java which uses
`@DataProvider`(parallel = true)) to add data-provider-thread-count="5" to ensure
consistent thread management between the suite and the `@DataProvider`.
♻️ Duplicate comments (1)
ui-test/src/main/java/runnerfiles/Runner.java (1)
299-307: Non-Windows feature path is still hard-coded to/home/mosip.This concern was raised in a previous review. The hard-coded path will break IDE execution on Linux/macOS. Consider using a project-relative path when
getRunType()returns"IDE".
🧹 Nitpick comments (2)
ui-test/src/main/java/runnerfiles/Runner.java (2)
62-71: Minor race condition oncachedPathinitialization.Multiple threads could simultaneously compute and assign
cachedPathingetGlobalResourcePath(). Since all threads would compute the same value, this is benign but wasteful. Consider using double-checked locking orAtomicReferencefor cleaner thread-safe lazy initialization.♻️ Optional fix using synchronized block
- private static String cachedPath = null; + private static volatile String cachedPath = null; + private static final Object CACHE_LOCK = new Object();And in
getGlobalResourcePath():if (cachedPath != null) { return cachedPath; } synchronized (CACHE_LOCK) { if (cachedPath == null) { // compute path... cachedPath = path; } return cachedPath; }
224-231: Clarify purpose of scenario array manipulation.The loop creates a new array containing only the first two elements of each scenario. If
super.scenarios()already returnsObject[][] { PickleWrapper, FeatureWrapper }, this transformation appears redundant. If there are additional elements being filtered out, consider adding a comment explaining why.
…b ui automation Signed-off-by: maheswaras <[email protected]>
…b ui automation Signed-off-by: maheswaras <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ui-test/src/main/java/runnerfiles/Runner.java`:
- Around line 98-149: The main method currently unconditionally calls
System.exit(0) which masks test failures; change startTestRunner() to return an
int TestNG exit status (e.g., 0 for success, >0 for failures), update any
callers to collect/aggregate statuses using Math.max(...) when multiple runner
instances are involved, and replace System.exit(0) with System.exit(status)
using that returned/aggregated value; also ensure the catch block sets a
non‑zero status on exceptions before exiting and that otpListener.bTerminate is
still set prior to calling System.exit(status).
♻️ Duplicate comments (1)
ui-test/src/main/java/runnerfiles/Runner.java (1)
299-306: Hard-coded non‑Windows feature path hurts portability.Line 304–305 still forces
/home/mosip/featurefiles/, which breaks IDE runs on Linux/macOS. Consider using a project‑relative path whengetRunType()isIDE, and keep the absolute path only for CI/JAR use.🔧 Suggested adjustment
- if (os.contains("windows")) { - System.setProperty("cucumber.features", "src\\test\\resources\\featurefiles\\"); - } else { - System.setProperty("cucumber.features", "/home/mosip/featurefiles/"); - } + if (os.contains("windows") || getRunType().equalsIgnoreCase("IDE")) { + System.setProperty( + "cucumber.features", + "src" + File.separator + "test" + File.separator + "resources" + File.separator + "featurefiles"); + } else { + System.setProperty("cucumber.features", "/home/mosip/featurefiles/"); + }
🧹 Nitpick comments (2)
ui-test/src/main/java/pages/Loginpage.java (1)
69-75: Prefer config-driven timeout over the hard-coded60.Line 70–73 hard-codes
60, while Line 43–45 already uses(BasePage.explicit_timeout) * 2. Keeping both aligned avoids surprise wait behavior across passcode flows.♻️ Suggested refactor
- 60)) { + (BasePage.explicit_timeout) * 2)) {ui-test/src/test/resources/featurefiles/mobileView.feature (1)
19-35: Optional: fix typos in step text and example headers for clarity.These won’t break if step defs match, but they make the suite harder to read/search (e.g., “langauge”, “cridentials”, “Vailidty”, “procced”). If you update these, remember to adjust step definitions accordingly.
✏️ Example cleanups (feature file only)
- And User verify that langauge button is displayed + And User verify that language button is displayed - And User verify sunbird cridentials button - And User click on sunbird cridentials button + And User verify sunbird credentials button + And User click on sunbird credentials button - Then user enter validity for data share content "<Vailidty>" - Then Use click on procced button + Then user enter validity for data share content "<Validity>" + Then User click on proceed button - | Vailidty | + | Validity |Also applies to: 44-45, 52-60, 71-73
…b ui automation Signed-off-by: maheswaras <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui-test/src/main/java/utils/BaseTest.java (1)
198-255: Synchronize shared test counters to prevent data corruption under parallel execution.
totalCount,passedCount,failedCount, andskippedCountare mutated inbeforeAll()andafterScenario()across concurrent threads without synchronization. The@DataProvider(parallel = true)annotation in Runner.java enables parallel scenario execution, making these non-atomic increments prone to lost updates. Additionally,pushReportsToS3()reads these counters unsynchronized to construct the report filename, resulting in inconsistent test counts and incorrect report names.Suggested fix using AtomicInteger
+import java.util.concurrent.atomic.AtomicInteger; ... -private static int passedCount = 0; -private static int failedCount = 0; -private static int totalCount = 0; -private static int skippedCount = 0; +private static final AtomicInteger passedCount = new AtomicInteger(); +private static final AtomicInteger failedCount = new AtomicInteger(); +private static final AtomicInteger totalCount = new AtomicInteger(); +private static final AtomicInteger skippedCount = new AtomicInteger(); ... - totalCount++; + totalCount.incrementAndGet(); ... - skippedCount++; + skippedCount.incrementAndGet(); ... - failedCount++; + failedCount.incrementAndGet(); ... - passedCount++; + passedCount.incrementAndGet(); ... - String newFileName = nameWithoutExt + "-T-" + totalCount + "-P-" + passedCount + "-F-" + failedCount + "-S-" - + skippedCount + ".html"; + String newFileName = nameWithoutExt + "-T-" + totalCount.get() + "-P-" + passedCount.get() + "-F-" + + failedCount.get() + "-S-" + skippedCount.get() + ".html";
🤖 Fix all issues with AI agents
In `@ui-test/src/main/java/utils/BaseTest.java`:
- Around line 206-241: The error-handling branches call
ExtentReportManager.getTest().warning(...) without null checks which can NPE and
abort afterScenario; update both places (the else branch that logs "Failed to
fetch BrowserStack session JSON..." and the catch block that logs "Failed to
fetch BrowserStack build/session info:") to first retrieve ExtentTest test =
ExtentReportManager.getTest(), and if test != null call test.warning(...),
otherwise call the class LOGGER (e.g., LOGGER.warn(...) or LOGGER.error(...)) as
a fallback so no NPE occurs and cleanup continues.
…b ui automation Signed-off-by: maheswaras <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ui-test/src/main/java/utils/BaseTest.java`:
- Around line 265-276: The static integer counters skippedCount, failedCount,
and passedCount in BaseTest are not thread-safe for parallel runs; replace their
types with java.util.concurrent.atomic.AtomicInteger (or LongAdder) and update
all increments in the block that checks isScenarioSkipped(), scenario.isFailed()
and the else branch to call incrementAndGet() (or increment()) instead of ++,
and read/report their values using get() where they are displayed; also add the
necessary import and update any other usages of these symbols to use the atomic
API.
- Around line 265-276: The ExtentReportManager.getTest() call can return null
(e.g., if createTest failed or thread-local was cleared), so before calling
.skip/.fail/.pass in the block that checks isScenarioSkipped() /
scenario.isFailed(), retrieve a local variable like ExtentTest test =
ExtentReportManager.getTest() and guard against null; if test is null, avoid
calling its methods and instead use a safe fallback (increment the
skippedCount/failedCount/passedCount as before and write a concise message to a
fallback logger or System.out including scenario.getName() and getSkipReason()),
otherwise call test.skip/test.fail/test.pass. Ensure references to
ExtentReportManager.getTest(), isScenarioSkipped(), and scenario.isFailed() are
used to locate the change.
- Around line 135-156: The current mobile-dimensions parsing in the block using
ConfigManager.getproperty("dimensions") and Integer.parseInt can throw and abort
scenarios; update the logic in the setMobileView/isMobileView branch to validate
that dim is non-null, contains a comma, and splits into exactly two parts, then
parse width/height inside a try-catch for NumberFormatException; on any
validation or parse failure call getDriver().manage().window().maximize(), log a
clear logger.warn including the malformed dim value and scenario.getName(), and
avoid letting exceptions propagate so scenarios continue running.
| public class BasePage { | ||
| private static final Logger logger = LoggerFactory.getLogger(BasePage.class); | ||
| public static int explicit_timeout; | ||
| static { |
There was a problem hiding this comment.
Fallbacks are not required for wait time, use the default time that comes from config properties
|
|
||
| public void clickOnElement(WebDriver driver, By locator) { | ||
| WebElement element = new WebDriverWait(driver, Duration.ofSeconds(30)) | ||
| WebElement element = new WebDriverWait(driver, Duration.ofSeconds((BasePage.explicit_timeout) * 2)) |
There was a problem hiding this comment.
Create a wait utill and call the methods here, also avoid multiplication of element waits
|
|
||
| public boolean isIssuerLogoDisplayed() { | ||
| return isElementIsVisible(driver, By.xpath("//*[@data-testid='ItemBox-Logo']")); | ||
| return isElementIsVisible(driver, By.xpath("//*[@data-testid='ItemBox-Logo']"), (BasePage.explicit_timeout)*2); |
There was a problem hiding this comment.
wait should be handled in basepage
| if (!isElementIsVisible(driver, | ||
| By.xpath( | ||
| "//div[@data-testid='confirm-passcode-container']//input[@type='password' and @maxlength='1']"), | ||
| 60)) { |
There was a problem hiding this comment.
don't pass the wait time in the page classes
| } | ||
|
|
||
| public boolean isDownloadedCardMenubuttonDisplayed() { | ||
| return isElementIsVisible(driver, By.xpath("(//button[@data-testid='icon-mini-view-card-menu'])[1]")); |
There was a problem hiding this comment.
test-id should be unique.. should not be duplicated if raise bug get updated as unique and update it in automation
| public Object[][] scenarios() { | ||
|
|
||
| Object[][] scenarios = super.scenarios(); | ||
| System.out.println("Number of scenarios provided: " + scenarios.length); |
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| loginpage.waituntilpagecompletelyloaded(); |
There was a problem hiding this comment.
instead try this while launching the browser
driver.manage().timeouts().pageLoadTimeout(Duration.ofSeconds(30));
| String basicAuth = "Basic " + Base64.getEncoder().encodeToString(auth.getBytes()); | ||
|
|
||
| HttpURLConnection conn = (HttpURLConnection) new URL(jsonUrl).openConnection(); | ||
| conn.setConnectTimeout(10_000); |
There was a problem hiding this comment.
why this is required
2 participants
…b ui automation
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.