address review comments

Author: cecille

src/python_testing/TestSpecParsingSupport.py

@@ -223,19 +223,19 @@ def test_write_optional(self):
 
     def test_derived_clusters(self):
         clusters: dict[int, XmlCluster] = {}
-        derived_clusters: dict[str, XmlCluster] = {}
+        pure_base_clusters: dict[str, XmlCluster] = {}
         ids_by_name: dict[str, int] = {}
         problems: list[ProblemNotice] = []
         base_cluster_xml = ElementTree.fromstring(BASE_CLUSTER_XML_STR)
         derived_cluster_xml = ElementTree.fromstring(DERIVED_CLUSTER_XML_STR)
         expected_global_attrs = [GlobalAttributeIds.FEATURE_MAP_ID, GlobalAttributeIds.ATTRIBUTE_LIST_ID,
                                  GlobalAttributeIds.ACCEPTED_COMMAND_LIST_ID, GlobalAttributeIds.GENERATED_COMMAND_LIST_ID, GlobalAttributeIds.CLUSTER_REVISION_ID]
 
-        add_cluster_data_from_xml(base_cluster_xml, clusters, derived_clusters, ids_by_name, problems)
-        add_cluster_data_from_xml(derived_cluster_xml, clusters, derived_clusters, ids_by_name, problems)
+        add_cluster_data_from_xml(base_cluster_xml, clusters, pure_base_clusters, ids_by_name, problems)
+        add_cluster_data_from_xml(derived_cluster_xml, clusters, pure_base_clusters, ids_by_name, problems)
 
         asserts.assert_equal(len(clusters), 1, "Unexpected number of clusters")
-        asserts.assert_equal(len(derived_clusters), 1, "Unexpected number of derived clusters")
+        asserts.assert_equal(len(pure_base_clusters), 1, "Unexpected number of derived clusters")
         asserts.assert_equal(len(ids_by_name), 1, "Unexpected number of IDs per name")
         asserts.assert_equal(len(problems), 0, "Unexpected number of problems")
         asserts.assert_equal(ids_by_name["Test Derived"], 0xFFFF, "Test derived name not added to IDs")
@@ -246,22 +246,23 @@ def test_derived_clusters(self):
         asserts.assert_equal(set(clusters[0xFFFF].accepted_commands.keys()), set([]), "Unexpected accepted commands")
         asserts.assert_equal(set(clusters[0xFFFF].generated_commands.keys()), set([]), "Unexpected generated commands")
 
