From 5e2a6308679cb7175c42dabef9113ba1ae94ca5e Mon Sep 17 00:00:00 2001 From: mk868 Date: Wed, 14 Aug 2024 14:25:01 +0200 Subject: [PATCH 01/11] [java] Add JSpecify annotations for WebDriver and 3 other interfaces (#14371) --- java/src/org/openqa/selenium/JavascriptExecutor.java | 9 ++++++--- java/src/org/openqa/selenium/OutputType.java | 2 ++ java/src/org/openqa/selenium/TakesScreenshot.java | 3 +++ java/src/org/openqa/selenium/WebDriver.java | 11 +++++++---- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/java/src/org/openqa/selenium/JavascriptExecutor.java b/java/src/org/openqa/selenium/JavascriptExecutor.java index ab660e5e25c35d..a744eb8d8df917 100644 --- a/java/src/org/openqa/selenium/JavascriptExecutor.java +++ b/java/src/org/openqa/selenium/JavascriptExecutor.java @@ -20,6 +20,8 @@ import java.util.Collections; import java.util.Set; import java.util.stream.Collectors; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; import org.openqa.selenium.internal.Require; /** @@ -30,6 +32,7 @@ * request or when trying to access another frame. Most times when troubleshooting failure it's best * to view the browser's console after executing the WebDriver request. */ +@NullMarked public interface JavascriptExecutor { /** * Executes JavaScript in the context of the currently selected frame or window. The script @@ -63,7 +66,7 @@ public interface JavascriptExecutor { * @param args The arguments to the script. May be empty * @return One of Boolean, Long, Double, String, List, Map or WebElement. Or null. */ - Object executeScript(String script, Object... args); + @Nullable Object executeScript(String script, @Nullable Object... args); /** * Execute an asynchronous piece of JavaScript in the context of the currently selected frame or @@ -139,7 +142,7 @@ public interface JavascriptExecutor { * @return One of Boolean, Long, String, List, Map, WebElement, or null. * @see WebDriver.Timeouts#scriptTimeout(java.time.Duration) */ - Object executeAsyncScript(String script, Object... args); + @Nullable Object executeAsyncScript(String script, @Nullable Object... args); /** * Commonly used scripts may be "pinned" to the WebDriver session, allowing them to be called @@ -186,7 +189,7 @@ default Set getPinnedScripts() { * * @see #executeScript(String, Object...) */ - default Object executeScript(ScriptKey key, Object... args) { + default @Nullable Object executeScript(ScriptKey key, @Nullable Object... args) { Require.stateCondition( key instanceof UnpinnedScriptKey, "Script key should have been generated by this driver"); diff --git a/java/src/org/openqa/selenium/OutputType.java b/java/src/org/openqa/selenium/OutputType.java index 1d0e013e41266c..78c38614d9ce32 100644 --- a/java/src/org/openqa/selenium/OutputType.java +++ b/java/src/org/openqa/selenium/OutputType.java @@ -22,6 +22,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Base64; +import org.jspecify.annotations.NullMarked; /** * Defines the output type for a screenshot. @@ -29,6 +30,7 @@ * @see TakesScreenshot * @param Type for the screenshot output. */ +@NullMarked public interface OutputType { /** Obtain the screenshot as base64 data. */ OutputType BASE64 = diff --git a/java/src/org/openqa/selenium/TakesScreenshot.java b/java/src/org/openqa/selenium/TakesScreenshot.java index 99a1173434a941..d37f2b05304917 100644 --- a/java/src/org/openqa/selenium/TakesScreenshot.java +++ b/java/src/org/openqa/selenium/TakesScreenshot.java @@ -16,6 +16,8 @@ // under the License. package org.openqa.selenium; +import org.jspecify.annotations.NullMarked; + /** * Indicates a driver or an HTML element that can capture a screenshot and store it in different * ways. @@ -29,6 +31,7 @@ * * @see OutputType */ +@NullMarked public interface TakesScreenshot { /** * Capture the screenshot and store it in the specified location. diff --git a/java/src/org/openqa/selenium/WebDriver.java b/java/src/org/openqa/selenium/WebDriver.java index 843d4fc4247bf3..a77c1dc3d8b4d1 100644 --- a/java/src/org/openqa/selenium/WebDriver.java +++ b/java/src/org/openqa/selenium/WebDriver.java @@ -22,6 +22,8 @@ import java.util.List; import java.util.Set; import java.util.concurrent.TimeUnit; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; import org.openqa.selenium.logging.LoggingPreferences; import org.openqa.selenium.logging.Logs; @@ -46,6 +48,7 @@ *

Most implementations of this interface follow W3C * WebDriver specification */ +@NullMarked public interface WebDriver extends SearchContext { // Navigation @@ -74,7 +77,7 @@ public interface WebDriver extends SearchContext { * * @return The URL of the page currently loaded in the browser */ - String getCurrentUrl(); + @Nullable String getCurrentUrl(); // General properties @@ -87,7 +90,7 @@ public interface WebDriver extends SearchContext { * @return The title of the current page, with leading and trailing whitespace stripped, or null * if one is not already set */ - String getTitle(); + @Nullable String getTitle(); /** * Find all elements within the current page using the given mechanism. This method is affected by @@ -142,7 +145,7 @@ public interface WebDriver extends SearchContext { * * @return The source of the current page */ - String getPageSource(); + @Nullable String getPageSource(); /** * Close the current window, quitting the browser if it's the last window currently open. @@ -261,7 +264,7 @@ interface Options { * @param name the name of the cookie * @return the cookie, or null if no cookie with the given name is present */ - Cookie getCookieNamed(String name); + @Nullable Cookie getCookieNamed(String name); /** * @return the interface for managing driver timeouts. From c3dd52e7c96b5f99bd9971d8022f6194bc608e82 Mon Sep 17 00:00:00 2001 From: Simon Benzer <69980130+shbenzer@users.noreply.github.com> Date: Wed, 14 Aug 2024 10:41:25 -0400 Subject: [PATCH 02/11] Bug 14229 fixed (#14389) --- common/src/web/javascriptPage.html | 4 ++++ javascript/atoms/domcore.js | 5 +++++ py/test/selenium/webdriver/common/visibility_tests.py | 1 + 3 files changed, 10 insertions(+) diff --git a/common/src/web/javascriptPage.html b/common/src/web/javascriptPage.html index 1690ad07dd7958..3fdb38a46f4c15 100644 --- a/common/src/web/javascriptPage.html +++ b/common/src/web/javascriptPage.html @@ -279,6 +279,10 @@

Type Stuff

+
+ +
+ diff --git a/javascript/atoms/domcore.js b/javascript/atoms/domcore.js index 0685fc9d0beab8..d773226358e4f1 100644 --- a/javascript/atoms/domcore.js +++ b/javascript/atoms/domcore.js @@ -169,6 +169,11 @@ bot.dom.core.isElement = function (node, opt_tagName) { if (opt_tagName && (typeof opt_tagName !== 'string')) { opt_tagName = opt_tagName.toString(); } + // because node.tagName.toUpperCase() fails when tagName is "tagName" + if (node instanceof HTMLFormElement) { + return !!node && node.nodeType == goog.dom.NodeType.ELEMENT && + (!opt_tagName || "FORM" == opt_tagName); + } return !!node && node.nodeType == goog.dom.NodeType.ELEMENT && (!opt_tagName || node.tagName.toUpperCase() == opt_tagName); }; diff --git a/py/test/selenium/webdriver/common/visibility_tests.py b/py/test/selenium/webdriver/common/visibility_tests.py index 458bc7c2681a78..7a5eef8b365695 100644 --- a/py/test/selenium/webdriver/common/visibility_tests.py +++ b/py/test/selenium/webdriver/common/visibility_tests.py @@ -29,6 +29,7 @@ def test_should_allow_the_user_to_tell_if_an_element_is_displayed_or_not(driver, assert driver.find_element(by=By.ID, value="none").is_displayed() is False assert driver.find_element(by=By.ID, value="suppressedParagraph").is_displayed() is False assert driver.find_element(by=By.ID, value="hidden").is_displayed() is False + assert driver.find_element(by=By.ID, value="aParentFormId").is_displayed() is True def test_visibility_should_take_into_account_parent_visibility(driver, pages): From e4d3db3976dd12d89e846260e287a9ca8db4dad2 Mon Sep 17 00:00:00 2001 From: Navin Chandra <98466550+navin772@users.noreply.github.com> Date: Thu, 15 Aug 2024 00:25:55 +0530 Subject: [PATCH 03/11] [py] fix type errors for `log.py`, `chromium/options.py`, `websocket_connection.py` and `chrome/options.py` (#14392) * fix type errors for `log.py` * fix type errors for `chromium/options.py` Signed-off-by: Navin Chandra * ignore mypy stub type error for `websocket_connection.py` Signed-off-by: Navin Chandra * fix type errors for `chrome/options.py` Signed-off-by: Navin Chandra * fix failing RBE tests due to `log.py` Signed-off-by: Navin Chandra --------- Signed-off-by: Navin Chandra Co-authored-by: Sri Harsha <12621691+harsha509@users.noreply.github.com> --- py/selenium/webdriver/chrome/options.py | 2 +- py/selenium/webdriver/chromium/options.py | 14 ++++++----- py/selenium/webdriver/common/log.py | 23 +++++++++++++------ .../webdriver/remote/websocket_connection.py | 2 +- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/py/selenium/webdriver/chrome/options.py b/py/selenium/webdriver/chrome/options.py index 2265d8b335e7a4..c03651075d170f 100644 --- a/py/selenium/webdriver/chrome/options.py +++ b/py/selenium/webdriver/chrome/options.py @@ -28,7 +28,7 @@ def default_capabilities(self) -> dict: def enable_mobile( self, - android_package: str = "com.android.chrome", + android_package: Optional[str] = "com.android.chrome", android_activity: Optional[str] = None, device_serial: Optional[str] = None, ) -> None: diff --git a/py/selenium/webdriver/chromium/options.py b/py/selenium/webdriver/chromium/options.py index 2b305d3a48aedb..733641cea44301 100644 --- a/py/selenium/webdriver/chromium/options.py +++ b/py/selenium/webdriver/chromium/options.py @@ -18,7 +18,9 @@ import base64 import os from typing import BinaryIO +from typing import Dict from typing import List +from typing import Optional from typing import Union from selenium.webdriver.common.desired_capabilities import DesiredCapabilities @@ -30,11 +32,11 @@ class ChromiumOptions(ArgOptions): def __init__(self) -> None: super().__init__() - self._binary_location = "" - self._extension_files = [] - self._extensions = [] - self._experimental_options = {} - self._debugger_address = None + self._binary_location: str = "" + self._extension_files: List[str] = [] + self._extensions: List[str] = [] + self._experimental_options: Dict[str, Union[str, int, dict, List[str]]] = {} + self._debugger_address: Optional[str] = None @property def binary_location(self) -> str: @@ -53,7 +55,7 @@ def binary_location(self, value: str) -> None: self._binary_location = value @property - def debugger_address(self) -> str: + def debugger_address(self) -> Optional[str]: """:Returns: The address of the remote devtools instance.""" return self._debugger_address diff --git a/py/selenium/webdriver/common/log.py b/py/selenium/webdriver/common/log.py index 8639b75fbdba76..74eb105c85310a 100644 --- a/py/selenium/webdriver/common/log.py +++ b/py/selenium/webdriver/common/log.py @@ -19,6 +19,10 @@ import pkgutil from contextlib import asynccontextmanager from importlib import import_module +from typing import Any +from typing import AsyncGenerator +from typing import Dict +from typing import Optional from selenium.webdriver.common.by import By @@ -45,10 +49,14 @@ def __init__(self, driver, bidi_session) -> None: self.cdp = bidi_session.cdp self.devtools = bidi_session.devtools _pkg = ".".join(__name__.split(".")[:-1]) - self._mutation_listener_js = pkgutil.get_data(_pkg, "mutation-listener.js").decode("utf8").strip() + # Ensure _mutation_listener_js is not None before decoding + _mutation_listener_js_bytes: Optional[bytes] = pkgutil.get_data(_pkg, "mutation-listener.js") + if _mutation_listener_js_bytes is None: + raise ValueError("Failed to load mutation-listener.js") + self._mutation_listener_js = _mutation_listener_js_bytes.decode("utf8").strip() @asynccontextmanager - async def mutation_events(self) -> dict: + async def mutation_events(self) -> AsyncGenerator[Dict[str, Any], None]: """Listen for mutation events and emit them as they are found. :Usage: @@ -76,12 +84,13 @@ async def mutation_events(self) -> dict: ) self.driver.pin_script(self._mutation_listener_js, script_key) self.driver.execute_script(f"return {self._mutation_listener_js}") - event = {} + + event: Dict[str, Any] = {} async with runtime.wait_for(self.devtools.runtime.BindingCalled) as evnt: yield event payload = json.loads(evnt.value.payload) - elements: list = self.driver.find_elements(By.CSS_SELECTOR, f"*[data-__webdriver_id={payload['target']}") + elements: list = self.driver.find_elements(By.CSS_SELECTOR, f"*[data-__webdriver_id={payload['target']}]") if not elements: elements.append(None) event["element"] = elements[0] @@ -90,7 +99,7 @@ async def mutation_events(self) -> dict: event["old_value"] = payload["oldValue"] @asynccontextmanager - async def add_js_error_listener(self): + async def add_js_error_listener(self) -> AsyncGenerator[Dict[str, Any], None]: """Listen for JS errors and when the contextmanager exits check if there were JS Errors. @@ -114,7 +123,7 @@ async def add_js_error_listener(self): js_exception.exception_details = exception.value.exception_details @asynccontextmanager - async def add_listener(self, event_type) -> dict: + async def add_listener(self, event_type) -> AsyncGenerator[Dict[str, Any], None]: """Listen for certain events that are passed in. :Args: @@ -134,7 +143,7 @@ async def add_listener(self, event_type) -> dict: await session.execute(self.devtools.page.enable()) session = self.cdp.get_session_context("runtime.enable") await session.execute(self.devtools.runtime.enable()) - console = {"message": None, "level": None} + console: Dict[str, Any] = {"message": None, "level": None} async with session.wait_for(self.devtools.runtime.ConsoleAPICalled) as messages: yield console diff --git a/py/selenium/webdriver/remote/websocket_connection.py b/py/selenium/webdriver/remote/websocket_connection.py index ee0e6ba6d26e69..3afbba46d5e1ef 100644 --- a/py/selenium/webdriver/remote/websocket_connection.py +++ b/py/selenium/webdriver/remote/websocket_connection.py @@ -20,7 +20,7 @@ from threading import Thread from time import sleep -from websocket import WebSocketApp +from websocket import WebSocketApp # type: ignore logger = logging.getLogger(__name__) From b3db1d62a94a87fa53c83afb356d67071387e1a2 Mon Sep 17 00:00:00 2001 From: Chris Gossett <54162250+cgossett@users.noreply.github.com> Date: Wed, 14 Aug 2024 17:56:01 -0400 Subject: [PATCH 04/11] Implement getAttributes() method of MBean, and update Number-based MBean attributes to return Number objects instead of Strings (#14153) --- .../org/openqa/selenium/grid/jmx/MBean.java | 14 ++- .../openqa/selenium/grid/router/JmxTest.java | 99 ++++++++++++++----- 2 files changed, 86 insertions(+), 27 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/jmx/MBean.java b/java/src/org/openqa/selenium/grid/jmx/MBean.java index 5586cb59ee8258..3bdd3111dc21fe 100644 --- a/java/src/org/openqa/selenium/grid/jmx/MBean.java +++ b/java/src/org/openqa/selenium/grid/jmx/MBean.java @@ -221,6 +221,8 @@ public Object getAttribute(String attribute) { return ((Map) res) .entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().toString())); + } else if (res instanceof Number) { + return res; } else { return res.toString(); } @@ -241,7 +243,17 @@ public void setAttribute(Attribute attribute) { @Override public AttributeList getAttributes(String[] attributes) { - return null; + AttributeList resultList = new AttributeList(); + + // if attributeNames is empty, return an empty result list + if (attributes == null || attributes.length == 0) return resultList; + + for (int i = 0; i < attributes.length; i++) { + Object value = getAttribute(attributes[i]); + resultList.add(new Attribute(attributes[i], value)); + } + + return resultList; } @Override diff --git a/java/test/org/openqa/selenium/grid/router/JmxTest.java b/java/test/org/openqa/selenium/grid/router/JmxTest.java index 815451d354bf17..2369abb4c60c13 100644 --- a/java/test/org/openqa/selenium/grid/router/JmxTest.java +++ b/java/test/org/openqa/selenium/grid/router/JmxTest.java @@ -27,6 +27,7 @@ import java.time.Duration; import java.time.Instant; import java.util.logging.Logger; +import javax.management.AttributeList; import javax.management.AttributeNotFoundException; import javax.management.InstanceNotFoundException; import javax.management.IntrospectionException; @@ -85,6 +86,9 @@ void shouldBeAbleToRegisterBaseServerConfig() { MBeanAttributeInfo[] attributeInfoArray = info.getAttributes(); assertThat(attributeInfoArray).hasSize(3); + AttributeList attributeList = getAttributeList(name, attributeInfoArray); + assertThat(attributeList).isNotNull().hasSize(3); + String uriValue = (String) beanServer.getAttribute(name, "Uri"); assertThat(uriValue).isEqualTo(baseServerOptions.getExternalUri().toString()); @@ -130,23 +134,26 @@ id, nodeUri, new ImmutableCapabilities(), caps, Instant.now()))) MBeanAttributeInfo[] attributeInfo = info.getAttributes(); assertThat(attributeInfo).hasSize(9); - String currentSessions = (String) beanServer.getAttribute(name, "CurrentSessions"); - assertThat(Integer.parseInt(currentSessions)).isZero(); + AttributeList attributeList = getAttributeList(name, attributeInfo); + assertThat(attributeList).isNotNull().hasSize(9); + + Object currentSessions = beanServer.getAttribute(name, "CurrentSessions"); + assertNumberAttribute(currentSessions, 0); - String maxSessions = (String) beanServer.getAttribute(name, "MaxSessions"); - assertThat(Integer.parseInt(maxSessions)).isEqualTo(1); + Object maxSessions = beanServer.getAttribute(name, "MaxSessions"); + assertNumberAttribute(maxSessions, 1); String status = (String) beanServer.getAttribute(name, "Status"); assertThat(status).isEqualTo("UP"); - String totalSlots = (String) beanServer.getAttribute(name, "TotalSlots"); - assertThat(Integer.parseInt(totalSlots)).isEqualTo(1); + Object totalSlots = beanServer.getAttribute(name, "TotalSlots"); + assertNumberAttribute(totalSlots, 1); - String usedSlots = (String) beanServer.getAttribute(name, "UsedSlots"); - assertThat(Integer.parseInt(usedSlots)).isZero(); + Object usedSlots = beanServer.getAttribute(name, "UsedSlots"); + assertNumberAttribute(usedSlots, 0); - String load = (String) beanServer.getAttribute(name, "Load"); - assertThat(Float.parseFloat(load)).isEqualTo(0.0f); + Object load = beanServer.getAttribute(name, "Load"); + assertNumberAttribute(load, 0.0f); String remoteNodeUri = (String) beanServer.getAttribute(name, "RemoteNodeUri"); assertThat(remoteNodeUri).isEqualTo(nodeUri.toString()); @@ -182,13 +189,14 @@ void shouldBeAbleToRegisterSessionQueueServerConfig() { MBeanAttributeInfo[] attributeInfoArray = info.getAttributes(); assertThat(attributeInfoArray).hasSize(2); - String requestTimeout = (String) beanServer.getAttribute(name, "RequestTimeoutSeconds"); - assertThat(Long.parseLong(requestTimeout)) - .isEqualTo(newSessionQueueOptions.getRequestTimeoutSeconds()); + AttributeList attributeList = getAttributeList(name, attributeInfoArray); + assertThat(attributeList).isNotNull().hasSize(2); + + Object requestTimeout = beanServer.getAttribute(name, "RequestTimeoutSeconds"); + assertNumberAttribute(requestTimeout, newSessionQueueOptions.getRequestTimeoutSeconds()); - String retryInterval = (String) beanServer.getAttribute(name, "RetryIntervalMilliseconds"); - assertThat(Long.parseLong(retryInterval)) - .isEqualTo(newSessionQueueOptions.getRetryIntervalMilliseconds()); + Object retryInterval = beanServer.getAttribute(name, "RetryIntervalMilliseconds"); + assertNumberAttribute(retryInterval, newSessionQueueOptions.getRetryIntervalMilliseconds()); } catch (InstanceNotFoundException | IntrospectionException | ReflectionException @@ -227,8 +235,11 @@ void shouldBeAbleToRegisterSessionQueue() { MBeanAttributeInfo[] attributeInfoArray = info.getAttributes(); assertThat(attributeInfoArray).hasSize(1); - String size = (String) beanServer.getAttribute(name, "NewSessionQueueSize"); - assertThat(Integer.parseInt(size)).isZero(); + AttributeList attributeList = getAttributeList(name, attributeInfoArray); + assertThat(attributeList).isNotNull().hasSize(1); + + Object size = beanServer.getAttribute(name, "NewSessionQueueSize"); + assertNumberAttribute(size, 0); } catch (InstanceNotFoundException | IntrospectionException | ReflectionException @@ -290,21 +301,57 @@ void shouldBeAbleToMonitorHub() throws Exception { MBeanInfo info = beanServer.getMBeanInfo(name); assertThat(info).isNotNull(); - String nodeUpCount = (String) beanServer.getAttribute(name, "NodeUpCount"); + MBeanAttributeInfo[] attributeInfoArray = info.getAttributes(); + assertThat(attributeInfoArray).hasSize(4); + + AttributeList attributeList = getAttributeList(name, attributeInfoArray); + assertThat(attributeList).isNotNull().hasSize(4); + + Object nodeUpCount = beanServer.getAttribute(name, "NodeUpCount"); LOG.info("Node up count=" + nodeUpCount); - assertThat(Integer.parseInt(nodeUpCount)).isEqualTo(1); + assertNumberAttribute(nodeUpCount, 1); - String nodeDownCount = (String) beanServer.getAttribute(name, "NodeDownCount"); + Object nodeDownCount = beanServer.getAttribute(name, "NodeDownCount"); LOG.info("Node down count=" + nodeDownCount); - assertThat(Integer.parseInt(nodeDownCount)).isZero(); + assertNumberAttribute(nodeDownCount, 0); - String activeSlots = (String) beanServer.getAttribute(name, "ActiveSlots"); + Object activeSlots = beanServer.getAttribute(name, "ActiveSlots"); LOG.info("Active slots count=" + activeSlots); - assertThat(Integer.parseInt(activeSlots)).isZero(); + assertNumberAttribute(activeSlots, 0); - String idleSlots = (String) beanServer.getAttribute(name, "IdleSlots"); + Object idleSlots = beanServer.getAttribute(name, "IdleSlots"); LOG.info("Idle slots count=" + idleSlots); - assertThat(Integer.parseInt(idleSlots)).isEqualTo(1); + assertNumberAttribute(idleSlots, 1); + } + } + + private AttributeList getAttributeList(ObjectName name, MBeanAttributeInfo[] attributeInfoArray) + throws InstanceNotFoundException, ReflectionException { + String[] attributeNames = new String[attributeInfoArray.length]; + for (int i = 0; i < attributeInfoArray.length; i++) { + attributeNames[i] = attributeInfoArray[i].getName(); } + + return beanServer.getAttributes(name, attributeNames); + } + + private void assertCommonNumberAttributes(Object attribute) { + assertThat(attribute).isNotNull(); + assertThat(attribute).isInstanceOf(Number.class); + } + + private void assertNumberAttribute(Object attribute, int expectedValue) { + assertCommonNumberAttributes(attribute); + assertThat(Integer.parseInt(attribute.toString())).isEqualTo(expectedValue); + } + + private void assertNumberAttribute(Object attribute, long expectedValue) { + assertCommonNumberAttributes(attribute); + assertThat(Long.parseLong(attribute.toString())).isEqualTo(expectedValue); + } + + private void assertNumberAttribute(Object attribute, float expectedValue) { + assertCommonNumberAttributes(attribute); + assertThat(Float.parseFloat(attribute.toString())).isEqualTo(expectedValue); } } From ed3edee0ac4f09a5555aec7c7ab20609d8b394f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Sautter?= Date: Thu, 15 Aug 2024 18:10:59 +0200 Subject: [PATCH 05/11] [grid] shutdown the client related to a websocket --- .../selenium/grid/node/ProxyNodeWebsockets.java | 15 ++++++++++++++- .../grid/router/ProxyWebsocketsIntoGrid.java | 15 ++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java b/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java index 1101dca55056a5..343828de340317 100644 --- a/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java +++ b/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java @@ -230,7 +230,20 @@ private Consumer createWsEndPoint( client.openSocket( new HttpRequest(GET, uri.toString()), new ForwardingListener(downstream, sessionConsumer, sessionId)); - return upstream::send; + + return (msg) -> { + try { + upstream.send(msg); + } finally { + if (msg instanceof CloseMessage) { + try { + client.close(); + } catch (Exception e) { + LOG.log(Level.WARNING, "Failed to shutdown the client of " + uri, e); + } + } + } + }; } private static class ForwardingListener implements WebSocket.Listener { diff --git a/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java b/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java index 6ba0b4e594979b..af71d27611ad41 100644 --- a/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java +++ b/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java @@ -71,7 +71,20 @@ public Optional> apply(String uri, Consumer downstrea WebSocket upstream = client.openSocket(new HttpRequest(GET, uri), new ForwardingListener(downstream)); - return Optional.of(upstream::send); + return Optional.of( + (msg) -> { + try { + upstream.send(msg); + } finally { + if (msg instanceof CloseMessage) { + try { + client.close(); + } catch (Exception e) { + LOG.log(Level.WARNING, "Failed to shutdown the client of " + sessionUri, e); + } + } + } + }); } catch (NoSuchSessionException e) { LOG.warning("Attempt to connect to non-existent session: " + uri); From 5bac4795ff6acc9f1ca1a6436aea0970ff98fb07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Sautter?= Date: Thu, 15 Aug 2024 18:12:30 +0200 Subject: [PATCH 06/11] [grid] release the upstream websocket --- .../netty/server/WebSocketUpgradeHandler.java | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java b/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java index 226be194c59506..e097533cc95b26 100644 --- a/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java +++ b/java/src/org/openqa/selenium/netty/server/WebSocketUpgradeHandler.java @@ -165,10 +165,15 @@ private void handleHttpRequest(ChannelHandlerContext ctx, HttpRequest req) { private void handleWebSocketFrame(ChannelHandlerContext ctx, WebSocketFrame frame) { if (frame instanceof CloseWebSocketFrame) { - CloseWebSocketFrame close = (CloseWebSocketFrame) frame.retain(); - handshaker.close(ctx.channel(), close); - // Pass on to the rest of the channel - ctx.fireChannelRead(close); + try { + CloseWebSocketFrame close = (CloseWebSocketFrame) frame.retain(); + handshaker.close(ctx.channel(), close); + // Pass on to the rest of the channel + ctx.fireChannelRead(close); + } finally { + // set null to ensure we do not send another close + ctx.channel().attr(key).set(null); + } } else if (frame instanceof PingWebSocketFrame) { ctx.write(new PongWebSocketFrame(frame.isFinalFragment(), frame.rsv(), frame.content())); } else if (frame instanceof PongWebSocketFrame) { @@ -187,7 +192,7 @@ private void handleWebSocketFrame(ChannelHandlerContext ctx, WebSocketFrame fram @Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { try { - Consumer consumer = ctx.channel().attr(key).get(); + Consumer consumer = ctx.channel().attr(key).getAndSet(null); if (consumer != null) { byte[] reason = Objects.toString(cause).getBytes(UTF_8); @@ -201,7 +206,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { try { consumer.accept(new CloseMessage(1011, new String(reason, UTF_8))); } catch (Exception ex) { - LOG.log(Level.FINE, "failed to send the close message", ex); + LOG.log(Level.FINE, "failed to send the close message, code: 1011", ex); } } } finally { @@ -209,4 +214,21 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { ctx.close(); } } + + @Override + public void channelInactive(ChannelHandlerContext ctx) throws Exception { + try { + super.channelInactive(ctx); + } finally { + Consumer consumer = ctx.channel().attr(key).getAndSet(null); + + if (consumer != null) { + try { + consumer.accept(new CloseMessage(1001, "channel got inactive")); + } catch (Exception ex) { + LOG.log(Level.FINE, "failed to send the close message, code: 1001", ex); + } + } + } + } } From 7612405e34d282992b26a22d7bee921753020026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Sautter?= Date: Sun, 18 Aug 2024 19:52:27 +0200 Subject: [PATCH 07/11] [grid] close the httpclient after connecting the websocket failed --- .../grid/node/ProxyNodeWebsockets.java | 36 ++++++++++-------- .../grid/router/ProxyWebsocketsIntoGrid.java | 37 +++++++++++-------- 2 files changed, 42 insertions(+), 31 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java b/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java index 343828de340317..e3f656c0691253 100644 --- a/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java +++ b/java/src/org/openqa/selenium/grid/node/ProxyNodeWebsockets.java @@ -226,24 +226,30 @@ private Consumer createWsEndPoint( LOG.info("Establishing connection to " + uri); HttpClient client = clientFactory.createClient(ClientConfig.defaultConfig().baseUri(uri)); - WebSocket upstream = - client.openSocket( - new HttpRequest(GET, uri.toString()), - new ForwardingListener(downstream, sessionConsumer, sessionId)); + try { + WebSocket upstream = + client.openSocket( + new HttpRequest(GET, uri.toString()), + new ForwardingListener(downstream, sessionConsumer, sessionId)); - return (msg) -> { - try { - upstream.send(msg); - } finally { - if (msg instanceof CloseMessage) { - try { - client.close(); - } catch (Exception e) { - LOG.log(Level.WARNING, "Failed to shutdown the client of " + uri, e); + return (msg) -> { + try { + upstream.send(msg); + } finally { + if (msg instanceof CloseMessage) { + try { + client.close(); + } catch (Exception e) { + LOG.log(Level.WARNING, "Failed to shutdown the client of " + uri, e); + } } } - } - }; + }; + } catch (Exception e) { + LOG.log(Level.WARNING, "Connecting to upstream websocket failed", e); + client.close(); + throw e; + } } private static class ForwardingListener implements WebSocket.Listener { diff --git a/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java b/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java index af71d27611ad41..3cdf7784f02e15 100644 --- a/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java +++ b/java/src/org/openqa/selenium/grid/router/ProxyWebsocketsIntoGrid.java @@ -68,24 +68,29 @@ public Optional> apply(String uri, Consumer downstrea HttpClient client = clientFactory.createClient(ClientConfig.defaultConfig().baseUri(sessionUri)); - WebSocket upstream = - client.openSocket(new HttpRequest(GET, uri), new ForwardingListener(downstream)); - - return Optional.of( - (msg) -> { - try { - upstream.send(msg); - } finally { - if (msg instanceof CloseMessage) { - try { - client.close(); - } catch (Exception e) { - LOG.log(Level.WARNING, "Failed to shutdown the client of " + sessionUri, e); + try { + WebSocket upstream = + client.openSocket(new HttpRequest(GET, uri), new ForwardingListener(downstream)); + + return Optional.of( + (msg) -> { + try { + upstream.send(msg); + } finally { + if (msg instanceof CloseMessage) { + try { + client.close(); + } catch (Exception e) { + LOG.log(Level.WARNING, "Failed to shutdown the client of " + sessionUri, e); + } } } - } - }); - + }); + } catch (Exception e) { + LOG.log(Level.WARNING, "Connecting to upstream websocket failed", e); + client.close(); + return Optional.empty(); + } } catch (NoSuchSessionException e) { LOG.warning("Attempt to connect to non-existent session: " + uri); return Optional.empty(); From 1da2f9cb69f2ff1c3bf705bcb144287a16a40f08 Mon Sep 17 00:00:00 2001 From: Angie Jones Date: Mon, 19 Aug 2024 06:00:37 -0500 Subject: [PATCH 08/11] Modern Modal (#14390) * New Modal Page * changed file name * adding tests * Running format script --------- Co-authored-by: Diego Molina Co-authored-by: Diego Molina --- .../src/web/modal_dialogs/modern_modal.html | 88 +++++++++++++++++++ .../org/openqa/selenium/ModernModalTest.java | 59 +++++++++++++ .../org/openqa/selenium/testing/Pages.java | 2 + 3 files changed, 149 insertions(+) create mode 100644 common/src/web/modal_dialogs/modern_modal.html create mode 100644 java/test/org/openqa/selenium/ModernModalTest.java diff --git a/common/src/web/modal_dialogs/modern_modal.html b/common/src/web/modal_dialogs/modern_modal.html new file mode 100644 index 00000000000000..44eca1375127c6 --- /dev/null +++ b/common/src/web/modal_dialogs/modern_modal.html @@ -0,0 +1,88 @@ + + + Modern Modal + + + + +

Modal dialog sample

+ + + + trigger modal + + +
+
+ × + I am a modal + +
+
+ + + + \ No newline at end of file diff --git a/java/test/org/openqa/selenium/ModernModalTest.java b/java/test/org/openqa/selenium/ModernModalTest.java new file mode 100644 index 00000000000000..85c6159bada8ec --- /dev/null +++ b/java/test/org/openqa/selenium/ModernModalTest.java @@ -0,0 +1,59 @@ +// Licensed to the Software Freedom Conservancy (SFC) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The SFC licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.openqa.selenium; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.openqa.selenium.support.ui.ExpectedConditions.visibilityOf; + +import org.junit.jupiter.api.Test; +import org.openqa.selenium.testing.JupiterTestBase; + +class ModernModalTest extends JupiterTestBase { + + @Test + void testButtonOpensModal() { + driver.get(pages.modernModalPage); + driver.findElement(By.id("trigger-modal-btn")).click(); + + WebElement modal = driver.findElement(By.id("modalContent")); + wait.until(visibilityOf(modal)); + assertThat(modal.isDisplayed()).isTrue(); + } + + @Test + void testLinkOpensModal() { + driver.get(pages.modernModalPage); + driver.findElement(By.id("trigger-modal-link")).click(); + + WebElement modal = driver.findElement(By.id("modalContent")); + wait.until(visibilityOf(modal)); + assertThat(modal.isDisplayed()).isTrue(); + } + + @Test + void testCloseModal() { + driver.get(pages.modernModalPage); + driver.findElement(By.id("trigger-modal-btn")).click(); + + WebElement modal = driver.findElement(By.id("modalContent")); + wait.until(visibilityOf(modal)); + + driver.findElement(By.id("modal-close")).click(); + assertThat(modal.isDisplayed()).isFalse(); + } +} diff --git a/java/test/org/openqa/selenium/testing/Pages.java b/java/test/org/openqa/selenium/testing/Pages.java index 751c7e98bf1f69..7126c0df24bb6a 100644 --- a/java/test/org/openqa/selenium/testing/Pages.java +++ b/java/test/org/openqa/selenium/testing/Pages.java @@ -54,6 +54,7 @@ public class Pages { public String mapVisibilityPage; public String metaRedirectPage; public String missedJsReferencePage; + public String modernModalPage; public String mouseInteractionPage; public String mouseOverPage; public String mouseTrackerPage; @@ -119,6 +120,7 @@ public Pages(AppServer appServer) { mapVisibilityPage = appServer.whereIs("map_visibility.html"); metaRedirectPage = appServer.whereIs("meta-redirect.html"); missedJsReferencePage = appServer.whereIs("missedJsReference.html"); + modernModalPage = appServer.whereIs("modal_dialogs/modern_modal.html"); mouseInteractionPage = appServer.whereIs("mouse_interaction.html"); mouseOverPage = appServer.whereIs("mouseOver.html"); mouseTrackerPage = appServer.whereIs("mousePositionTracker.html"); From d8a7172a2a3a591af0852203449c81eb13aead2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Boni=20Garc=C3=ADa?= Date: Mon, 19 Aug 2024 13:34:22 +0200 Subject: [PATCH 09/11] [rust] Use the Debug format specifier to display error messages (#14388) --- rust/src/logger.rs | 4 ++++ rust/src/main.rs | 6 +++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/rust/src/logger.rs b/rust/src/logger.rs index 36c154001bc37c..72e7c4f541978c 100644 --- a/rust/src/logger.rs +++ b/rust/src/logger.rs @@ -262,4 +262,8 @@ impl Logger { print!("{}", self.get_json_blog(&self.minimal_json)); } } + + pub fn is_debug_enabled(&self) -> bool { + self.debug || self.trace + } } diff --git a/rust/src/main.rs b/rust/src/main.rs index 4bf799e781396f..5b42caa1e77df3 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -265,7 +265,11 @@ fn main() { log.warn(&err); flush_and_exit(OK, log, Some(err)); } else { - log.error(&err); + let error_msg = log + .is_debug_enabled() + .then(|| format!("{:?}", err)) + .unwrap_or_else(|| err.to_string()); + log.error(error_msg); flush_and_exit(DATAERR, log, Some(err)); } }); From 25c83764e013da6140dddc84c4b56ee0663291cb Mon Sep 17 00:00:00 2001 From: Manuel Blanco Date: Mon, 19 Aug 2024 14:21:46 +0200 Subject: [PATCH 10/11] Refactor ChromeDriverFunctionalTest: (#14398) Refactor ChromeDriverFunctionalTest: Remove redundant permission constants Removed the CLIPBOARD_READ and CLIPBOARD_WRITE constants from the class level in ChromeDriverFunctionalTest to avoid redundancy. These constants are now defined within the canSetPermission method, reducing unnecessary visibility and improving code cohesion. This change simplifies maintenance and enhances code clarity without altering existing functionality. Co-authored-by: Diego Molina --- .../openqa/selenium/chrome/ChromeDriverFunctionalTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java b/java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java index 110f71b618c993..ebf7000be83cb5 100644 --- a/java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java +++ b/java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java @@ -50,9 +50,6 @@ class ChromeDriverFunctionalTest extends JupiterTestBase { - private final String CLIPBOARD_READ = "clipboard-read"; - private final String CLIPBOARD_WRITE = "clipboard-write"; - @Test @NoDriverBeforeTest public void builderGeneratesDefaultChromeOptions() { @@ -109,7 +106,9 @@ void canSetPermission() { HasPermissions permissions = (HasPermissions) driver; driver.get(pages.clicksPage); + String CLIPBOARD_READ = "clipboard-read"; assumeThat(checkPermission(driver, CLIPBOARD_READ)).isEqualTo("prompt"); + String CLIPBOARD_WRITE = "clipboard-write"; assumeThat(checkPermission(driver, CLIPBOARD_WRITE)).isEqualTo("granted"); permissions.setPermission(CLIPBOARD_READ, "denied"); From f4ef7be6f717fc3523a400111c9d8283d3241bb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Sautter?= Date: Mon, 19 Aug 2024 20:09:22 +0200 Subject: [PATCH 11/11] [java] enabled and fixed unit tests of the http client --- java/test/org/openqa/selenium/remote/http/BUILD.bazel | 6 +++++- .../openqa/selenium/remote/internal/HttpClientTestBase.java | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/java/test/org/openqa/selenium/remote/http/BUILD.bazel b/java/test/org/openqa/selenium/remote/http/BUILD.bazel index 95c5f1b49d590b..a3dec01d6abd2f 100644 --- a/java/test/org/openqa/selenium/remote/http/BUILD.bazel +++ b/java/test/org/openqa/selenium/remote/http/BUILD.bazel @@ -5,7 +5,10 @@ load("//java:version.bzl", "TOOLS_JAVA_VERSION") java_test_suite( name = "small-tests", size = "small", - srcs = glob(["*.java"]), + srcs = glob([ + "*.java", + "jdk/*.java", + ]), javacopts = [ "--release", TOOLS_JAVA_VERSION, @@ -15,6 +18,7 @@ java_test_suite( "//java/src/org/openqa/selenium:core", "//java/src/org/openqa/selenium/remote/http", "//java/test/org/openqa/selenium/environment", + "//java/test/org/openqa/selenium/remote/internal:test-lib", "//java/test/org/openqa/selenium/testing:annotations", artifact("org.assertj:assertj-core"), artifact("com.google.guava:guava"), diff --git a/java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java b/java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java index ded07b80bb433c..a50b4c11f75e32 100644 --- a/java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java +++ b/java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java @@ -208,7 +208,7 @@ public void shouldAllowConfigurationFromSystemProperties() { delegate = req -> { try { - Thread.sleep(1100); + Thread.sleep(3000); } catch (InterruptedException e) { throw new RuntimeException(e); } @@ -216,11 +216,11 @@ public void shouldAllowConfigurationFromSystemProperties() { }; try { System.setProperty("webdriver.httpclient.connectionTimeout", "1"); - System.setProperty("webdriver.httpclient.readTimeout", "300"); + System.setProperty("webdriver.httpclient.readTimeout", "2"); System.setProperty("webdriver.httpclient.version", "HTTP_1_1"); ClientConfig clientConfig = ClientConfig.defaultConfig(); assertThat(clientConfig.connectionTimeout()).isEqualTo(Duration.ofSeconds(1)); - assertThat(clientConfig.readTimeout()).isEqualTo(Duration.ofSeconds(300)); + assertThat(clientConfig.readTimeout()).isEqualTo(Duration.ofSeconds(2)); assertThat(clientConfig.version()).isEqualTo("HTTP_1_1"); HttpClient client = createFactory().createClient(clientConfig.baseUri(URI.create(server.whereIs("/"))));