-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Test] Creating CADMIN 1.15 python test module #35524
base: master
Are you sure you want to change the base?
Conversation
- Creating TC_CADMIN_1_15.py test module following test steps in draft PR from Cecille here: CHIP-Specifications/chip-test-plans#4636 - Removing test_TC_CADMIN_1_15.yaml as no longer needed once python module is created.
Changed Files
|
PR #35524: Size comparison from d1279a8 to fb1ce98 Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
PR #35524: Size comparison from d1279a8 to ff650d5 Full report (12 builds for nrfconnect, nxp, qpg, stm32, tizen)
|
- Fixing linting errors
PR #35524: Size comparison from d1279a8 to ed04de6 Full report (12 builds for nrfconnect, nxp, qpg, stm32, tizen)
|
- Made minor linting fixes
PR #35524: Size comparison from d1279a8 to cdad862 Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Updated comment verbage to correct information - Updated and removed some expected results for test steps as necessary
PR #35524: Size comparison from d1279a8 to a62f7c6 Increases above 0.2%:
Full report (5 builds for nxp, stm32, tizen)
|
PR #35524: Size comparison from d1279a8 to 9aa3455 Increases above 0.2%:
Full report (77 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments, looks good, just need to wait on the dns-sd stuff.
src/python_testing/TC_CADMIN_1_15.py
Outdated
|
||
|
||
class TC_CADMIN_1_15(MatterBaseTest): | ||
async def OpenCommissioningWindow(self, th, expectedErrCode) -> CommissioningParameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment on CADMIN-1.11 - it seems clearer here to have a None error code. Also add the type annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be resolved, thank you
src/python_testing/TC_CADMIN_1_15.py
Outdated
|
||
|
||
class TC_CADMIN_1_15(MatterBaseTest): | ||
async def OpenCommissioningWindow(self, th, expectedErrCode) -> CommissioningParameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this function appears in a couple of CADMIN tests, it seems like it would make sense to consolidate. Let's get these all in as-is, and then pull them into a single file with a single implementation of this function. The bonus there is we can run them all in one go and save some time at testing. Well, not through the TH, but in the CI.
src/python_testing/TC_CADMIN_1_15.py
Outdated
nodeId=self.dut_node_id, setupPinCode=setupPinCode, | ||
filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=self.discriminator) | ||
|
||
def generate_unique_random_value(self, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this function get used anywhere? left over from something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I will remove this, it was left over in the template from when TC_CADMIN_1_9 test module was created.
src/python_testing/TC_CADMIN_1_15.py
Outdated
|
||
def steps_TC_CADMIN_1_15(self) -> list[TestStep]: | ||
return [ | ||
TestStep(1, "Commissioning, already done", is_commissioning=True), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something's wacky with the step numbering here - doesn't quite match the test plan, i don't think. Would you mind double checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test steps in the test script should now match the test plan
src/python_testing/TC_CADMIN_1_15.py
Outdated
asserts.fail("Expected number of fabrics not correct") | ||
|
||
if 2 in fabric_indexes2: | ||
asserts.fail("fabricIndexes should consist of indexes 1, 3, and 4 at this time") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the assertion comment here? The index numbers aren't guaranteed to start from 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching this, I have updated the asertion message to report that the fabric indexes should not contain the fabric_idx_2 set value
fabric_indexes = [fabric.fabricIndex for fabric in fabrics2] | ||
if len(fabrics2) != initial_number_of_fabrics + 1: | ||
# len of fabrics is expected to be 2, if 2 not found then we assert failure | ||
asserts.fail(f"Expected number of fabrics not correct, should show 2, but instead shows {str(len(fabrics2))}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually care too much, but you don't need str operator in an f string. less typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I will remember and change this!
Added changes from Ceciile to this test module Co-authored-by: C Freeman <[email protected]>
- Added type annotations for OpenCommissioningWindow function - Added None type expectedErrorCode variable - Updated test step numbering to match test plan
- Updated test step 15 failure assertion to not start from number 1, but instead to correctly make sure that fabric_idx_cr2 does not exist in current fabric indexes list
PR #35524: Size comparison from 958c1cb to 3043332 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Implementing DNS-SD feature from Raul to cover test step 8
- Resolving linting errors
PR #35524: Size comparison from 789e941 to 05c032d Full report (3 builds for cc32xx, stm32)
|
Testing