Update to Selenium 4.33.0 +semver:feature #150
Conversation
mialeska
added
enhancement
WalkthroughThe Maven dependency Changes
Possibly related PRs
Suggested labels
Poem
✨ 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.
No description provided.