[Python] Improve typing#71812
Draft
MarekPikula wants to merge 27 commits intoproject-chip:masterfrom
Draft
Conversation
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by updating type hints to use PEP 604 and PEP 585 syntax, adding missing return type annotations, and enhancing null safety through explicit checks. It also includes the removal of redundant type stub files. The review feedback recommends moving the initialization of the process attribute to the constructor in the Subprocess class to prevent potential attribute errors and removing the corresponding redundant assignment in the run method.
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes and tightens Python typing across the Matter Python testing infrastructure and related test tooling, while removing several out-of-sync stub (.pyi) files and improving editor/type-checker module resolution.
Changes:
- Replace legacy
typing.*constructs with Python 3.11+ syntax (e.g.,X | None,list[str],Self,TypeAlias,ParamSpec/Concatenate) and add missing return/arg annotations across multiple modules. - Remove stale
matter/typings/matter/testing/*.pyistubs and adjust project configuration for Pyright/Mypy. - Make a few small runtime-safety tweaks (e.g.,
Nonechecks around subprocess handles; safer cache entry type checks).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/python_testing/matter_testing_infrastructure/matter/typings/matter/testing/tasks.pyi | Deleted outdated stub for matter.testing.tasks. |
| src/python_testing/matter_testing_infrastructure/matter/typings/matter/testing/pics.pyi | Deleted outdated stub for matter.testing.pics. |
| src/python_testing/matter_testing_infrastructure/matter/typings/matter/testing/decorators.pyi | Deleted outdated stub for matter.testing.decorators. |
| src/python_testing/matter_testing_infrastructure/matter/typings/matter/testing/apps.pyi | Deleted outdated stub for matter.testing.apps. |
| src/python_testing/matter_testing_infrastructure/matter/typings/matter/testing/init.pyi | Deleted package stub placeholder. |
| src/python_testing/matter_testing_infrastructure/matter/testing/tasks.py | Modernize typing and adjust subprocess/thread helper typing/guards. |
| src/python_testing/matter_testing_infrastructure/matter/testing/taglist_and_topology_test.py | Add explicit comparator return type. |
| src/python_testing/matter_testing_infrastructure/matter/testing/spec_parsing.py | Broad typing modernization and explicit TypeAlias/return annotations. |
| src/python_testing/matter_testing_infrastructure/matter/testing/runner.py | Add/adjust annotations, TYPE_CHECKING imports, and hook signatures. |
| src/python_testing/matter_testing_infrastructure/matter/testing/matter_testing.py | Typing modernization; remove redundant async runner helper; add richer annotations. |
| src/python_testing/matter_testing_infrastructure/matter/testing/decorators.py | Add ParamSpec/Concatenate-based typing for async decorators. |
| src/python_testing/matter_testing_infrastructure/matter/testing/conformance.py | Modernize optionals and tighten callable signatures/returns. |
| src/python_testing/matter_testing_infrastructure/matter/testing/choice_conformance.py | Add missing return type for helper. |
| src/python_testing/matter_testing_infrastructure/matter/testing/basic_composition.py | Typing modernization; remove redundant attribute type declarations. |
| src/python_testing/matter_testing_infrastructure/matter/testing/apps.py | Safer subprocess typing and extra_args defaults; guard subprocess handle usage. |
| src/controller/python/matter/clusters/init.py | Export ClusterObjects at package level for import convenience. |
| scripts/tests/chipyaml/paths_finder.py | Add type checks/annotations for cached path resolution and CLI commands. |
| scripts/tests/chiptest/test_definition.py | Typing improvements (return types, generics) for chiptest harness structures. |
| scripts/tests/chiptest/runner.py | Typing improvements for log matching and executor return types. |
| scripts/tests/chiptest/linux.py | Add subprocess import and return type for executor wrapper. |
| scripts/tests/chiptest/darwin.py | Add subprocess import and return type for executor wrapper. |
| scripts/tests/chiptest/init.py | Modernize typing (use Iterable, set[str]) across YAML test discovery helpers. |
| scripts/py_matter_yamltests/matter/yamltests/parser.py | Fix optional dataclass field typing for config object. |
| pyproject.toml | Configure Pyright extraPaths and adjust Mypy settings for stricter checking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
|
PR #71812: Size comparison from 3f9cd16 to 8d192e0 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71812 +/- ##
=======================================
Coverage 54.52% 54.52%
=======================================
Files 1588 1588
Lines 108569 108572 +3
Branches 13365 13365
=======================================
+ Hits 59201 59204 +3
Misses 49368 49368 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
|
PR #71812: Size comparison from 3f9cd16 to 80f687a Full report (16 builds for cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Co-authored-by: Copilot <copilot@github.com> Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/controller/python/matter/clusters/Attribute.py:360
AttributeCache.GetUpdatedAttributeCache()is annotated as returningdict[int, list[Cluster]], but the implementation builds and returns a nested mapping keyed by endpoint then by cluster type (and then by attribute type / DataVersion key), as described in the docstring and used throughout the code (endpointCache[clusterType] = ...). Please update the return type to reflect the actual structure (or introduce a dedicated TypeAlias used here and inAsyncReadTransaction.ReadResponse.attributes) to avoid propagating incorrect types to callers.
def GetUpdatedAttributeCache(self) -> dict[int, list[Cluster]]:
''' This converts the raw TLV data into a cluster object format.
Two formats are available:
1. Attribute-View (returnClusterObject=False): Dict[EndpointId,
Dict[ClusterObjectType,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def _get_all_matching_endpoints(test_instance, accept_function: EndpointCheckFunction) -> list[int]: | ||
| async def _get_all_matching_endpoints(test_instance: "MatterBaseTest", accept_function: EndpointCheckFunction) -> list[int]: | ||
| """ Returns a list of endpoints matching the accept condition. """ | ||
| wildcard = await test_instance.default_controller.Read(test_instance.dut_node_id, [(Clusters.Descriptor), Attribute.AttributePath(None, None, GlobalAttributeIds.ATTRIBUTE_LIST_ID), Attribute.AttributePath(None, None, GlobalAttributeIds.FEATURE_MAP_ID), Attribute.AttributePath(None, None, GlobalAttributeIds.ACCEPTED_COMMAND_LIST_ID)]) |
There was a problem hiding this comment.
In _get_all_matching_endpoints, the Read() call passes (Clusters.Descriptor) which is just the class object, not a 1-tuple. The controller Read() type expects attribute selectors like (Clusters.Descriptor,) (a single-element tuple) for the wildcard-cluster form, and other call sites (e.g. MatterBaseTest._populate_wildcard) already use that form. Please add the trailing comma (and ideally format the selector list across multiple lines) so this call type-checks consistently and matches the intended selector shape.
Suggested change
| wildcard = await test_instance.default_controller.Read(test_instance.dut_node_id, [(Clusters.Descriptor), Attribute.AttributePath(None, None, GlobalAttributeIds.ATTRIBUTE_LIST_ID), Attribute.AttributePath(None, None, GlobalAttributeIds.FEATURE_MAP_ID), Attribute.AttributePath(None, None, GlobalAttributeIds.ACCEPTED_COMMAND_LIST_ID)]) | |
| wildcard = await test_instance.default_controller.Read( | |
| test_instance.dut_node_id, | |
| [ | |
| (Clusters.Descriptor,), | |
| Attribute.AttributePath(None, None, GlobalAttributeIds.ATTRIBUTE_LIST_ID), | |
| Attribute.AttributePath(None, None, GlobalAttributeIds.FEATURE_MAP_ID), | |
| Attribute.AttributePath(None, None, GlobalAttributeIds.ACCEPTED_COMMAND_LIST_ID), | |
| ], | |
| ) |
Comment on lines
685
to
690
| class AsyncReadTransaction: | ||
| @dataclass | ||
| class ReadResponse: | ||
| attributes: dict[Any, Any] | ||
| attributes: dict[int, list[Cluster]] | ||
| events: list[ClusterEvent] | ||
| tlvAttributes: dict[int, Any] |
There was a problem hiding this comment.
AsyncReadTransaction.ReadResponse type annotations don’t match the values being populated:
attributescomes fromAttributeCache.GetUpdatedAttributeCache(), which returns a nested dict keyed by endpoint and cluster/attribute types (notdict[int, list[Cluster]]).eventsis populated withEventReadResultinstances (self._events: List[EventReadResult]) rather thanClusterEvent.
Please correct these field types (or use TypeAliases) so downstream code (e.g. endpoint/cluster/attribute indexing) type-checks correctly.
Comment on lines
3320
to
3331
| @@ -3409,15 +3331,15 @@ async def IssueNOCChain(self, csr: Clusters.OperationalCredentials.Commands.CSRR | |||
| ''' | |||
There was a problem hiding this comment.
IssueNOCChain is async and returns the awaited result of wrap_future(ctx_future), but it’s annotated/documented as returning asyncio.Future. Callers await it and then treat the result as an issued NOC chain object. Please change the return annotation (and docstring) to the actual result type (e.g. the NOCChain dataclass) so mypy doesn’t infer Future[...] and break type checking at call sites.
|
PR #71812: Size comparison from 3f9cd16 to ffbac5e Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is a batch of typing improvements I collected recently. It makes the typing situation more concrete and modernizes the notations since we have Python 3.11 enabled (e.g.,
list[str]instead oftyping.List[str]).Other changes:
Related issues
None
Testing
Local tests with
run_test_suite.pyand CI.Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines