MOSIP-43846 : Reporting Enhancements for Admin UI Automation #800
Conversation
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
WalkthroughAdds a centralized ExtentReports manager and TestNG listeners for per-test reporting and known‑issue skipping, extends Commons with descriptive overloads and wait/log utilities, updates BaseClass setup/teardown to integrate reporting and screenshots, updates many UI tests to use descriptive actions, and switches the Docker base image. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TestNG
participant KnownIssueListener
participant AdminTestListener
participant BaseClass
participant TestCase
participant Commons
participant AdminExtentReportManager
participant S3[(S3/MinIO)]
TestNG->>KnownIssueListener: beforeInvocation(method)
KnownIssueListener-->>TestNG: throws SkipException if listed
TestNG->>AdminTestListener: onStart(suite)
AdminTestListener->>AdminExtentReportManager: initReport()
TestNG->>BaseClass: setUp(testMethod)
BaseClass->>AdminExtentReportManager: createTest(testName)
TestNG->>TestCase: run `@Test`
TestCase->>Commons: performAction(locator/value, "description")
Commons->>AdminExtentReportManager: logStepWithLocator(description, locatorHtml)
alt failure
TestCase->>BaseClass: tearDown(result)
BaseClass->>AdminExtentReportManager: attachScreenshotFromBase64(base64, title)
BaseClass->>AdminExtentReportManager: incrementFailed()
else success
BaseClass->>AdminExtentReportManager: incrementPassed()
end
AdminTestListener->>AdminExtentReportManager: flushReport()
AdminExtentReportManager->>S3: pushReportToS3(reportFilePath) (optional)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
44-48: AdminTestListener is only registered for the programmatic suite pathHere the listener is added only when
ConfigManager.gettestcases()is non-empty. For runs that use XML suite files (elsebranch),AdminTestListenerwill not be attached unless the XML also declares it. If you want consistent reporting across both paths, consider also registering the listener in the XML-based flow (e.g., viarunner.addListener(new AdminTestListener())or equivalent XML configuration).uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
36-232: Extent report manager looks solid; only minor robustness tweaks are optionalThe lazy, synchronized initialization, ThreadLocal
ExtentTest, and git-info fallbacks are all well-structured and should behave correctly under normal and parallel TestNG runs. If you ever need to run these tests from different working directories or CI agents, you might consider (optionally) makingreportPathconfigurable and ensuring thetest-outputdirectory exists before creating theExtentSparkReporter, but that’s a nice-to-have rather than a blocker.uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DocumentCategoriesTest.java (1)
28-39: Consider consolidating name + description edits to avoid layered save flowsHere you call
Commons.edit(driver(), data + 1/2, By.id("name"), ...), and then separatelyCommons.enterondescriptionfollowed byCommons.create. SinceCommons.editalready encapsulates click‑ellipsis → edit → enter new value → create → confirm, layering another manualenter+createon top can be a bit opaque and may result in multiple save/confirm cycles for the same record.If the UI really requires updating both name and description in one pass, consider introducing a small helper (e.g.,
editCategoryNameAndDescription(...)) insideCommonsthat performs all the necessary field updates and a single save/confirm sequence. That would make the intent clearer and reduce coupling to the exact button/confirm sequence in the UI.uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/CenterTest.java (1)
38-42: Consider logging the exception details in the catch block.While Reporter.log captures that the zone dropdown is unavailable, logging the exception details could help diagnose unexpected failures.
Apply this diff to add exception logging:
try { Commons.dropdown(driver(), By.id("zone"), "Selected Zone"); } catch (Exception e) { Reporter.log("Zone dropdown not available", true); + Reporter.log("Exception: " + e.getMessage(), true); }uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (1)
32-35: Consider logging exception details for better diagnostics.The empty catch block silently swallows exceptions, which could make debugging harder if unexpected errors occur.
Apply this diff to add exception logging:
try { Commons.dropdown(driver(), By.id("zone"), "Selected Zone"); } catch (Exception e) { + Reporter.log("Zone dropdown not available or failed: " + e.getMessage(), true); }uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DynamicFieldTest.java (1)
24-25: Clarify string concatenation intent for edit values.
data + 1anddata + 2perform string concatenation (e.g.,"20251202051210"→"202512020512101"). This creates unique values but may be confusing. Consider using more explicit suffixes likedata + "_v1"or documenting the intent.- Commons.edit(driver(), data + 1, By.id("code"), "Editing Dynamic Field Code"); + Commons.edit(driver(), data + "_v1", By.id("code"), "Editing Dynamic Field Code");uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
444-460: Inconsistent sleep usage - use Commons.wait() instead of Thread.sleep().The
calendar(String date, String locale, String description)method usesThread.sleep(500)directly (lines 444, 449, 455, 460), while the othercalendaroverload usesCommons.wait(500). For consistency and proper exception handling, useCommons.wait()throughout.Commons.click(driver(), By.xpath("//*[@class='mat-datepicker-toggle']//button"), "Opened the datepicker"); - Thread.sleep(500); + wait(500); Commons.click(driver(), By.xpath("//*[@class='mat-calendar-arrow']"), "Expanded year/month selector in datepicker"); - Thread.sleep(500); + wait(500); String yearText = convertDigits(String.valueOf(year), locale); Commons.click(driver(), By.xpath("//*[normalize-space(text())='" + yearText + "']"), "Selected year: " + yearText); - Thread.sleep(500); + wait(500); List<WebElement> months = driver().findElements(By.xpath("//*[@class='mat-calendar-body-cell-content']")); months.get(month - 1).click(); - Thread.sleep(500); + wait(500);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BlockListTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/CenterTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/CenterTypeTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceSpecificationTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceTypesTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DocumentCategoriesTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DocumentTypes.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DynamicFieldTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/HolidaysTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineSpecificationTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTypesTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/TemplateTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java(6 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java(2 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/CenterTest.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseTestCaseFunc.java (1)
BaseTestCaseFunc(30-230)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DocumentCategoriesTest.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-198)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DocumentTypes.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-198)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/TemplateTest.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DynamicFieldTest.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BlockListTest.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
TestRunner(19-204)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Screenshot.java (1)
Screenshot(17-33)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTypesTest.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-198)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineSpecificationTest.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-198)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceSpecificationTest.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-198)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Screenshot.java (1)
Screenshot(17-33)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceTest.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(21-490)
🔇 Additional comments (19)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DocumentTypes.java (1)
10-37: CRUD flow unchanged; Commons logging integration looks goodThis test now delegates all interactions to the descriptive
Commonshelpers while preserving the existing CRUD sequence (create → filter → edit → activate → edit → deactivate). The step messages are clear and should render nicely in the new Extent report.uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTypesTest.java (1)
9-35: Machine types test is cleanly instrumented for reportingThe CRUD steps are unchanged and now consistently go through
Commonswith clear descriptions (navigation, create, filter, edits, activate/deactivate). This aligns with the new reporting pattern and doesn’t introduce functional risk.uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/TemplateTest.java (1)
9-40: Template CRUD test now provides detailed, step-level loggingThe test still follows the same CRUD sequence but now uses the descriptive
Commonsoverloads for clicks, inputs, and dropdown selections. This should improve traceability in the Extent report without changing behavior.uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/CenterTypeTest.java (1)
11-35: CenterType CRUD test is consistent and well-describedThe refactor to
Commonswith descriptive messages keeps the original CRUD logic intact while making each step explicit in the report (navigation, create, two edit passes, activate/deactivate). Looks good.uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/HolidaysTest.java (1)
13-37: LGTM! Enhanced logging improves test observability.The descriptive messages added to all Commons method calls provide clear, step-by-step documentation of test actions. This aligns well with the PR's objective of improving reporting for Admin UI automation.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceTypesTest.java (1)
13-39: LGTM! Descriptive messages enhance test traceability.The added descriptive messages provide clear context for each UI interaction, making test reports more informative and easier to debug.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceSpecificationTest.java (1)
13-38: LGTM! Clear and consistent logging enhancement.The descriptive messages follow a consistent pattern across the test suite and provide valuable context for each test step.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DeviceTest.java (1)
17-43: LGTM! Comprehensive logging for device CRUD operations.The descriptive messages cover all device management operations (create, filter, edit, activate, deactivate, decommission) with clear, actionable descriptions.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/CenterTest.java (1)
20-78: LGTM! Comprehensive center configuration with enhanced logging.The test now includes detailed logging for all center configuration steps, including hierarchy levels, service selections, and exceptional holiday handling. The defensive try-catch for the optional zone dropdown is a good practice.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineSpecificationTest.java (1)
13-38: LGTM! Clear machine specification test logging.The descriptive messages provide excellent traceability for machine specification CRUD operations.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (1)
16-53: LGTM! Comprehensive machine CRUD test with locale-aware calendar handling.The test includes detailed logging for all machine management operations, with proper locale handling for date selection. The defensive try-catch for the optional zone dropdown is appropriate.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java (1)
21-58: LGTM! Enhanced bulk upload flow with iterative operation testing.The refactored test now systematically validates all three operations (Insert, Update, Delete) through a clean loop structure. The enhanced logging provides clear context for each step, and the assertion improvements make test results more readable.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (2)
146-153: LGTM - Cleanup order is correct.The
finallyblock properly ensures driver cleanup and report flushing regardless of test outcome. The order (flush before remove) correctly persists data before clearing the ThreadLocal.
49-51: This review comment is based on code that does not exist in the current codebase. ThesetUp()method does not have aMethod testMethodparameter, does not callAdminExtentReportManager.initReport(), and does not contain the code snippet shown. Lines 49-51 actually contain OS detection logic for Docker setup, not report initialization. TheAdminExtentReportManagerclass is not imported or used anywhere in this file.Likely an incorrect or invalid review comment.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/DynamicFieldTest.java (1)
11-34: LGTM - Test enhanced with descriptive logging.The CRUD test flow is clear, and the descriptive messages provide good traceability in the ExtentReport. The action sequence (create → filter → edit → activate → edit → deactivate) covers the expected lifecycle operations.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BlockListTest.java (1)
12-43: LGTM - Test structure with descriptive logging is clear.Apart from the filter mismatch noted above, the test flow is well-structured with clear descriptive messages for each action. The second edit/filter cycle (lines 37-39) correctly matches the edited value.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (3)
33-46: LGTM - Helper methods for logging are well-implemented.The
logStepandgetLocatorhelper methods provide clean abstraction for logging. The null check ingetLocator(line 38) prevents NPE, and the locator formatting is readable.
48-69: LGTM - Filter methods with description delegation.The filter method correctly logs the description with filter value, and the no-argument overload properly delegates to the description-enabled version with a default message.
220-248: LGTM - Activate/Deactivate methods are well-structured.Both methods properly log the action description via
AdminExtentReportManager.logStep(), handle confirmation popups conditionally, and delegate from no-description overloads correctly.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BlockListTest.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
127-132: Incorrect fallback inenter()- JavaScript click instead of setValue.The generic exception handler uses
executor.executeScript("arguments[0].click();", ...)which clicks the element instead of entering text. This will leave the field empty on failure.} catch (Exception e) { Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) + "' width='900' height='450'/></p>"); + logger.error("Enter failed: " + e.getMessage()); JavascriptExecutor executor = (JavascriptExecutor) driver; - executor.executeScript("arguments[0].click();", driver.findElement(by)); + executor.executeScript("arguments[0].value = arguments[1];", driver.findElement(by), value); }uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
92-118: Login steps are logged before test entry is created.The
Commons.clickandCommons.entercalls for login (lines 92-102) execute beforeAdminExtentReportManager.createTest()(line 116). SincegetTest()returnsnullat that point, these login steps won't appear in the ExtentReport for the test.Move test creation earlier, right after
initReport():public void setUp(Method testMethod) throws Exception { AdminExtentReportManager.initReport(); + + String className = this.getClass().getSimpleName(); + String methodName = testMethod.getName(); + String lang = "unknown"; + try { + lang = ConfigManager.getloginlang(); + } catch (Exception ignored) { + } + String cleanName = className + " | " + methodName + " | Language: " + lang; + AdminExtentReportManager.createTest(cleanName); + AdminExtentReportManager.logStep("Setup started for " + cleanName); Reporter.log("BaseClass setup started", true); // ... rest of setup including login steps ... - - if (AdminExtentReportManager.getTest() == null) { - // ... test creation code ... - } + AdminExtentReportManager.logStep("Setup complete for " + cleanName); }uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
36-64: Duplicate test status reporting with BaseClass.tearDown.Both
AdminTestListener(here) andBaseClass.tearDownlog pass/fail/skip status and calltest.pass(),test.fail(), ortest.skip(). This will produce duplicate entries in the ExtentReport.Consolidate status reporting in one place. Since
BaseClass.tearDownalready handles screenshot capture on failure, consider:
- Keeping status logging only in
BaseClass.tearDownand removing it from the listener, or- Keeping it only in the listener and removing the status calls from
BaseClass.tearDownOption 2 is cleaner since listeners are the intended TestNG mechanism for status handling:
// In BaseClass.tearDown, remove the status logging: - } else if (result != null && result.getStatus() == ITestResult.SUCCESS) { - ExtentTest test = AdminExtentReportManager.getTest(); - if (test != null) { - test.pass("✅ Test Passed: " + result.getName()); - } - } else if (result != null && result.getStatus() == ITestResult.SKIP) { - ExtentTest test = AdminExtentReportManager.getTest(); - if (test != null) { - test.skip("⚠️ Test Skipped: " + result.getName()); - } - } + } // Keep only the FAILURE branch for screenshot capture, but remove the test.fail() calls
🧹 Nitpick comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (2)
58-64: Exception fallback infilter()only clicks element.The catch block's JS fallback clicks the
byelement, but at this point in the flow, the operation has already partially failed (Filter button clicked, data entered). The fallback doesn't complete the filter operation meaningfully.Consider logging the failure more explicitly and potentially retrying the full filter sequence, or simply propagating the exception after logging.
444-460: Inconsistent wait method usage.The locale-aware
calendarmethod usesThread.sleep(500)directly (lines 444, 449, 455, 460) while the othercalendaroverload usesCommons.wait(500). For consistency and proper exception handling, useCommons.wait(500)throughout.- Thread.sleep(500); + Commons.wait(500);uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
66-69: Consider removing per-test flush in BaseClass.
onFinishcorrectly flushes the report at suite completion. However,BaseClass.tearDownalso callsflushReport()after each test (line 157 in BaseClass). Flushing once at suite end is more efficient.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
81-81: Deprecated implicit wait API.
implicitlyWait(long, TimeUnit)is deprecated in Selenium 4. UseimplicitlyWait(Duration.ofSeconds(20))instead.+import java.time.Duration; ... - driver().manage().timeouts().implicitlyWait(20, TimeUnit.SECONDS); + driver().manage().timeouts().implicitlyWait(Duration.ofSeconds(20));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java(6 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Screenshot.java (1)
Screenshot(17-33)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-204)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)
🔇 Additional comments (8)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (4)
33-46: Well-structured logging helpers.The
logStepandgetLocatorhelpers cleanly abstract locator formatting and step logging. Good use of null-safe handling ingetLocator.
139-157: Dropdown method with first-item selection.The method correctly logs the step, handles the aria-owns attribute parsing, and selects the first option. The exception fallback pattern is consistent with other methods.
250-276: Edit workflow with enhanced logging.The
editmethod properly logs each step with descriptive messages. The assertion at line 256-257 validates the existing value differs from new data before editing.
220-248: Well-implemented deactivate/activate workflows.Both methods properly log steps with descriptive messages, handle conditional popups, and have clean overload delegation patterns.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (2)
19-22: Report initialization on suite start.The
onStartmethod properly initializes the ExtentReport at the beginning of the test context. This is the correct lifecycle hook for suite-level initialization.
24-34: Test creation with parameterized name support.Good handling of parameterized tests by appending parameter values to the test name for better report clarity.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (2)
141-151: Null checks added for SUCCESS and SKIP branches.The null guards for
AdminExtentReportManager.getTest()in the SUCCESS (lines 142-145) and SKIP (lines 147-150) branches address the past review concern about potential NPEs.Note: As mentioned in the AdminTestListener review, this status reporting duplicates the listener's behavior. Consider consolidating to one location.
152-159: Robust cleanup in finally block.The finally block ensures driver cleanup and test removal regardless of test outcome. The null check before quit is good defensive programming.
Consider removing
flushReport()here sinceAdminTestListener.onFinishhandles suite-level flushing, which is more efficient than per-test flushing.
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
127-132: Bug: JavaScript fallback clicks instead of entering text.The generic exception handler still uses
arguments[0].click()instead of setting the element's value. This was flagged in a previous review and should be addressed.} catch (Exception e) { Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) + "' width='900' height='450'/></p>"); + logger.error("Enter failed: " + e.getMessage()); JavascriptExecutor executor = (JavascriptExecutor) driver; - executor.executeScript("arguments[0].click();", driver.findElement(by)); + executor.executeScript("arguments[0].value = arguments[1];", driver.findElement(by), value); }uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
93-119: Login steps are logged before test entry is created.
Commons.clickandCommons.entercalls for login (lines 93-103) execute beforeAdminExtentReportManager.createTest()at line 117. SincegetTest()returnsnullat that point, these login steps won't appear in the ExtentReport. MovecreateTest()earlier, right afterinitReport().public void setUp(Method testMethod) throws Exception { AdminExtentReportManager.initReport(); + String className = this.getClass().getSimpleName(); + String methodName = testMethod.getName(); + String lang = "unknown"; + try { + lang = ConfigManager.getloginlang(); + } catch (Exception ignored) { + } + String cleanName = className + " | " + methodName + " | Language: " + lang; + AdminExtentReportManager.createTest(cleanName); + AdminExtentReportManager.logStep("Setup started for " + cleanName); + Reporter.log("BaseClass setup started", true); // ... driver setup ... Commons.click(driver(), By.xpath("//*[@id='kc-locale-dropdown']"), "Clicked on language dropdown"); // ... login steps ... - if (AdminExtentReportManager.getTest() == null) { - // ... test creation logic ... - } + AdminExtentReportManager.logStep("Setup complete"); }
🧹 Nitpick comments (5)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (4)
58-64: Questionable exception recovery: clicking filter input after failure.The catch block executes a JavaScript click on the filter input element (
by), which doesn't logically recover from a failed filter operation. This appears to be copy-pasted fallback code that may mask the actual failure rather than properly handling it. Consider logging the error and rethrowing, or implementing meaningful recovery logic.catch (Exception e) { Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) + "' width='900' height='450'/></p>"); - logger.info(e.getMessage()); - JavascriptExecutor executor = (JavascriptExecutor) driver; - executor.executeScript("arguments[0].click();", driver.findElement(by)); + logger.error("Filter operation failed: " + e.getMessage()); + throw new RuntimeException("Filter operation failed", e); }
171-175: Inconsistent error handling: missing screenshot capture.Unlike
dropdown(driver, by, description)at line 151, this overload and others (dropdowncenter,dropdownwithBy value) don't capture screenshots on failure. For consistent debugging, align error handling across all dropdown variants.
422-424: Minor inconsistency: month selection bypasses logging wrapper.Month selection uses raw
WebElement.click()instead ofCommons.click(), missing the step logging and error handling. Consider wrapping this for consistency:- List<WebElement> cli = driver().findElements(By.xpath("//*[@class='mat-calendar-body-cell-content']")); - cli.get(month - 1).click(); + List<WebElement> months = driver().findElements(By.xpath("//*[@class='mat-calendar-body-cell-content']")); + WebElement monthElement = months.get(month - 1); + monthElement.click(); + AdminExtentReportManager.logStep("Selected month index: " + month);
394-410: Fragile date parsing without format validation.The date parsing assumes
MM/DD/YYYYformat and uses complex substring operations. Malformed input could causeStringIndexOutOfBoundsExceptionor incorrect date selection. Consider usingDateTimeFormatterfor safer parsing:// Example safer approach: DateTimeFormatter formatter = DateTimeFormatter.ofPattern("MM/dd/yyyy"); LocalDate parsedDate = LocalDate.parse(date, formatter); int month = parsedDate.getMonthValue(); int day = parsedDate.getDayOfMonth(); int year = parsedDate.getYear();uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
8-8: Unused import:java.util.concurrent.TimeUnit.This import is no longer needed since
Durationis now used forimplicitlyWait()at line 82.-import java.util.concurrent.TimeUnit;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java(6 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Screenshot.java (1)
Screenshot(17-33)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
🔇 Additional comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (3)
33-46: LGTM! Clean helper methods for logging.The
logStepandgetLocatorhelpers provide clean abstractions for ExtentReport logging with formatted locator strings.
364-366: Previously flagged issue has been addressed.The
createRes(WebDriver)method now correctly delegates tocreateRes(driver, "Create Resource")instead ofcreate().
482-487: LGTM with note on existing technical debt.The comment on line 483 acknowledges the need for a proper wait implementation. The try-catch pattern for element visibility checks is appropriate.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
122-147: LGTM! Robust tearDown implementation.The tearDown method properly:
- Uses
alwaysRun = trueto ensure cleanup- Captures failure screenshots with null checks
- Cleans up driver in finally block
- Removes the test from ExtentReportManager
The previous NPE concern about SUCCESS/SKIP branches doesn't apply here since only FAILURE is handled (for screenshot capture), and the cleanup always runs.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
109-135: Bug: enter()/enterSensitive() fallback clicks the element instead of setting its valueIn both
enter(...)andenterSensitive(...), the genericcatch (Exception e)currently does:JavascriptExecutor executor = (JavascriptExecutor) driver; executor.executeScript("arguments[0].click();", driver.findElement(by));This executes a JS click on the element instead of setting the intended text, so if the normal
sendKeyspath fails, the field is left unchanged while the test continues. That’s a functional error in the recovery path, especially for fields where the default value can still let the flow proceed.You likely want to set the value via JS (and optionally fire change/input events), reusing the same
valueargument:- } catch (Exception e) { - Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) - + "' width='900' height='450'/></p>"); - JavascriptExecutor executor = (JavascriptExecutor) driver; - executor.executeScript("arguments[0].click();", driver.findElement(by)); - } + } catch (Exception e) { + Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) + + "' width='900' height='450'/></p>"); + logger.error("Enter failed on " + by + ": " + e.getMessage()); + JavascriptExecutor executor = (JavascriptExecutor) driver; + executor.executeScript( + "arguments[0].value = arguments[1];", + driver.findElement(by), + value); + }Apply the same pattern to
enterSensitive(...)so that the sensitive value is still reliably populated even when the WebDriver path fails.Also applies to: 137-159
🧹 Nitpick comments (7)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
21-21: Unused import:ExtentTest.This import is not directly used in the class.
AdminExtentReportManager.getTest()returns anExtentTest, but the result is never assigned to a local variable.-import com.aventstack.extentreports.ExtentTest;uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (6)
48-68: Filter helpers: behavior OK, but JS fallback target seems questionableThe overall structure (log step → click filter → enter text / dropdown → click apply) looks good, and the new descriptions will help in reports. However, in the generic
catchblocks you fall back toexecutor.executeScript("arguments[0].click();", driver.findElement(by));, which clicks the filter field/selector (by) rather than the “Filter” or “Apply” buttons that likely failed. That makes the recovery path a bit arbitrary and may not actually apply the filter; consider refining the fallback to target the specific control that failed (or simply rethrow after logging).Also applies to: 70-87
89-107: click(): generic catch and JS-click fallback are acceptable but broadThe enriched logging and screenshot-on-failure behavior are useful, and JS-based click is a reasonable fallback for WebDriver interaction issues. The only concern is the broad
catch (Exception e), which can swallow programming errors (e.g., NPE) and try to compensate with a JS click; if you ever debug flaky tests, it may help to narrow this to WebDriver-related exceptions or rethrow after logging for unexpected exception types.
161-200: Dropdown helpers are consistent; watch out for values containing quotesThe dropdown helpers nicely standardize logging and reuse
click(...), and the different overloads (index 0, text value,Byvalue, “center” variant) look internally consistent. One small edge case: indropdown(WebDriver, By, String, String)you build an XPath using the rawvalue:String val = "'" + value + "'"; By.xpath("//span[contains(text()," + val + ")]")If any option text ever contains a single quote, that XPath will break. If such values are possible in your domains, consider adding a small helper to escape quotes in
valuebefore building the XPath.Also applies to: 202-222, 224-239
405-413: getPreAppend(): consider logging instead of printStackTraceWrapping
ConfigManager.getpreappend()in atry/catchis fine, bute.printStackTrace()will dump to stdout/stderr instead of your structured logs. For consistency with the rest of this class, you may want to log the exception vialogger.error("Failed to get preappend", e);and possibly defaultpreappendto an empty string to avoid"null"prefixes.
415-455: Calendar helpers: improved, locale-aware path looks good; consider harmonizing implementationsBoth calendar helpers assume
dateis inMM/dd/yyyyand do minimal parsing. The new locale-aware overload (calendar(String date, String locale, String description)) is cleaner (no special-casing 10/20/30) and sensibly uses index-based month selection plusconvertDigitsfor year/day, which should work well for locales like Arabic.The older
calendar(String date, String description)still uses the more brittle substring/replace("0","")logic for month/day. If you plan to keep both, consider refactoring that first method to reuse the simpler parsing logic from the locale-aware variant (e.g., parse month/day/year the same way and delegate) to reduce divergence and reduce the risk of format-related bugs.Also applies to: 457-494
504-510: isElementDisplayed(): behavior OK; optional logging for unexpected failuresReturning
falseon any exception is often what you want in polling helpers like this, and the small sleep is fine for these tests. To aid debugging when the underlying issue is more serious (e.g., driver already quit), you might consider logging the exception at debug/trace once, or narrowing to WebDriver-related exceptions, instead of completely swallowing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java(6 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(21-233)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Screenshot.java (1)
Screenshot(17-33)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
🔇 Additional comments (7)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (5)
49-52: LGTM!The
Method testMethodparameter enables dynamic test naming, andinitReport()initializes the ExtentReports instance appropriately at the start of setup.
78-81: LGTM!Good modernization: using
Duration.ofSeconds(20)aligns with Selenium 4's API and avoids the deprecatedTimeUnit-based overload.
104-118: Verify test creation coordination withAdminTestListener.The conditional
if (AdminExtentReportManager.getTest() == null)suggests thatAdminTestListenermay already create the test beforesetUpruns. Confirm whether the listener consistently creates tests, as this determines if this block is necessary fallback logic or dead code. If not always created by the listener, login steps (lines 92-102) could execute without a test context.Additionally, the silent exception catch at line 111 could mask configuration issues with
getloginlang(). Consider logging the exception or making the fallback explicit.
100-102: VerifyCommons.enterSensitivemasks the password in logs.Good that sensitive input is routed through a dedicated method. Ensure the implementation does not log the actual value.
121-145: Robust tearDown implementation following TestNG best practices.Good practices:
alwaysRun = trueensures cleanup regardless of test outcome- Screenshot capture on FAILURE is wrapped in try-catch to prevent masking other failures
- Finally block guarantees driver cleanup and thread-local removal
AdminExtentReportManager.removeTest()called in finally ensures report cleanup regardless of exceptionThe design correctly uses
@AfterMethod(ITestResult)for result-dependent actions (failure screenshot capture) while delegating real-time result logging (SUCCESS/SKIP) toITestListenercallbacks orAdminExtentReportManager—this separation is consistent with TestNG best practices.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (2)
21-31: BaseClass extension and locator-aware logging look consistentExtending
BaseClassand centralizing logging throughappendDate,logStep, andgetLocatoris coherent, andgetLocator(By by)safely handlesnulland standard SeleniumBy.toString()formats. No issues from a correctness standpoint.Also applies to: 33-46
241-243: Verify double-cancel behavior in editCenter() — two distinct cancel buttons?The concern about identical locators for sequential cancel clicks is worth examining, but I cannot access the repository to verify the actual implementation of
editCenter()or confirm whether there are indeed two distinct cancel buttons in the UI flow.To resolve this, check:
- The actual
editCenter()method implementation in the file- Whether the UI contains two separate cancel buttons or if the same element is intentionally clicked twice
- If two distinct buttons exist, adjust the second click to use the appropriate index (
[2]) or alternative locator
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (2)
154-180: Consider addingvolatileto counter fields for visibility.While the increment methods are properly synchronized, the counter fields (
passedCount,failedCount,skippedCount) lack thevolatilekeyword, which means reads from getter methods may see stale values in multi-threaded execution. Since these are reporting metrics rather than critical state, the impact is low, but addingvolatilewould guarantee visibility across threads.Apply this diff at the field declarations (lines 35-37):
- private static int passedCount = 0; - private static int failedCount = 0; - private static int skippedCount = 0; + private static volatile int passedCount = 0; + private static volatile int failedCount = 0; + private static volatile int skippedCount = 0;
215-219: Enhance HTML escaping to include quotes.The
escapeHtmlmethod only escapes&,<, and>, but omits quotes ("and'). While not a security concern in this test-reporting context, incomplete escaping could cause malformed HTML if locator strings contain quotes. Adding quote escaping ensures robust report rendering.Apply this diff:
private static String escapeHtml(String s) { if (s == null) return ""; - return s.replace("&", "&").replace("<", "<").replace(">", ">"); + return s.replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace("\"", """) + .replace("'", "'"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java(1 hunks)uitest-admin/src/main/resources/testngFile/testng.xml(1 hunks)
🔇 Additional comments (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (2)
40-61: LGTM! Initialization logic is sound.The report initialization with lazy instantiation and synchronized access is correct for parallel test execution. The dynamic report naming with git details and timestamp is a good practice for traceability.
221-230: LGTM! Screenshot attachment is properly guarded.The null checks for both the test context and base64 data, combined with exception handling, make this a robust implementation.
uitest-admin/src/main/resources/testngFile/testng.xml (1)
29-29: LGTM! Listener integration is correct.Adding
AdminTestListenerto the TestNG listeners aligns with the new ExtentReports infrastructure. Placing it before theEmailableReportlistener ensures proper initialization order for the reporting lifecycle.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
uitest-admin/Dockerfile (1)
1-40: Unresolved critical issue: Alpine base image incompatible with chromium-driver and build dependencies.This comment mirrors the critical finding from the previous review. The Dockerfile still uses
eclipse-temurin:21-jre-alpinewithapk add chromium-chromedriver, which will fail because:
- Alpine's package repository does not provide
chromium-chromedriveras a standalone package- For a UI testing container requiring Selenium WebDriver automation, Alpine lacks the necessary browser automation toolchain
The prior recommendation stands: switch to a Debian-based image to preserve
apt-getcompatibility and ensurechromium-driveravailability.-FROM eclipse-temurin:21-jre-alpine +FROM eclipse-temurin:21-jre-jammyThen update the package installation to use
apt-getand correct Debian package names:-RUN apk -q update && \ - apk add --no-cache wget curl unzip xvfb libxi chromium chromium-chromedriver bash shadow && \ +RUN apt-get -qq update && \ + apt-get install -y --no-install-recommends wget curl unzip xvfb libxi6 chromium-browser chromium-chromedriver && \
🧹 Nitpick comments (1)
uitest-admin/Dockerfile (1)
42-50: Simplify redundant permission and ownership setup.The COPY instructions and subsequent permission fixes can be consolidated for clarity:
- Line 43:
--chmod=771is non-standard (typically 755 for executables).- Lines 45/49: JAR permissions are set after COPY; these can use
--chmodin the COPY instruction instead.- Line 50:
chown -Rafter COPY with--chownis redundant.Suggested refactor:
-COPY --chown=${container_user_uid}:${container_user} --chmod=771 ./entrypoint.sh ${work_dir}/entrypoint.sh -COPY --chown=${container_user}:${container_user} ./src/main/resources/ ${work_dir}/resources/ -COPY --chown=${container_user_uid}:${container_user} ./target/*.jar ${work_dir} - -# --- Fix permissions for entrypoint and JARs --- -RUN chmod +x ${work_dir}/entrypoint.sh && \ - find ${work_dir} -name "*.jar" -exec chmod 644 {} \; && \ - chown -R ${container_user}:${container_user} ${work_dir}; +COPY --chown=${container_user}:${container_user} --chmod=755 ./entrypoint.sh ${work_dir}/entrypoint.sh +COPY --chown=${container_user}:${container_user} --chmod=644 ./src/main/resources/ ${work_dir}/resources/ +COPY --chown=${container_user}:${container_user} --chmod=644 ./target/*.jar ${work_dir}(Note: Assumes Docker BuildKit is enabled; adjust as needed for your build environment.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uitest-admin/Dockerfile(1 hunks)
🔇 Additional comments (1)
uitest-admin/Dockerfile (1)
52-59: User/entrypoint setup is correct.The non-root user switching and entrypoint configuration are properly formatted for Alpine/Linux containers. Once the base image is updated to Debian (to resolve the critical chromium-driver issue), these lines will continue to work as intended.
Summary
The Dockerfile introduces critical incompatibilities with the Alpine base image that must be resolved before merge. The previous review identified this blocker, and it remains unaddressed in the current code:
- Alpine does not provide
chromium-chromedriveras a separate package, breaking WebDriver-dependent UI tests.- Package management mismatch: the file uses
apkcommands (Alpine), but references were made toapt-getconcepts from prior context.Action required: Switch to
eclipse-temurin:21-jre-jammy(or similar Debian-based variant) and adapt the package installation to useapt-getwith Debian package names. This is a prerequisite for the reporting/testing enhancements mentioned in the PR objectives.Secondary optimization available: consolidate redundant COPY and permission-fixing commands.
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
144-149: Incorrect fallback in enter() - JavaScript click instead of setValue.The generic exception handler uses
executor.executeScript("arguments[0].click();", ...)which clicks the element instead of entering text. This leaves the field empty on failure.Apply this diff to fix:
} catch (Exception e) { Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) + "' width='900' height='450'/></p>"); + logger.error("Enter failed: " + e.getMessage()); JavascriptExecutor executor = (JavascriptExecutor) driver; - executor.executeScript("arguments[0].click();", driver.findElement(by)); + executor.executeScript("arguments[0].value = arguments[1];", driver.findElement(by), value); }uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (2)
257-264: Fix SLF4J logging format strings - placeholders not replaced.Lines 258, 260, and 263 use
"... {} " + variablewhich concatenates instead of passing as parameter. The{}placeholder won't be replaced.if (isStoreSuccess) { - LOGGER.info("Admin Extent report successfully pushed to S3/MinIO: {} " + reportFile.getName()); + LOGGER.info("Admin Extent report successfully pushed to S3/MinIO: {}", reportFile.getName()); } else { - LOGGER.error("Failed to push Admin Extent report to S3/MinIO: { } " + reportFile.getName()); + LOGGER.error("Failed to push Admin Extent report to S3/MinIO: {}", reportFile.getName()); } } catch (Exception e) { - LOGGER.error("Exception while pushing Admin Extent report to S3/MinIO: {} " + e.getMessage()); + LOGGER.error("Exception while pushing Admin Extent report to S3/MinIO", e); }
102-105: Restore interrupt status when catching InterruptedException.The
InterruptedExceptionis caught but neither logged nor used to restore the thread's interrupt status.try { exit = process.waitFor(); } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + LOGGER.warn("Interrupted while waiting for git command", ie); }
🧹 Nitpick comments (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java (1)
29-34: Consider using if-else-if or switch for mutually exclusive conditions.The three
ifstatements are mutually exclusive based oncountvalue. Usingif-else-ifor aswitchwould be clearer and slightly more efficient.- if (count == 0) - Commons.dropdown(driver(), By.id("operation"), By.id("Insert"), "Selected Insert operation"); - if (count == 1) - Commons.dropdown(driver(), By.id("operation"), By.id("Update"), "Selected Update operation"); - if (count == 2) - Commons.dropdown(driver(), By.id("operation"), By.id("Delete"), "Selected Delete operation"); + switch (count) { + case 0: + Commons.dropdown(driver(), By.id("operation"), By.id("Insert"), "Selected Insert operation"); + break; + case 1: + Commons.dropdown(driver(), By.id("operation"), By.id("Update"), "Selected Update operation"); + break; + case 2: + Commons.dropdown(driver(), By.id("operation"), By.id("Delete"), "Selected Delete operation"); + break; + }uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (2)
77-83: Exception handler fallback may not recover correctly.The catch block uses JS click on the filter element
by, but the try block performs multiple operations (click Filter, enter data, click Apply). A single JS click won't properly recover the filter workflow.Consider logging the failure and rethrowing, or implementing a proper retry of the full sequence:
} catch (Exception e) { Reporter.log("<p><img src='data:image/png;base64," + Screenshot.ClickScreenshot(driver) + "' width='900' height='450'/></p>"); - logger.info(e.getMessage()); - JavascriptExecutor executor = (JavascriptExecutor) driver; - executor.executeScript("arguments[0].click();", driver.findElement(by)); + logger.error("Filter operation failed: " + e.getMessage()); + throw new RuntimeException("Filter operation failed", e); }
467-468: Inconsistent click usage - bypasses logging.The month selection uses direct
cli.get(month - 1).click()instead ofCommons.click(), so this action won't appear in the ExtentReport. Consider wrapping for consistency.List<WebElement> cli = driver().findElements(By.xpath("//*[@class='mat-calendar-body-cell-content']")); - cli.get(month - 1).click(); + WebElement monthElement = cli.get(month - 1); + AdminExtentReportManager.logStep("Selected month at index: " + (month - 1)); + monthElement.click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java(6 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(24-526)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
TestRunner(19-204)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Screenshot.java (1)
Screenshot(17-33)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-175)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(24-268)
🔇 Additional comments (8)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/BulkUploadTest.java (1)
21-57: Good adoption of descriptive logging pattern.The test correctly uses the new Commons overloads with descriptive messages for each UI action, enhancing report traceability. The explicit waits before element interactions and screenshot capture are properly implemented.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (2)
36-51: Good implementation of explicit wait utilities.The wait utilities properly use
WebDriverWaitwithDuration.ofSeconds(20)andExpectedConditions, replacing hardcoded waits. This addresses the previous review feedback about avoiding hard-coded waits.
408-410: Delegation fix verified.The
createRes(WebDriver)method now correctly delegates tocreateRes(driver, "Create Resource")instead ofcreate(), addressing the previous review feedback.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
44-72: Null guards properly implemented for test status methods.All three status handlers (
onTestSuccess,onTestFailure,onTestSkipped) now include null checks forgetTest(), addressing the previous review feedback about potential NPEs.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (2)
92-103: Login steps may not appear in ExtentReport.The login actions (lines 92-102) execute in
@BeforeMethod, which runs beforeAdminTestListener.onTestStartcreates the ExtentTest. SinceAdminExtentReportManager.getTest()returns null at this point, these descriptive log calls won't appear in the report.If login step visibility in reports is important, consider creating the test entry early in
setUp()instead of relying on the listener:public void setUp(Method testMethod) throws Exception { AdminExtentReportManager.initReport(); + + // Create test entry early so login steps are logged + String className = this.getClass().getSimpleName(); + String methodName = testMethod.getName(); + String lang = "unknown"; + try { lang = ConfigManager.getloginlang(); } catch (Exception ignored) {} + String testName = className + " | " + methodName + " | Language: " + lang; + AdminExtentReportManager.createTest(testName); + Reporter.log("BaseClass setup started", true);Note: This would require updating
AdminTestListener.onTestStartto check if a test already exists.
106-131: Well-structured tearDown with proper resource cleanup.The tearDown method correctly:
- Captures failure screenshots with proper null checks
- Uses try-finally to ensure driver quit and ThreadLocal cleanup
- Handles exceptions gracefully during screenshot capture
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (2)
40-76: Well-designed report initialization and thread-safe test management.The report manager correctly:
- Uses lazy initialization with double-checked locking for
initReport()- Manages per-thread ExtentTest instances via ThreadLocal
- Includes defensive guards throughout
182-230: Good logging utilities with proper null safety.The logging methods (
logStep,logStepWithLocator,logLocator,attachScreenshotFromBase64) include appropriate null checks for both the test context and input parameters, with fallback warnings when no test is active.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
83-111: Duplicate S3 upload persists.As noted in a previous review,
flushReport()at line 86 internally pushes the report to S3, and thenpushReportToS3(newReportName)at line 110 uploads again. This results in the report being uploaded twice (once with the original name fromflushReport, once with the renamed file).Consider modifying
flushReport()to only flush without S3 push, or skip the explicitpushReportToS3call here ifflushReport()already handles it.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (3)
97-112: Swallowed InterruptedException inrunCommandloses interrupt signal.
InterruptedExceptionis caught and ignored, so the thread’s interrupt status is cleared and callers cannot detect it. Best practice is to restore the interrupt flag and optionally log the interruption.Suggested change:
private static String runCommand(String... command) throws IOException { Process process = new ProcessBuilder(command).redirectErrorStream(true).start(); try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) { String line = reader.readLine(); int exit = 0; try { exit = process.waitFor(); } catch (InterruptedException ie) { - } + Thread.currentThread().interrupt(); + LOGGER.warn("Interrupted while waiting for command: {}", String.join(" ", command), ie); + } if (exit == 0 && line != null) { return line.trim(); } }
241-252: Avoid S3 upload fromflushReport()to prevent duplicate uploads.
flushReport()currently both flushes the Extent report and callspushReportToS3(reportPath). Based on the surrounding changes (AdminTestListener and EmailableReport), S3 upload is also triggered from the listener after the report is renamed, which can lead to duplicate uploads (and possibly different filenames in storage).Consider letting the listener own the upload step and keep
flushReport()focused on flushing only:public static synchronized void flushReport() { if (extent != null) { extent.flush(); LOGGER.info("Extent report flushed successfully."); - - try { - pushReportToS3(reportPath); - } catch (Exception e) { - LOGGER.error("Error while uploading report: ", e); - } } }Please double-check how
flushReport()andpushReportToS3()are invoked fromAdminTestListenerto ensure there is exactly one upload per final report file.
258-273: Fix SLF4J placeholder usage and log full exception in S3 upload.The log statements concatenate values into the message while also using
{}placeholders, so the placeholders are never populated. The exception log also only printse.getMessage()instead of the stack trace.Refactor to use proper SLF4J parameterized logging and include the exception:
public static synchronized void pushReportToS3(String reportFilePath) { if (ConfigManager.getPushReportsToS3().equalsIgnoreCase("yes")) { S3Adapter s3Adapter = new S3Adapter(); File reportFile = new File(reportFilePath); boolean isStoreSuccess = false; try { isStoreSuccess = s3Adapter.putObject(ConfigManager.getS3Account(), "Adminui", null, null, reportFile.getName(), reportFile); if (isStoreSuccess) { - LOGGER.info("Admin Extent report successfully pushed to S3/MinIO: {} " + reportFile.getName()); + LOGGER.info("Admin Extent report successfully pushed to S3/MinIO: {}", + reportFile.getName()); } else { - LOGGER.error("Failed to push Admin Extent report to S3/MinIO: { } " + reportFile.getName()); + LOGGER.error("Failed to push Admin Extent report to S3/MinIO: {}", + reportFile.getName()); } } catch (Exception e) { - LOGGER.error("Exception while pushing Admin Extent report to S3/MinIO: {} " + e.getMessage()); + LOGGER.error("Exception while pushing Admin Extent report to S3/MinIO for file {}", + reportFile.getName(), e); } } }
🧹 Nitpick comments (5)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
57-61: Consider addingKnownIssueListenerto the programmatic suite.When tests are specified via
ConfigManager.gettestcases(), onlyAdminTestListeneris added to the suite (line 61). However,KnownIssueListeneris only added in the else branch (line 153) for the testng.xml-based execution path. This inconsistency means known issues won't be skipped when running specific test cases programmatically.suite.addListener("io.mosip.testrig.adminui.utility.EmailableReport"); suite.addListener("io.mosip.testrig.adminui.utility.AdminTestListener"); + suite.addListener("io.mosip.testrig.adminui.utility.KnownIssueListener");uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
12-18: Remove unused imports.The following imports are not used in this class:
OutputType,TakesScreenshot,WebDriver, andBaseClass.-import org.openqa.selenium.OutputType; -import org.openqa.selenium.TakesScreenshot; -import org.openqa.selenium.WebDriver; - -import io.mosip.testrig.adminui.utility.BaseClass; import io.mosip.testrig.adminui.kernel.util.ConfigManager;uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
110-113: Remove unnecessarydelete()afterrenameTo().The
renameTo()method moves the file, so the original file no longer exists at the old path. Thedelete()call on line 112 is redundant and will likely returnfalsesince the file has already been moved.if (orignialReportFile.exists()) { if (orignialReportFile.renameTo(newReportFile)) { - orignialReportFile.delete(); logger.info("Report File re-named successfully!");
117-129: Remove dead code:isStoreSuccess2is always true.The variable
isStoreSuccess2is initialized totrueand never modified, making the conditionisStoreSuccess && isStoreSuccess2equivalent to justisStoreSuccess. The comment suggests this was placeholder code for a second upload that was never implemented.boolean isStoreSuccess = false; - boolean isStoreSuccess2 = true; // or remove this flag until a second upload is added try { isStoreSuccess = s3Adapter.putObject(ConfigManager.getS3Account(), "Adminui", null, null, newString, newReportFile); logger.info("isStoreSuccess:: " + isStoreSuccess); - - /* Need to figure how to handle EXTENT report handling */ - } catch (Exception e) { logger.error("error occured while pushing the object" + e.getMessage()); } - if (isStoreSuccess && isStoreSuccess2) { + if (isStoreSuccess) { logger.info("Pushed report to S3"); } else { logger.error("Failed while pushing file to S3"); }uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
35-38: Counter visibility could be made fully thread-safe (optional).
passedCount,failedCount,skippedCount, andknownIssueCountare incremented in synchronized methods but read in unsynchronized getters. In practice this is unlikely to hurt here, but readers could see slightly stale values under concurrent access.If you want fully consistent visibility with minimal change, either:
- Mark the counters
volatile, or- Make the getters synchronized as well.
Given the test-runner context this is a minor/optional refinement.
Also applies to: 155-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(13 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/KnownIssueListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(7 hunks)uitest-admin/src/main/resources/config/Known_Issues.txt(1 hunks)uitest-admin/src/main/resources/testngFile/testng.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- uitest-admin/src/main/resources/config/Known_Issues.txt
🧰 Additional context used
🧬 Code graph analysis (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-175)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(24-277)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseTestCaseFunc.java (1)
BaseTestCaseFunc(30-230)
🔇 Additional comments (6)
uitest-admin/src/main/resources/testngFile/testng.xml (1)
28-32: LGTM! Listener order is appropriate.The listener registration order is correct:
KnownIssueListener(IInvokedMethodListener) intercepts before test execution to skip known issues,AdminTestListener(ITestListener) manages the reporting lifecycle, andEmailableReport(IReporter) generates the final HTML report.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
38-48: LGTM! Known issues loading is well-implemented.The try-with-resources ensures proper resource cleanup, empty lines are correctly filtered, and the graceful fallback on missing/unreadable file is appropriate for an optional feature.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
44-81: LGTM! Null guards properly implemented.The null checks for
ExtentTestinonTestSuccess,onTestFailure, andonTestSkippedprevent NPEs ifcreateTestwasn't invoked. The known issue detection inonTestSkippedcorrectly integrates withTestRunner.knownIssues.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/KnownIssueListener.java (1)
9-27: LGTM! Clean implementation of known issue skipping.The listener correctly intercepts test methods before invocation, checks against the known issues list, and uses
SkipExceptionwhich is the standard TestNG mechanism for skipping tests. The guard at line 14 ensures only test methods are evaluated.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
639-648: LGTM! Known issue filtering is correctly implemented.The
getKnownIssueSubsetmethod properly filters test results based onTestRunner.knownIssuesand returns an immutable set viaSet.copyOf().
685-706: LGTM! TestResult properly separates known issues from skipped tests.The logic correctly extracts known issues from the raw skipped set, removes them to create
finalSkipped, and maintains separate counts and result lists. This ensures accurate categorization in the report.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
uitest-admin/Dockerfile (1)
2-2: Alpine migration attempted but core package availability concern unresolved—verify or revert to Debian.You've addressed the package manager by switching to
apk, but the previous critical review flagged thatchromium-driveris not available as a separate package in Alpine Linux. While Alpine's naming convention useschromium-chromedriver, this package must be verified to exist and provide WebDriver capabilities for Selenium UI testing. Without confirmation, the build will fail at runtime.Recommendation: Before proceeding, verify that
chromium-chromedriveris available in Alpine repositories and compatible with your Selenium version. If verification fails, revert to a Debian-based image (e.g.,eclipse-temurin:21-jre-jammy) to ensure compatibility with the broader ecosystem.Alpine Linux chromium-chromedriver package availability and Selenium WebDriver supportAlso applies to: 28-29
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (3)
31-31: Consider making the knownIssues list unmodifiable after initialization.The
knownIssueslist is exposed as a public static mutable field, which allows external code to modify it. Since this list is populated once during initialization and should remain constant throughout test execution, consider wrapping it withCollections.unmodifiableList()after loading the known issues.Apply this pattern in the main method after loading (around line 48):
} logger.info("Known Issues Loaded: " + knownIssues); + knownIssues = Collections.unmodifiableList(knownIssues); } catch (Exception e) {Don't forget to add the import:
import java.util.Collections;
81-124: Consider using else-if for mutually exclusive test selection.The current implementation uses independent
ifstatements, which checks all conditions even after a match is found. Since test names should be unique, usingelse ifwould be more efficient and convey the mutual exclusivity more clearly.Apply this pattern:
- if (test.equals("blocklistedwordsCRUD")) + if (test.equals("blocklistedwordsCRUD")) classes.add(blocklistedwordsCRUD); - - if (test.equals("bulkUploadCRUD")) + else if (test.equals("bulkUploadCRUD")) classes.add(bulkUploadCRUD); - - if (test.equals("centerCRUD")) + else if (test.equals("centerCRUD")) classes.add(centerCRUD); // ... continue for all test casesAlternatively, consider using a
Map<String, XmlClass>for cleaner lookup:Map<String, XmlClass> testMap = new HashMap<>(); testMap.put("blocklistedwordsCRUD", blocklistedwordsCRUD); testMap.put("bulkUploadCRUD", bulkUploadCRUD); // ... add all mappings for (String test : Scenarioname) { XmlClass testClass = testMap.get(test); if (testClass != null) { classes.add(testClass); } }
203-213: Method naming convention and spelling (optional improvement).The method name
GetKernalFilenamehas two minor issues:
- Capital 'G' violates Java naming conventions (should be
getKernelFilename)- "Kernal" is misspelled (should be "Kernel")
Since this method is referenced externally (e.g., in
BaseTestCaseFunc), renaming would require updates across the codebase.The logic could also be simplified:
-public static String GetKernalFilename() { - String path = System.getProperty("env.user"); - String kernalpath = null; - if (System.getProperty("env.user") == null || System.getProperty("env.user").equals("")) { - kernalpath = "Kernel.properties"; - - } else { - kernalpath = "Kernel_" + path + ".properties"; - } - return kernalpath; +public static String getKernelFilename() { + String envUser = System.getProperty("env.user"); + if (envUser == null || envUser.isEmpty()) { + return "Kernel.properties"; + } + return "Kernel_" + envUser + ".properties"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(7 hunks)uitest-admin/src/main/resources/config/Known_Issues.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-admin/src/main/resources/config/Known_Issues.txt
🧰 Additional context used
🧬 Code graph analysis (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseTestCaseFunc.java (1)
BaseTestCaseFunc(30-230)
🔇 Additional comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
4-5: LGTM!The imports are necessary for loading the Known Issues file and are used correctly.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java
Outdated
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
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)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
186-198:getResourcePath()IDE path should handle missingpath.config(and provide a safe fallback).
Ifpath.configis unset,user.dir + nullyields a bad path and downstream file loads (Known_Issues, testngFile, etc.) will fail.} else if (checkRunType().equalsIgnoreCase("IDE")) { - String path = System.getProperty("user.dir") + System.getProperty("path.config"); + String pathConfig = System.getProperty("path.config"); + if (pathConfig == null || pathConfig.trim().isEmpty()) { + // fallback to classpath-based location (previous behavior) + path = new File(TestRunner.class.getClassLoader().getResource("").getPath()).getAbsolutePath(); + } else { + path = System.getProperty("user.dir") + pathConfig; + } if (path.contains("test-classes")) path = path.replace("test-classes", "classes"); return path; }
♻️ Duplicate comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
143-160: Listener registration still differs between execution paths.
In the IDE/file-based path you only dorunner.addListener(new KnownIssueListener()), while the ConfigManager path addsAdminTestListener+EmailableReporttoo. If thetestngFile/*.xmlsuites don’t declare those listeners, reporting behavior will diverge.#!/bin/bash set -euo pipefail # 1) Do testng suite XMLs already include listeners? echo "Searching for listener declarations in suite XMLs..." fd -t d 'testngFile' . -0 | while IFS= read -r -d '' d; do echo "== dir: $d" fd -e xml . "$d" -0 | xargs -0 rg -n --no-heading 'listener|KnownIssueListener|AdminTestListener|EmailableReport' || true done # 2) Confirm only one programmatic listener is added in IDE path echo echo "Programmatic listener registrations:" rg -n --no-heading 'addListener\(' uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java
🧹 Nitpick comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (4)
29-36: MakeknownIssuesinitialization resilient + avoid exposing a mutable public list.
Right nowknownIssuesis loaded only inmain(...); any other entrypoint callingstartTestRunner()will skip loading. Also,public static List<String> knownIssuesis globally mutable.Consider: move loading into
startTestRunner()(idempotent), make the fieldfinal, and expose via an unmodifiable view / getter.- public static List<String> knownIssues = new ArrayList<>(); + public static final List<String> knownIssues = new ArrayList<>(); public static void startTestRunner() throws Exception { + loadKnownIssuesOnce(); File homeDir = null; TestNG runner = new TestNG(); ... } + private static volatile boolean knownIssuesLoaded = false; + private static void loadKnownIssuesOnce() { + if (knownIssuesLoaded) return; + knownIssuesLoaded = true; + try (BufferedReader br = new BufferedReader(new InputStreamReader( + new FileInputStream(getResourcePath() + "/config/Known_Issues.txt"), StandardCharsets.UTF_8))) { + String line; + while ((line = br.readLine()) != null) { + if (!line.trim().isEmpty()) knownIssues.add(line.trim()); + } + logger.info("Known Issues Loaded: " + knownIssues); + } catch (Exception e) { + logger.warn("Known_Issues.txt not found or unreadable: " + e.getMessage()); + } + }Also applies to: 42-53
85-132: Avoid the longifchain for scenario →XmlClassselection.
This is easy to drift/typo and hard to extend. A simple map (plus a warning for unknown scenario tokens) is lower-maintenance.- String[] Scenarioname = ConfigManager.gettestcases().split(","); - for (String test : Scenarioname) { - if (test.equals("blocklistedwordsCRUD")) classes.add(blocklistedwordsCRUD); - if (test.equals("bulkUploadCRUD")) classes.add(bulkUploadCRUD); - ... - } + Map<String, XmlClass> scenarioToClass = new HashMap<>(); + scenarioToClass.put("blocklistedwordsCRUD", blocklistedwordsCRUD); + scenarioToClass.put("bulkUploadCRUD", bulkUploadCRUD); + ... + for (String token : ConfigManager.gettestcases().split(",")) { + XmlClass clazz = scenarioToClass.get(token.trim()); + if (clazz != null) classes.add(clazz); + else logger.warn("Unknown testcase token: " + token); + }
164-168: Be cautious with globalSystem.getProperties().setProperty(...)for run-specific settings.
These are JVM-global and can leak across runs (especially if you ever execute multiple suites in one JVM). If TestNG supports setting these via its APIs (or suite params) in your version, prefer that; otherwise, at least keep these mutations tightly scoped/consistent.
209-219: Fix naming/typos and simplify null/blank handling inGetKernalFilename().
Kernal/kernalpathis likely meant to beKernel/kernelPath, and you can rely on the already-readpathvariable.- public static String GetKernalFilename() { - String path = System.getProperty("env.user"); - String kernalpath = null; - if (System.getProperty("env.user") == null || System.getProperty("env.user").equals("")) { - kernalpath = "Kernel.properties"; - } else { - kernalpath = "Kernel_" + path + ".properties"; - } - return kernalpath; - } + public static String getKernelFilename() { + String envUser = System.getProperty("env.user"); + if (envUser == null || envUser.trim().isEmpty()) return "Kernel.properties"; + return "Kernel_" + envUser.trim() + ".properties"; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(7 hunks)
🔇 Additional comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
62-83: Listener wiring in the ConfigManager path looks good.
RegisteringEmailableReport,AdminTestListener, andKnownIssueListenerat the suite level matches the PR’s reporting goals.
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
95-138: UseoutputDirectoryconsistently for rename and logging instead oftestng.output.dirThe rename + logging block mixes
outputDirectorywithSystem.getProperty("testng.output.dir")anduser.dir. This is brittle if the system property is unset or diverges from theoutputDirectorypassed by TestNG; you can end up logging/renaming paths that don’t match where the report was actually written.You already have
outputDirectoryfrom TestNG, so you can simplify and harden the logic by using it for both the original and renamed files and logging viagetAbsolutePath():- String reportDir = outputDirectory; // or System.getProperty("testng.output.dir") - File orignialReportFile = new File(reportDir, oldString); - logger.info("reportFile is::" + System.getProperty("user.dir") + "/" + System.getProperty("testng.output.dir") - + "/" + System.getProperty("emailable.report2.name")); - - File newReportFile = new File( - System.getProperty("user.dir") + "/" + System.getProperty("testng.output.dir") + "/" + newString); - logger.info("New reportFile is::" + System.getProperty("user.dir") + "/" - + System.getProperty("testng.output.dir") + "/" + newString); - - if (orignialReportFile.exists()) { - if (orignialReportFile.renameTo(newReportFile)) { - orignialReportFile.delete(); + File originalReportFile = new File(outputDirectory, oldString); + File newReportFile = new File(outputDirectory, newString); + + logger.info("reportFile is::" + originalReportFile.getAbsolutePath()); + logger.info("New reportFile is::" + newReportFile.getAbsolutePath()); + + if (originalReportFile.exists()) { + if (originalReportFile.renameTo(newReportFile)) { + originalReportFile.delete();This keeps the filesystem operations aligned with how the report is created and avoids dependence on an external system property.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
TestRunner(24-221)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
🔇 Additional comments (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
189-205: Known-issues column and styling integration look consistentThe additions for known-issues styling (
.known_issuesodd/even, orange backgrounds, etc.) and the new “# Known Issues” column in the suite summary and scenario summaries are wired consistently:
- The CSS class naming pattern (
known_issues+odd/even) matches thecssClassPrefixusage inwriteScenarioSummary.- Per-test known-issues counts are pulled via
testResult.getKnownIssueTestCount()and rendered with an appropriate attention color.- Scenario-level known-issue rows reuse the same summary machinery as other statuses.
No functional issues spotted here; the integration looks clean and consistent with existing patterns.
Also applies to: 261-263, 285-287, 361-363, 424-427
639-648: Known-issue partitioning from skipped tests is correct; just ensure name matching strategy is intentionalThe new logic to classify known-issue results is sound:
getKnownIssueSubsetfilters the skipped-set based onTestRunner.knownIssues.contains(r.getName()).- You then create
finalSkippedas a mutable copy ofrawSkippedandremoveAll(knownIssueTests), so known issues are excluded from standard skipped metrics.skippedTestResults/skippedTestCountare based onfinalSkipped, and known issues get their ownknownIssueTestResults/knownIssueTestCountand dedicated summary sections.One thing to double-check is the identifier you use for matching:
if (TestRunner.knownIssues.contains(r.getName())) { filtered.add(r); }
ITestResult.getName()is typically the test method name. If yourKnown_Issues.txtentries (and any other consumers ofTestRunner.knownIssues) are using a different format (e.g.,ClassName#methodNameor groups), this may not match as expected and known issues will stay classified as regular skipped tests.If the intent is to key by pure method name, this is fine; otherwise, consider aligning the match key (e.g., including class, parameters, or groups) across
KnownIssueListenerand this helper.Also applies to: 685-707, 808-815
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
262-288: Known-issues column in suite summary is wired correctlyThe new
# Known Issuesheader (Line [262]) and per-test calculation usinggetKnownIssueTestCount()(Lines [286–288]) are aligned with the known-issue tracking inTestResult. The conditional CSS class"num thich-orange-bg"will highlight suites that have any known issues.One thing to be aware of: the totals row (Lines [310–317]) still doesn’t aggregate known issues, so the last column only totals duration, not KIs. If you later decide to surface an overall “Total Known Issues” count in the table, you’ll need to extend that row as well.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (3)
241-246: flushReport() correctly avoids duplicate S3 uploads
flushReport()(Lines [241–246]) now only callsextent.flush()and logs success, with no S3 upload side‑effects. This avoids the duplicate upload issue that can arise if a separate listener (e.g.,AdminTestListener) handles pushing the renamed report to S3/MinIO at the end of the run.This separation of concerns is a good improvement.
97-112: HandleInterruptedExceptioninrunCommandinstead of silently swallowing itIn
runCommand(...)(Lines [97–112]),InterruptedExceptionis caught and ignored (Lines [103–106]):
- This clears the thread’s interrupted status and hides the fact that the git command was interrupted.
- In long‑running test runners, this can make cooperative shutdown or cancellation harder to reason about.
A minimal improvement is to restore the interrupt flag and log the interruption:
- int exit = 0; - try { - exit = process.waitFor(); - } catch (InterruptedException ie) { - } + int exit = 0; + try { + exit = process.waitFor(); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + LOGGER.warn("Interrupted while waiting for git command", ie); + }This keeps existing behavior for successful commands while preserving the interrupt signal for callers.
What is the recommended best practice in Java for handling InterruptedException when blocking on Process.waitFor() if the method cannot rethrow it?
252-269: Fix SLF4J placeholder usage and harden S3 push against null flagsThe S3 upload logic is generally fine, but there are a couple of issues:
- SLF4J formatting:
- Lines [261–267] concatenate the file name or exception message onto the format string, so the
{}placeholders are never used. The logs will literally contain{}instead of substituting arguments.- Null‑safety on the flag:
- Line [253] calls
ConfigManager.getPushReportsToS3().equalsIgnoreCase("yes"); if the config returnsnull, this willNullPointerException.You can address both with a small refactor:
- public static synchronized void pushReportToS3(String reportFilePath) { - if (ConfigManager.getPushReportsToS3().equalsIgnoreCase("yes")) { + public static synchronized void pushReportToS3(String reportFilePath) { + if ("yes".equalsIgnoreCase(ConfigManager.getPushReportsToS3())) { S3Adapter s3Adapter = new S3Adapter(); File reportFile = new File(reportFilePath); boolean isStoreSuccess = false; try { isStoreSuccess = s3Adapter.putObject(ConfigManager.getS3Account(), "Adminui", null, null, reportFile.getName(), reportFile); if (isStoreSuccess) { - LOGGER.info("Admin Extent report successfully pushed to S3/MinIO: {} " + reportFile.getName()); + LOGGER.info("Admin Extent report successfully pushed to S3/MinIO: {}", reportFile.getName()); } else { - LOGGER.error("Failed to push Admin Extent report to S3/MinIO: { } " + reportFile.getName()); + LOGGER.error("Failed to push Admin Extent report to S3/MinIO: {}", reportFile.getName()); } } catch (Exception e) { - LOGGER.error("Exception while pushing Admin Extent report to S3/MinIO: {} " + e.getMessage()); + LOGGER.error("Exception while pushing Admin Extent report to S3/MinIO", e); } } }This restores proper parameterized logging, ensures the push flag can be safely absent, and logs the full exception instead of just the message.
In SLF4J, what is the correct way to use `{}` placeholders when logging variables, and is it recommended to pass exceptions as a separate argument to capture the full stack trace?
🧹 Nitpick comments (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
190-205: CSS additions for status rows look fine; consider removing duplication and tidying class namesThe new CSS classes for passed/skipped/failed/ignored/known-issue rows and the specific background colors are coherent and match the
cssClassPrefixvalues used inwriteScenarioSummary().Minor cleanups you may want to defer:
.stripe td {background-color: #E6EBF9}is now printed twice (Lines [188] and [205]); one of them can be removed.- Class name
.thich-orange-bg(Lines [192], [287]) looks like a typo of "thick" — harmless but slightly confusing.Functionally this is fine as-is.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
155-189: Counters and total calculation are consistent with known‑issues supportThe four counters (
passedCount,failedCount,skippedCount,knownIssueCount) and their synchronized incrementers (Lines [155–169]) plus the simple accessors (Lines [171–189]) provide a clear central place to aggregate test outcomes, including known issues.Just note:
- These are static and never reset, so they accumulate across the JVM lifetime. If your TestNG suite runs multiple times in the same process, you may want a reset hook (e.g., at the start of a run) to avoid cross‑run leakage.
Otherwise, the arithmetic in
getTotalCount()(Line [187]) correctly includes known issues in the overall total.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java
🔇 Additional comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
362-363: Scenario summary for known issues and duration formatting looks good
- Adding the
"Scenarios — Known Issues"section (Lines [362–363]) reuses the same class/grouping mechanism as other states and hooks intogetKnownIssueTestResults(), which keeps the model consistent.- Switching to
String.format("%.2f", scenarioDurationMin)for scenario durations (Line [425]) yields a clear, consistent display in minutes without changing semantics.No functional concerns here.
Also applies to: 425-425
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (3)
24-40: ExtentReports initialization and per‑thread test management look appropriateThe overall structure — a single static
ExtentReportsinstance, report path with timestamp (Line [50]), dark‑themeExtentSparkReporter, andThreadLocal<ExtentTest>for per‑test logging (Lines [26–28], [64–69]) — is coherent and matches typical ExtentReports usage in parallel TestNG runs.
initReport()is synchronized and idempotent (Lines [41–62]), which prevents duplicate initialization under concurrency.- Including git branch/commit and environment in the report name/system info is a nice touch for traceability.
No functional issues spotted in this part.
Also applies to: 41-63
79-95: System info, git metadata, and environment extraction are reasonable and fail‑safe
addSystemInfo()(Lines [79–95]) only runs once, logs what it’s adding, and falls back gracefully ifConfigManager.getloginlang()throws.fetchGitDetails()andloadFromGitProperties()(Lines [114–140]) correctly prefer CLI data and fall back togit.properties, with clear logging when CLI fails or properties are missing.safeEnvName()(Lines [142–153]) strips protocol and extracts an env token from the admin portal URL, returning"unknown"only when needed.These pieces give you good context in the report while failing safely if git/env metadata can’t be resolved.
Also applies to: 114-140, 142-153
191-239: Logging helpers and screenshot attachment are straightforward and safeThe helper methods:
logStep(...),logStepWithLocator(...), andlogLocator(...)(Lines [191–222]) correctly guard against missing thread‑local tests and provide useful WARN logs in those cases.escapeHtml(...)(Lines [224–228]) ensures locator HTML is safely rendered inside<pre>tags.attachScreenshotFromBase64(...)(Lines [230–239]) only attempts to attach when both the current test and base64 string are non‑null and logs (without failing the test) if media attachment throws.These utilities are well‑scoped and should make test code cleaner without introducing runtime risk.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
634-643: Add null-safety guard aroundTestRunner.knownIssuesto prevent NPE.Line 638 directly accesses
TestRunner.knownIssues.contains(...)without checking for null. IfTestRunner.knownIssuesis ever uninitialized, the report generation will fail with aNullPointerException. This issue was flagged in a previous review but remains unaddressed.Apply this diff to add a null-safe guard:
private static Set<ITestResult> getKnownIssueSubset(Set<ITestResult> resultsSet) { List<ITestResult> filtered = new ArrayList<>(); - - for (ITestResult r : resultsSet) { - if (TestRunner.knownIssues.contains(r.getName())) { - filtered.add(r); + + if (TestRunner.knownIssues != null && !TestRunner.knownIssues.isEmpty()) { + for (ITestResult r : resultsSet) { + if (TestRunner.knownIssues.contains(r.getName())) { + filtered.add(r); + } } } return Set.copyOf(filtered);Based on past review comments, this ensures behavior is unchanged when
knownIssuesis populated but avoids hard failure if configuration is missing.
🧹 Nitpick comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
106-109: Remove redundantdelete()call after successfulrenameTo().
renameTo()moves the file atomically; if it succeeds (line 107 returnstrue),orignialReportFileno longer exists, so line 108'sdelete()is a no-op. This is harmless but confusing.Apply this diff to remove the redundant delete:
if (orignialReportFile.renameTo(newReportFile)) { - orignialReportFile.delete(); logger.info("Report File re-named successfully!");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(13 hunks)
🔇 Additional comments (6)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (6)
685-701: Known-issue separation logic is correct; ensure null-safety fix is applied.The logic properly separates known issues from skipped tests:
- Line 687 extracts known issues from raw skipped results
- Lines 688-689 remove known issues from the skipped set
- Lines 694, 698 use the cleaned
finalSkippedfor skipped test results and counts- Lines 700-701 track known issues separately
This ensures known issues are not double-counted as regular skipped tests. However, line 687 inherits the null-safety issue from
getKnownIssueSubset(see separate comment on lines 634-643).
803-809: LGTM: Clean API for known-issue data.The new public getters expose known-issue results and counts following standard Java Bean conventions, providing a clean interface for callers to access the enhanced reporting data.
71-92: LGTM: Report generation flow correctly resets and accumulates known issues.Line 72 properly resets
totalKnownIssuesat the start of each report generation, addressing the prior review concern about stale accumulation across multiple runs. The try-catch wrapper (lines 73-78) with early return prevents NPE if writer creation fails. Known-issue totals are correctly summed after the report is written (lines 88-92).
94-104: LGTM: Consistent path handling and correct total calculation.Line 94 correctly includes known issues in the total test case count (known issues are not double-counted because they're removed from skipped in the
TestResultconstructor). Lines 101-104 useoutputDirectoryconsistently for both original and renamed file paths, addressing the prior review concern about path divergence.
111-129: LGTM: S3 upload logic is clean and properly guarded.The S3 upload flow is config-driven (line 111) and properly wrapped in try-catch (lines 114-123). Line 113 correctly uses only
isStoreSuccess(the unusedisStoreSuccess2flag from the prior review has been removed), and error handling logs both success and failure cases appropriately.
184-199: LGTM: UI enhancements cohesively integrate known-issue tracking.The CSS styling (lines 184-199), suite summary column header (line 256), known-issue count display (lines 280-282), and scenario summary section (lines 356-357) all work together to present known-issue data consistently throughout the HTML report.
Also applies to: 256-256, 280-282, 356-357
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
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-admin/pom.xml (1)
32-42: Critical: Duplicatetestng.versionproperty definition.The
testng.versionproperty is defined twice with conflicting values:
- Line 32:
7.10.1- Line 42:
6.11Maven will use the last definition (6.11), which is problematic because:
- TestNG 6.11 is from 2017 and extremely outdated
- TestNG 6.11 is incompatible with Java 21 (specified as compiler target)
- TestNG 6.11 likely has compatibility issues with ExtentReports 5.1.2
- Missing critical bug fixes and security patches
🔎 Proposed fix
<properties> <guava.version>31.1-jre</guava.version> <testng.version>7.10.1</testng.version> <!-- automationtests version --> <wink-json4j-provider.version>1.1.2-incubating</wink-json4j-provider.version> <kernel.version>1.2.0.1</kernel.version> <zjsonpatch.version>0.4.7</zjsonpatch.version> <extentreports.aventstack.version>3.0.0</extentreports.aventstack.version> <extentreports.relevantcodes.version>2.41.2</extentreports.relevantcodes.version> <json-path.version>2.4.0</json-path.version> <maven.model.version>3.3.9</maven.model.version> <rest.assured.version>3.0.7</rest.assured.version> - <testng.version>6.11</testng.version> <zt.zip.version>1.13</zt.zip.version>
🧹 Nitpick comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (1)
184-198: Prefer semantic CSS class names for known issues styling.The CSS classes for known issues use styling-derived names (
thich-orange-bg,orange-bg) rather than semantic names. While functional, semantic names like.known-issue-bgor.known-issue-rowwould be more maintainable if the color scheme changes.🔎 Optional refactor for semantic naming
-.thich-orange-bg {background-color: #E6934B} +.known-issue-header {background-color: #E6934B} .green-bg {background-color: #6FD96F} .attn {background-color: #FF4C4C} .passedodd td {background-color: #6FD96F;} .passedeven td {background-color: #6FD96F;} .skippedodd td {background-color: #FFE7A6;} .skippedeven td {background-color: #FFE7A6;} .failedodd td {background-color: #FF4C4C;} .failedeven td {background-color: #FF4C4C;} .ignoredodd td {background-color: #808080;} .ignoredeven td {background-color: #808080;} .known_issuesodd td {background-color: #E6934B;} .known_issueseven td {background-color: #E6934B;}Then update line 281 to use the new class name.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
4-5: Remove unused FileReader import.The
FileReaderimport at line 5 is unused since the code properly usesFileInputStreamwithInputStreamReaderand UTF-8 charset (lines 44-45).🔎 Proposed cleanup
import java.io.BufferedReader; -import java.io.FileReader; import java.util.ArrayList;uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (2)
8-8: Unused import.
BaseTestCaseFuncis imported but not used in this test class.🔎 Proposed fix
-import io.mosip.testrig.adminui.utility.BaseTestCaseFunc;
43-57: Consider adding test assertions to verify expected outcomes.The extended CRUD workflow is well-structured with descriptive logging. However, the test lacks explicit assertions to verify that operations succeeded as expected (e.g., machine created, name updated, status changed).
While the current pattern (verification via no exceptions) is common, adding assertions would make test failures more informative and catch subtle issues:
// Example assertions that could be added: Commons.createRes(driver(), "Created Machine Record"); // Assert.assertTrue(Commons.isElementVisible(driver(), By.xpath("//success message"))); Commons.filter(driver(), By.id("name"), data, "Filtered by Machine Name"); // Assert.assertTrue(Commons.getFilteredResultCount(driver()) > 0); Commons.activate(driver(), "Activated Machine Record"); // Assert.assertEquals(Commons.getRecordStatus(driver()), "Active");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
uitest-admin/pom.xml(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java(13 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/KnownIssueListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(7 hunks)uitest-admin/src/main/resources/config/Known_Issues.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/KnownIssueListener.java
🧰 Additional context used
🧬 Code graph analysis (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-175)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(24-526)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminExtentReportManager.java (1)
AdminExtentReportManager(24-271)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
TestRunner(26-236)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/fw/util/AdminTestUtil.java (1)
AdminTestUtil(34-323)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseTestCaseFunc.java (1)
BaseTestCaseFunc(30-230)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
TestRunner(26-236)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
🔇 Additional comments (10)
uitest-admin/pom.xml (1)
78-78: The ExtentReports 5.1.2 version upgrade is valid and secure.Version 5.1.2 is the latest release with bug fixes for tag navigation and ID conversion, and no known security vulnerabilities were identified. This version is compatible with modern TestNG versions, making the upgrade from 5.0.9 safe to proceed.
uitest-admin/src/main/resources/config/Known_Issues.txt (1)
1-3: LGTM! Clear documentation format.The configuration file provides clear examples and documentation for the known issues format. The delimiter pattern (
------) matches the parsing logic in TestRunner.uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/EmailableReport.java (2)
634-643: Known issues separation logic is correct.The
getKnownIssueSubsethelper correctly filters skipped tests that match entries inTestRunner.knownIssues. The logic properly separates known issues from regular skipped tests in the TestResult constructor (lines 687-701), ensuring they are not double-counted.Note: The lack of null check on
TestRunner.knownIssuesat line 638 was previously raised and the maintainer chose to retain the current implementation for compatibility.
71-136: Report generation flow correctly integrates known issues.The implementation properly:
- Resets
totalKnownIssuesat the start of report generation (line 72)- Accumulates known issues in the total count (lines 88-92, 94)
- Renames the report file to include all metrics including KI (lines 96-116)
- Uses consistent path handling via
outputDirectory(lines 101-104)The S3 upload logic is clear and handles rename failures gracefully.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (2)
37-68: LGTM! Known issues loading is robust and well-structured.The implementation correctly:
- Declares the public static
knownIssuesmap for cross-class access- Uses UTF-8 encoding for consistent file reading across platforms (lines 44-45)
- Validates and filters lines appropriately (empty, comments, missing delimiter)
- Parses bugId and testCaseName safely with length validation (lines 55-61)
- Provides clear logging and graceful error handling (lines 64, 66-68)
The format matches the documented structure in Known_Issues.txt and integrates cleanly with both AdminTestListener and EmailableReport.
80-82: LGTM! Listener registration is now consistent across execution paths.Both the ConfigManager path (lines 81-82) and IDE path (line 174) now properly register the necessary listeners including
KnownIssueListener, ensuring known issues are handled uniformly regardless of execution mode.Also applies to: 174-174
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java (1)
91-119: The review comment is based on an incorrect premise. TheflushReport()method (lines 241-246) only callsextent.flush()to write the HTML file to disk—it does not internally callpushReportToS3(). The two methods are independent, andpushReportToS3()is called explicitly by the caller. Therefore, the report is not uploaded twice; it is flushed to disk with the original name, then uploaded to S3/MinIO with the renamed name as intended.Likely an incorrect or invalid review comment.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (3)
16-18: LGTM!Configuration values are properly loaded from ConfigManager using the appropriate getter methods.
28-34: LGTM!The calendar selection and dropdown interactions are properly implemented with descriptive messages for enhanced reporting.
41-41: LGTM!Registration center selection is appropriately added to the machine creation workflow.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (3)
36-36: Consider encapsulating theknownIssuesmap.The public mutable static field allows external modification. For test utilities, consider exposing it via a getter or making it unmodifiable after initialization:
private static Map<String, String> knownIssues = new HashMap<>(); public static Map<String, String> getKnownIssues() { return Collections.unmodifiableMap(knownIssues); }
99-145: Consider using a Map for test case lookup.The repeated
ifstatements could be simplified with a Map-based lookup, improving maintainability when adding new test cases. Additionally, unknown test names are silently ignored, which could mask configuration typos.🔎 Example refactor
Map<String, XmlClass> testClassMap = Map.ofEntries( Map.entry("blocklistedwordsCRUD", blocklistedwordsCRUD), Map.entry("bulkUploadCRUD", bulkUploadCRUD), // ... other entries ); for (String test : Scenarioname) { XmlClass xmlClass = testClassMap.get(test); if (xmlClass != null) { classes.add(xmlClass); } else { logger.warn("Unknown test case: " + test); } }
223-231: Minor: CacheSystem.getProperty("env.user")to avoid redundant calls.The method calls
System.getProperty("env.user")three times. Consider caching it in a local variable. Also note the method name contains a typo ("Kernal" → "Kernel"), but this appears to be pre-existing and matchesConfigManager.GetKernalFilename().🔎 Proposed fix
public static String GetKernalFilename() { String path = System.getProperty("env.user"); String kernalpath = null; - if (System.getProperty("env.user") == null || System.getProperty("env.user").equals("")) { + if (path == null || path.isEmpty()) { kernalpath = "Kernel.properties"; } else { kernalpath = "Kernel_" + path + ".properties"; } return kernalpath; }uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (1)
3-3: Consider broader logger usage for test progress tracking.The logger field is declared but only used once in the catch block. Since this PR focuses on reporting enhancements, consider adding logger statements at key test milestones (e.g., test start, before major sections like creation/edit/deactivation) to provide better traceability in logs independent of the ExtentReport flow.
💡 Example enhancement
@Test public void machineCRUD() throws Exception { + logger.info("Starting machineCRUD test"); String holidayDateCenter = ConfigManager.getholidayDateCenter(); String publicKey = ConfigManager.getpublicKey(); String signPublicKey = ConfigManager.getsignPublicKey(); Commons.click(driver(), By.id("admin/resources"), "Clicked on Resources menu"); // ... existing code ... Commons.createRes(driver(), "Created Machine Record"); + logger.info("Machine created successfully, proceeding to filter and edit"); // ... rest of test ... + logger.info("machineCRUD test completed successfully"); }Also applies to: 11-11, 37-37
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/AdminTestListener.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T13:30:35.392Z
Learnt from: BhuvanaShreeBS
Repo: mosip/admin-ui PR: 800
File: uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java:23-26
Timestamp: 2025-12-19T13:30:35.392Z
Learning: In uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java, the MAC address "1.2.3.4.5.6" and IP address "2.3.4.5.6" test values use non-standard formats intentionally to maintain existing behavior and compatibility with the test infrastructure.
Applied to files:
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java
🧬 Code graph analysis (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (3)
uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseClass.java (1)
BaseClass(27-175)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/Commons.java (1)
Commons(24-526)
🔇 Additional comments (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (3)
79-81: LGTM!Listener registration now consistently includes all three listeners (EmailableReport, AdminTestListener, KnownIssueListener) in the ConfigManager path.
178-181: LGTM!Fixed the typo in the property name (
testng.outpur.dir→testng.output.dir).
173-174: Remove this comment—testng.xml already includes all required listeners.The testng.xml file includes all three listeners in its
<listeners>section (KnownIssueListener, AdminTestListener, EmailableReport), so when the IDE execution path callsrunner.setTestSuites(suitefiles), all listeners are active. Both execution paths (ConfigManager and IDE) result in consistent listener registration.Likely an incorrect or invalid review comment.
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (1)
30-30: Verify dropdown method overload behavior and test reliability.The dropdown calls at lines 30, 35, and 40 use a 3-parameter signature (driver, By locator, description), yet the documented
Commons.dropdownmethod expects 4 parameters including a value to select. This indicates an overloaded version exists that auto-selects options.Verify:
- Whether the 3-parameter overload reliably selects the expected option for these dropdowns
- What selection strategy is used (first available, random, or another approach)
- Test stability if selection behavior differs from expectations in the automation environment
Also applies to: 35-35, 40-40
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java
Show resolved
Hide resolved
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Bhuvanashree B S <bhuvanashree.b@cyberpwn.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (1)
35-39: Exception handling improved; consider using debug-level logging.Good improvement catching
NoSuchElementExceptionspecifically instead of the broadException. This correctly distinguishes between expected cases (element not present) and unexpected failures.However, for an optional/expected scenario like this,
logger.debug()would be more appropriate thanlogger.info()to avoid cluttering standard log output.🔎 Optional refinement for log level
try { Commons.dropdown(driver(), By.id("zone"), "Selected Zone"); -} catch (NoSuchElementException e) { - logger.info("Zone dropdown not available"); +} catch (NoSuchElementException e) { + logger.debug("Zone dropdown not available in this test context"); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java(1 hunks)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-19T13:30:35.392Z
Learnt from: BhuvanaShreeBS
Repo: mosip/admin-ui PR: 800
File: uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java:23-26
Timestamp: 2025-12-19T13:30:35.392Z
Learning: In uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java, the MAC address "1.2.3.4.5.6" and IP address "2.3.4.5.6" test values use non-standard formats intentionally to maintain existing behavior and compatibility with the test infrastructure.
Applied to files:
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java
🧬 Code graph analysis (1)
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (4)
uitest-admin/src/main/java/io/mosip/testrig/adminui/dbaccess/DBManager.java (1)
DBManager(26-153)uitest-admin/src/main/java/io/mosip/testrig/adminui/fw/util/AdminTestUtil.java (1)
AdminTestUtil(34-323)uitest-admin/src/main/java/io/mosip/testrig/adminui/kernel/util/ConfigManager.java (1)
ConfigManager(13-680)uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/BaseTestCaseFunc.java (1)
BaseTestCaseFunc(30-230)
🔇 Additional comments (7)
uitest-admin/src/main/java/io/mosip/testrig/adminui/testcase/MachineTest.java (2)
3-3: LGTM! Logger initialization supports enhanced reporting.The Log4j logger is properly initialized and aligns well with the PR objective of enhanced ExtentReport logging capabilities.
Also applies to: 12-12
16-58: Excellent enhancement of test reporting with descriptive action messages.The comprehensive addition of descriptive messages to all test actions (navigation, input, selection, CRUD operations) directly achieves the PR objective of enhanced ExtentReport logging. The test flow is well-structured, covering the full machine lifecycle: create → filter → edit → activate → edit → deactivate → decommission.
The descriptive messages provide clear traceability for each action, which will significantly improve test reports and debugging capabilities.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java (5)
4-4: LGTM! Imports support UTF-8 file reading.The new imports properly support reading the Known Issues file with explicit UTF-8 encoding, addressing the charset concern from previous reviews.
Also applies to: 6-6, 8-8, 17-19
36-36: LGTM! Known Issues loading properly implemented.The implementation correctly addresses previous review comments:
- UTF-8 encoding via
InputStreamReaderwithStandardCharsets.UTF_8- Edge case handling with
split("------", 2)to handle test names containing the delimiter- Validation of trimmed parts before adding to map
- Appropriate exception handling with warning log for an optional feature
The public static map is thread-safe since it's populated once at startup and only read afterward.
Also applies to: 43-67
178-178: LGTM! Report configuration properly set.The output directory and report naming configuration correctly incorporate environment details and timestamp for unique report identification.
Also applies to: 180-181
223-233: LGTM! Minor formatting adjustments.The formatting changes maintain existing logic while improving code layout. The method correctly determines the kernel properties filename based on environment configuration.
79-81: KnownIssueListener is registered twice in the IDE execution path.The testng.xml file (lines 28-32) already declares all three listeners:
EmailableReport,AdminTestListener, andKnownIssueListener. When the IDE path loads this XML, all listeners are automatically registered. The programmatic addition at line 173 is therefore redundant forKnownIssueListener. Consider removing the programmatic registration to avoid duplicate listener invocation, or verify that duplicate registration is intentional.
uitest-admin/src/main/java/io/mosip/testrig/adminui/utility/TestRunner.java
Show resolved
Hide resolved
4 participants
Improved Admin UI Automation with Enhanced ExtentReport Logging
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.