[Create Support Module] Add IDM support module for code reuse amongst IDM test modules #41641
Conversation
… IDM test modules Created idm_support.py following the cadmin_support pattern to share common functionality across IDM test modules. Moved 30+ shared methods from TC_IDM_1_2, TC_IDM_1_4, TC_IDM_2_2, and TC_IDM_4_2 into IDMBaseTest class created in idm_support module. This reduces duplication and makes test files cleaner and easier to maintain.
|
PR #41641: Size comparison from e156205 to 90f168f Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
| try: | ||
| return inspect.getmembers(cluster.Commands, inspect.isclass) | ||
| except AttributeError: | ||
| return [] |
There was a problem hiding this comment.
Could we add a comment about why this ignore is needed (here and in other places)? Why do we treat this error as a empty list instead of allowing it to propagate up?
There was a problem hiding this comment.
Good question! I've added a comment to explain this here now.
Quick context: This function came from TC_IDM_1_2.py - I just moved it to the support module without changing the logic.
Why return empty list: Looking at the code and its usage in IDM_1_2 not all clusters have a Commands attribute. When the test iterates through all clusters looking for ones with commands, some clusters throw AttributeError because they simply don't define any commands.
Returning an empty list lets the test safely check if not members: continue and move on to the next cluster, rather than crashing or needing special handling for every cluster that doesn't support commands.
Added a docstring note and inline comment to make this clearer. Let me know if you think there's a better approach to use here!
There was a problem hiding this comment.
Please see below response from your next comment below, since I hadn't read that earlier I have now updated to using the hasattr that you have suggested below here as well.
This makes the intent clearer and easier to understand. Thank you!
| # Inspect returns all the classes, not just the ones we want, so use a try | ||
| # here incase we're inspecting a builtin class | ||
| try: | ||
| return cmd_class if cmd_class.is_client else None |
There was a problem hiding this comment.
could we use hasattr and not have try/catch?
There was a problem hiding this comment.
Absolutely, great suggestion! Using hasattr is much clearer.
I've now updated both get_all_cmds_for_cluster_id and client_cmd from IDM_1_2 to use hasattr instead of try/catch. This makes the intent more explicit and easier to read.
Thanks for the improvement! 👍
| # ======================================================================== | ||
|
|
||
| @staticmethod | ||
| def is_valid_uint32_value(var): |
There was a problem hiding this comment.
why are these not in src/python_testing/matter_testing_infrastructure/matter/testing/matter_asserts.py ?
There was a problem hiding this comment.
Looking at the matter_asserts module it appears to now have this covered, so there is no longer any need to have this in the IDM support module. I've removed them from idm_support.py and updated TC_IDM_4_2.py to import and use is_valid_uint_value() directly from matter_asserts. Much cleaner this way.
Thank you for bringing this to light!
| # Descriptor and Cluster Reading Utilities | ||
| # ======================================================================== | ||
|
|
||
| async def get_descriptor_server_list(self, ctrl: ChipDeviceCtrl, ep: int = None): |
There was a problem hiding this comment.
| async def get_descriptor_server_list(self, ctrl: ChipDeviceCtrl, ep: int = None): | |
| async def get_descriptor_server_list(self, ctrl: ChipDeviceCtrl, ep: Optional[int] = None): |
There was a problem hiding this comment.
Is there a reason to allow None or can we use int and default to ROOT_NODE_ENDPOINT_ID instead?
There was a problem hiding this comment.
Same comment for all instances below
There was a problem hiding this comment.
Great suggestion!
You're right - there's no reason to use None here. I've updated all the functions to default directly to ROOT_NODE_ENDPOINT_ID instead. This simplifies the code by removing the None checks and makes the default behavior more explicit.
Changed from ep: int = None to ep: int = ROOT_NODE_ENDPOINT_ID in all 7 affected functions. Much cleaner now!
- Added docstring and inline comment explaining why empty list is returned when clusters lack a Commands attribute.
Makes the code intent clearer and more Pythonic per Andrei's feedback.
…ameters - Simplifies code by removing None checks and makes default behavior more explicit.
|
PR #41641: Size comparison from e156205 to 9a393d4 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
| return None | ||
|
|
||
|
|
||
| # ============================================================================ |
There was a problem hiding this comment.
Does this class get used anywhere?
There was a problem hiding this comment.
Good catch!
Yeah, when I was creating this module I was using the cadmin_support module we created for the CADMIN tests as a reference - it has both a CADMINSupport helper class and a CADMINBaseTest base class.
So I copied that structure over to use with the IDM tests thinking I might need both, but appears I only needed the IDMBaseTest class for all the IDM functionality and never ended up using the helper class.
Sorry, Completely forgot to clean it up. I'll remove it now. Thanks!
|
PR #41641: Size comparison from 3548c02 to de3b275 Full report (4 builds for nrfconnect, realtek, stm32)
|
|
PR #41641: Size comparison from 3548c02 to 07607a3 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
4 participants
Summary
Created idm_support.py following the cadmin_support pattern to share common functionality across IDM test modules. Moved 30+ shared methods from TC_IDM_1_2, TC_IDM_1_4, TC_IDM_2_2, and TC_IDM_4_2 into IDMBaseTest class created in idm_support module.
This reduces duplication and makes test files cleaner and easier to maintain.
Related issues
Resolves follow-up task: project-chip/matter-test-scripts#690
Testing
This is validated by the following test modules: TC_IDM_1_2.py, TC_IDM_1_4.py, TC_IDM_2_2.py, TC_IDM_4_2.py
Used following example command in WSL Linux for validation of TC_IDM_1_2, TC_IDM_2_2.py, and TC_IDM_4_2.py:
Following command was used for validation of TC_IDM_1_4.py:
Note
Need to add TC_IDM_3_2 test module functions in PR #41066 that can be shared amongst other test modules once that PR gets merged.
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