Update to Selenium 4.33.0 +semver:feature #150
Conversation
mialeska
added
enhancement
WalkthroughThe Maven dependency Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (1)
pom.xml (1)
84-86: Verify dependency version bump aligns with Selenium upgrade plan.The PR title refers to upgrading to Selenium 4.33.0, yet the
aquality-selenium-coredependency was only bumped from 4.5.0 to 4.6.0. Please confirm that:
- Version 4.6.0 of
aquality-selenium-coreindeed pulls in Selenium 4.33.0 under the hood.- No API changes in the new core version require code adjustments or additional tests.
Additionally, for better maintainability, consider moving
<version>4.6.0</version>into a Maven property (e.g.${aquality.selenium.core.version}) so it can be reused consistently across modules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pom.xml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: aquality-automation.aquality-selenium-java (Run tests)
- GitHub Check: aquality-automation.aquality-selenium-java (Analyse code with SonarQube)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/aquality/selenium/browser/devtools/DevToolsHandling.java(1 hunks)src/main/java/aquality/selenium/browser/devtools/EmulationHandling.java(4 hunks)src/main/java/aquality/selenium/browser/devtools/JavaScriptHandling.java(2 hunks)src/main/java/aquality/selenium/browser/devtools/NetworkHandling.java(5 hunks)src/test/java/tests/usecases/devtools/DeviceEmulationTest.java(1 hunks)src/test/java/tests/usecases/devtools/NetworkSpeedEmulationTest.java(1 hunks)src/test/java/tests/usecases/devtools/OverrideUserAgentTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/test/java/tests/usecases/devtools/NetworkSpeedEmulationTest.java
- src/test/java/tests/usecases/devtools/DeviceEmulationTest.java
- src/test/java/tests/usecases/devtools/OverrideUserAgentTest.java
- src/main/java/aquality/selenium/browser/devtools/JavaScriptHandling.java
- src/main/java/aquality/selenium/browser/devtools/DevToolsHandling.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: aquality-automation.aquality-selenium-java (Run tests)
- GitHub Check: aquality-automation.aquality-selenium-java (Analyse code with SonarQube)
🔇 Additional comments (9)
src/main/java/aquality/selenium/browser/devtools/NetworkHandling.java (6)
11-11: LGTM! DevTools import updated correctly.The import statement has been properly updated from the older version to v137, maintaining consistency with the Selenium 4.33.0 upgrade.
25-25: LGTM! Static import updated consistently.The static import correctly references the v137 network class, maintaining consistency with the updated import on line 11.
29-29: LGTM! Javadoc reference updated correctly.The documentation reference has been properly updated to point to the v137 version, ensuring documentation accuracy.
202-202: LGTM! Method call updated to use v137 API.The
getResponseBodymethod call correctly uses the v137 network class, maintaining consistency with the version upgrade.
343-343: LGTM! Additional Optional parameters added consistently.The method call maintains the same pattern as line 326-327, correctly adding three additional
Optional.empty()parameters while preserving the existing connectionType parameter logic.
326-327:❓ Verification inconclusive
Verify the additional Optional parameters are correct.
The method signature has been updated with three additional
Optional.empty()parameters. While this appears to follow the v137 API changes, please ensure these parameters correspond to the correct method signature.
🏁 Script executed:
#!/bin/bash # Description: Verify the emulateNetworkConditions method signature in v137 # Expected: Find the method signature with the additional optional parameters ast-grep --pattern 'emulateNetworkConditions($_, $_, $_, $_, $_, $_, $_, $_)'Length of output: 807
🏁 Script executed:
#!/bin/bash # Display the beginning of NetworkHandling.java to locate the emulateNetworkConditions method definition sed -n '1,200p' src/main/java/aquality/selenium/browser/devtools/NetworkHandling.javaLength of output: 8051
Verify v137 emulateNetworkConditions signature
I see two invocations of
emulateNetworkConditionsinNetworkHandling.javathat now supply eight arguments (fourOptional.empty()placeholders):
- Lines 326–327:
tools.sendCommand(emulateNetworkConditions( offline, latency, downloadThroughput, uploadThroughput, Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()));- Lines 342–343:
tools.sendCommand(emulateNetworkConditions( offline, latency, downloadThroughput, uploadThroughput, Optional.of(ConnectionType.fromString(connectionType)), Optional.empty(), Optional.empty(), Optional.empty()));Please confirm these four trailing
Optionalparameters match the updated v137 API signature for:emulateNetworkConditions( Boolean offline, Number latency, Number downloadThroughput, Number uploadThroughput, Optional<ConnectionType> connectionType, Optional<?> param6, Optional<?> param7, Optional<?> param8 )Ensure each placeholder aligns with the intended CDP command parameters.
src/main/java/aquality/selenium/browser/devtools/EmulationHandling.java (3)
5-8: LGTM! DevTools protocol imports correctly upgraded.The import statements have been properly updated from v85 to v137, which aligns with the PR objective of upgrading to Selenium 4.33.0. This change is consistent across the DevTools-related classes in the project.
48-49: LGTM! Backward compatibility maintained through proper delegation.The existing 3-parameter
setGeolocationOverridemethod now correctly delegates to the expanded 7-parameter version with empty optionals for the new parameters (altitude, altitudeAccuracy, heading, speed). This maintains backward compatibility while supporting the updated DevTools API.
68-72: LGTM! Method signature correctly expanded for DevTools v137 API.The expanded method signature properly adds the four new optional parameters (
altitude,altitudeAccuracy,heading,speed) required by the DevTools v137setGeolocationOverridecommand. The implementation correctly passes all seven parameters to the underlying DevTools command.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/manytools/ManyToolsForm.java (2)
16-16: Consider making the XPath selector more specific.The current XPath
//button[@mode='primary']is quite generic and could potentially match multiple primary buttons on a page, leading to unpredictable behavior.Consider making the XPath more specific by adding additional attributes or context:
- private final IButton btnAgree = getElementFactory().getButton(By.xpath("//button[@mode='primary']"), "Agree"); + private final IButton btnAgree = getElementFactory().getButton(By.xpath("//button[@mode='primary' and contains(text(), 'Agree')]"), "Agree");Or if you know the specific context where this button appears:
- private final IButton btnAgree = getElementFactory().getButton(By.xpath("//button[@mode='primary']"), "Agree"); + private final IButton btnAgree = getElementFactory().getButton(By.xpath("//div[contains(@class, 'consent')]//button[@mode='primary']"), "Agree");
29-33: Enhance robustness of the consent button handling.The consent button handling logic is functionally correct, but could be made more robust to handle edge cases and potential race conditions.
Consider these improvements for better reliability:
- if (btnAgree.state().isDisplayed()) - { - btnAgree.click(); - btnAgree.state().waitForNotDisplayed(); - } + if (btnAgree.state().isDisplayed()) { + try { + btnAgree.state().waitForClickable(); + btnAgree.click(); + btnAgree.state().waitForNotDisplayed(); + } catch (TimeoutException e) { + // Log and continue if consent button interaction fails + AqualityServices.getLogger().info("Consent button interaction failed, continuing with page load"); + } + }This adds:
- Explicit wait for clickability to avoid race conditions
- Specific exception handling for consent button interactions
- Logging for debugging purposes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/manytools/ManyToolsForm.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: aquality-automation.aquality-selenium-java (Run tests)
- GitHub Check: aquality-automation.aquality-selenium-java (Analyse code with SonarQube)
🔇 Additional comments (1)
src/test/java/manytools/ManyToolsForm.java (1)
5-5: Import addition looks good.The import for
IButtonis correctly added to support the new button functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/java/automationpractice/forms/ChallengingDomForm.java (2)
11-13: Name constant does not match visible header – consider unifying for clarity
"Header of Challenging Dom"differs in case/spelling from the actual page text ("Challenging DOM").
Keeping the string identical to the UI copy (or extracting it into a constant shared with the locator) avoids brittle assertions and duplicate literals.
Example:- super(By.xpath(LOCATOR_CHALLENGING_DOM_FORM), "Header of Challenging Dom"); + super(By.xpath(LOCATOR_CHALLENGING_DOM_FORM), "Challenging DOM header");
19-21: Guard against invalid indexes & over-broad XPathThe dynamic XPath
//tr[%d]/td[%d]assumes 1-based positive indices and the first table on the page.
Adding simple pre-conditions and scoping the search to the intended table will prevent cryptic test failures whenrow <= 0,column <= 0, or another table is introduced.public ILabel getCell(int row, int column) { - return getElementFactory().getLabel(By.xpath(String.format("//tr[%d]/td[%d]", row, column)), - String.format("Cell at row %d and column %d", row, column)); + if (row < 1 || column < 1) { + throw new IllegalArgumentException("Row and column must be 1-based and positive"); + } + String xpath = String.format("(//table)[1]//tr[%d]/td[%d]", row, column); + return getElementFactory().getLabel(By.xpath(xpath), + String.format("Cell at row %d, column %d", row, column)); }src/test/java/tests/integration/LocatorTests.java (1)
68-94: Variable names don’t reflect expected row – lowers readability
actualCellRaw3Column5GotWith…is obtained with.above(cellInRow5Column5)which actually yields row 4, yet the helper variable and assertion message talk about row 3.
Misleading names slow future maintenance even if the logic still passes.Rename the helpers (and similar occurrences below) or adjust the locator so that the name and content stay in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/automationpractice/forms/ChallengingDomForm.java(1 hunks)src/test/java/tests/integration/LocatorTests.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: aquality-automation.aquality-selenium-java (Analyse code with SonarQube)
- GitHub Check: aquality-automation.aquality-selenium-java (Run tests)
🔇 Additional comments (1)
src/test/java/tests/integration/LocatorTests.java (1)
47-59: Refresh-loop may hide genuine failures
waitForkeeps refreshing the page until the element appears.
If the element never shows up, the test wastes time on full page reloads instead of failing fast.
Consider limiting the maximum number of retries or using the framework’s built-in timeout instead of manualrefresh()to keep feedback tight.
No description provided.