Open
navin772
opened this issue 2 weeks ago
•
4 comments
User description
🔗 Related Issues
💥 What does this PR do?
Adds support for set_network_conditions command in the emulation module - https://w3c.github.io/webdriver-bidi/#command-emulation-setNetworkConditions
🔧 Implementation Notes
💡 Additional Considerations
Only Chrome supports this from 143, Edge might also support from 143 (we have 142 currently in trunk).
🔄 Types of changes
New feature (non-breaking change which adds functionality and tests!)
PR Type
Enhancement
Description
Adds set_network_conditions emulation command for BiDi
Supports offline network condition emulation via contexts
Supports offline network condition emulation via user contexts
Includes comprehensive tests for both context types
Diagram Walkthrough
flowchart LR
A["Emulation Module"] -->|"add method"| B["set_network_conditions"]
B -->|"accepts"| C["offline parameter"]
B -->|"accepts"| D["contexts or user_contexts"]
C -->|"creates"| E["networkConditions payload"]
D -->|"validates"| F["mutual exclusivity"]
E -->|"executes"| G["BiDi command"]
H["Test Suite"] -->|"validates"| I["offline with contexts"]
H -->|"validates"| J["offline with user_contexts"]
File Walkthrough
Relevant files
Enhancement
emulation.py
Add set_network_conditions emulation method
py/selenium/webdriver/common/bidi/emulation.py
Adds new set_network_conditions method to Emulation class
Accepts offline parameter to enable/disable offline mode
Accepts either contexts or user_contexts parameters
Includes validation to ensure mutual exclusivity and required parameters
Executes BiDi command with appropriate networkConditions payload
Below is a summary of compliance checks for this PR:
Security Compliance
⚪
Protocol parameter risk
Description: The code sends None for 'networkConditions' to clear overrides, which may be invalid per protocol; if the backend echoes or logs parameters unsafely this could cause unexpected behavior—verify protocol requires explicit clearing to avoid unintended states. emulation.py [460-465]
Referred Code
if offline:
params["networkConditions"] = {"type": "offline"}
else:
# if offline is False or None, then clear the override
params["networkConditions"] = None
Investigate and fix repeated "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04 with Chrome 65 / chromedriver 2.35 and Selenium 3.9.0.
Ensure subsequent ChromeDriver instantiations do not log the ConnectFailure error while the first instance works.
Provide guidance or code changes that prevent the connection refusal across multiple sessions.
Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance.
Status: No audit logs: The new method executes a critical emulation command without emitting any audit log entries indicating who triggered it, when, and with what parameters.
Referred Code
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.
contexts: List of browsing context IDs to apply the conditions to.
user_contexts: List of user context IDs to apply the conditions to.
Raises:
ValueError: If both contexts and user_contexts are provided, or if neither
contexts nor user_contexts are provided.
"""
if contexts is not None and user_contexts is not None:
raise ValueError("Cannot specify both contexts and user_contexts")
if contexts is None and user_contexts is None:
... (clipped 16 lines)
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation
Status: Input validation: The method validates mutual exclusivity but does not validate types/emptiness of lists or handle execution failures from the BiDi command with contextual errors.
Referred Code
if contexts is not None and user_contexts is not None:
raise ValueError("Cannot specify both contexts and user_contexts")
if contexts is None and user_contexts is None:
raise ValueError("Must specify either contexts or user_contexts")
params: dict[str, Any] = {}
if offline:
params["networkConditions"] = {"type": "offline"}
else:
# if offline is False or None, then clear the override
params["networkConditions"] = None
if contexts is not None:
params["contexts"] = contexts
elif user_contexts is not None:
params["userContexts"] = user_contexts
self.conn.execute(command_builder("emulation.setNetworkConditions", params))
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: Weak validation: External inputs like context IDs are passed through without validation or sanitization, and there is no safeguard against empty lists or invalid identifiers before sending to the backend.
Referred Code
if contexts is not None and user_contexts is not None:
raise ValueError("Cannot specify both contexts and user_contexts")
if contexts is None and user_contexts is None:
raise ValueError("Must specify either contexts or user_contexts")
params: dict[str, Any] = {}
if offline:
params["networkConditions"] = {"type": "offline"}
else:
# if offline is False or None, then clear the override
params["networkConditions"] = None
if contexts is not None:
params["contexts"] = contexts
elif user_contexts is not None:
params["userContexts"] = user_contexts
self.conn.execute(command_builder("emulation.setNetworkConditions", params))
✅ Implement the full network conditions specSuggestion 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.
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 overridesSuggestion 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.
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.
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"] = None
[ ] Apply / Chat
Suggestion 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 claritySuggestion 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.
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.
Also, do browsers support latency and download/upload throughput conditions? If so, we should add those.
They will support these in the future, currently as per spec, only offline is supported.
Is CI failing because we don't have Chrome 143 yet?
Only the windows CI is failing because they are not using the pinned browser, they use default gh actions chrome browser which is currently at 142, we had this issue earlier too. Once github has 143 as latest browser, the CI will pass.
Change the default and type hint for offline to False (offline=False)
Actually, the spec doesn't allows a boolean value, I added it for API simplicity, type: "offline" is what we send, I abstracted it to a boolean.
None resets to default.
@cgoldberg do you see any issue with this approach?
I think having just True and False as supported values is more simpler and intuitive, updated the PR to support only bool where False clears the override.
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}