-
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
TC-TCCM Python Migration #36767
base: master
Are you sure you want to change the base?
TC-TCCM Python Migration #36767
Changes from 2 commits
f2b7922
52b4ad6
06e5730
ff5e2eb
bfa96d4
4a745ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
# | ||
# Copyright (c) 2024 Project CHIP Authors | ||
# All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
# | ||
|
||
# === BEGIN CI TEST ARGUMENTS === | ||
# test-runner-runs: | ||
# run1: | ||
# app: ${ENERGY_MANAGEMENT_APP} | ||
# app-args: > | ||
# --discriminator 1234 | ||
# --KVS kvs1 | ||
# --trace-to json:${TRACE_APP}.json | ||
# --enable-key 000102030405060708090a0b0c0d0e0f | ||
# --application water-heater | ||
# script-args: > | ||
# --storage-path admin_storage.json | ||
# --commissioning-method on-network | ||
# --discriminator 1234 | ||
# --passcode 20202021 | ||
# --hex-arg enableKey:000102030405060708090a0b0c0d0e0f | ||
# --endpoint 2 | ||
# --trace-to json:${TRACE_TEST_JSON}.json | ||
# --trace-to perfetto:${TRACE_TEST_PERFETTO}.perfetto | ||
# factory-reset: true | ||
# quiet: true | ||
# === END CI TEST ARGUMENTS === | ||
|
||
import logging | ||
|
||
from tc_mode_base import TC_MODE_BASE | ||
|
||
import chip.clusters as Clusters | ||
from chip.testing.matter_testing import MatterBaseTest, TestStep, async_test_body, default_matter_test_main | ||
from mobly import asserts | ||
|
||
|
||
class TC_TCCM_1_2(MatterBaseTest, TC_MODE_BASE): | ||
|
||
def desc_TC_TCCM_1_2(self) -> str: | ||
return "[TC-TCCM-1.2] Cluster attributes with DUT as Server" | ||
|
||
def steps_TC_TCCM_1_2(self) -> list[TestStep]: | ||
steps = [ | ||
TestStep(1, "Commissioning, already done", is_commissioning=True), | ||
TestStep(2, "TH reads from the DUT the SupportedModes attribute."), | ||
TestStep(3, "TH reads from the DUT the CurrentMode attribute."), | ||
TestStep(4, "TH reads from the DUT the OnMode attribute."), | ||
TestStep(5, "TH reads from the DUT the StartUpMode attribute.") | ||
] | ||
return steps | ||
|
||
def pics_TC_TCCM_1_2(self) -> list[str]: | ||
pics = [ | ||
"TCCM.S" | ||
] | ||
return pics | ||
|
||
@async_test_body | ||
async def test_TC_TCCM_1_2(self): | ||
|
||
endpoint = self.get_endpoint(default=1) | ||
|
||
attributes = Clusters.RefrigeratorAndTemperatureControlledCabinetMode.Attributes | ||
|
||
self.initialize_tc_base(requested_cluster=Clusters.RefrigeratorAndTemperatureControlledCabinetMode, | ||
cluster_objects=Clusters.Objects.RefrigeratorAndTemperatureControlledCabinetMode, | ||
endpoint=endpoint, attributes=attributes) | ||
|
||
self.step(1) | ||
|
||
self.step(2) | ||
await self.check_supported_modes_and_labels() | ||
additional_tags = [Clusters.RefrigeratorAndTemperatureControlledCabinetMode.Enums.ModeTag.kRapidCool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor question / possible nit - is this the entire enum there, or a subset? If it's the fully list, can you pull the full list by walking the enum? If it's a subset, can you add a comment noting that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I see in the different TCs, these tags are a subset of the whole enum and seem to be different for each cluster. Added a comment for this specific cluster. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we could somehow shorten things by giving a name to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already named |
||
Clusters.RefrigeratorAndTemperatureControlledCabinetMode.Enums.ModeTag.kRapidFreeze] | ||
await self.check_if_labels_in_lists(requiredtags=additional_tags) | ||
|
||
self.step(3) | ||
await self.read_and_check_mode(mode=attributes.CurrentMode) | ||
|
||
self.step(4) | ||
await self.read_and_check_mode(mode=attributes.OnMode, is_nullable=True) | ||
|
||
self.step(5) | ||
await self.read_and_check_mode(mode=attributes.StartUpMode, is_nullable=True) | ||
|
||
|
||
if __name__ == "__main__": | ||
default_matter_test_main() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
# | ||
# Copyright (c) 2023 Project CHIP Authors | ||
# All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
# | ||
|
||
import logging | ||
|
||
from mobly import asserts | ||
from chip.clusters.Types import NullValue | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class TC_MODE_BASE: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs some doc comments. I would avoid using filler works like base/support/helper/core/common because files and classes named like that end up being a dumping ground of unrelated things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. class naming should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the names format and filename. |
||
|
||
def initialize_tc_base(self, endpoint, attributes, requested_cluster, cluster_objects, *args): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also needs comments. Why is this a separate method and not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not sure of the "best" way to handle this additional class (also having problems with super().init when using TC_MODE_BASE along with MatterBaseTest in the test case). I've replaced this function with the init and reduced the parameters, still not sure if this is the "best" way tho. |
||
self.commonTags = {0x0: 'Auto', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where do these magic constants come from? Do we have them from codegen or could we reference spec locations? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are defined in the spec for most of the "mode" clusters and seem to be common between them, this seems to be the spec (https://github.com/CHIP-Specifications/chip-test-plans/blob/master/src/cluster/modebase_common.adoc). However it seems that those values are part of the enum of each cluster so it is actually not needed to add them in a different structure. |
||
0x1: 'Quick', | ||
0x2: 'Quiet', | ||
0x3: 'LowNoise', | ||
0x4: 'LowEnergy', | ||
0x5: 'Vacation', | ||
0x6: 'Min', | ||
0x7: 'Max', | ||
0x8: 'Night', | ||
0x9: 'Day'} | ||
self.specificTags = [tag.value for tag in requested_cluster.Enums.ModeTag] | ||
self.supported_modes_dut = set() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. having these as members seems to introduce a call dependency: you need to call check_supported_modes_and_labels and only then read_and_check_mode Could we instead make things return values and not store internal state in the class? that would allow decoupling call order and make it more obvious to callers what data is used there (i.e. that the data from check is used in read_and_check) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense. I overlook this dependency. Removed those members and used them as parameters in the other calls. |
||
self.cluster_objects = cluster_objects | ||
self.supported_modes = None | ||
self.endpoint = endpoint | ||
self.attributes = attributes | ||
|
||
async def check_supported_modes_and_labels(self): | ||
# Get the supported modes | ||
self.supported_modes = await self.read_single_attribute_check_success(endpoint=self.endpoint, | ||
cluster=self.cluster_objects, | ||
attribute=self.attributes.SupportedModes) | ||
|
||
# Check if the list of supported modes is larger than 2 | ||
asserts.assert_greater_equal(len(self.supported_modes), 2, "SupportedModes must have at least 2 entries!") | ||
# Check that supported modes are less than 255 | ||
asserts.assert_less_equal(len(self.supported_modes), 255, "SupportedModes must have at most 255 entries!") | ||
|
||
# Check for repeated labels or modes | ||
labels = set() | ||
for mode_options_struct in self.supported_modes: | ||
# Verify that the modes in all ModeOptionsStruct in SupportedModes are unique. | ||
if mode_options_struct.mode in self.supported_modes_dut: | ||
asserts.fail("SupportedModes can't have repeated Mode values") | ||
else: | ||
self.supported_modes_dut.add(mode_options_struct.mode) | ||
# Verify that the labels in all ModeOptionsStruct in SupportedModes are unique. | ||
if mode_options_struct.label in labels: | ||
asserts.fail("SupportedModes can't have repeated Label values") | ||
else: | ||
labels.add(mode_options_struct.label) | ||
|
||
async def check_if_labels_in_lists(self, requiredtags=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this need to be async? looked briefly for some await in impl and did not see anything obvious. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My mistake, this is actually not needed. |
||
# Verify the ModeTags on each ModeOptionsStruct | ||
for mode_options_struct in self.supported_modes: | ||
# Shuld have at least one entry | ||
if len(mode_options_struct.modeTags) == 0: | ||
asserts.fail("The ModeTags field should have at least one entry.") | ||
|
||
# Check each ModelTag | ||
at_least_one_common_or_derived = False | ||
for tag in mode_options_struct.modeTags: | ||
# Value should not larger than 16bits | ||
if tag.value > 0xFFFF or tag.value < 0: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we use some constants for these magic values? especially for common tag logic I would imagine we already have some helpers somewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wasn't able to find helpers for this so I moved them, however not sure if that is the correct way to define constants according to the project guidelines. |
||
asserts.fail("Tag should not be larger than 16bits.") | ||
|
||
# Check if is tag is common, derived or mfg. | ||
is_mfg = (0x8000 <= tag.value <= 0xBFFF) | ||
if (tag.value not in self.commonTags and | ||
tag.value not in self.specificTags and | ||
not is_mfg): | ||
asserts.fail("Mode tag value is not a common, derived or vendor tag.") | ||
|
||
# Confirm if tag is common or derived. | ||
if not is_mfg: | ||
at_least_one_common_or_derived = True | ||
|
||
if not at_least_one_common_or_derived: | ||
asserts.fail("There should be at least one common or derived tag on each ModeOptionsStruct") | ||
|
||
if requiredtags: | ||
has_required_tags = False | ||
for mode_options_struct in self.supported_modes: | ||
has_required_tags = any(tag.value in requiredtags for tag in mode_options_struct.modeTags) | ||
if has_required_tags: | ||
break | ||
asserts.assert_true(has_required_tags, "No ModeOptionsStruct has the required tags.") | ||
|
||
async def read_and_check_mode(self, mode, is_nullable=False): | ||
mode_value = await self.read_single_attribute_check_success(endpoint=self.endpoint, cluster=self.cluster_objects, attribute=mode) | ||
is_valid = mode_value in self.supported_modes_dut | ||
if is_nullable and mode_value == NullValue: | ||
is_valid = True | ||
asserts.assert_true(is_valid, f"{mode} not supported") |
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.
maybe composition over inheritance would be better.
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 double-inheritance tends to happen in the tests because of the way the base framework finds the full test set. ie _find_test_class breaks when you inherit from MatterBaseTest and then re-inherit from another class. Definitely would be good to fix that, but it's a bit orthogonal