[Camera] Update TC-AVSM Test Cases (TC-AVSM-2.1, TC-AVSM-2.13) #40406
+200
−27
Conversation
This file contains hidden or 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
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds verification steps to the TC_AVSM_2_1 test case by adding assertions after reading attribute values. The changes are generally good, but there are a couple of recurring issues with the new assertions. Firstly, there are redundant assert_is_not_none calls, as the helper function read_single_attribute_check_success already handles this check. Secondly, the matter_asserts functions are used with incorrect description strings, which would lead to confusing assertion failure messages. I've added a comment on the first occurrence of this pattern with a suggested fix that should be applied throughout the file for consistency and correctness.
|
PR #40406: Size comparison from dab4098 to 3fcbc87 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
yufengwangca
approved these changes
Aug 1, 2025
pidarped
approved these changes
Aug 1, 2025
chulspro
changed the title
Signed-off-by: Subodh Singh <subodh.singh@samsung.com> Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
chulspro
changed the title
|
PR #40406: Size comparison from dab4098 to 8bf363d Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #40406: Size comparison from f263ab7 to 26026a8 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
marktrayer
approved these changes
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
marktrayer
reviewed
Aug 7, 2025
| @@ -264,20 +284,23 @@ async def test_TC_AVSM_2_1(self): | |||
| endpoint=endpoint, cluster=cluster, attribute=attr.AllocatedVideoStreams | |||
| ) | |||
| logger.info(f"Rx'd AllocatedVideoStreams: {value}") | |||
| asserts.assert_is_not_none(value, "AllocatedVideoStreams is None") | |||
There was a problem hiding this comment.
This also needs to verify that values of the fields within the list of structs
There was a problem hiding this comment.
Thank you for review. Since video stream is not allocated, the returned list in empty, how should we verify the struct?
There was a problem hiding this comment.
That will possibly depend on the order in which test cases are run, either way, we could deliberately allocate a stream and then verify the population is correct. We can address that in a follow-up PR though.
marktrayer
reviewed
Aug 7, 2025
|
|
||
| self.step(18) | ||
| if self.pics_guard(self.check_pics("AVSM.S.A0010")): | ||
| value = await self.read_single_attribute_check_success( | ||
| endpoint=endpoint, cluster=cluster, attribute=attr.AllocatedAudioStreams | ||
| ) | ||
| logger.info(f"Rx'd AllocatedAudioStreams: {value}") | ||
| asserts.assert_is_not_none(value, "AllocatedAudioStreams is None") |
There was a problem hiding this comment.
This also needs to verify the values within the fields of the list of structs
There was a problem hiding this comment.
Thank you for review. Since audio stream is not allocated, the returned list in empty, how should we verify the struct?
There was a problem hiding this comment.
That will possibly depend on the order in which test cases are run, either way, we could deliberately allocate a stream and then verify the population is correct. We can address that in a follow-up PR though.
marktrayer
reviewed
Aug 7, 2025
|
|
||
| self.step(19) | ||
| if self.pics_guard(self.check_pics("AVSM.S.A0011")): | ||
| value = await self.read_single_attribute_check_success( | ||
| endpoint=endpoint, cluster=cluster, attribute=attr.AllocatedSnapshotStreams | ||
| ) | ||
| logger.info(f"Rx'd AllocatedSnapshotStreams: {value}") | ||
| asserts.assert_is_not_none(value, "AllocatedSnapshotStreams is None") |
There was a problem hiding this comment.
This also needs to verify the values within the fields of the list of structs
There was a problem hiding this comment.
Thank you for review. Since snapshot stream is not allocated, the returned list in empty, how should we verify the struct?
There was a problem hiding this comment.
That will possibly depend on the order in which test cases are run, either way, we could deliberately allocate a stream and then verify the population is correct. We can address that in a follow-up PR though.
There was a problem hiding this comment.
The contents of AllocatedSnapshotStreams should be tested by a different test case that is functional. Checking that read succeeds here is actually not relevant and is already done by TC-IDM-10.2. If not checking contents, we should not blind-read.
There was a problem hiding this comment.
What should happen is that the attribute is read, and if not an empty list (as no streams are maybe allocated), then the contents of the struct verified. I'm fine with doing this in a follow-up PR though.
marktrayer
reviewed
Aug 7, 2025
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Subodh Singh <subodh.singh@samsung.com> Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Subodh Singh <subodh.singh@samsung.com> Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Subodh Singh <subodh.singh@samsung.com> Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
Signed-off-by: Subodh Singh <subodh.singh@samsung.com> Signed-off-by: Charles Kim <chulspro.kim@samsung.com>
chulspro
force-pushed
the
feature/contribution-camera-22
branch
from
819ff81 to
7dff55f
Compare
| @@ -161,276 +164,443 @@ async def test_TC_AVSM_2_1(self): | |||
| endpoint=endpoint, cluster=cluster, attribute=attr.MaxConcurrentEncoders | |||
| ) | |||
| logger.info(f"Rx'd MaxConcurrentEncoders: {value}") | |||
| asserts.assert_is_not_none(value, "MaxConcurrentEncoders is None") | |||
| matter_asserts.assert_valid_uint8(value, "MaxConcurrentEncoders") | |||
|
|
|||
| self.step(3) | |||
| if self.pics_guard(self.check_pics("AVSM.S.A0001")): | |||
There was a problem hiding this comment.
Please replace ALL of these PICS guards with:
if await self.attribute_guard(endpoint=endpoint, attribute=attr.ATTR_NAME_HERE):
So:
if self.pics_guard(self.check_pics("AVSM.S.A0001")):
value = await self.read_single_attribute_check_success(
endpoint=endpoint, cluster=cluster, attribute=attr.MaxEncodedPixelRate
)
becomes:
if await self.attribute_guard(endpoint=endpoint, attribute=attr.MaxEncodedPixelRate):
value = await self.read_single_attribute_check_success(
endpoint=endpoint, cluster=cluster, attribute=attr.MaxEncodedPixelRate
)
This will remove dependies on PICS for this test, and make it more likely to catch issues.
tcarmelveilleux
approved these changes
Aug 22, 2025
|
|
||
| self.step(13) | ||
| if self.pics_guard(self.check_pics("AVSM.S.A000B")): | ||
| value = await self.read_single_attribute_check_success( | ||
| endpoint=endpoint, cluster=cluster, attribute=attr.MaxNetworkBandwidth | ||
| ) | ||
| logger.info(f"Rx'd MaxNetworkBandwidth: {value}") | ||
| matter_asserts.assert_valid_uint32(value, "MaxNetworkBandwidth") |
There was a problem hiding this comment.
This cannot be zero. Please check > 1
Comment on lines
+375
to
+377
| asserts.assert_less( | ||
| value, cluster.Enums.TriStateAutoEnum.kUnknownEnumValue, "NightVision is not a valid TriStateAutoEnum" | ||
| ) |
There was a problem hiding this comment.
The correct method for all enum assertions:
asserts.assert_in(value, cluster.Enums.TristateAutoEnum, "some error")
This properly validates enums with gaps and is less brittle.
Ex:
>>> from enum import IntEnum
>>> class SomeEnum(IntEnum):
... kFirst = 1
... kThird = 3
...
>>> a = 1
>>> a in SomeEnum
True
>>> a = 2
>>> a in SomeEnum
False
>>>
|
PR #40406: Size comparison from e086a20 to 04c303f Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
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.
Addresses the following issues:
project-chip/matter-test-scripts#669
project-chip/certification-tool#682
Testing
Build python wheel and activate venv:
Run TC-AVSM-2.1, TC-AVSM-2.13: