project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.34k stars 1.97k forks source link

Need a better way to check for successful attribute read in python tests #35372

Open tleacmcsa opened 2 weeks ago

tleacmcsa commented 2 weeks ago

As discussed in this PR: https://github.com/project-chip/connectedhomeip/pull/35322#discussion_r1739403129

It is awkward to check for successful read as shown in the following test code:


            for server in server_list:
                cluster = Clusters.ClusterObjects.ALL_CLUSTERS[server]
                data = await dev_ctrl.ReadAttribute(dut_node_id, [(endpoint, cluster.Attributes.GeneratedCommandList),
                                                                  (endpoint, cluster.Attributes.AcceptedCommandList),
                                                                  (endpoint, cluster.Attributes.AttributeList),
                                                                  (endpoint, cluster.Attributes.FeatureMap),
                                                                  (endpoint, cluster.Attributes.ClusterRevision)])
                for endpoint, clusters in data.items():
                    for cluster, attributes in clusters.items():
                        for attribute, value in attributes.items():
                            asserts.assert_false(isinstance(value, ValueDecodeFailure) and
                                                 isinstance(value.Reason, InteractionModelError) and
                                                 value.Reason.status == Status.AccessRestricted,
                                                 "AccessRestricted is not allowed on Global attributes")

There should be a better way to verify all attributes read ended in success with values.

andy31415 commented 2 weeks ago

@tleacmcsa

check for successful read - the test from this example is a lot more targeted than that: it specifically tests that the error is not AccessRestricted, but does allow any other error whatsoever. Is this what is intended?

I fully agree we should make it easy to check for "no errors at all" however I am not sure we should optimize for this kind of specific exclusions. Overall I am questioning it somehow: why do we allow global attribute read failure as long as they fail with a different reason?

cecille commented 2 weeks ago

See https://github.com/project-chip/connectedhomeip/blob/master/src/python_testing/matter_testing_support.py#L1152

tleacmcsa commented 2 weeks ago

@andy31415 Perhaps the test is too specific, but the goal is to ensure none are AccessRestricted.

@cecille is there a multi-attribute read version that checks for success?