Skip to content

[py] Route Safari, STP and WebView2 to their handlers - #17728

Merged
cgoldberg merged 3 commits into
SeleniumHQ:trunkfrom
v-dermichev:fix-py-safari-remote-connection-handler
Jun 29, 2026
Merged

[py] Route Safari, STP and WebView2 to their handlers#17728
cgoldberg merged 3 commits into
SeleniumHQ:trunkfrom
v-dermichev:fix-py-safari-remote-connection-handler

Conversation

@v-dermichev

@v-dermichev v-dermichev commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

🔗 Related Issues

No existing issue; the root cause and reproduction are described below.
Related (the broader warning topic): #14686, #14792.

💥 What does this PR do?

get_remote_connection() picks the per-browser RemoteConnection by comparing the requested browserName against string literals, but several options classes emit values those literals don't match:

  • SafariOptions emits "safari" (DesiredCapabilities.SAFARI), while the branch compared against "Safari" (capital S);
  • SafariOptions with use_technology_preview = True emits "Safari Technology Preview";
  • EdgeOptions with use_webview = True emits "webview2".

In each case selection fell through to the base RemoteConnection. That has two effects: the browser-specific commands (e.g. those registered by SafariRemoteConnection) are lost, and the base RemoteConnection receives remote_server_addr directly and emits a remote_server_addr DeprecationWarning whose message points at client_config, away from the real cause.

This matches the values each options class actually emits, consistent with the chrome and firefox branches.

Highlighted issues can be reproduced by running tests included in this PR, 3 will fail (STP, WebView2 and Safari)

🔧 Implementation Notes

  • Selection now matches every browserName each options class can emit: Edge → ("MicrosoftEdge", "webview2") (WebView2), Safari → ("safari", "Safari Technology Preview") (Technology Preview); Chrome/Firefox emit a single value. This mirrors the existing in ("MicrosoftEdge", "webview2") check already used elsewhere in webdriver.py.
  • The test drives selection from each *Options().to_capabilities() (with the use_webview / use_technology_preview flags set), so the emitted capability value and the handler lookup cannot drift apart without the test catching it.

🤖 AI assistance

  • No substantial AI assistance used

💡 Additional Considerations

An unrecognised browserName still falls back to the base RemoteConnection (and would trip the same warning); that is the intended default. The broader "remote_server_addr deprecation fires from Selenium's own default path" topic is tracked in #14686 / #14792.

🔄 Types of changes

  • Bug fix (backwards compatible)

get_remote_connection() compared browserName against string literals,
but some options classes emit values those literals don't match:
SafariOptions emits "safari" / "Safari Technology Preview", and
EdgeOptions(use_webview=True) emits "webview2". Those sessions fell back
to the base RemoteConnection, losing the browser-specific commands and
raising a remote_server_addr DeprecationWarning whose message points
away from the real cause.

Match the emitted values, consistent with the chrome and firefox
branches, and add a test that drives handler selection from each options
class's emitted capabilities.
@selenium-ci selenium-ci added the C-py Python Bindings label Jun 28, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix get_remote_connection routing for Safari, STP, and Edge WebView2
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

Description

• Route Safari, Safari Technology Preview, and WebView2 sessions to correct RemoteConnection
 handlers.
• Match browserName values emitted by Options.to_capabilities() to avoid handler fall-through.
• Add unit coverage to prevent future drift between option capabilities and handler selection.
Diagram

