Update to Selenium 4.36.0 +semver:feature #154
Conversation
Update DevTools references to use V140 Fix issues in javadocs Deprecate canEmulate() method
mialeska
added
enhancement
WalkthroughThe PR updates the aquality-selenium-core dependency to version 4.9.0 and upgrades DevTools protocol imports from v138 to v140 in main source files and v139 in test files. Deprecation annotations and Javadoc improvements are added across multiple classes without altering runtime behavior or control flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes are predominantly repetitive import version upgrades (v138 → v139/v140) applied consistently across multiple files, accompanied by straightforward Javadoc additions. One test method removal and a single @deprecated annotation are the only logic-adjacent modifications. The homogeneous nature of the import updates and documentation additions minimizes cognitive load per file. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/aquality/selenium/browser/devtools/JavaScriptHandling.java (1)
83-89: ConcurrentModificationException risk: modifying Set while iteratingBoth clearScriptCallbackBindings and clearInitializationScripts remove from the same collection they iterate, which can throw ConcurrentModificationException.
public void clearScriptCallbackBindings() { logger.info("loc.browser.javascript.scriptcallbackbindings.clear"); - bindings.forEach(scriptName -> { - bindings.remove(scriptName); - tools.sendCommand(Runtime.removeBinding(scriptName)); - }); + for (String scriptName : new java.util.ArrayList<>(bindings)) { + tools.sendCommand(Runtime.removeBinding(scriptName)); + } + bindings.clear(); } public void clearInitializationScripts() { logger.info("loc.browser.javascript.initializationscripts.clear"); - initializationScripts.forEach(this::removeInitializationScriptCore); + for (InitializationScript script : new java.util.ArrayList<>(initializationScripts)) { + removeInitializationScriptCore(script); + } }Also applies to: 146-149
src/main/java/aquality/selenium/browser/devtools/EmulationHandling.java (1)
25-31: Incorrect class reference in constructor JavadocMentions NetworkHandling instead of EmulationHandling.
- * Initializes a new instance of the {@link NetworkHandling} class. + * Initializes a new instance of the {@link EmulationHandling} class.
🧹 Nitpick comments (6)
src/main/java/aquality/selenium/browser/devtools/JavaScriptHandling.java (1)
202-205: Pair enable/disable for the same DevTools domainstartEventMonitoring sends Runtime.enable(), but stopEventMonitoring calls events.disable(). Prefer disabling the same domain for symmetry.
public void stopEventMonitoring() { logger.info("loc.browser.javascript.event.monitoring.stop"); - events.disable(); + tools.sendCommand(org.openqa.selenium.devtools.v140.runtime.Runtime.disable()); }src/main/java/aquality/selenium/elements/actions/JsActions.java (2)
71-76: Guard against null highlightState to avoid NPEPublic API can be called with null; highlightElement(HighlightState) dereferences highlightState via equals().
public void highlightElement(HighlightState highlightState) { - if ((AqualityServices.getBrowserProfile().isElementHighlightEnabled() && !highlightState.equals(HighlightState.NOT_HIGHLIGHT)) - || highlightState.equals(HighlightState.HIGHLIGHT)) { + if (highlightState == null) { + highlightState = HighlightState.DEFAULT; + } + if ((AqualityServices.getBrowserProfile().isElementHighlightEnabled() && highlightState != HighlightState.NOT_HIGHLIGHT) + || highlightState == HighlightState.HIGHLIGHT) { executeScript(JavaScript.BORDER_ELEMENT); } }
36-41: Minor Javadoc polish“Setting attribute value.” → “Sets attribute value.” for consistency with other method docs. Optional.
src/main/java/aquality/selenium/browser/devtools/NetworkHandling.java (2)
203-205: Style: use existing static import for getResponseBodyYou already import static org.openqa.selenium.devtools.v140.network.Network.*. Using the static import here improves consistency.
-String responseBody = tools.sendCommand(org.openqa.selenium.devtools.v140.network.Network.getResponseBody(requestId)).getBody(); +String responseBody = tools.sendCommand(getResponseBody(requestId)).getBody();
83-86: Clarify Javadoc for useTheseCredentialsParameter type is Supplier; current text is vague.
- * @param useTheseCredentials parameters, such as URI matcher and credentials. + * @param useTheseCredentials supplier that provides credentials for matched requests.src/main/java/aquality/selenium/browser/devtools/EmulationHandling.java (1)
35-42: Add deprecation rationale and guidancecanEmulate() is annotated @deprecated but lacks a Javadoc note on why and what to use instead. Add guidance and removal timeline.
/** * Tells whether emulation is supported. * * @return true if the emulation is supported, false otherwise. + * @deprecated This check is no longer reliable across modern browsers/DevTools. + * Prefer attempting the specific emulation command (e.g., setDeviceMetricsOverride, + * setUserAgentOverride) and handle failures. Subject to removal in a future major release. */ @Deprecated public boolean canEmulate() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
pom.xml(1 hunks)src/main/java/aquality/selenium/browser/devtools/DevToolsHandling.java(1 hunks)src/main/java/aquality/selenium/browser/devtools/EmulationHandling.java(17 hunks)src/main/java/aquality/selenium/browser/devtools/JavaScriptHandling.java(1 hunks)src/main/java/aquality/selenium/browser/devtools/NetworkHandling.java(4 hunks)src/main/java/aquality/selenium/elements/actions/JsActions.java(4 hunks)src/main/java/aquality/selenium/elements/interfaces/IShadowRootExpander.java(18 hunks)src/main/java/aquality/selenium/forms/Form.java(2 hunks)src/main/java/aquality/selenium/logging/DevToolsCommandLoggingOptions.java(1 hunks)src/main/java/aquality/selenium/logging/HttpExchangeLoggingOptions.java(7 hunks)src/test/java/tests/usecases/devtools/DeviceEmulationTest.java(1 hunks)src/test/java/tests/usecases/devtools/EmulationTests.java(0 hunks)src/test/java/tests/usecases/devtools/NetworkSpeedEmulationTest.java(1 hunks)src/test/java/tests/usecases/devtools/OverrideUserAgentTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/tests/usecases/devtools/EmulationTests.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-29T14:35:07.825Z
Learnt from: mialeska
PR: aquality-automation/aquality-selenium-java#148
File: src/test/java/tests/usecases/devtools/DeviceEmulationTest.java:6-7
Timestamp: 2024-11-29T14:35:07.825Z
Learning: In this project, the use of custom DevTools Protocol version (v131) in test files is intentional, even if it differs from the version used in the main source code.
Applied to files:
src/main/java/aquality/selenium/browser/devtools/JavaScriptHandling.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: aquality-automation.aquality-selenium-java (Analyse code with SonarQube)
- GitHub Check: aquality-automation.aquality-selenium-java (Run tests)
🔇 Additional comments (9)
src/main/java/aquality/selenium/forms/Form.java (2)
33-36: LGTM: Javadoc improvements enhance clarity.The added parameter documentation for the constructor properly describes the
locatorandnameparameters.
64-65: LGTM: Javadoc formatting improved.The additional blank line improves the readability of the method documentation.
src/main/java/aquality/selenium/logging/DevToolsCommandLoggingOptions.java (1)
20-20: LGTM: Parameter documentation added.The
@paramtag properly documents thecommandparameter for the setter method.src/test/java/tests/usecases/devtools/DeviceEmulationTest.java (1)
6-7: LGTM: DevTools protocol imports updated consistently.Both
EmulationandDisplayFeatureimports have been correctly updated to v139, maintaining consistency with other test file updates in this PR.src/test/java/tests/usecases/devtools/NetworkSpeedEmulationTest.java (1)
5-5: LGTM: ConnectionType import updated to v139.The import has been correctly updated to use the v139 network model, consistent with other test file updates in this PR.
src/main/java/aquality/selenium/logging/HttpExchangeLoggingOptions.java (1)
16-17: LGTM: Comprehensive Javadoc improvements.All getter and setter methods now have properly formatted documentation with
@returnand@paramtags. The blank lines before the tags follow Javadoc best practices and improve readability.Also applies to: 25-26, 34-35, 43-44, 52-53, 61-62, 70-71, 79-80, 88-89, 97-98, 106-107, 115-116
src/main/java/aquality/selenium/browser/devtools/DevToolsHandling.java (1)
12-13: LGTM: DevTools protocol version consistently updated to v140 in main source.Verification confirms all main source DevTools-related imports are consistently using v140. No legacy versions remain. The version split between main source (v140) and test files (v131) is intentional per project conventions.
src/test/java/tests/usecases/devtools/OverrideUserAgentTest.java (1)
11-11: LGTM: DevTools protocol version v139 consistently applied across all test files.Verification confirms that all test file imports have been correctly updated to DevTools v139, with no remaining v138 imports. The target file (OverrideUserAgentTest.java:11) and companion test files (DeviceEmulationTest.java, NetworkSpeedEmulationTest.java) are now aligned on the same protocol version.
pom.xml (1)
73-73: Verification confirmed: aquality-selenium-core 4.9.0 transitive dependency and DevTools support validated.The script output confirms aquality-selenium-core 4.9.0 includes Selenium 4.36.0 as a transitive dependency. Selenium 4.36.0 adds CDP for Chrome 140, which aligns with the DevTools v140 support mentioned in the PR title. The dependency update is correct and ready.
Update DevTools references to use V140
Fix issues in javadocs
Deprecate canEmulate() method