[py][bidi]: add emulation command `set_network_conditions` by navin772 · Pull Request #16702 · SeleniumHQ/selenium
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | Impact |
| High-level |
✅ Implement the full network conditions spec
Suggestion Impact:The commit partially aligns the method signature/documentation with the suggested API by changing `offline` from `bool | None = None` to `bool = False` and updating the docstring to reflect clearing the override when False. However, it does not implement the broader spec support for latency/download/upload throughput.
code diff:
def set_network_conditions(
self,
- offline: bool | None = None,
+ offline: bool = False,
contexts: list[str] | None = None,
user_contexts: list[str] | None = None,
) -> None:
"""Set network conditions for the given contexts or user contexts.
Args:
- offline: True to emulate offline network conditions, False or None to disable.
+ offline: True to emulate offline network conditions, False to clear the override.
contexts: List of browsing context IDs to apply the conditions to.
The set_network_conditions method should be extended to support the full
WebDriver BiDi specification, which includes emulating various network speeds
with specific latency and throughput, not just the 'offline' state.
Examples:
py/selenium/webdriver/common/bidi/emulation.py [435-471]
def set_network_conditions(
self,
offline: bool | None = None,
contexts: list[str] | None = None,
user_contexts: list[str] | None = None,
) -> None:
"""Set network conditions for the given contexts or user contexts.
Args:
offline: True to emulate offline network conditions, False or None to disable.
... (clipped 27 lines)
Solution Walkthrough:
Before:
class Emulation:
def set_network_conditions(
self,
offline: bool | None = None,
contexts: list[str] | None = None,
user_contexts: list[str] | None = None,
) -> None:
# ...
params: dict[str, Any] = {}
if offline:
params["networkConditions"] = {"type": "offline"}
else:
# if offline is False or None, then clear the override
params["networkConditions"] = None
# ...
self.conn.execute(command_builder("emulation.setNetworkConditions", params))After:
class Emulation:
def set_network_conditions(
self,
offline: bool = False,
latency: int | None = None,
download_throughput: int | None = None,
upload_throughput: int | None = None,
contexts: list[str] | None = None,
user_contexts: list[str] | None = None,
) -> None:
# ...
params: dict[str, Any] = {}
if offline:
params["networkConditions"] = {"type": "offline"}
elif latency is not None or download_throughput is not None or upload_throughput is not None:
conditions = {"type": "network"}
if latency is not None: conditions["latency"] = latency
# ... add other throughput values
params["networkConditions"] = conditions
else:
params["networkConditions"] = None
# ...
self.conn.execute(command_builder("emulation.setNetworkConditions", params))Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that the PR only partially implements the WebDriver BiDi spec for set_network_conditions, omitting network speed and latency emulation, which is a significant part of the feature.
| Medium
|
Learned
best practice |
✅ Always cleanup network overrides
Suggestion Impact:The commit wrapped the network offline setting and assertion in a try/finally and moved the reset into the finally block, ensuring cleanup even on failure. It also applied a similar pattern to the user context case.
code diff:
+ try:
+ # Set offline
+ driver.emulation.set_network_conditions(offline=True, contexts=[context_id])
+ assert is_online(driver, context_id) is False
+ finally:
+ # Reset
+ driver.emulation.set_network_conditions(offline=False, contexts=[context_id])
+ assert is_online(driver, context_id) is True
Wrap the network conditions override with try/finally and reset in finally to
guarantee cleanup even if assertions fail.
py/test/selenium/webdriver/common/bidi_emulation_tests.py [584-598]
@pytest.mark.xfail_firefox
@pytest.mark.xfail_edge
def test_set_network_conditions_offline_with_context(driver, pages):
context_id = driver.current_window_handle
driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
assert is_online(driver, context_id) is True
- # Set offline
- driver.emulation.set_network_conditions(offline=True, contexts=[context_id])
- assert is_online(driver, context_id) is False
+ try:
+ # Set offline
+ driver.emulation.set_network_conditions(offline=True, contexts=[context_id])
+ assert is_online(driver, context_id) is False
+ finally:
+ # Always reset
+ driver.emulation.set_network_conditions(offline=None, contexts=[context_id])
- # Reset
- driver.emulation.set_network_conditions(offline=None, contexts=[context_id])
assert is_online(driver, context_id) is True[Suggestion processed]
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Pattern 1: Ensure external contexts/resources are cleaned up using try/finally to prevent leaks.
| Low
|
| General |
Improve method signature for clarity
To improve clarity and avoid unexpected side effects, change the offline
parameter in set_network_conditions to be a required boolean instead of bool |
None.
py/selenium/webdriver/common/bidi/emulation.py [460-464]
if offline:
params["networkConditions"] = {"type": "offline"}
else:
- # if offline is False or None, then clear the override
+ # if offline is False, then clear the override
params["networkConditions"] = NoneSuggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies that offline=False and offline=None have the same behavior, which could be confusing. Making offline a required boolean would improve API clarity, but the current implementation is consistent with other methods in the same class where None is used to clear an override.
| Low
|
✅ Use explicit boolean for test clarity
Suggestion Impact:The commit changed the reset calls from offline=None to offline=False, and also wrapped sections in try/finally to ensure reset, matching the suggestion's intent for explicit resetting.
code diff:
+ try:
+ # Set offline
+ driver.emulation.set_network_conditions(offline=True, contexts=[context_id])
+ assert is_online(driver, context_id) is False
+ finally:
+ # Reset
+ driver.emulation.set_network_conditions(offline=False, contexts=[context_id])
+ assert is_online(driver, context_id) is True
In the test test_set_network_conditions_offline_with_context, use offline=False
instead of offline=None to explicitly reset the network conditions, which
improves test readability.
py/test/selenium/webdriver/common/bidi_emulation_tests.py [596-598]
# Reset
-driver.emulation.set_network_conditions(offline=None, contexts=[context_id])
+driver.emulation.set_network_conditions(offline=False, contexts=[context_id])
assert is_online(driver, context_id) is True [Suggestion processed]
Suggestion importance[1-10]: 4
__
Why: The suggestion to use offline=False instead of offline=None for resetting network conditions is a valid point for improving test clarity and explicitness, though the current implementation with None is functionally correct.
| Low
|
|
| |