graph TD
  T["Unit test: handler selection"] --> O["Options.to_capabilities()"] --> G["get_remote_connection()"] --> D{"browserName match"} --> H["Browser-specific RemoteConnection"]
  D --> B["Base RemoteConnection (fallback)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Normalize browserName (e.g., casefold) before matching
  • ➕ Avoids casing mismatches like Safari vs safari without enumerating variants
  • ➕ May reduce future compatibility issues with different vendor casing
  • ➖ Does not help when variants are semantically different strings (e.g., 'Safari Technology Preview', 'webview2')
  • ➖ Can mask genuinely distinct capability values that should be handled explicitly
2. Use a mapping table (dict) of browserName -> handler
  • ➕ Centralizes the supported names and makes additions clearer
  • ➕ Easier to test and extend without growing elif chains
  • ➖ Mostly stylistic for the current small set of browsers
  • ➖ Still requires enumerating all emitted variants (the core fix here)

Recommendation: Keep the PR’s explicit matching of all emitted browserName variants and the capability-driven test. It directly aligns handler selection with what Options.to_capabilities() actually produces (including STP and WebView2) and the test prevents future drift, which is the primary failure mode that caused the bug.

Files changed (2) +51 / -2

Bug fix (1) +2 / -2
webdriver.pyMatch Safari/STP and Edge WebView2 browserName variants to correct handlers +2/-2

Match Safari/STP and Edge WebView2 browserName variants to correct handlers

• Updates get_remote_connection() to recognize Edge's 'webview2' and Safari's emitted values ('safari' and 'Safari Technology Preview'). This prevents falling back to the base RemoteConnection and preserves browser-specific command registration.

py/selenium/webdriver/remote/webdriver.py

Tests (1) +49 / -0
remote_connection_tests.pyAdd parameterized test for handler selection from Options capabilities +49/-0

Add parameterized test for handler selection from Options capabilities

• Adds a unit test that builds capabilities via each Options().to_capabilities() (including STP and WebView2 flags) and asserts the returned connection type is the expected browser-specific RemoteConnection. This locks the handler selection behavior to the actual capability values emitted by options.

py/test/unit/selenium/webdriver/remote/remote_connection_tests.py

@qodo-code-review

qodo-code-review Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 17 rules

Grey Divider


Action required

1. Safari name not handled ✗ Dismissed 🐞 Bug ≡ Correctness
Description
get_remote_connection() no longer matches capabilities with browserName == "Safari", so those
requests fall back to the base RemoteConnection and lose SafariRemoteConnection command
registrations. This is a behavior regression from the prior explicit "Safari" branch and can break
callers using legacy/driver-emitted capability values.
Code

py/selenium/webdriver/remote/webdriver.py[131]

+    elif browser_name in ("safari", "Safari Technology Preview"):
Evidence
The PR changes Safari routing to only match "safari" and "Safari Technology Preview", removing prior
support for "Safari". Selenium’s own tests demonstrate a Safari session can report "Safari" as the
browserName, making this regression plausible for real callers/capability payloads.

py/selenium/webdriver/remote/webdriver.py[105-148]
py/test/selenium/webdriver/safari/launcher_tests.py[26-28]
py/conftest.py[274-281]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`get_remote_connection()` stopped matching `browserName == "Safari"` (capital S). Any callers still sending that value will now fall through to the base `RemoteConnection`, losing Safari-specific command registrations.

## Issue Context
Selenium’s own Safari launcher test asserts that Safari reports `browserName` as `"Safari"` (capital S) in at least one configuration, so this value exists in the ecosystem and was previously supported by `get_remote_connection()`.

## Fix Focus Areas
- py/selenium/webdriver/remote/webdriver.py[117-136]

### Suggested change
Expand the Safari match to include the legacy value, e.g.:
- `elif browser_name in ("Safari", "safari", "Safari Technology Preview"):`

(Alternatively, normalize `browser_name` once and compare case-insensitively, but keep the explicit STP string as needed.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Test docstring missing Args ✓ Resolved 📘 Rule violation ✧ Quality
Description
test_get_remote_connection_selects_browser_specific_handler has parameters but its new docstring
does not follow Google-style Args: documentation. This reduces consistency and makes the test
contract harder to understand and maintain.
Code

py/test/unit/selenium/webdriver/remote/remote_connection_tests.py[R643-648]

+def test_get_remote_connection_selects_browser_specific_handler(
+    options: BaseOptions,
+    prepare_options: Callable[[BaseOptions], None] | None,
+    expected_handler: type[RemoteConnection],
+) -> None:
+    """Test that each browserName, including variant capabilities, selects its RemoteConnection handler."""
Evidence
PR Compliance ID 337804 requires Google-style docstrings including an Args: section when a
function has parameters beyond self/cls. The added test function includes parameters but only
has a one-line summary docstring with no Args: section.

Rule 337804: Enforce Google-style docstrings with Args/Returns/Raises sections
py/test/unit/selenium/webdriver/remote/remote_connection_tests.py[643-648]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The newly added test function docstring is not Google-style and does not document its parameters under an `Args:` section.

## Issue Context
Compliance requires Google-style docstrings with `Args:`/`Returns:`/`Raises:` sections as applicable for new/modified public functions. This test has parameters (`options`, `prepare_options`, `expected_handler`) but no `Args:` section.

## Fix Focus Areas
- py/test/unit/selenium/webdriver/remote/remote_connection_tests.py[643-648]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread py/selenium/webdriver/remote/webdriver.py
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit fbbf570

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit d231739

@cgoldberg cgoldberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.. I just kicked off CI.

@cgoldberg

Copy link
Copy Markdown
Member

@v-dermichev thanks for contributing!

@cgoldberg
cgoldberg merged commit e1f5608 into SeleniumHQ:trunk Jun 29, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants