Add more unit tests for SoftwareDiagnostics cluster
#41246
Merged
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
|
PR #41246: Size comparison from 7f569a7 to 22fd1f1 Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #41246: Size comparison from 7f569a7 to e18ab40 Full report (36 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, 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 #41246 +/- ##
=======================================
Coverage 50.94% 50.95%
=======================================
Files 1378 1378
Lines 100698 100698
Branches 13058 13056 -2
=======================================
+ Hits 51302 51310 +8
+ Misses 49396 49388 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e18ab40 to
7cd9644
Compare
|
PR #41246: Size comparison from 4df7a27 to 7cd9644 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
gd-mauri
reviewed
Oct 6, 2025
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp
Outdated
Show resolved
Hide resolved
7cd9644 to
7b8e089
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for the ResetWatermark command and the SoftwareFault event in the SoftwareDiagnostics cluster. The implementation is well done. I've identified one high-severity issue in the new SoftwareFaultListenerTest where a global listener is not cleaned up after the test, which could cause other tests to fail. Please see my detailed comment for the suggested fix.
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request adds valuable unit tests for the SoftwareDiagnostics cluster, specifically for the ResetWatermark command and the SoftwareFault event. The new tests are well-structured and follow existing patterns in the codebase. The changes to the build files and test helpers are appropriate. I have one minor suggestion to improve code conciseness.
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp
Show resolved
Hide resolved
|
PR #41246: Size comparison from 867bf47 to f9a08bd Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
f9a08bd to
0ecb518
Compare
|
PR #41246: Size comparison from e72c900 to 0ecb518 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
0ecb518 to
acdf6be
Compare
|
PR #41246: Size comparison from bfddff8 to acdf6be Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
acdf6be to
fc292bc
Compare
|
PR #41246: Size comparison from f902839 to fc292bc Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
approved these changes
Oct 14, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR adds unit tests for the SoftwareDiagnostics cluster, specifically targeting the ResetWatermark command and SoftwareFault event functionality. The changes enhance test coverage for cluster operations that were previously untested.
Key changes:
- Adds unit test for the
ResetWatermarkcommand to verify it correctly calls through to the provider - Adds unit test for the
SoftwareFaultevent listener to verify event generation and data integrity - Introduces test infrastructure dependencies to support event testing
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp |
Adds new unit tests for ResetWatermark command and SoftwareFault event, includes necessary headers and test infrastructure |
src/app/clusters/software-diagnostics-server/tests/BUILD.gn |
Adds dependency on server-cluster testing framework to support new test functionality |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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
Add unit tests for the
ResetWatermarkcommandSoftwareFaulteventRelated issues
#41059
Testing
Adds new unit tests, just needs to pass CI.
Notes
There is the attribute
ThreadMetricsthat can be tested but isn't. It needs anAttributeValueEncoderand possibly anAttributeValueDecoderto be set up to be tested, and this can be made very easy with an infrustructure for this. There is alreadyTestServerClusterContextimplemented, so a more capable infrastructure can be made from it.