[py][bidi]: add speculation module#17162
Conversation
Review Summary by QodoAdd speculation module to Python BiDi bindings
WalkthroughsDescription• Adds speculation module to Python BiDi bindings • Implements prefetch status event handling and subscription management • Provides event handler registration, removal, and cleanup methods • Includes comprehensive test coverage for prefetch status events File Changes1. py/selenium/webdriver/common/bidi/speculation.py
|
Code Review by Qodo
1. Missing speculationRules fixture
|
selenium-ci
added
C-py
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| def _add_speculation_rules_and_link(driver, prefetch_url): | ||
| driver.execute_script(f"addSpeculationRulesAndLink('{prefetch_url}')") | ||
|
|
||
|
|
||
| def test_speculation_module_initialized(driver): | ||
| assert driver.speculation is not None | ||
|
|
||
|
|
||
| def test_prefetch_status_updated_with_pending_and_ready_events(driver, pages): | ||
| """Test that prefetch status updated events are received with pending and ready statuses.""" | ||
| events_received = [] | ||
|
|
||
| def on_prefetch_status_updated(event): | ||
| events_received.append(event) | ||
|
|
||
| callback_id = driver.speculation.add_event_handler("prefetch_status_updated", on_prefetch_status_updated) | ||
|
|
||
| try: | ||
| url = pages.url("bidi/speculationRules.html") | ||
| prefetch_url = pages.url("bidi/emptyPage.html") | ||
| driver.browsing_context.navigate( | ||
| context=driver.current_window_handle, | ||
| url=url, | ||
| wait=ReadinessState.COMPLETE, | ||
| ) | ||
|
|
||
| _add_speculation_rules_and_link(driver, prefetch_url) | ||
|
|
||
| # Wait for at least two events (pending + ready) | ||
| WebDriverWait(driver, 10).until(lambda _: len(events_received) >= 2) | ||
|
|
There was a problem hiding this comment.
1. Missing speculationrules fixture 🐞 Bug ✓ Correctness
The new speculation tests navigate to bidi/speculationRules.html and call addSpeculationRulesAndLink(), but this PR does not add that HTML/JS fixture. The test web server returns 404 for missing files, so tests will fail (or error on undefined JS) before any BiDi events can be asserted.
Agent Prompt
### Issue description
The speculation BiDi tests navigate to `bidi/speculationRules.html` and execute `addSpeculationRulesAndLink(...)`, but the PR does not include this HTML/JS fixture. The test webserver will return 404 for missing files, causing the tests to fail early.
### Issue Context
The webserver serves from `common/src/web` and returns 404 if the file doesn’t exist.
### Fix Focus Areas
- common/src/web/bidi/speculationRules.html[1-200]
- py/test/selenium/webdriver/common/bidi_speculation_tests.py[25-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Pull request overview
Adds initial Python BiDi “speculation” module support (prefetch status events) and exposes it on RemoteWebDriver, along with a new pytest suite intended to validate event subscription behavior.
Changes:
- Introduces
selenium.webdriver.common.bidi.speculationwith event parsing and subscribe/unsubscribe helpers. - Exposes the module as
driver.speculationin PythonRemoteWebDriver. - Adds Python tests for
speculation.prefetchStatusUpdatedevent handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
py/selenium/webdriver/common/bidi/speculation.py |
New Speculation BiDi module (event classes + subscription management). |
py/selenium/webdriver/remote/webdriver.py |
Adds driver.speculation property and internal caching. |
py/test/selenium/webdriver/common/bidi_speculation_tests.py |
New end-to-end tests for speculation prefetch status events. |
| # Reload and trigger new speculation rules with different target | ||
| driver.browsing_context.navigate( | ||
| context=driver.current_window_handle, | ||
| url=url, | ||
| wait=ReadinessState.COMPLETE, | ||
| ) | ||
|
|
||
| second_prefetch_url = pages.url("blank.html") | ||
| _add_speculation_rules_and_link(driver, second_prefetch_url) | ||
|
|
||
| assert len(events_received) == initial_count |
There was a problem hiding this comment.
This test triggers a second prefetch after unsubscribing but immediately asserts the event count didn’t change. Because prefetch status events are asynchronous, the assertion can pass even if events arrive slightly later (making the test non-verifying/flaky). Add a short wait after triggering the second prefetch and assert the count remains unchanged over that window (or wait for a condition that would fail if an event is received).
| # Reverse mapping: BiDi event name to event class | ||
| _BIDI_TO_CLASS: dict[str, type] = { | ||
| "speculation.prefetchStatusUpdated": PrefetchStatusUpdated, |
There was a problem hiding this comment.
_EVENT_MAP and _BIDI_TO_CLASS contain the same information in two places. This duplication is easy to get out of sync when adding events (and clear_event_handlers() depends on _BIDI_TO_CLASS to clean up). Derive _BIDI_TO_CLASS from _EVENT_MAP (or store event_class alongside callback IDs) so a single source of truth is used.
| # Reverse mapping: BiDi event name to event class | |
| _BIDI_TO_CLASS: dict[str, type] = { | |
| "speculation.prefetchStatusUpdated": PrefetchStatusUpdated, | |
| # Reverse mapping: BiDi event name to event class, derived from _EVENT_MAP | |
| _BIDI_TO_CLASS: dict[str, type] = { | |
| bidi_event_name: event_class for _, (bidi_event_name, event_class) in _EVENT_MAP.items() |
| for bidi_event, callback_ids in list(self.subscriptions.items()): | ||
| event_class = self._BIDI_TO_CLASS.get(bidi_event) | ||
| if event_class: | ||
| for callback_id in callback_ids: | ||
| self.conn.remove_callback(event_class, callback_id) | ||
| self.conn.execute(self._session.unsubscribe(bidi_event)) | ||
|
|
There was a problem hiding this comment.
clear_event_handlers() only unsubscribes when event_class is found in _BIDI_TO_CLASS. If the maps ever drift, this will leave remote-end event subscriptions active (and leak callbacks locally). Consider always calling session.unsubscribe(bidi_event) even if the event class can’t be resolved, and/or ensuring the reverse mapping is automatically generated so this can’t happen.
| """BiDi implementation of the speculation module. | ||
|
|
||
| The speculation module contains commands for managing the remote end | ||
| behavior for prefetches, prerenders, and speculation rules. |
There was a problem hiding this comment.
The docstring says the speculation module provides “commands for managing … prefetches, prerenders, and speculation rules”, but the current Speculation implementation only exposes event subscription helpers. Consider adjusting the wording to match the actual API surface (events-only), or implement the described commands if they’re intended to be part of this module in this PR.
| """BiDi implementation of the speculation module. | |
| The speculation module contains commands for managing the remote end | |
| behavior for prefetches, prerenders, and speculation rules. | |
| """BiDi interface for the speculation module. | |
| This class exposes helpers for subscribing to and handling speculation | |
| module events (such as prefetch status updates) and managing the | |
| associated event subscriptions. |
| """Returns a speculation module object for BiDi speculation commands. | ||
|
|
||
| The speculation module contains commands for managing the remote end | ||
| behavior for prefetches, prerenders, and speculation rules. | ||
|
|
||
| Returns: | ||
| An object containing access to BiDi speculation events. |
There was a problem hiding this comment.
This property’s docstring describes “commands for managing … prefetches, prerenders, and speculation rules”, but driver.speculation currently only provides event handler registration/removal. Please align the docstring with the actual feature set to avoid confusing users.
| """Returns a speculation module object for BiDi speculation commands. | |
| The speculation module contains commands for managing the remote end | |
| behavior for prefetches, prerenders, and speculation rules. | |
| Returns: | |
| An object containing access to BiDi speculation events. | |
| """Returns a speculation module object for BiDi speculation events. | |
| The speculation module exposes events related to prefetches, prerenders, | |
| and speculation rules, and allows registering and removing event | |
| handlers for those events. | |
| Returns: | |
| A Speculation object used to register and remove BiDi speculation | |
| event handlers. |
| driver.execute_script(f"addSpeculationRulesAndLink('{prefetch_url}')") | ||
|
|
||
|
|
There was a problem hiding this comment.
The helper relies on a page-scoped JavaScript function addSpeculationRulesAndLink(...), but this PR doesn’t add the corresponding test fixture / script. As-is, this will raise a JS error at runtime and the speculation tests will fail. Consider inlining the DOM+speculationrules setup via execute_script (similar to other bindings) or add the missing fixture that defines this function.
| driver.execute_script(f"addSpeculationRulesAndLink('{prefetch_url}')") | |
| driver.execute_script( | |
| """ | |
| const prefetchUrl = arguments[0]; | |
| // Create speculation rules script | |
| const script = document.createElement('script'); | |
| script.type = 'speculationrules'; | |
| script.textContent = JSON.stringify({ | |
| prefetch: [ | |
| { | |
| source: 'document', | |
| href: prefetchUrl, | |
| }, | |
| ], | |
| }); | |
| document.head.appendChild(script); | |
| // Create a link to the prefetched URL so tests can navigate via click | |
| const link = document.createElement('a'); | |
| link.href = prefetchUrl; | |
| link.textContent = 'navigate'; | |
| document.body.appendChild(link); | |
| """, | |
| prefetch_url, | |
| ) |
| url = pages.url("bidi/speculationRules.html") | ||
| prefetch_url = pages.url("bidi/emptyPage.html") | ||
| driver.browsing_context.navigate( |
There was a problem hiding this comment.
pages.url("bidi/speculationRules.html") points to a fixture that doesn’t exist under common/src/web/bidi/ in this repo. These tests will fail with a 404 unless that HTML fixture is added (and contains the required speculationrules/link setup).
| def test_speculation_module_initialized(driver): | ||
| assert driver.speculation is not None | ||
|
|
||
|
|
||
| def test_prefetch_status_updated_with_pending_and_ready_events(driver, pages): |
There was a problem hiding this comment.
Speculation BiDi support appears to be unavailable in Firefox (the repo’s .NET speculation test is explicitly ignored for Firefox). These tests should be marked xfail_firefox/skipped accordingly to avoid consistent failures when the Python suite runs against Firefox.
🔗 Related Issues
💥 What does this PR do?
Adds the speculation module to python bindings - https://wicg.github.io/nav-speculation/prefetch.html#speculation-module
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes