-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Fix] Possible fix for matter-test-scripts issue #228: Remove PICS from RVC tests #34181
base: master
Are you sure you want to change the base?
[Fix] Possible fix for matter-test-scripts issue #228: Remove PICS from RVC tests #34181
Conversation
- Removed PICS from python RVC Clean and RVC Run tests - Replaced with commands and attributes gathered from DUT.
PR #34181: Size comparison from 70744ed to fd9f558 Full report (39 builds for cc13x4_26x4, cc32xx, cyw30739, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
- Removed "generated_command_list" variable as not needed in TC_RVCRUNM_2_2 test module
PR #34181: Size comparison from 70744ed to 3881efe Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, tizen)
|
- Removed unused variable "RVCRun_gencmd_list"
PR #34181: Size comparison from 70744ed to f871894 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
app_pid = self.matter_test_config.app_pid | ||
if app_pid == 0: | ||
asserts.fail("The --app-pid flag must be set when PICS_SDK_CI_ONLY is set") | ||
app_pid = self.matter_test_config.app_pid |
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.
You can just leave this CI check in for now. The question of how to determine whether an app is running in the CI is fairly open, but this PICS code is kind of meta. The goal for these issues is to remove the PICS codes for things that are represented on the device already (clusters, features, attributes, commands etc). This one is fine.
- Reverted the changes done to the self.is_ci if check block that was made as it was not passing through CI/CD tests.
PR #34181: Size comparison from 70744ed to a9b96f7 Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, 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.
General Q for the RVC folks - do you think we need these checks at all, or should we just remove the checks entirely?
In 1.4 we're moving to a system where you need to pass conformance and PICS checks before running any other tests, so in theory you won't see non-conformant devices come through to here. Or would you prefer to leave these in as an additional check?
|
||
self.print_step(1, "Commissioning, already done") | ||
|
||
if self.check_pics("RVCCLEANM.S.A0000"): | ||
if supported_modes_attr_id in attribute_list: |
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.
Q for the RVC folks - this is mandatory, right? Can we just remove this check?
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.
Yes it is. Same of A0001
. Should this check be made somewhere else, or is it dealt by the data model?
@@ -124,7 +129,7 @@ async def test_TC_RVCCLEANM_1_2(self): | |||
asserts.assert_true(has_vacuum_or_mop_mode_tag, | |||
"At least one ModeOptionsStruct entry must include either the Vacuum or Mop mode tag") | |||
|
|||
if self.check_pics("RVCCLEANM.S.A0001"): | |||
if current_mode_attr_id in attribute_list: |
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.
ditto here
chg_mode_cmd_id = commands.ChangeToMode.command_id | ||
chg_rsp_cmd_id = commands.ChangeToModeResponse.command_id | ||
|
||
if supported_modes_attr_id not in attribute_list: |
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.
same q here - these are all mandatory, would it make sense to just remove these given that we have conformance checks now?
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.
If there are other ways of checking this, yes.
PR #34181: Size comparison from 9fc8c77 to 4f912dd Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
||
self.print_step(1, "Commissioning, already done") | ||
|
||
if self.check_pics("RVCCLEANM.S.A0000"): | ||
if supported_modes_attr_id in attribute_list: |
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.
Yes it is. Same of A0001
. Should this check be made somewhere else, or is it dealt by the data model?
asserts.assert_true(self.check_pics("RVCCLEANM.S.C01.Tx"), "RVCCLEANM.S.C01.Tx must be supported") | ||
|
||
attributes = Clusters.RvcCleanMode.Attributes | ||
if app_pid != 0: |
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 like this. Should this be indented one less together with like 94 and line 93 be removed?
Same comment in clean 2.2 and run 2.1 and 2.2.
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 believe you are correct, I have unindented lines 95 and 94 as the self.is_ci condition check on line 93 does not appear to be needed after running some tests without it. I have done the same in clean 2.2 and run 2.1 and 2.2 test modules as well.
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 have re-added the if self.is_ci checks as they appear to be needed for these tests
chg_mode_cmd_id = commands.ChangeToMode.command_id | ||
chg_rsp_cmd_id = commands.ChangeToModeResponse.command_id | ||
|
||
if supported_modes_attr_id not in attribute_list: |
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.
If there are other ways of checking this, yes.
asserts.fail("Supported modes needs to be supported attribute") | ||
|
||
if current_mode_attr_id not in attribute_list: | ||
asserts.fail("Current mode needs to be supported attribute") |
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 guess these can be removed as well now. Same in the following scripts.
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 have removed these conditional checks for current mode and supported mode attributes from the RVC Clean and Run test modules.
- Removed initial step for if self.is_ci check as does not appear to be needed - Removed attribute checks as supported and current mode attributes are mandatory
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 think the CI checks still need to be put back, but this looks otherwise correct to me.
@hicklin - would you mind taking another look?
PR #34181: Size comparison from c98b1f5 to 2ed9e31 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
- Updated to restore using if self.is_cli checks
PR #34181: Size comparison from 019dcc7 to 8ef7176 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Following changes were applied and used:
-- Replaced with commands and attributes gathered from DUT