TC-CNET-4.13: Update cnet 4.13 test as per spec #33024
Open
Conversation
This file contains 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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple and
lazarkov
pullapprove
bot
requested review from
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yufengwangca
|
PR #33024: Size comparison from 47c6d46 to aaa8a20 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
Kshitijjain21
changed the title
|
PR #33024: Size comparison from ae50802 to ebccaae Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
andy31415
reviewed
Apr 18, 2024
|
|
||
| The test case is not verifiable in RPI platform. As MaxNetworks value is 1 but expected is 4 | ||
| ( Pre-Condition) | ||
| disabled: true |
There was a problem hiding this comment.
This is a new disabled (i.e. manual) test step. We generally do not allow new manual test steps anymore.
There was a problem hiding this comment.
Thanks for the feedback. The change done here is allow this test to align with the matter spec. Since the test was a manual test and has not been changed to an automated/semi automated test what would be the correct course of action to get it to align with the spec?
There was a problem hiding this comment.
@andy31415 since this is a old cert test, we can make updates to the manual test.
@Kshitijjain21 But we can not merge this change unless test plan PR is approved.
There was a problem hiding this comment.
This adds several new steps that are manual ... so it kind of makes the problem worse. I understand the justification of "we add a test step in a long list of test steps that is already manual" however I also am concerned that this does increase the count of manual stuff invocation. We should not do that.
…ectedhomeip into update-CNET-4.13-test
|
PR #33024: Size comparison from 8a4dffc to e6a2f46 Decreases (2 builds for efr32)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
@Kshitijjain21 @cjandhyala this is a very old PR with merge conflicts. Since this was considering adding manual tests, what do we do with it? Can we update/automate or should it be closed? |
@andy31415 Happy to close and reopen this after the test plan change has been merged in. Although this test require that device can support up to 4 networks. Currently in the matter code its hard coded to 1 network per device and would require changes to support 4 network. Will wait on @cjandhyala thought before closing this PR. |
We can close and re-open, however I would note that we will still reject any manual tests addition in the SDK (including extra tests because existing tests are already manual ... we can update existing manual tests because that makes things not worse, but new tests should not be manual, even for clusters that already have manual tests as that makes the problem worse). Do we have a plan to automate things? |
Hi @andy31415, I am currently not sure on the plan of automating this test. |
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.
The test plan states that the 'TH reads the network attribute and saves the list 'OriginalNetworkList'' after successfully sending an ArmFailSafe and AddOrUpdateWiFiNetwork commands. Later on in the test (step 14 and 15) the TH send another ArmFailSafe with the expiry length set to 0 and then reads the network. The verification in the test plans states that the networks attribute list should be same as the 'OriginalNetworkList'.
In the matter spec under 'Behavior on expiry of Fail-Safe timer' it mentions to 'Reset the configuration of all Network Commissioning Networks attribute to their state prior to the Fail-Safe being armed'.
Based on the steps mentioned in the test plan for CNET 4.13 and matter specs there should be a commission complete command after adding new network info which should be the 'OriginalNetworkList'.
Note: The DUT's max network needs to be greater than 1.
Test plan PR - https://github.com/CHIP-Specifications/chip-test-plans/pull/4125
Link to slack - https://csamembers.slack.com/archives/C02723N0C7N/p1713277617971359