[py] Allow ClientConfig when instantiating local webdrivers #16570
Conversation
… in WebDriver classes
…e_server_addr is always set to service.service_url for local drivers.
|
https://github.com/iampopovich/selenium/actions/runs/19208403903 |
Add unit tests for ClientConfig support across multiple WebDriver implementations Add unit tests for ClientConfig support in Firefox, IE, and Chrome WebDrivers - Implemented unit tests for FirefoxDriver to validate ClientConfig handling. - Added tests for IE WebDriver to ensure proper ClientConfig acceptance and passing. - Created tests for Chrome WebDriver to verify ClientConfig integration. - All tests check for correct handling of remote server address, keep-alive settings, and timeouts. - Fixed assertion issues in tests to compare ClientConfig attributes correctly.
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
…Chromium, Firefox, IE, and Safari WebDriver implementations
There was a problem hiding this comment.
Pull request overview
This PR adds ClientConfig support to all local WebDriver classes (Chrome, Edge, Firefox, IE, Safari), enabling advanced HTTP and WebSocket configuration for local driver connections. The implementation makes remote_server_addr optional in ClientConfig and introduces a normalization utility that automatically sets it to the local service URL.
Key Changes:
- Made
remote_server_addrparameter optional inClientConfigclass - Added
client_configparameter to all WebDriver constructors with precedence over legacykeep_aliveparameter - Implemented
normalize_local_driver_config()utility to ensure local drivers always connect to the correct service URL - Added comprehensive unit and functional tests for the new functionality
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| py/selenium/webdriver/remote/client_config.py | Made remote_server_addr optional to support local driver configuration |
| py/selenium/webdriver/common/utils.py | Added normalize_local_driver_config() utility to handle ClientConfig normalization for local drivers |
| py/selenium/webdriver/chrome/webdriver.py | Added client_config parameter with documentation and example usage |
| py/selenium/webdriver/edge/webdriver.py | Added client_config parameter and forwarded to ChromiumDriver parent |
| py/selenium/webdriver/firefox/webdriver.py | Added client_config parameter with normalization and connection setup |
| py/selenium/webdriver/ie/webdriver.py | Added client_config parameter with normalization and connection setup |
| py/selenium/webdriver/safari/webdriver.py | Added client_config parameter with normalization and connection setup |
| py/selenium/webdriver/chromium/webdriver.py | Implemented client_config normalization in base ChromiumDriver class |
| py/test/unit/selenium/webdriver/chrome/chrome_webdriver_tests.py | Unit tests for Chrome driver ClientConfig support |
| py/test/unit/selenium/webdriver/edge/edge_webdriver_tests.py | Unit tests for Edge driver ClientConfig support |
| py/test/unit/selenium/webdriver/firefox/firefox_webdriver_tests.py | Unit tests for Firefox driver ClientConfig support |
| py/test/unit/selenium/webdriver/ie/ie_webdriver_tests.py | Unit tests for IE driver ClientConfig support |
| py/test/unit/selenium/webdriver/chromium/webdriver_tests.py | Comprehensive unit tests for ChromiumDriver ClientConfig functionality |
| py/test/selenium/webdriver/chromium/webdriver_timeout_test.py | Functional tests for ClientConfig timeout handling and preservation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| options: Options | None = None, | ||
| service: Service | None = None, | ||
| keep_alive: bool = True, | ||
| client_config: Optional[ClientConfig] = None, |
There was a problem hiding this comment.
The type hint Optional[ClientConfig] is used but Optional is not imported. This will cause a NameError at runtime. Add from typing import Optional to the imports.
| options: ChromiumOptions | None = None, | ||
| service: ChromiumService | None = None, | ||
| keep_alive: bool = True, | ||
| client_config: Optional[ClientConfig] = None, |
There was a problem hiding this comment.
The type hint Optional[ClientConfig] is used but Optional is not imported. This will cause a NameError at runtime. Add from typing import Optional to the imports.
|
|
||
| executor = SafariRemoteConnection( | ||
| remote_server_addr=self.service.service_url, | ||
| keep_alive=keep_alive, |
There was a problem hiding this comment.
The keep_alive parameter is passed to SafariRemoteConnection even though it's already included in the client_config. This creates redundancy and could lead to confusion about which value takes precedence. Consider removing the keep_alive parameter since client_config should contain all necessary configuration.
| keep_alive=keep_alive, |
|
|
||
| executor = FirefoxRemoteConnection( | ||
| remote_server_addr=self.service.service_url, | ||
| keep_alive=keep_alive, |
There was a problem hiding this comment.
The keep_alive parameter is passed to FirefoxRemoteConnection even though it's already included in the client_config. This creates redundancy and could lead to confusion about which value takes precedence. Consider removing the keep_alive parameter since client_config should contain all necessary configuration.
| keep_alive=keep_alive, |
| remote_server_addr=self.service.service_url, | ||
| browser_name=browser_name, | ||
| vendor_prefix=vendor_prefix, | ||
| keep_alive=keep_alive, |
There was a problem hiding this comment.
The keep_alive parameter is passed to ChromiumRemoteConnection even though it's already included in the client_config. This creates redundancy and could lead to confusion about which value takes precedence. Consider removing the keep_alive parameter since client_config should contain all necessary configuration.
| keep_alive=keep_alive, |
| def normalize_local_driver_config( | ||
| service_url: str, user_config: Optional[ClientConfig] = None, **defaults | ||
| ) -> ClientConfig: | ||
| """Creates a ClientConfig for local drivers.""" |
There was a problem hiding this comment.
The function lacks comprehensive documentation. It should clearly document that when user_config is provided, the defaults parameters are ignored and only the service_url is normalized to ensure local drivers connect to the correct endpoint. This is important for users to understand the precedence behavior.
| """Creates a ClientConfig for local drivers.""" | |
| """Create a ClientConfig for local drivers, with clear precedence rules. | |
| When ``user_config`` is not provided, a new ``ClientConfig`` is created | |
| using ``service_url`` (as ``remote_server_addr``) and any additional | |
| keyword arguments supplied via ``defaults``. | |
| When ``user_config`` is provided, its existing values are copied and | |
| only ``remote_server_addr`` is updated to the given ``service_url`` so | |
| that local drivers connect to the correct endpoint. In this case, all | |
| values passed via ``defaults`` are ignored. | |
| Args: | |
| service_url: Base URL of the local WebDriver service; normalized into | |
| ``remote_server_addr`` on the returned configuration. | |
| user_config: Existing ``ClientConfig`` to start from. If not ``None``, | |
| its fields are copied and only ``remote_server_addr`` is | |
| overridden with ``service_url``. | |
| **defaults: Additional keyword arguments used only when | |
| ``user_config`` is ``None`` to construct a new ``ClientConfig``. | |
| Returns: | |
| A ``ClientConfig`` instance configured for a local driver with the | |
| precedence behavior described above. | |
| """ |
| config = ClientConfig( | ||
| remote_server_addr="http://localhost:9515", | ||
| websocket_timeout=10 |
There was a problem hiding this comment.
The example shows setting remote_server_addr in the ClientConfig for a local driver, but according to the PR description and implementation, this value will be overridden by the service URL. This could mislead users into thinking they need to provide this parameter. Consider either removing it from the example or adding a comment explaining that it will be automatically set to the service URL for local drivers.
| config = ClientConfig( | |
| remote_server_addr="http://localhost:9515", | |
| websocket_timeout=10 | |
| # For local drivers, remote_server_addr is automatically set from the Service URL. | |
| config = ClientConfig( | |
| websocket_timeout=10, |
| """Test that None timeout in ClientConfig is converted to default (120).""" | ||
| # Arrange | ||
| mock_finder.return_value.get_browser_path.return_value = None | ||
| mock_finder.return_value.get_driver_path.return_value = "/path/to/driver" | ||
|
|
||
| # ClientConfig with explicit None timeout (or not set) | ||
| client_config = ClientConfig( | ||
| remote_server_addr="http://localhost:9515", | ||
| # timeout not set, defaults to None in ClientConfig | ||
| ) | ||
|
|
||
| # Act | ||
| with patch.object(mock_service, "start"): | ||
| driver = ChromiumDriver( | ||
| service=mock_service, | ||
| options=chromium_options, | ||
| client_config=client_config, | ||
| ) | ||
|
|
||
| # Assert | ||
| call_kwargs = mock_remote_connection_class.call_args[1] | ||
| actual_config = call_kwargs["client_config"] | ||
| # When driver normalizes config without remote_server_addr, it uses default timeout=120 | ||
| # if the client_config didn't specify a timeout | ||
| assert actual_config.timeout is None or actual_config.timeout == 120 |
There was a problem hiding this comment.
The test comment and assertion are misleading. When a ClientConfig is provided with no explicit timeout, the timeout is set to socket.getdefaulttimeout() by ClientConfig's constructor, not 120. The normalize_local_driver_config function only applies the default timeout=120 when user_config is None. In this test case, since a ClientConfig instance is provided, the timeout will be socket.getdefaulttimeout() (typically None), not 120. The assertion should only check for None or clarify the expected behavior.
| """ | ||
|
|
||
| from unittest.mock import Mock, patch, MagicMock | ||
| from urllib3.exceptions import ConnectTimeoutError, ReadTimeoutError |
There was a problem hiding this comment.
Import of 'ConnectTimeoutError' is not used.
Import of 'ReadTimeoutError' is not used.
| from urllib3.exceptions import ConnectTimeoutError, ReadTimeoutError |
| from selenium.webdriver.chromium.service import ChromiumService | ||
| from selenium.webdriver.chromium.webdriver import ChromiumDriver | ||
| from selenium.webdriver.remote.client_config import ClientConfig | ||
| from selenium.common.exceptions import SessionNotCreatedException, TimeoutException |
There was a problem hiding this comment.
Import of 'SessionNotCreatedException' is not used.
Import of 'TimeoutException' is not used.
| from selenium.common.exceptions import SessionNotCreatedException, TimeoutException |
2 participants
User description
🔗 Related Issues
relates to #16282
💥 What does this PR do?
This pull request introduces support for advanced HTTP and WebSocket configuration in all major Selenium WebDriver classes through a new
client_configparameter. The change allows users to pass aClientConfiginstance for fine-grained control over connection settings, which takes precedence over legacy parameters likekeep_alive. The constructors for Chrome, Edge, Firefox, Internet Explorer, and Safari drivers are updated to accept and normalize this new configuration, ensuring consistent behavior and clear documentation for users.Major feature: Advanced connection configuration
Added an optional
client_configparameter (type:ClientConfig) to the constructors of all major WebDriver classes (chrome,edge,firefox,ie,safari), enabling advanced HTTP/WebSocket settings. The parameter is documented to take precedence overkeep_aliveand other legacy options, and example usage is provided in each class docstring. [1] [2] [3] [4] [5]For each driver, the constructor now normalizes
client_configby always settingremote_server_addrto the local driver service URL. If aclient_configis provided by the user, its fields are copied and merged with the service URL, ensuring local drivers always connect to the correct endpoint. [1] [2] [3] [4]Integration and documentation
The new
client_configparameter is passed through to the corresponding remote connection classes (ChromiumRemoteConnection,FirefoxRemoteConnection,SafariRemoteConnection,RemoteConnection), allowing these classes to use the advanced configuration for HTTP and WebSocket connections. [1] [2] [3] [4]Imports for
ClientConfigare added to all affected driver files to support the new parameter. [1] [2] [3] [4] [5]Docstrings for all affected constructors are updated to clearly explain the purpose and precedence of
client_config, including usage examples for both default and custom configurations. [1] [2] [3] [4] [5]🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes
PR Type
Enhancement
Description
Added
client_configparameter to all major WebDriver classesEnables advanced HTTP/WebSocket configuration for local drivers
Normalizes
remote_server_addrto service URL for local driversComprehensive unit tests for ClientConfig support across drivers
Diagram Walkthrough
File Walkthrough
7 files
Add ClientConfig parameter to Chrome WebDriverImplement ClientConfig normalization in ChromiumDriverAdd ClientConfig parameter to Edge WebDriverAdd ClientConfig parameter to Firefox WebDriverAdd ClientConfig parameter to IE WebDriverAdd ClientConfig parameter to Safari WebDriverMake remote_server_addr optional in ClientConfig6 files
Add functional tests for ClientConfig timeout handlingAdd unit tests for Chrome ClientConfig supportAdd comprehensive unit tests for ChromiumDriver ClientConfigAdd unit tests for Edge ClientConfig supportAdd unit tests for Firefox ClientConfig supportAdd unit tests for IE ClientConfig support