TC-TCCM Python Migration #36767

tersal · 2024-12-09T19:55:00Z

This change is the implementation of the test case TC-TCCM-1.2, part of the goal was to create a "base" implementation that can be reused by similar tests

Part of work for project-chip/matter-test-scripts#424

semanticdiff-com · 2024-12-09T19:55:03Z

Review changes with

Changed Files

File	Status
src/python_testing/TC_TCCM_1_2.py	0% smaller
src/python_testing/modebase_cluster_check.py	0% smaller

CLAassistant · 2024-12-09T19:55:07Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

andy31415 · 2024-12-09T20:30:13Z

src/python_testing/tc_mode_base.py

+logger = logging.getLogger(__name__)
+
+
+class TC_MODE_BASE:


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.

class naming should be CamelCase

Changed the names format and filename.

andy31415 · 2024-12-09T20:30:36Z

src/python_testing/tc_mode_base.py

+
+class TC_MODE_BASE:
+
+    def initialize_tc_base(self, endpoint, attributes, requested_cluster, cluster_objects, *args):


also needs comments. Why is this a separate method and not __init__ ?

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.

andy31415 · 2024-12-09T20:30:59Z

src/python_testing/tc_mode_base.py

+class TC_MODE_BASE:
+
+    def initialize_tc_base(self, endpoint, attributes, requested_cluster, cluster_objects, *args):
+        self.commonTags = {0x0: 'Auto',


where do these magic constants come from? Do we have them from codegen or could we reference spec locations?

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.

andy31415 · 2024-12-09T20:31:45Z

src/python_testing/TC_TCCM_1_2.py

+from mobly import asserts
+
+
+class TC_TCCM_1_2(MatterBaseTest, TC_MODE_BASE):


maybe composition over inheritance would be better.

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

cecille · 2024-12-09T20:51:20Z

src/python_testing/TC_TCCM_1_2.py

+from mobly import asserts
+
+
+class TC_TCCM_1_2(MatterBaseTest, TC_MODE_BASE):


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

cecille · 2024-12-09T20:53:09Z

src/python_testing/TC_TCCM_1_2.py

+
+        self.step(2)
+        await self.check_supported_modes_and_labels()
+        additional_tags = [Clusters.RefrigeratorAndTemperatureControlledCabinetMode.Enums.ModeTag.kRapidCool,


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?

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.

I wonder if we could somehow shorten things by giving a name to Clusters.RefrigeratorAndTemperatureControlledCabinetMode.Enums as well as Clusters.RefrigeratorAndTemperatureControlledCabinetMode.Attributes (probably like enums and attributes) as they make the code lines very long.

we already named attributes in the checks class.

andy31415 · 2024-12-09T21:07:42Z

src/python_testing/tc_mode_base.py

+            else:
+                labels.add(mode_options_struct.label)
+
+    async def check_if_labels_in_lists(self, requiredtags=None):


does this need to be async? looked briefly for some await in impl and did not see anything obvious.

My mistake, this is actually not needed.

andy31415 · 2024-12-09T21:08:21Z

src/python_testing/tc_mode_base.py

+            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:


could we use some constants for these magic values? especially for common tag logic I would imagine we already have some helpers somewhere.

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.

github-actions · 2024-12-11T01:47:31Z

PR #36767: Size comparison from 43167a0 to ff5e2eb

Full report (3 builds for cc32xx, stm32)

platform	target	config	section	`43167a0`	`ff5e2eb`
cc32xx	air-purifier	CC3235SF_LAUNCHXL	FLASH	631050	631050
			RAM	205824	205824
	lock	CC3235SF_LAUNCHXL	FLASH	669646	669646
			RAM	205968	205968
stm32	light	STM32WB5MM-DK	FLASH	484720	484720
			RAM	144880	144880

andy31415 · 2024-12-11T16:20:11Z

src/python_testing/tc_mode_base.py

+END_MFGTAGS_RANGE = 0xBFFF
+
+
+class ClusterModeCheck:


Based on the description:

This class holds common checks between different test cases

this is based on ModeBase common things

How about naming this ModeBaseClusterChecks (and adjust the file name as well to not contain base) ?

Changed the name for the class and also the name of the file. I'm not sure if I can completely remove the word "base" since the name of the "parent" cluster is ModeBase, but hopefully it is less general now.

andy31415 · 2024-12-11T16:23:41Z

src/python_testing/tc_mode_base.py

+
+
+    Attributes:
+        requested_cluster: A reference to the cluster to be tested, it should be a derived from the Mode Base cluster.


nit: consider modebase_derived_cluster (since the documentation says this should be a derived from the Mode Base cluster)?

That makes sense, changed the name of the parameter.

andy31415 · 2024-12-11T16:27:18Z

src/python_testing/tc_mode_base.py

+        self.modeTags = [tag.value for tag in requested_cluster.Enums.ModeTag]
+        self.requested_cluster = requested_cluster
+        self.attributes = requested_cluster.Attributes
+        self.supported_modes_dut = set()


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)

Yeah, that makes sense. I overlook this dependency.

Removed those members and used them as parameters in the other calls.

tersal added 2 commits December 9, 2024 11:16

Add base test and TCCM implementation

f2b7922

Merge branch 'project-chip:master' into tc-tccm-1-2-python-test

52b4ad6

github-actions bot added tests matter-1.4-te2-script-change Script changes before end of Matter 1.4 TE2 labels Dec 9, 2024

andy31415 reviewed Dec 9, 2024

View reviewed changes

cecille reviewed Dec 9, 2024

View reviewed changes

andy31415 reviewed Dec 9, 2024

View reviewed changes

tersal added 2 commits December 10, 2024 11:20

Merge branch 'project-chip:master' into tc-tccm-1-2-python-test

06e5730

Refactor and comments

ff5e2eb

andy31415 reviewed Dec 11, 2024

View reviewed changes

tersal added 2 commits December 11, 2024 14:13

Rearrange code and reduce dependencies

bfa96d4

Merge branch 'master' into tc-tccm-1-2-python-test

4a745ba

tersal marked this pull request as ready for review December 20, 2024 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TC-TCCM Python Migration #36767

TC-TCCM Python Migration #36767

tersal commented Dec 9, 2024 •

edited by andy31415

Loading

semanticdiff-com bot commented Dec 9, 2024 •

edited

Loading

CLAassistant commented Dec 9, 2024

andy31415 Dec 9, 2024 •

edited

Loading

andy31415 Dec 9, 2024

tersal Dec 12, 2024

andy31415 Dec 9, 2024

tersal Dec 11, 2024

andy31415 Dec 9, 2024

tersal Dec 11, 2024

andy31415 Dec 9, 2024

cecille Dec 9, 2024

cecille Dec 9, 2024

cecille Dec 9, 2024

tersal Dec 11, 2024

andy31415 Dec 11, 2024

andy31415 Dec 11, 2024

andy31415 Dec 9, 2024

tersal Dec 11, 2024

andy31415 Dec 9, 2024

tersal Dec 11, 2024

github-actions bot commented Dec 11, 2024

andy31415 Dec 11, 2024 •

edited

Loading

tersal Dec 20, 2024

andy31415 Dec 11, 2024

tersal Dec 20, 2024

andy31415 Dec 11, 2024

tersal Dec 12, 2024


		class TC_MODE_BASE:

		def initialize_tc_base(self, endpoint, attributes, requested_cluster, cluster_objects, *args):

		from mobly import asserts


		class TC_TCCM_1_2(MatterBaseTest, TC_MODE_BASE):



		Attributes:
		requested_cluster: A reference to the cluster to be tested, it should be a derived from the Mode Base cluster.

TC-TCCM Python Migration #36767

Are you sure you want to change the base?

TC-TCCM Python Migration #36767

Conversation

tersal commented Dec 9, 2024 • edited by andy31415 Loading

semanticdiff-com bot commented Dec 9, 2024 • edited Loading

CLAassistant commented Dec 9, 2024

andy31415 Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2024

andy31415 Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tersal commented Dec 9, 2024 •

edited by andy31415

Loading

semanticdiff-com bot commented Dec 9, 2024 •

edited

Loading

andy31415 Dec 9, 2024 •

edited

Loading

andy31415 Dec 11, 2024 •

edited

Loading