[py][bidi]: add BiDi script module commands by navin772 · Pull Request #15880 · SeleniumHQ/selenium

navin772 · 2025-06-09T08:24:54Z

User description

🔗 Related Issues

💥 What does this PR do?

Adds the low level API commands for the Script module - https://w3c.github.io/webdriver-bidi/#module-script

🔧 Implementation Notes

I have only implemented the commonly used types like EvaluateResult, RealmInfo, RealmType, Source and ResultOwnership. Other types are very nested and not required to be implemented explicitly, we can directly pass them as dict.

💡 Additional Considerations

This PR adds the low level APIs only (which are private), I will open another PR that adds the remaining High level BiDi script APIs:

pin()
unpin()
execute()
addDomMutationHandler()
removeDomMutationHandler()

🔄 Types of changes

New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Tests

Description

Add BiDi script module commands to Python bindings
- Implement low-level API for script module (evaluate, callFunction, disown, etc.)
- Add support for preload scripts, realms, and result ownership
- Provide dataclasses for script-related types (RealmInfo, EvaluateResult, etc.)
Add comprehensive tests for BiDi script module
- Test all new script commands and options (preload, disown, serialization, user activation, etc.)
- Validate error handling and edge cases for script APIs

Changes walkthrough 📝

Relevant files

Enhancement

script.py `Implement BiDi script module commands and types` py/selenium/webdriver/common/bidi/script.py Implement BiDi script module commands (evaluate, callFunction, disown, etc.) Add dataclasses for script-related types (RealmInfo, EvaluateResult, Source, etc.) Support preload scripts, realms, result ownership, and serialization options Add error handling for invalid argument combinations	+374/-0

Tests

bidi_script_tests.py `Add comprehensive tests for BiDi script module` py/test/selenium/webdriver/common/bidi_script_tests.py Add extensive tests for BiDi script module commands and options Test preload scripts, evaluate/callFunction, disown, realms, serialization, user activation Validate error handling and edge cases for script APIs Add utility for shadow root detection in serialization tests	+434/-0

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

qodo-code-review · 2025-06-09T08:25:26Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 1234 - Not compliant Non-compliant requirements: • Fix issue where JavaScript in link's href is not triggered on click() in Selenium 2.48 • Ensure compatibility with Firefox 42.0 • Restore functionality that worked in version 2.47.1 5678 - Not compliant Non-compliant requirements: • Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4 • Resolve connection refused errors for subsequent ChromeDriver instances • Ensure first ChromeDriver instance works without console errors
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Parameter Validation The validation in add_preload_script only checks for both contexts and user_contexts being not None, but doesn't validate if they are empty lists. Empty lists should probably be treated as None for consistency. if contexts is not None and user_contexts is not None: raise ValueError("Cannot specify both contexts and user_contexts") Test Coverage The disown test expects an exception when using disowned handles but catches any Exception. This should be more specific to catch the actual expected exception type from the WebDriver BiDi specification. with pytest.raises(Exception): driver.script.call_function( "function(obj) { return obj.foo; }", await_promise=False, target={"context": driver.current_window_handle}, arguments=[{"handle": handle}], )

qodo-code-review · 2025-06-09T08:26:13Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category Suggestion Impact

Possible issue

✅ ~~Validate required JSON fields~~

Suggestion Impact:

The suggestion was implemented with improvements. The commit added validation for required fields across multiple classes (RealmInfo, Source, EvaluateResult, ScriptMessage, RealmDestroyed) using individual field checks with specific error messages, and changed from json.get() to direct dictionary access for required fields.

code diff:

+        if "realm" not in json:
+            raise ValueError("Missing required field 'realm' in RealmInfo")
+        if "origin" not in json:
+            raise ValueError("Missing required field 'origin' in RealmInfo")
+        if "type" not in json:
+            raise ValueError("Missing required field 'type' in RealmInfo")
+
         return cls(
-            realm=json.get("realm"),
-            origin=json.get("origin"),
-            type=json.get("type"),
+            realm=json["realm"],
+            origin=json["origin"],
+            type=json["type"],
             context=json.get("context"),
             sandbox=json.get("sandbox"),
         )
@@ -83,7 +90,7 @@
     context: Optional[str] = None
 
     @classmethod
-    def from_json(cls, json: dict) -> "Source":
+    def from_json(cls, json: Dict[str, Any]) -> "Source":
         """Creates a Source instance from a dictionary.
 
         Parameters:
@@ -94,8 +101,11 @@
         -------
             Source: A new instance of Source.
         """
