selenium icon indicating copy to clipboard operation
selenium copied to clipboard

[py] Add tests for CDP code generation script

Open cgoldberg opened this issue 5 months ago • 3 comments

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.

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

File Walkthrough

Relevant files
Enhancement
+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 

cgoldberg avatar Aug 18 '25 13:08 cgoldberg

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

qodo-code-review[bot] avatar Aug 18 '25 13:08 qodo-code-review[bot]

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 <!-- /improve --apply_suggestion=2 -->
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 <!-- /improve_multi --more_suggestions=true -->

qodo-code-review[bot] avatar Aug 18 '25 13:08 qodo-code-review[bot]

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 avatar Aug 18 '25 14:08 cgoldberg