-        asserts.assert_true("Test Base" in derived_clusters, "Base ID not found in derived clusters")
-        asserts.assert_equal(set(derived_clusters["Test Base"].attributes.keys()), set(
+        asserts.assert_true("Test Base" in pure_base_clusters, "Base ID not found in derived clusters")
+        asserts.assert_equal(set(pure_base_clusters["Test Base"].attributes.keys()), set(
             [0, 1, 2, 3] + expected_global_attrs), "Unexpected attribute list")
-        asserts.assert_equal(set(derived_clusters["Test Base"].accepted_commands.keys()), set([0]), "Unexpected accepted commands")
-        asserts.assert_equal(set(derived_clusters["Test Base"].generated_commands.keys()),
+        asserts.assert_equal(set(pure_base_clusters["Test Base"].accepted_commands.keys()),
+                             set([0]), "Unexpected accepted commands")
+        asserts.assert_equal(set(pure_base_clusters["Test Base"].generated_commands.keys()),
                              set([1]), "Unexpected generated commands")
-        asserts.assert_equal(str(derived_clusters["Test Base"].accepted_commands[0].conformance),
+        asserts.assert_equal(str(pure_base_clusters["Test Base"].accepted_commands[0].conformance),
                              "M", "Unexpected conformance on base accepted command")
-        asserts.assert_equal(str(derived_clusters["Test Base"].generated_commands[1].conformance),
+        asserts.assert_equal(str(pure_base_clusters["Test Base"].generated_commands[1].conformance),
                              "M", "Unexpected conformance on base generated command")
 
-        asserts.assert_equal(len(derived_clusters["Test Base"].unknown_commands),
+        asserts.assert_equal(len(pure_base_clusters["Test Base"].unknown_commands),
                              0, "Unexpected number of unknown commands in base")
         asserts.assert_equal(len(clusters[0xFFFF].unknown_commands), 2, "Unexpected number of unknown commands in derived cluster")
 
-        combine_derived_clusters_with_base(clusters, derived_clusters, ids_by_name)
+        combine_derived_clusters_with_base(clusters, pure_base_clusters, ids_by_name)
         # Ensure the base-only attribute (1) was added to the derived cluster
         asserts.assert_equal(set(clusters[0xFFFF].attributes.keys()), set(
             [0, 1, 2, 3] + expected_global_attrs), "Unexpected attribute list")
@@ -284,12 +285,12 @@ def test_derived_clusters(self):
 
     def test_missing_command_direction(self):
         clusters: dict[int, XmlCluster] = {}
-        derived_clusters: dict[str, XmlCluster] = {}
+        pure_base_clusters: dict[str, XmlCluster] = {}
         ids_by_name: dict[str, int] = {}
         problems: list[ProblemNotice] = []
         cluster_xml = ElementTree.fromstring(CLUSTER_WITH_UNKNOWN_COMMAND)
 
-        add_cluster_data_from_xml(cluster_xml, clusters, derived_clusters, ids_by_name, problems)
+        add_cluster_data_from_xml(cluster_xml, clusters, pure_base_clusters, ids_by_name, problems)
         check_clusters_for_unknown_commands(clusters, problems)
         asserts.assert_equal(len(problems), 1, "Unexpected number of problems found")
         asserts.assert_equal(problems[0].location.cluster_id, 0xFFFE, "Unexpected problem location (cluster id)")

src/python_testing/spec_parsing_support.py

@@ -348,7 +348,7 @@ def parse_attributes(self) -> dict[uint, XmlAttribute]:
             ), read_access=Clusters.AccessControl.Enums.AccessControlEntryPrivilegeEnum.kView, write_access=Clusters.AccessControl.Enums.AccessControlEntryPrivilegeEnum.kUnknownEnumValue, write_optional=False)
         return attributes
 
-    def get_command_direction(self, element: ElementTree.Element) -> CommandType:
+    def get_command_type(self, element: ElementTree.Element) -> CommandType:
         try:
             if element.attrib['direction'].lower() == 'responsefromserver':
                 return CommandType.GENERATED
@@ -360,7 +360,7 @@ def get_command_direction(self, element: ElementTree.Element) -> CommandType:
     def parse_unknown_commands(self) -> list[XmlCommand]:
         commands = []
         for element, conformance_xml, access_xml in self.command_elements:
-            if self.get_command_direction(element) != CommandType.UNKNOWN:
+            if self.get_command_type(element) != CommandType.UNKNOWN:
                 continue
             code = int(element.attrib['id'], 0)
             conformance = self.parse_conformance(conformance_xml)
@@ -370,7 +370,7 @@ def parse_unknown_commands(self) -> list[XmlCommand]:
     def parse_commands(self, command_type: CommandType) -> dict[uint, XmlCommand]:
         commands = {}
         for element, conformance_xml, access_xml in self.command_elements:
-            if self.get_command_direction(element) != command_type:
+            if self.get_command_type(element) != command_type:
                 continue
             code = int(element.attrib['id'], 0)
             conformance = self.parse_conformance(conformance_xml)
@@ -412,7 +412,16 @@ def get_problems(self) -> list[ProblemNotice]:
         return self._problems
 
 
-def add_cluster_data_from_xml(xml: ElementTree.Element, clusters: dict[int, XmlCluster], derived_clusters: dict[str, XmlCluster], ids_by_name: dict[str, int], problems: list[ProblemNotice]) -> None:
+def add_cluster_data_from_xml(xml: ElementTree.Element, clusters: dict[int, XmlCluster], pure_base_clusters: dict[str, XmlCluster], ids_by_name: dict[str, int], problems: list[ProblemNotice]) -> None:
+    ''' Adds cluster data to the supplied dicts as appropriate
+
+        xml: XML element read from from the XML cluster file
+        clusters: dict of id -> XmlCluster. This function will append new clusters as appropriate to this dict.
+        pure_base_clusters: dict of base name -> XmlCluster. This data structure is used to hold pure base clusters that don't have
+                            an ID. This function will append new pure base clusters as approrpriate to this dict.
+        ids_by_name: dict of cluster name -> ID. This function will append new IDs as appropriate to this dict.
+        problems: list of any problems encountered during spec parsing. This function will append problems as appropriate to this list.
+    '''
     cluster = xml.iter('cluster')
     for c in cluster:
         name = c.attrib['name']
@@ -433,7 +442,7 @@ def add_cluster_data_from_xml(xml: ElementTree.Element, clusters: dict[int, XmlC
         if cluster_id:
             clusters[cluster_id] = new
         else:
-            derived_clusters[name] = new
+            pure_base_clusters[name] = new
 
 
 def check_clusters_for_unknown_commands(clusters: dict[int, XmlCluster], problems: list[ProblemNotice]):
@@ -446,14 +455,14 @@ def check_clusters_for_unknown_commands(clusters: dict[int, XmlCluster], problem
 def build_xml_clusters() -> tuple[list[XmlCluster], list[ProblemNotice]]:
     dir = os.path.join(os.path.dirname(os.path.realpath(__file__)), '..', '..', 'data_model', 'clusters')
     clusters: dict[int, XmlCluster] = {}
-    derived_clusters: dict[str, XmlCluster] = {}
+    pure_base_clusters: dict[str, XmlCluster] = {}
     ids_by_name: dict[str, int] = {}
     problems: list[ProblemNotice] = []
     for xml in glob.glob(f"{dir}/*.xml"):
         logging.info(f'Parsing file {xml}')
         tree = ElementTree.parse(f'{xml}')
         root = tree.getroot()
-        add_cluster_data_from_xml(root, clusters, derived_clusters, ids_by_name, problems)
+        add_cluster_data_from_xml(root, clusters, pure_base_clusters, ids_by_name, problems)
 
     # There are a few clusters where the conformance columns are listed as desc. These clusters need specific, targeted tests
     # to properly assess conformance. Here, we list them as Optional to allow these for the general test. Targeted tests are described below.
@@ -475,11 +484,11 @@ def remove_problem(location: typing.Union[CommandPathLocation, FeaturePathLocati
         clusters[action_id].accepted_commands[c].conformance = optional()
         remove_problem(CommandPathLocation(endpoint_id=0, cluster_id=action_id, command_id=c))
 
-    combine_derived_clusters_with_base(clusters, derived_clusters, ids_by_name)
+    combine_derived_clusters_with_base(clusters, pure_base_clusters, ids_by_name)
 
     for alias_base_name, aliased_clusters in CLUSTER_ALIASES.items():
         for id, (alias_name, pics) in aliased_clusters.items():
-            base = derived_clusters[alias_base_name]
+            base = pure_base_clusters[alias_base_name]
             new = deepcopy(base)
             new.derived = alias_base_name
             new.name = alias_name
@@ -514,7 +523,7 @@ def remove_problem(location: typing.Union[CommandPathLocation, FeaturePathLocati
     return clusters, problems
 
 
-def combine_derived_clusters_with_base(xml_clusters: dict[int, XmlCluster], derived_clusters: dict[str, XmlCluster], ids_by_name: dict[str, int]) -> None:
+def combine_derived_clusters_with_base(xml_clusters: dict[int, XmlCluster], pure_base_clusters: dict[str, XmlCluster], ids_by_name: dict[str, int]) -> None:
     ''' Overrides base elements with the derived cluster values for derived clusters. '''
 
     def combine_attributes(base: dict[uint, XmlAttribute], derived: dict[uint, XmlAttribute], cluster_id: uint) -> dict[uint, XmlAttribute]:
@@ -547,7 +556,7 @@ def combine_attributes(base: dict[uint, XmlAttribute], derived: dict[uint, XmlAt
             if base_name in ids_by_name:
                 base = xml_clusters[ids_by_name[c.derived]]
             else:
-                base = derived_clusters[base_name]
+                base = pure_base_clusters[base_name]
 
             feature_map = deepcopy(base.feature_map)
             feature_map.update(c.feature_map)