[Test] Creating TC_CADMIN_1_22 and TC_CADMIN_1_24 python test modules #35632
+172
−650
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
Contributor
j-ororke
commented
Sep 17, 2024
•
j-ororke
commented
- Created CADMIN_1_22 python test module following steps in PR here: TC_CADMIN_1_22
- Created CADMIN_1_24 python test module following steps in PR here: TC_CADMIN_1_24
- Removed following yaml test modules:
-
- Test_TC_CADMIN_1_21.yaml, Test_TC_CADMIN_1_22.yaml, Test_TC_CADMIN_1_23.yaml, Test_TC_CADMIN_1_24.yaml
- Created CADMIN_1_22 python test module following steps in PR here: - Created CADMIN_1_24 python test module following steps in PR here: - Removed following yaml test modules: -- Test_TC_CADMIN_1_21.yaml, Test_TC_CADMIN_1_22.yaml, Test_TC_CADMIN_1_23.yaml, Test_TC_CADMIN_1_24.yaml
|
Review changes with Changed Files
|
github-actions
bot
added
app
tests
matter-1.4-te2-script-change
|
PR #35632: Size comparison from 31894f6 to e244d98 Full report (11 builds for nxp, qpg, stm32, tizen)
|
- Resolved linting issues
|
PR #35632: Size comparison from 31894f6 to c8a2509 Full report (82 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex and
harsha-rajendran
pullapprove
bot
requested review from
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yunhanw-google
cecille
reviewed
Sep 20, 2024
- Merged TC_CADMIN_1_24 and TC_CADMIN_1_22 standalone test modules into a single test module - Found enum for window closed value and replaced it in the tests - Removed generate_unique_value and AttemptCommission functions - Updated CI arguments format to YAML'esque format
- Renamed class to TC_CADMIN_1_22_24
- Resolving lint errors
|
PR #35632: Size comparison from 0a0e9ce to 979df75 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35632: Size comparison from af3727b to 252d55f Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Updated to include dependency location change for matter_testing support module
|
PR #35632: Size comparison from 1f4e81e to 9259c24 Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Resolved issues with CI args in TC_CADMIN_1_22_24 test module - Removed yaml calls for 1_21, 1_22, 1_23, and 1_24, also removed yaml call for 1_9 from manualTests.json
|
PR #35632: Size comparison from 1f4e81e to fd13214 Full report (67 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
cecille
removed
the
matter-1.4-te2-script-change
cecille
approved these changes
Oct 29, 2024
tehampson
reviewed
Oct 29, 2024
|
|
||
| except Exception as e: | ||
| logging.exception('Error running OpenCommissioningWindow %s', e) | ||
| asserts.assert_true(False, 'Failed to open commissioning window') |
There was a problem hiding this comment.
Suggested change
| asserts.assert_true(False, 'Failed to open commissioning window') | |
| asserts.fail('Failed to open commissioning window') |
tehampson
reviewed
Oct 29, 2024
Comment on lines
96
to
97
| if window_status != Clusters.AdministratorCommissioning.Enums.CommissioningWindowStatusEnum.kWindowNotOpen: | ||
| asserts.fail("Commissioning window is expected to be closed, but was found to be open") |
There was a problem hiding this comment.
I think this is the right code
Suggested change
| if window_status != Clusters.AdministratorCommissioning.Enums.CommissioningWindowStatusEnum.kWindowNotOpen: | |
| asserts.fail("Commissioning window is expected to be closed, but was found to be open") | |
| asserts.assert_equal(window_status, Clusters.AdministratorCommissioning.Enums.CommissioningWindowStatusEnum.kWindowNotOpen, "Commissioning window is expected to be closed, but was found to be open") |
|
|
||
| self.step(6) | ||
| window_status2 = await self.get_window_status() | ||
| if window_status2 != Clusters.AdministratorCommissioning.Enums.CommissioningWindowStatusEnum.kWindowNotOpen: |
There was a problem hiding this comment.
Same issue here, just do asserts.assert_equal
There was a problem hiding this comment.
Thank you, that did make the code much cleaner
|
|
||
| self.step(4) | ||
| window_status = await self.get_window_status() | ||
| if window_status != Clusters.AdministratorCommissioning.Enums.CommissioningWindowStatusEnum.kWindowNotOpen: |
There was a problem hiding this comment.
Thank you, I have updated this here now.
|
|
||
| self.step(6) | ||
| window_status2 = await self.get_window_status() | ||
| if window_status2 != Clusters.AdministratorCommissioning.Enums.CommissioningWindowStatusEnum.kWindowNotOpen: |
| except ChipStackError as e: | ||
| # Since we provided 901 seconds as the timeout duration, | ||
| # we should not be able to open comm window as duration is too long. | ||
| asserts.assert_equal(e.err, 0x00000585, |
There was a problem hiding this comment.
For better readability I would suggest you do something like _FAILURE_ERROR_CODE = 0x00000585 where _FAILURE_ERROR_CODE is a descriptive name about the errors like INVALID_TIMEOUT_ARGUMENT_ERROR, or whatever 0x00000585 means so that someone else can more easily see the error that you are expecting here
There was a problem hiding this comment.
Thank you,
That is a very good point, I have updated the comments to make those show what the error code is referring to
| nodeid=self.dut_node_id, timeout=179, iteration=10000, discriminator=self.discriminator, option=1) | ||
|
|
||
| except ChipStackError as e: | ||
| # Since we provided 901 seconds as the timeout duration, |
There was a problem hiding this comment.
This comment seems like copy paste and should say 179
| except ChipStackError as e: | ||
| # Since we provided 901 seconds as the timeout duration, | ||
| # we should not be able to open comm window as duration is too long. | ||
| asserts.assert_equal(e.err, 0x00000585, |
There was a problem hiding this comment.
Again if you could update this to be more readable that would be great
There was a problem hiding this comment.
Thank you, I have updated the comments to be a bit cleaner here now
tehampson
reviewed
Oct 29, 2024
| await self.th1.OpenCommissioningWindow( | ||
| nodeid=self.dut_node_id, timeout=180, iteration=10000, discriminator=self.discriminator, option=1) | ||
|
|
||
| self.step(3) |
There was a problem hiding this comment.
Step 3 for TC_CADMIN_1_24 in https://github.com/CHIP-Specifications/chip-test-plans/pull/4673/ says to wait 181 seconds for this to expire. It doesn't say to issue a revoke command. Should the testplan be updated or should you be waiting 181s?
- Resolving comments from tehampson as these were needed changes to be made -- Substituted to using cleaner code for assert.asserts_equal instead of just assert.failure -- Resolving copy paste error in TC_CADMIN_1_4 test -- Made comments cleaner by rewording to include what the error code was referring too
github-actions
bot
added
the
matter-1.4-te2-script-change
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.