Updates TC-SC-5.2 and TC-SC-6.1 as per Test Plan Changes #39338
Conversation
There was a problem hiding this comment.
Code Review
The pull request updates two YAML-based test scripts, Test_TC_SC_5_2.yaml and Test_TC_SC_6_1.yaml, to reflect changes in the test plan.
In Test_TC_SC_5_2.yaml, new steps have been added to test group removal and verification, and existing steps have been renumbered. These changes appear correct and well-integrated.
In Test_TC_SC_6_1.yaml, steps 7 and 8 have been significantly modified: the DUT now sends commands (KeySetRemove and KeySetReadAllIndices) instead of reading attributes. The PICS codes, chip-tool commands, and sample verification logs have been updated accordingly. The content of these changes seems correct.
A key point for clarification is whether these updated steps in Test_TC_SC_6_1.yaml should now be enabled, as they are currently marked disabled: true.
Summary of Findings
- Test Steps Disabled: In
Test_TC_SC_6_1.yaml, the updated Steps 7 and 8 remaindisabled: true. It's important to clarify if the test plan changes require these steps to be active. If so, they should be enabled for the test to be effective.
Merge Readiness
The changes in Test_TC_SC_5_2.yaml are good. The content updates in Test_TC_SC_6_1.yaml for steps 7 and 8 also seem correct based on their descriptions. However, there's a significant question regarding whether these updated steps in Test_TC_SC_6_1.yaml should be enabled (they are currently disabled: true).
I recommend that this point be clarified and, if necessary, addressed before merging to ensure the test script accurately reflects the current test plan's execution requirements. As an AI, I am not authorized to approve pull requests, and I recommend that others review and approve this code before merging, as well. Based on this review, I would request changes to address the disabled: true status clarification.
|
PR #39338: Size comparison from 49594a5 to 044d8d7 Full report (3 builds for cc32xx, stm32)
|
|
PR #39338: Size comparison from 49594a5 to 2897dfe Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
Please add more context to the description of this PR:
Thanks! |
Description: TC-SC-5.2 Same for Step 10. TC-SC-6.1 Steps to reproduce:
Test Plan References: Script is passing in the CI |
| "Step 9: TH sends a RemoveGroup Command to the Groups cluster with the | ||
| GroupID field set to 0x0101. The command is sent as a group command | ||
| using GroupID 0x0103" | ||
| PICS: G.S.C03.Rsp |
There was a problem hiding this comment.
| PICS: G.S.C03.Rsp |
command is mandatory
There was a problem hiding this comment.
If this was intended to stop this test step from running on a device that doesn't have a groups cluster, that's not going to work either because it targets EP0 and the groups clusters are not on that endpoint. The PICS are per endpoint, so this test step will basically not ever run unless you have a groups cluster on EP0, which no one should have.
| @@ -220,7 +220,37 @@ tests: | |||
| value: "" | |||
|
|
|||
| - label: | |||
| "Step 9: TH removes the Group key set that was added by sending a | |||
| "Step 9: TH sends a RemoveGroup Command to the Groups cluster with the | |||
There was a problem hiding this comment.
So this test presupposes a groups cluster, but is run against all commissionees.
Can you check with Gibran? He is supposed to be looking at the groups tests and the PICS markings to understand if they are correct and I can guarantee you that this one is not.
| verification: | | ||
| ./chip-tool groupkeymanagement read group-key-map 1 0 | ||
| ./chip-tool groupkeymanagement key-set-remove 42 1 0 |
There was a problem hiding this comment.
is this ever going to be automatable in yaml? If no, there's no point in fiddling further with this file as it will need to be re-written anyway.
Testing
Updates TC-SC-5.2 and TC-SC-6.1 as per Test Plan Changes
Addresses:
[TC-SC-5.2, 6.1] Script has to be updated as per Test Plan #17214