Skip to content

[py]: fix W3C capabilities built from a list of options (create_matches) - #17897

Merged
navin772 merged 2 commits into
SeleniumHQ:trunkfrom
navin772:py-create-matches
Aug 11, 2026
Merged

navin772 merged 2 commits into
SeleniumHQ:trunkfrom
navin772:py-create-matches

Conversation

@navin772

Copy link
Copy Markdown
Member

🔗 Related Issues

💥 What does this PR do?

Passing a list of options (WebDriver(options=[...])) built a broken new-session payload: create_matches crashed or dropped capabilities for 3+/mixed-browser lists, and start_session double-wrapped the result.

Now alwaysMatch keeps only capabilities identical across all options (disjoint from firstMatch), and the W3C object is no longer wrapped twice.

🔧 Implementation Notes

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude code
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 10, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix W3C capability matching for option lists and avoid double-wrapping

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Compute W3C alwaysMatch as the true intersection across all provided options.
• Prevent start_session from re-wrapping already-formed W3C capability payloads.
• Add regressions for 3+ options and mixed-browser option lists.
Diagram

graph TD
A["Caller"] --> B["WebDriver.__init__"] --> C["create_matches(options[])" ] --> D{"W3C caps?"}
D -->|"Yes"| E["Deepcopy + add se:remoteUrl"] --> F["Execute NEW_SESSION"] --> G["Remote end"]
D -->|"No"| H["_create_caps wrapper"] --> F --> G
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Build firstMatch entries without mutating option dicts
  • ➕ Avoids in-place deletion of keys from dicts returned by to_capabilities()
  • ➕ Makes the transformation more obviously side-effect free
  • ➖ Slightly more code (need filtered copies per option)
  • ➖ Does not materially change correctness given current usage pattern
2. Compute intersection via iterative dict filtering (reduce-style)
  • ➕ Can be expressed compactly and generalized (progressively narrow alwaysMatch)
  • ➖ Harder to read than the current 'compare against first set' approach
  • ➖ Still needs explicit handling for unhashable list/dict values

Recommendation: Keep the PR’s approach: deriving alwaysMatch by validating each key/value from the first option against all others is straightforward, spec-aligned, and avoids unhashable-set pitfalls. If future contributors worry about side effects, consider a follow-up refactor to build filtered firstMatch copies instead of deleting keys in-place.

Files changed (2) +69 / -27

Bug fix (1) +26 / -27
webdriver.pyFix create_matches intersection logic and prevent W3C double-wrapping +26/-27

Fix create_matches intersection logic and prevent W3C double-wrapping

• Replaces adjacent-pair matching with a true all-options intersection for alwaysMatch, keeping non-shared capabilities in firstMatch. Updates start_session to detect an already-formed W3C capabilities object and only wrap flat capability dicts, while still injecting se:remoteUrl correctly.

py/selenium/webdriver/remote/webdriver.py

Tests (1) +43 / -0
new_session_tests.pyAdd regressions for multi-option firstMatch and new-session payload shape +43/-0

Add regressions for multi-option firstMatch and new-session payload shape

• Adds unit tests covering three-option lists with mixed browsers, ensuring unique capabilities remain in firstMatch and alwaysMatch only contains true common keys. Verifies that passing a list of options does not produce a double-wrapped capabilities payload in NEW_SESSION.

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. New tests lack type hints 📘 Rule violation ✧ Quality
Description
Newly added test functions are missing parameter and return type annotations (e.g., mocker is
untyped and no -> None is declared). This violates the project requirement to annotate new
function signatures and reduces type-checking clarity.
Code

py/test/unit/selenium/webdriver/remote/new_session_tests.py[174]

+def test_list_of_options_is_not_double_wrapped_in_new_session(mocker):
Evidence
PR Compliance ID 337802 requires explicit type annotations for parameters and return types on newly
added functions. The newly added tests define functions without return annotations, and
test_list_of_options_is_not_double_wrapped_in_new_session also introduces an untyped mocker
parameter.

Rule 337802: Require type annotations on new function and method signatures
py/test/unit/selenium/webdriver/remote/new_session_tests.py[145-145]
py/test/unit/selenium/webdriver/remote/new_session_tests.py[159-159]
py/test/unit/selenium/webdriver/remote/new_session_tests.py[174-174]

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

## Issue description
Newly added test functions are missing type annotations on their signatures (including a `-> None` return type, and a type for the `mocker` fixture parameter).

## Issue Context
Per the compliance checklist, all new/modified function and method signatures should have explicit parameter and return annotations.

## Fix Focus Areas
- py/test/unit/selenium/webdriver/remote/new_session_tests.py[145-145]
- py/test/unit/selenium/webdriver/remote/new_session_tests.py[159-159]
- py/test/unit/selenium/webdriver/remote/new_session_tests.py[174-174]

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


Grey Divider

Context used
✅ Compliance rules (platform): 17 rules

Grey Divider

Tip of the day
💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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

qodo-code-review Bot commented Aug 11, 2026 •

Copy link
Copy Markdown
Contributor

No code changes since the last review — review skipped

Qodo Logo

@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 👍

@navin772
navin772 merged commit 86293a1 into SeleniumHQ:trunk Aug 11, 2026
31 checks passed
@navin772
navin772 deleted the py-create-matches branch August 11, 2026 13:34
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