feat/TE-24462-Browser Permissions for Edge #93
Conversation
WalkthroughA new Maven project for Microsoft Edge site permissions has been established. The project introduces an Changes
Sequence DiagramsequenceDiagram
participant User
participant EdgePermissionAction
participant WebDriver
participant EdgeSettingsPage
User->>EdgePermissionAction: Execute with URL and Permission
EdgePermissionAction->>WebDriver: Open new tab
EdgePermissionAction->>EdgeSettingsPage: Navigate to settings
EdgePermissionAction->>EdgeSettingsPage: Set permission
EdgePermissionAction->>WebDriver: Refresh page
EdgePermissionAction->>WebDriver: Switch back to original tab
EdgePermissionAction-->>User: Return result
Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
| com.testsigma.sdk.Result result = com.testsigma.sdk.Result.SUCCESS; | ||
| String permissionName_ = permissionName.getValue().toString(); | ||
| String permissionValue_ = permissionValue.getValue().toString(); | ||
| String origWindowHandle = driver.getWindowHandle(); | ||
| List<String> origWindowsList = new ArrayList<>(driver.getWindowHandles()); | ||
| WebDriverWait wait = new WebDriverWait(driver, Duration.ofSeconds(10)); | ||
|
|
||
| try { | ||
| String edgeSettingsUrl = SETTINGS_URL_PREFIX + targetUrl.getValue().toString(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hard-coded sleeps with dynamic waits
Frequent 1500ms Thread.sleep() calls can increase the overall test run time and introduce flaky behavior. Prefer Selenium explicit waits (e.g., waiting for specific page states or elements) rather than fixed sleeps.
JIRA
https://testsigma.atlassian.net/browse/TE-24462
Fix
Browser Permissions for Edge
Summary by CodeRabbit
New Features
Chores