+        if "realm" not in json:
+            raise ValueError("Missing required field 'realm' in Source")
+
         return cls(
-            realm=json.get("realm"),
+            realm=json["realm"],
             context=json.get("context"),
         )
 
@@ -110,7 +120,7 @@
     exception_details: Optional[dict] = None
 
     @classmethod
-    def from_json(cls, json: dict) -> "EvaluateResult":
+    def from_json(cls, json: Dict[str, Any]) -> "EvaluateResult":
         """Creates an EvaluateResult instance from a dictionary.
 
         Parameters:
@@ -121,9 +131,14 @@
         -------
             EvaluateResult: A new instance of EvaluateResult.
         """
+        if "realm" not in json:
+            raise ValueError("Missing required field 'realm' in EvaluateResult")
+        if "type" not in json:
+            raise ValueError("Missing required field 'type' in EvaluateResult")
+
         return cls(
-            type=json.get("type"),
-            realm=json.get("realm"),
+            type=json["type"],
+            realm=json["realm"],
             result=json.get("result"),
             exception_details=json.get("exceptionDetails"),
         )
@@ -140,7 +155,7 @@
         self.source = source
 
     @classmethod
-    def from_json(cls, json: dict) -> "ScriptMessage":
+    def from_json(cls, json: Dict[str, Any]) -> "ScriptMessage":
         """Creates a ScriptMessage instance from a dictionary.
 
         Parameters:
@@ -151,10 +166,17 @@
         -------
             ScriptMessage: A new instance of ScriptMessage.
         """
+        if "channel" not in json:
+            raise ValueError("Missing required field 'channel' in ScriptMessage")
+        if "data" not in json:
+            raise ValueError("Missing required field 'data' in ScriptMessage")
+        if "source" not in json:
+            raise ValueError("Missing required field 'source' in ScriptMessage")
+
         return cls(
-            channel=json.get("channel"),
-            data=json.get("data"),
-            source=Source.from_json(json.get("source", {})),
+            channel=json["channel"],
+            data=json["data"],
+            source=Source.from_json(json["source"]),
         )
 
 
@@ -167,7 +189,7 @@
         self.realm_info = realm_info
 
     @classmethod
-    def from_json(cls, json: dict) -> "RealmCreated":
+    def from_json(cls, json: Dict[str, Any]) -> "RealmCreated":
         """Creates a RealmCreated instance from a dictionary.
 
         Parameters:
@@ -190,7 +212,7 @@
         self.realm = realm
 
     @classmethod
-    def from_json(cls, json: dict) -> "RealmDestroyed":
+    def from_json(cls, json: Dict[str, Any]) -> "RealmDestroyed":
         """Creates a RealmDestroyed instance from a dictionary.
 
         Parameters:
