Refactor events and attributes from matter testing python framework #39672
Conversation
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
… it is only being used by ClusterAttributeChangeAccumulator.await_sequence_of_reports()
There was a problem hiding this comment.
Code Review
The pull request refactors the event and attribute reporting mechanism in the matter testing python framework. It introduces a new module event_attribute_reporting.py and updates import statements and callback instantiations across multiple test files. The changes primarily involve replacing EventChangeCallback and ClusterAttributeChangeAccumulator from matter_testing with their counterparts from the new module. It's recommended to verify the compatibility and functionality of the new implementation.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
PR #39672: Size comparison from f57e14b to 88f4661 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39672: Size comparison from f57e14b to 3bee317 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39672: Size comparison from f57e14b to 5163d7d Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39672: Size comparison from f57e14b to d4f5d89 Full report (13 builds for nrfconnect, nxp, telink)
|
|
PR #39672: Size comparison from 781ff6f to 4646ced Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39672: Size comparison from e0f8e39 to 64fa1b8 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39672: Size comparison from e0f8e39 to e61d9b2 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors event and attribute reporting logic by merging four classes into two, EventCallback and AttributeCallback, and updating numerous test cases to use them. The refactoring centralizes the subscription handling logic, which is a great improvement.
My review has identified several issues, including some critical bugs in the new AttributeCallback class that could cause runtime errors. Specifically, there are issues with its initialization logic, a call to a non-existent _put method on a queue, and flawed validation logic. I've also found a potential issue with subscription lifetime in one of the updated tests and some areas for improving code quality, such as a malformed f-string and code duplication. I've provided detailed comments and suggestions for these points. Addressing these will be important for the stability and correctness of the new testing utilities.
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
|
PR #39672: Size comparison from e0f8e39 to e102749 Full report (3 builds for cc32xx, stm32)
|
|
PR #39672: Size comparison from e0f8e39 to 14a5f8a Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
juandediosg
reviewed
Jul 3, 2025
This was referenced Jul 7, 2025
cecille
reviewed
Jul 8, 2025
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
src/python_testing/matter_testing_infrastructure/chip/testing/event_attribute_reporting.py
Outdated
Show resolved
Hide resolved
juandediosg
approved these changes
Jul 8, 2025
|
LGTM ✅
|
juandediosg
approved these changes
Jul 10, 2025
|
PR #39672: Size comparison from 43234ba to 0b2f2be Increases above 0.2%:
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
approved these changes
Jul 11, 2025
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.
First approve #39527. (EDIT: Done)
This PR solves issue #592
The idea is to merge 4 subscription-based classes into 2 because they have overlapping functionalities. This merge included refactoring and reorganization of some tests.
Updates:
SimpleEventCallbackandEventChangeCallbackclasses intoEventSubscriptionHandlerclassAttributeChangeCallbackandClusterAttributeChangeAccumulatorclasses intoAttributeSubscriptionHandlerclassSimpleEventCallbackclass (nowEventSubscriptionHandler) don't instantiate a queue, this is done by the new classWaitForAttributeReport()andwait_for_report()towait_for_attribute_report()inAttributeSubscriptionHandlerclassevent_attribute_reporting.pyTC_ACE_1_2.py,TestGroupTableReports.pyandTC_ACL_2_11.pyand imported fromevent_attribute_reporting.pyTesting
In the first terminal:
rm /tmp/chip_*In the second terminal:
You can replace the test file with others that were updated in this PR.