[Fix] Possible fix for matter-test-scripts issue #228: Remove PICS from RVC tests #34181
+194
−170
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
- 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"
6 tasks
|
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)
|
cecille
reviewed
Jul 4, 2024
| 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.
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.
cecille
reviewed
Jul 4, 2024
- 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)
|
cecille
reviewed
Jul 9, 2024
There was a problem hiding this comment.
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.
Q for the RVC folks - this is mandatory, right? Can we just remove this check?
There was a problem hiding this comment.
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: | |||
| 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.
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.
If there are other ways of checking this, yes.
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn and
Damian-Nordic
pullapprove
bot
requested review from
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yunhanw-google
|
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)
|
hicklin
requested changes
Aug 16, 2024
|
|
||
| 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.
Yes it is. Same of A0001. Should this check be made somewhere else, or is it dealt by the data model?
| 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.
If there are other ways of checking this, yes.
- 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
github-actions
bot
added
the
matter-1.4-te2-script-change
cecille
removed
the
matter-1.4-te2-script-change
github-actions
bot
added
the
matter-1.4-te2-script-change
|
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)
|
j-ororke
removed
the
matter-1.4-te2-script-change
github-actions
bot
added
the
matter-1.4-te2-script-change
- 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)
|
hicklin
requested changes
Oct 31, 2024
| @@ -96,14 +96,24 @@ async def test_TC_RVCCLEANM_2_1(self): | |||
| 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") | |||
| self.app_pipe = self.app_pipe + str(app_pid) | |||
There was a problem hiding this comment.
I think that this is needed as write_to_app_pipe is used later without specifying the app_pipe_name. See here.
There was a problem hiding this comment.
Thank you @hicklin,
You are correct that was definitely needed in there and not sure how I had deleted it.
However, I have returned it to the code now.
| if self.is_ci: | ||
| 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.c") | ||
| self.app_pipe = self.app_pipe + str(app_pid) |
There was a problem hiding this comment.
I think that this is needed as write_to_app_pipe is used later without specifying the app_pipe_name.
There was a problem hiding this comment.
Thank you, you are indeed correct that was definitely needed in there and not sure how I had deleted it.
However, I have returned it to the code now.
| @@ -90,15 +90,25 @@ async def test_TC_RVCRUNM_2_1(self): | |||
| if self.is_ci: | |||
| 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.c") | |||
| self.app_pipe = self.app_pipe + str(app_pid) | |||
There was a problem hiding this comment.
Thank you, you are correct that was definitely needed in there. I have returned it to its rightful place here now.
| if self.is_ci: | ||
| 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.c") | ||
| self.app_pipe = self.app_pipe + str(app_pid) |
There was a problem hiding this comment.
Thank you, you are correct that was definitely needed in there. I have returned it to its rightful place here now.
- Adding back in the self.app_pipe code as it appears to have been accidentally removed
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.
Following changes were applied and used:
-- Replaced with commands and attributes gathered from DUT