@@ -201,7 +223,10 @@
         -------
             RealmDestroyed: A new instance of RealmDestroyed.
         """
-        return cls(realm=json.get("realm"))
+        if "realm" not in json:
+            raise ValueError("Missing required field 'realm' in RealmDestroyed")
+
+        return cls(realm=json["realm"])

The from_json method should validate that required fields (realm, origin, type)
are present in the JSON data. Currently, it allows None values for required
fields which could cause issues downstream.

py/selenium/webdriver/common/bidi/script.py [57-75]

 @classmethod
 def from_json(cls, json: dict) -> "RealmInfo":
     """Creates a RealmInfo instance from a dictionary.
 
     Parameters:
     -----------
         json: A dictionary containing the realm information.
 
     Returns:
     -------
         RealmInfo: A new instance of RealmInfo.
     """
+    if not json.get("realm") or not json.get("origin") or not json.get("type"):
+        raise ValueError("Required fields 'realm', 'origin', and 'type' must be present")
+    
     return cls(
         realm=json.get("realm"),
         origin=json.get("origin"),
         type=json.get("type"),
         context=json.get("context"),
         sandbox=json.get("sandbox"),
     )

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the from_json method for RealmInfo does not validate the presence of required fields (realm, origin, type). The RealmInfo dataclass defines these as non-optional strings, so allowing None values from json.get() could lead to downstream errors. Adding validation improves robustness by ensuring the data conforms to the expected structure.

Medium

Update

shbenzer

LGTM, great work @navin772

navin772 · 2025-06-09T17:20:50Z

@shbenzer thanks for the review! I have one doubt, currently the low level APIs implemented in this PR are available via the driver instance like - driver.script.add_preload_script(js_script).

I am planning to add these high level APIs for the script module:

pin()
unpin()
execute()
addDomMutationHandler()
removeDomMutationHandler()

These will also be available via the same driver instance - driver.script.pin(), is that good or should we do something else? I don't want to make the low-level API private since those can be used by users/other projects that uses selenium.

shbenzer · 2025-06-09T17:30:01Z

@shbenzer thanks for the review! I have one doubt, currently the low level APIs implemented in this PR are available via the driver instance like - driver.script.add_preload_script(js_script).

I am planning to add these high level APIs for the script module:

pin()

unpin()

execute()

addDomMutationHandler()

removeDomMutationHandler()

These will also be available via the same driver instance - driver.script.pin(), is that good or should we do something else? I don't want to make the low-level API private since those can be used by users/other projects that uses selenium.

methods prepended with _ are marked as private in python but are still accessible

navin772 · 2025-06-09T17:36:29Z

methods prepended with _ are marked as private in python but are still accessible

yes, they are accessible, but I am not sure if that's a nice way to access the low level API, since we are not doing the _ private marking in other modules.

shbenzer · 2025-06-09T19:04:32Z

I used _ in the Network module, but I’m definitely open to other suggestions

navin772 · 2025-06-10T17:09:18Z

@shbenzer I will add the _ prefix to make them low level API. Other option might be to add a prefix like bidi_add_preload_script() but since we already use _ in other module, it should be consistent.

shbenzer

LGTM Great job @navin772

navin772 · 2025-06-17T05:50:54Z

The //java/test/org/openqa/selenium/bidi/storage:StorageCommandsTest-edge test is failing in the CI and recent runs on trunk which is unrelated to this PR.
I tried debugging the test but it is passing locally. Probably a Edge browser version issue.

navin772 added 5 commits June 7, 2025 22:27

add bidi script commands

f4ae779

add bidi script tests

6724b42

improve some test assertions

831cd0c

modify tests for result_ownership and serialization_options

df26042

modify tests for disown and user_activation

eb4bc25

selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jun 9, 2025

Merge branch 'trunk' into script-bidi-py

166e584

qodo-code-review bot added the Review effort 4/5 label Jun 9, 2025

resolve mypy type errors

a02f5db

navin772 requested review from cgoldberg and shbenzer June 9, 2025 13:04

shbenzer approved these changes Jun 9, 2025

View reviewed changes

change to private methods

ecec96e

navin772 requested a review from shbenzer June 10, 2025 18:11

shbenzer approved these changes Jun 17, 2025

View reviewed changes

Merge branch 'trunk' into script-bidi-py

5a345ef

navin772 merged commit e033b6f into SeleniumHQ:trunk Jun 17, 2025
15 of 16 checks passed

navin772 deleted the script-bidi-py branch June 18, 2025 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[py][bidi]: add BiDi script module commands#15880

[py][bidi]: add BiDi script module commands#15880
navin772 merged 9 commits intoSeleniumHQ:trunkfrom
navin772:script-bidi-py

navin772 commented Jun 9, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Jun 9, 2025

Uh oh!

qodo-code-review bot commented Jun 9, 2025 •

edited

Loading

Uh oh!

shbenzer left a comment

Uh oh!

navin772 commented Jun 9, 2025

Uh oh!

shbenzer commented Jun 9, 2025

Uh oh!

navin772 commented Jun 9, 2025

Uh oh!

shbenzer commented Jun 9, 2025 •

edited

Loading

Uh oh!

navin772 commented Jun 10, 2025

Uh oh!

shbenzer left a comment

Uh oh!

navin772 commented Jun 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

navin772 commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-code-review bot commented Jun 9, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review bot commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

shbenzer left a comment

Choose a reason for hiding this comment

Uh oh!

navin772 commented Jun 9, 2025

Uh oh!

shbenzer commented Jun 9, 2025

Uh oh!

navin772 commented Jun 9, 2025

Uh oh!

shbenzer commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

navin772 commented Jun 10, 2025

Uh oh!

shbenzer left a comment

Choose a reason for hiding this comment

Uh oh!

navin772 commented Jun 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

navin772 commented Jun 9, 2025 •

edited

Loading

qodo-code-review bot commented Jun 9, 2025 •

edited

Loading

shbenzer commented Jun 9, 2025 •

edited

Loading