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 | |
| Tests |
cdp_generate_tests.pyAdd 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 |
|
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
|
PR Code Suggestions ✨
Explore these optional code suggestions:
| Category | Suggestion | 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 -->
| |
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.