[Debug] Add automatic device attribute dump on composition test failures #41020
+44
−12
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
- Move log_structured_data to module level in basic_composition support module for reusability - Add on_fail override in BasicCompositionTests to automatically dump device data - Ensures all composition tests automatically log device attributes on failure - Improves debugging by providing IDM-12.1 equivalent data without manual intervention - Add missing logging import to TC_DeviceConformance.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds automatic device attribute dumping on composition test failures, which will significantly improve debugging. The approach of moving log_structured_data to a shared module and using an on_fail hook in the base test class is clean and reusable. I have one suggestion to improve code cleanliness by removing a couple of unused imports.
- Remove on_fail() override as it's now inherited from BasicCompositionTests - Remove unused log_structured_data import
|
PR #41020: Size comparison from a05e91d to 5edf777 Full report (31 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41020 +/- ##
=======================================
Coverage 51.01% 51.01%
=======================================
Files 1386 1386
Lines 100988 100988
Branches 13081 13081
=======================================
+ Hits 51519 51520 +1
+ Misses 49469 49468 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… composition test failures - Change TC_DeviceConformance from (MatterBaseTest, DeviceConformanceTests) to (DeviceConformanceTests, MatterBaseTest) - Change TC_DeviceBasicComposition from (MatterBaseTest, BasicCompositionTests) to (BasicCompositionTests, MatterBaseTest) - This ensures BasicCompositionTests.teardown_class() takes precedence in Method Resolution Order - Enables automatic device attribute dumps when composition tests detect problems - Improves debugging efficiency by providing IDM-12.1 equivalent data without manual intervention The inheritance order change allows the teardown_class override in BasicCompositionTests to execute before MatterBaseTest.teardown_class, ensuring device data is dumped before the standard problem logging occurs.
j-ororke
changed the title
…t the failure msg correctly
|
PR #41020: Size comparison from a05e91d to 6756ca7 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
cecille
reviewed
Sep 24, 2025
src/python_testing/matter_testing_infrastructure/matter/testing/basic_composition.py
Outdated
Show resolved
Hide resolved
Restoring fail_current_test() as per Cecille's suggestion, thank you!
Updated to using module-level logger instance instead of using logging directly
|
PR #41020: Size comparison from 3d8bebe to b149ff6 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
cecille
reviewed
Sep 30, 2025
| self.xml_device_types, problems = build_xml_device_types(dm) | ||
| self.problems.extend(problems) | ||
|
|
||
| def teardown_class(self): |
There was a problem hiding this comment.
Does this work? The basic information / composition tests do that double-inheritance thing. I was thinking this would happen on the MatterBaseTest teardown.
Though we can actually do proper inheritance now, so if you want to change this, that could make sense too.
There was a problem hiding this comment.
Hey Cecille,
You're totally right - I was overcomplicating this with the inheritance gymnastics! 😅
I've now moved the device attribute dumping functionality from BasicCompositionTests.teardown_class() into MatterBaseTest.teardown_class() where it feels like it belongs more.
This way:
- No more multiple inheritance headaches
- All Matter tests get the debugging goodness automatically
- Much cleaner code that follows proper inheritance patterns
- Everything still works exactly as before
Thanks for steering me in the right direction! The device dumps will now help debug failures across the entire test suite, not just the composition tests.
Much appreciated!
…eTest for universal test debugging
|
PR #41020: Size comparison from 60d7fa3 to 855d881 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
cecille
reviewed
Oct 16, 2025
| if suffix > attribute_standard_range_max and suffix < global_range_min: | ||
| self.record_error(self.get_test_name(), location=location, | ||
| problem=f"Manufacturer attribute in undefined range {manufacturer_value} in cluster {cluster_id}", | ||
| problem=f"Manufacturer attribute in undefined range { |
There was a problem hiding this comment.
We're about to update the restyler so this doesn't happen - there was a bug in the restyler that is causing this weirdness. When that lands, would you mind reverting these changes? I think this might break python prior to 3.12.
There was a problem hiding this comment.
Merged in master after the PR for your restylizer change got merged and reverted the changes for the f-str lines in this file here now.
| _, txt_str = self.dump_wildcard(None) | ||
| # Only dump the text format - it's more readable for debugging | ||
| self._log_structured_data('==== FAILURE_DUMP_txt: ', txt_str) | ||
| else: |
There was a problem hiding this comment.
I don't know if it's worthwhile logging these - it might just confuse people. Ditto with the exception.
There was a problem hiding this comment.
Ah ok, understood, I have removed the logging statements here and the logging statement in the exception below from the code here now.
| LOGGER.info("dump_wildcard method not available - skipping device attribute dump") | ||
| else: | ||
| LOGGER.debug("No device attribute data available (endpoints_tlv not populated)") | ||
| except Exception as e: |
There was a problem hiding this comment.
what would cause an exception here? you should be catching both failure cases with the hasattr checks.
If you do need this, can you please change to be the specific exception rather than Exception?
There was a problem hiding this comment.
You are right that the hasattr checks handle the missing attribute cases.
The exception handling is for the data access and serialization that happens inside dump_wildcard() itself, things such as:
- Dictionary key access when traversing endpoints_tlv
- JSON serialization of TLV data
- String formatting with pformat()
I've changed it to catch only the specific exceptions that could realistically occur during those operations: AttributeError, KeyError, ValueError, and TypeError. This way we're not masking unexpected errors, just preventing data dump failures from interfering with the actual test failure reporting.
| LOGGER.info("###########################################################") | ||
|
|
||
| # Attempt to dump device attribute data for debugging when problems are found | ||
| self._dump_device_attributes_on_failure() |
There was a problem hiding this comment.
Can you please move this to above the problem dump? That will make the actual problems appear next to the failure message rather than forcing people to scroll up to above the device dump.
There was a problem hiding this comment.
Updated, moved the problem dump to above the problem logging.
|
PR #41020: Size comparison from 43c9fb4 to 13fb594 Full report (22 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
- Fix f-strings in TC_DeviceBasicComposition.py for Python < 3.12 - Move device dump before problem logging for better failure visibility - Remove noisy log messages from device dump on failure - Use specific exceptions instead of catching all exceptions so that it isnt blanketed.
|
PR #41020: Size comparison from 43c9fb4 to 530ac3a Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
cecille
reviewed
Nov 6, 2025
| # Don't let data access or serialization errors interfere with the original test failure | ||
| pass | ||
|
|
||
| def _log_structured_data(self, start_tag: str, dump_string: str): |
There was a problem hiding this comment.
This seems like a duplicate of the function in basic_composition.py
There was a problem hiding this comment.
Thanks for catching that!
I’ve removed this function from the basic_composition support module. There’s a test, test_TC_IDM_12_1 in the TC_DeviceBasicComposition module, that uses this function directly; it originally came from there (previously embedded inside that test).
I’ve now updated that test to inherit the function from here, and renamed this function so that it doesn't appear to be a private function anymore.
… module, now inheriting it directly from matter_testing in test_TC_IDM_12_1 test
… module, now inheriting it directly from matter_testing in test_TC_IDM_12_1 test in TC_DeviceBasicCompositon test module
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Automatically dump device attribute data when composition tests fail to improve debugging efficiency.
Previously, when conformance tests like TC_DeviceConformance or TC_DeviceBasicComposition failed, developers had to manually run IDM-12.1 to collect device attribute dumps for debugging. This PR adds automatic device data logging on test failure by:
The structured logs use clear begin/end markers making it easy to extract device data from test failure logs, eliminating the manual step of requesting IDM-12.1 dumps and significantly speeding up the debugging process.
Note: Currently applies to TC_DeviceConformance and TC_DeviceBasicComposition. Other composition test classes (TC_DA_1_2, TC_AccessChecker, TC_IDM_2_2, etc.) would need similar inheritance order changes to inherit this behavior.
Related issues
Resolves task: 680
Testing
Readability checklist
descriptive
“When in Rome…”
rule (coding style)