feat/TE-24462-Browser Permissions for Edge#93
Conversation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
microsoft_edge_site_permissions/src/main/java/com/testsigma/addons/web/EdgePermissionAction.java (4)
16-21: Leverage descriptive actionText for better user clarity
The action text is sufficiently descriptive. However, consider including usage instructions or a more detailed summary in the description. This helps end-users understand any prerequisites or known constraints for the Edge browser.
23-30: Validate test data for null or empty inputs
When storing user-provided values, consider validating if the test data is null or empty to prevent NullPointerException or unexpected behavior when building theedge://settingsURL.+if (targetUrl == null || targetUrl.getValue() == null) { + throw new IllegalArgumentException("Target URL is required and cannot be null"); +}
86-101: Finer-grained exception messages for debugging
While the error handling is comprehensive, consider adding more contextual information (like which element was not found or timed out) to assist in diagnosing test failures quickly. Each exception message can reference the specific step or permission being set.
120-135: Use more robust locators or alternative fallback logic
The XPath relies on visible text matching for the permission value. If the UI language or text changes, this may break the action. Consider a fallback locator or a robust data attribute for better maintainability.microsoft_edge_site_permissions/pom.xml (1)
24-64: Unify test frameworks or confirm necessity
Both JUnit (lines 37-41) and TestNG (lines 43-46) dependencies are present. If there’s no requirement for them simultaneously, consider standardizing on a single framework to reduce project complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
microsoft_edge_site_permissions/pom.xml(1 hunks)microsoft_edge_site_permissions/src/main/java/com/testsigma/addons/web/EdgePermissionAction.java(1 hunks)microsoft_edge_site_permissions/src/main/resources/testsigma-sdk.properties(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- microsoft_edge_site_permissions/src/main/resources/testsigma-sdk.properties
🔇 Additional comments (3)
microsoft_edge_site_permissions/src/main/java/com/testsigma/addons/web/EdgePermissionAction.java (2)
3-5: Validate imports and inheritance
The usage ofWebActionfrom the Testsigma SDK is appropriate for custom web actions. The imports appear relevant. Ensure no unwanted or unused libraries are being pulled in.
48-54: Prevent potential new tab handle race
Right after switching to a new window, there's a possibility of a race condition if the tab isn't fully opened before retrieving its handle. The existing wait might mitigate this, but consider handling it with a well-defined explicit wait for the new tab to be recognized.microsoft_edge_site_permissions/pom.xml (1)
66-96: Validate plugin versions and library compatibility
The maven-shade-plugin and maven-source-plugin configurations look consistent. Double-check that the Selenium 4.14.1 and Testsigma SDK 1.2.18 versions match any known compatibility requirements for the Edge testing scenario.
JIRA
https://testsigma.atlassian.net/browse/TE-24462
Fix
Browser Permissions for Edge