Skip to content

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Aug 18, 2025

User description

🔗 Related Issues

Related to #16161

💥 What does this PR do?

This PR pulls in the tests from the upstream HyperionGray/python-chrome-devtools-protocol repo that we get our CDP generation code from (py/generate.py). It adds it to our Python unit test suite.

This PR also adds some type annotations to py/generate.py that exist in the upstream version.

The tests currently all pass against our modified version of py/generate.py.

This will be helpful if we need to modify the generation code.


PR Type

Tests


Description

  • Add comprehensive test suite for CDP code generation script

  • Import tests from upstream HyperionGray repository

  • Add type annotations to generation functions

  • Ensure test coverage for all CDP generation components


Diagram Walkthrough

flowchart LR
  A["Upstream Tests"] --> B["py/test/unit/cdp_generate_tests.py"]
  C["py/generate.py"] --> D["Type Annotations Added"]
  B --> E["Test Coverage for CDP Generation"]
  D --> E
Loading

File Walkthrough

Relevant files
Enhancement
generate.py
Add type annotations to CDP generator                                       

py/generate.py

  • Add type annotations to to_json() and from_json() methods
  • Update comment to reflect modified upstream source
  • Improve flake8 configuration for line length
+8/-7     
Tests
cdp_generate_tests.py
Add comprehensive CDP generation test suite                           

py/test/unit/cdp_generate_tests.py

  • Import comprehensive test suite from upstream repository
  • Add tests for docstring generation and escaping
  • Include tests for all CDP type generation (primitive, array, enum,
    class)
  • Add command and event generation test coverage
  • Test domain imports and Sphinx documentation generation
+866/-0 

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 18, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Specificity

The added return/param annotations in generated code snippets (e.g., AXValueSourceType, AXValue) look hardcoded in the generator; ensure these names are derived dynamically from the current type/domain being generated to avoid incorrect annotations when generating different enums/classes.

def_to_json = dedent('''\
    def to_json(self) -> str:
        return self.value''')

def_from_json = dedent('''\
    @classmethod
    def from_json(cls, json: str) -> AXValueSourceType:
        return cls(json)''')
Flake8 Scope

Narrowing flake8 ignore to E501 is good, but confirm other style errors from generated code won’t surface and fail CI since this file intentionally emits long lines and possibly other patterns; consider module-local noqa pragmas where needed.

# flake8: noqa: E501

Copy link
Contributor

qodo-merge-pro bot commented Aug 18, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix generic enum return typing
Suggestion Impact:The commit changed the from_json return type from a specific enum (AXValueSourceType/AXValue) to the dynamically generated enum class ({self.id}), addressing the hard-coded type issue. It also updated typing imports.

code diff:

@@ -393,9 +393,9 @@
             def to_json(self) -> str:
                 return self.value''')
 
-        def_from_json = dedent('''\
+        def_from_json = dedent(f'''\
             @classmethod
-            def from_json(cls, json: str) -> AXValueSourceType:
+            def from_json(cls, json: str) -> {self.id}:
                 return cls(json)''')
 
         code = f'class {self.id}(enum.Enum):\n'
@@ -447,9 +447,9 @@
 
         # Emit from_json() method. The properties are sorted in the same order
         # as above for readability.
-        def_from_json = dedent('''\
+        def_from_json = dedent(f'''\
             @classmethod
-            def from_json(cls, json: T_JSON_DICT) -> AXValue:
+            def from_json(cls, json: T_JSON_DICT) -> {self.id}:
                 return cls(
         ''')
         from_jsons = []

Avoid hard-coding a specific enum type in a generic enum generator. Returning a
concrete type like AXValueSourceType will break for other enums. Use the current
class (cls) as the return type hint to keep generated code valid for all enums.

py/generate.py [392-399]

 def_to_json = dedent('''\
             def to_json(self) -> str:
                 return self.value''')
 
         def_from_json = dedent('''\
             @classmethod
-            def from_json(cls, json: str) -> AXValueSourceType:
+            def from_json(cls, json: str) -> "typing.Self":
                 return cls(json)''')

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug where a specific type AXValueSourceType is hardcoded in a generic enum code generator, which would cause incorrect code generation for all other enum types.

High
Generalize class return typing
Suggestion Impact:The commit replaced hard-coded return types (e.g., AXValue, AXValueSourceType) in generated from_json methods with the dynamic class identifier {self.id}, and also adjusted typing imports (adding Self), aligning with the suggestion to generalize return typing.

code diff:

@@ -393,9 +393,9 @@
             def to_json(self) -> str:
                 return self.value''')
 
-        def_from_json = dedent('''\
+        def_from_json = dedent(f'''\
             @classmethod
-            def from_json(cls, json: str) -> AXValueSourceType:
+            def from_json(cls, json: str) -> {self.id}:
                 return cls(json)''')
 
         code = f'class {self.id}(enum.Enum):\n'
@@ -447,9 +447,9 @@
 
         # Emit from_json() method. The properties are sorted in the same order
         # as above for readability.
-        def_from_json = dedent('''\
+        def_from_json = dedent(f'''\
             @classmethod
-            def from_json(cls, json: T_JSON_DICT) -> AXValue:
+            def from_json(cls, json: T_JSON_DICT) -> {self.id}:
                 return cls(
         ''')
         from_jsons = []

Do not hard-code a specific class name in the generator for object types. Use
typing.Self (or the class generic) as the return type so the generated from_json
has the correct annotation for any class.

py/generate.py [450-454]

 def_from_json = dedent('''\
             @classmethod
-            def from_json(cls, json: T_JSON_DICT) -> AXValue:
+            def from_json(cls, json: T_JSON_DICT) -> "typing.Self":
                 return cls(
 ''')

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug where a specific type AXValue is hardcoded in a generic class code generator, which would cause incorrect code generation for all other class types.

High
General
Use literal dict initialization

Initialize the JSON dict with a literal for clarity and potential minor
performance benefits. Using {} is idiomatic and avoids the extra name lookup for
dict.

py/generate.py [438-441]

 def_to_json = dedent('''\
             def to_json(self) -> T_JSON_DICT:
-                json: T_JSON_DICT = dict()
+                json: T_JSON_DICT = {}
 ''')
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion proposes using {} instead of dict() for dictionary initialization, which is a valid and common stylistic preference for improved readability and minor performance gain.

Low
  • Update

@cgoldberg
Copy link
Contributor Author

This won't work without some bazel changes because the py/generate.py script is not included it runs tests. I am going to mark this as a draft for now.

@cgoldberg cgoldberg marked this pull request as draft August 18, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants