Testing infrastructure for cluster commands #41551
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
LyudmilaKostanyan
moved this to In Progress
in Cluster and example long term maintainability
|
PR #41551: Size comparison from 7e83b9f to 77f1ef0 Full report (37 builds for bl602, 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 #41551 +/- ##
=======================================
Coverage 51.05% 51.05%
=======================================
Files 1385 1385
Lines 100882 100882
Branches 13055 13055
=======================================
Hits 51508 51508
Misses 49374 49374 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LyudmilaKostanyan
changed the title
LyudmilaKostanyan
force-pushed
the
CommandTesting
branch
from
77f1ef0 to
a00edfc
Compare
|
PR #41551: Size comparison from 4f17295 to a00edfc Full report (9 builds for cc13x4_26x4, cc32xx, realtek, stm32)
|
|
PR #41551: Size comparison from 4f17295 to 302e573 Full report (5 builds for cc32xx, realtek, stm32)
|
|
PR #41551: Size comparison from 4f17295 to d8e1532 Full report (33 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, psoc6, qpg, realtek, stm32, telink)
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a reusable testing infrastructure for cluster commands by creating a centralized CommandTesting module that eliminates code duplication across cluster test suites.
Key changes:
- New centralized
CommandTestingmodule with reusableMockCommandHandlerandInvokeOperationhelper classes - Refactored Diagnostic Logs Cluster tests to use the centralized mock implementation
- Updated Push AV Stream Transport tests to inherit from the centralized mock while preserving specialized functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/clusters/testing/CommandTesting.h | Defines centralized MockCommandHandler and InvokeOperation classes for command testing |
| src/app/clusters/testing/CommandTesting.cpp | Implements core functionality for mock command response handling and TLV encoding/decoding |
| src/app/clusters/testing/BUILD.gn | Adds build dependencies for the new CommandTesting module |
| src/app/clusters/push-av-stream-transport-server/tests/TestPushAVStreamTransportCluster.cpp | Refactors to inherit from centralized MockCommandHandler |
| src/app/clusters/push-av-stream-transport-server/tests/BUILD.gn | Updates build dependency reference |
| src/app/clusters/diagnostic-logs-server/tests/TestDiagnosticLogsCluster.cpp | Replaces local mock with centralized implementation |
| src/app/clusters/diagnostic-logs-server/tests/BUILD.gn | Adds testing module dependency |
Add new testing utilities in src/app/clusters/testing/: - CommandTesting.h: Header with MockCommandHandler and InvokeOperation classes. - CommandTesting.cpp: Implementation of non-template methods for MockCommandHandler. Refactoring: - Refactored TestDiagnosticLogsCluster to use Testing::MockCommandHandler directly - Updated TestPushAVStreamTransportCluster to inherit from Testing::MockCommandHandler while preserving specialized features
- Refactor MockCommandHandler: integrate status handling into FallibleAddStatus. - Update TestPushAVStreamTransportCluster.
- Remove unused overrides
- Replace single ResponseRecord with std::vector<ResponseRecord> for multiple responses - Add methods to work with response arrays and specific responses by index - Update existing methods to work with first response for backward compatibility - Simplify test classes by removing duplicate functionality - Add explicit response count assertions in tests
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
LyudmilaKostanyan
force-pushed
the
CommandTesting
branch
from
93e4c38 to
5ccf691
Compare
LyudmilaKostanyan
moved this from In Progress
to In review
in Cluster and example long term maintainability
|
PR #41551: Size comparison from 7bad8a9 to 46e400c Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
LyudmilaKostanyan
moved this from In review
to In Progress
in Cluster and example long term maintainability
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/app/clusters/push-av-stream-transport-server/tests/TestPushAVStreamTransportCluster.cpp:1
- The
GetSubjectDescriptoroverride is removed but was never provided by the baseTesting::MockCommandHandlerclass. This will cause compilation failure if any code callsGetSubjectDescriptor()on this mock. The base class should provide a default implementation or this override should be retained.
/*
|
PR #41551: Size comparison from 1992150 to 92823ff Full report (3 builds for realtek, stm32)
|
…andling - Updated `Invoke` method in `ClusterTester` to handle response decoding more robustly, including support for `NullObjectType`. - Enhanced test for `JoinGroupCommand` to utilize the new invocation method. - Added necessary includes for `NullObjectType` in relevant files.
Contributor
Author
|
/gemini review |
![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 does a great job of centralizing command testing infrastructure by introducing a CommandTesting module with a reusable MockCommandHandler. This significantly reduces code duplication across multiple test suites, as seen in the refactoring of tests for Diagnostic Logs, Push AV Stream Transport, and Groupcast clusters. The new ClusterTester::Invoke method is a notable improvement, simplifying command invocation and response handling in tests. The overall changes enhance test maintainability and consistency. I've found a couple of minor opportunities for cleanup related to the refactoring, which are detailed in the comments.
|
PR #41551: Size comparison from 1992150 to 397dc02 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
reviewed
Nov 6, 2025
andy31415
approved these changes
Nov 6, 2025
|
PR #41551: Size comparison from cc93907 to a5d1988 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
added
the
sdk-maintainer-approved
github-project-automation
bot
moved this from In Progress
to Done
in Cluster and example long term maintainability
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 introduces a centralized
CommandTestingmodule to standardize and simplify command testing across cluster test suites. The changes eliminate code duplication by providing a reusableMockCommandHandlerimplementation and improve test maintainability.What was added:
CommandTestingmodule: Createdsrc/app/clusters/testing/CommandTesting.handCommandTesting.cppwith a standardizedMockCommandHandlerclassInvokeOperationhelper class for streamlined command invocation in testsWhat was refactored:
MockCommandHandlerimplementation with the centralizedTesting::MockCommandHandlerMockCommandHandlerwhile preserving specialized functionality for status trackingMockCommandHandlerand the new overloadedInvokemethodRelated issues
Main issue: #41513
Testing