Add "DiscoveryCommands" to yaml repl tests, make TestDiscovery.yaml…

… pass (#24763)

* Make discoverCommissionable work for now. More items to follow

* TestDiscovery now passes

* Minor update because short discriminator was wrong before

* Stop trying to auto-cast short discriminator in discovery commands

* Undo minmdns high verbosity

* Restyle

* Disable Test_TC_OO_2_4

* Set filter for MC to none (not needed)

* Update after review comments

* Update double-disable of flaky test... separating flaky from todo

* Undo restyle only change

* Make Test flaky instead of failing ... since it seems sometimes it may pass

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>

scripts/tests/chiptest/__init__.py

@@ -88,7 +88,6 @@ def _GetManualTests() -> Set[ManualTest]:
     manualtests.add(ManualTest(yaml="Test_TC_MEDIAPLAYBACK_6_2.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="Test_TC_MEDIAPLAYBACK_6_3.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="Test_TC_MEDIAPLAYBACK_6_4.yaml", reason="TODO"))
-    manualtests.add(ManualTest(yaml="Test_TC_OO_2_4.yaml", reason="Flaky"))
     manualtests.add(ManualTest(yaml="Test_TC_PCC_2_1.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="Test_TC_PS_2_1.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="Test_TC_SC_5_1.yaml", reason="TODO"))
@@ -101,11 +100,15 @@ def _GetManualTests() -> Set[ManualTest]:
     manualtests.add(ManualTest(yaml="Test_TC_WNCV_2_5.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="TestClusterMultiFabric.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="TestCommissionerNodeId.yaml", reason="TODO"))
-    manualtests.add(ManualTest(yaml="TestDiscovery.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="TestEvents.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="TestGroupMessaging.yaml", reason="TODO"))
     manualtests.add(ManualTest(yaml="TestMultiAdmin.yaml", reason="TODO"))
 
+    # Failing, unclear why. Likely repl specific, used to pass however first
+    # failure point seems unrelated. Historically this seems (very?) flaky
+    # in repl.
+    manualtests.add(ManualTest(yaml="Test_TC_OO_2_4.yaml", reason="Flaky"))
+
     # Examples:
     #
     # Currently these are not in ciTests.json, however yaml logic currently
@@ -114,8 +117,7 @@ def _GetManualTests() -> Set[ManualTest]:
     # This is on purpose for now to make it harder to orphan files, however
     # we can reconsider as things evolve.
     manualtests.add(ManualTest(yaml="Config_Example.yaml", reason="Example"))
-    manualtests.add(ManualTest(
-        yaml="Config_Variables_Example.yaml", reason="Example"))
+    manualtests.add(ManualTest(yaml="Config_Variables_Example.yaml", reason="Example"))
     manualtests.add(ManualTest(yaml="PICS_Example.yaml", reason="Example"))
     manualtests.add(ManualTest(yaml="Response_Example.yaml", reason="Example"))
     manualtests.add(ManualTest(yaml="Test_Example.yaml", reason="Example"))

scripts/tests/chiptest/yamltest_with_chip_repl_tester.py

@@ -15,6 +15,7 @@
 #    limitations under the License.
 
 import atexit
+import logging
 import os
 import tempfile
 import traceback
@@ -32,7 +33,7 @@
 from chip.ChipStack import *
 from chip.yaml.runner import ReplTestRunner
 from matter_yamltests.definitions import SpecDefinitionsFromPaths
-from matter_yamltests.parser import TestParser
+from matter_yamltests.parser import PostProcessCheckStatus, TestParser
 
 _DEFAULT_CHIP_ROOT = os.path.abspath(
     os.path.join(os.path.dirname(__file__), "..", "..", ".."))
@@ -118,6 +119,11 @@ def _StackShutDown():
                 post_processing_result = test_step.post_process_response(
                     decoded_response)
                 if not post_processing_result.is_success():
+                    logging.warning(f"Test step failure in 'test_step.label'")
+                    for entry in post_processing_result.entries:
+                        if entry.state == PostProcessCheckStatus.SUCCESS:
+                            continue
+                        logging.warning("%s: %s", entry.state, entry.message)
                     raise Exception(f'Test step failed {test_step.label}')
         except Exception:
             print(traceback.format_exc())

src/app/tests/suites/TestDiscovery.yaml

@@ -20,6 +20,9 @@ config:
     discriminator:
         type: int16u
         defaultValue: 3840
+    shortDiscriminator:
+        type: int16u
+        defaultValue: 15
     vendorId:
         type: int16u
         defaultValue: 65521
@@ -146,7 +149,7 @@ tests:
       arguments:
           values:
               - name: "value"
-                value: discriminator
+                value: shortDiscriminator
 
     - label: "Check Commissioning Mode (_CM)"
       cluster: "DiscoveryCommands"

src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp

@@ -37,8 +37,7 @@ CHIP_ERROR DiscoveryCommands::FindCommissionableByShortDiscriminator(
 {
     ReturnErrorOnFailure(SetupDiscoveryCommands());
 
-    uint64_t shortDiscriminator = static_cast<uint64_t>((value.value >> 8) & 0x0F);
-    chip::Dnssd::DiscoveryFilter filter(chip::Dnssd::DiscoveryFilterType::kShortDiscriminator, shortDiscriminator);
+    chip::Dnssd::DiscoveryFilter filter(chip::Dnssd::DiscoveryFilterType::kShortDiscriminator, value.value);
     return mDNSResolver.DiscoverCommissionableNodes(filter);
 }
 

src/controller/python/chip/yaml/runner.py

@@ -25,7 +25,7 @@
 import chip.interaction_model
 import chip.yaml.format_converter as Converter
 import stringcase
-from chip import ChipDeviceCtrl
+from chip.ChipDeviceCtrl import ChipDeviceController, discovery
 from chip.clusters.Attribute import AttributeStatus, SubscriptionTransaction, TypedAttributePath, ValueDecodeFailure
 from chip.exceptions import ChipStackError
 from chip.yaml.errors import ParsingError, UnexpectedParsingError
@@ -98,7 +98,7 @@ def pics_enabled(self):
         return self._pics_enabled
 
     @abstractmethod
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         pass
 
 
@@ -109,7 +109,7 @@ def __init__(self, test_step):
         if not _PSEUDO_CLUSTERS.supports(test_step):
             raise ParsingError(f'Default cluster {test_step.cluster} {test_step.command}, not supported')
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         resp = asyncio.run(_PSEUDO_CLUSTERS.execute(self._test_step))
         return _ActionResult(status=_ActionStatus.SUCCESS, response=None)
 
@@ -118,7 +118,7 @@ class InvokeAction(BaseAction):
     '''Single invoke action to be executed.'''
 
     def __init__(self, test_step, cluster: str, context: _ExecutionContext):
-        '''Converts 'test_step' to invoke command action that can execute with ChipDeviceCtrl.
+        '''Converts 'test_step' to invoke command action that can execute with ChipDeviceController.
 
         Args:
           'test_step': Step containing information required to run invoke command action.
@@ -162,7 +162,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext):
         else:
             self._request_object = command_object
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         try:
             resp = asyncio.run(dev_ctrl.SendCommand(
                 self._node_id, self._endpoint, self._request_object,
@@ -179,7 +179,7 @@ class ReadAttributeAction(BaseAction):
     '''Single read attribute action to be executed.'''
 
     def __init__(self, test_step, cluster: str, context: _ExecutionContext):
-        '''Converts 'test_step' to read attribute action that can execute with ChipDeviceCtrl.
+        '''Converts 'test_step' to read attribute action that can execute with ChipDeviceController.
 
         Args:
           'test_step': Step containing information required to run read attribute action.
@@ -224,7 +224,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext):
             raise UnexpectedParsingError(
                 f'ReadAttribute doesnt have valid attribute_type. {self.label}')
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         try:
             raw_resp = asyncio.run(dev_ctrl.ReadAttribute(self._node_id,
                                                           [(self._endpoint, self._request_object)],
@@ -289,7 +289,7 @@ def __init__(self, test_step):
             # Timeout is provided in seconds we need to conver to milliseconds.
             self._timeout_ms = request_data_as_dict['timeout'] * 1000
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         try:
             if self._expire_existing_session:
                 dev_ctrl.ExpireSessions(self._node_id)
@@ -326,7 +326,7 @@ class SubscribeAttributeAction(ReadAttributeAction):
     '''Single subscribe attribute action to be executed.'''
 
     def __init__(self, test_step, cluster: str, context: _ExecutionContext):
-        '''Converts 'test_step' to subscribe attribute action that can execute with ChipDeviceCtrl.
+        '''Converts 'test_step' to subscribe attribute action that can execute with ChipDeviceController.
 
         Args:
           'test_step': Step containing information required to run write attribute action.
@@ -349,7 +349,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext):
                 f'SubscribeAttribute action does not have max_interval {self.label}')
         self._max_interval = test_step.max_interval
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         try:
             subscription = asyncio.run(
                 dev_ctrl.ReadAttribute(self._node_id, [(self._endpoint, self._request_object)],
@@ -381,7 +381,7 @@ class WriteAttributeAction(BaseAction):
     '''Single write attribute action to be executed.'''
 
     def __init__(self, test_step, cluster: str, context: _ExecutionContext):
-        '''Converts 'test_step' to write attribute action that can execute with ChipDeviceCtrl.
+        '''Converts 'test_step' to write attribute action that can execute with ChipDeviceController.
 
         Args:
           'test_step': Step containing information required to run write attribute action.
@@ -425,7 +425,7 @@ def __init__(self, test_step, cluster: str, context: _ExecutionContext):
         # Create a cluster object for the request from the provided YAML data.
         self._request_object = attribute(request_data)
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         try:
             resp = asyncio.run(
                 dev_ctrl.WriteAttribute(self._node_id, [(self._endpoint, self._request_object)],
@@ -463,7 +463,7 @@ def __init__(self, test_step, context: _ExecutionContext):
         if self._output_queue is None:
             raise UnexpectedParsingError(f'Could not find output queue')
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         try:
             # While there should be a timeout here provided by the test, the current codegen version
             # of YAML tests doesn't have a per test step timeout, only a global timeout for the
@@ -495,7 +495,7 @@ def __init__(self, test_step):
         self._setup_payload = request_data_as_dict['payload']
         self._node_id = request_data_as_dict['nodeId']
 
-    def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         resp = dev_ctrl.CommissionWithCode(self._setup_payload, self._node_id)
 
         if resp:
@@ -504,10 +504,78 @@ def run_action(self, dev_ctrl: ChipDeviceCtrl) -> _ActionResult:
             return _ActionResult(status=_ActionStatus.ERROR, response=None)
 
 
+class DiscoveryCommandAction(BaseAction):
+    """DiscoveryCommand implementation (FindCommissionable* methods)."""
+
+    @staticmethod
+    def _filter_for_step(test_step) -> (discovery.FilterType, any):
+        """Given a test step, figure out the correct filters to give to
+           DiscoverCommissionableNodes.
+        """
+
+        if test_step.command == 'FindCommissionable':
+            return discovery.FilterType.NONE, None
+
+        if test_step.command == 'FindCommissionableByCommissioningMode':
+            # this is just a "_CM" subtype
+            return discovery.FilterType.COMMISSIONING_MODE, None
+
+        # all the items below require a "value" to use for filtering
+        args = test_step.arguments['values']
+        request_data_as_dict = Converter.convert_list_of_name_value_pair_to_dict(args)
+
+        filter = request_data_as_dict['value']
+
+        if test_step.command == 'FindCommissionableByDeviceType':
+            return discovery.FilterType.DEVICE_TYPE, filter
+
+        if test_step.command == 'FindCommissionableByLongDiscriminator':
+            return discovery.FilterType.LONG_DISCRIMINATOR, filter
+
+        if test_step.command == 'FindCommissionableByShortDiscriminator':
+            return discovery.FilterType.SHORT_DISCRIMINATOR, filter
+
+        if test_step.command == 'FindCommissionableByVendorId':
+            return discovery.FilterType.VENDOR_ID, filter
+
+        raise UnexpectedParsingError(f'Invalid command: {test_step.command}')
+
+    def __init__(self, test_step):
+        super().__init__(test_step)
+        self.filterType, self.filter = DiscoveryCommandAction._filter_for_step(test_step)
+
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
+        devices = dev_ctrl.DiscoverCommissionableNodes(
+            filterType=self.filterType, filter=self.filter, stopOnFirst=True, timeoutSecond=5)
+
+        # Devices will be a list: [CommissionableNode(), ...]
+        logging.info("Discovered devices: %r" % devices)
+
+        if not devices:
+            logging.error("No devices found")
+            return _ActionResult(status=_ActionStatus.ERROR, response="NO DEVICES FOUND")
+        elif len(devices) > 1:
+            logging.warning("Commissionable discovery found multiple results!")
+
+        return _ActionResult(status=_ActionStatus.SUCCESS, response=devices[0])
+
+
+class NotImplementedAction(BaseAction):
+    """Raises a "NOT YET IMPLEMENTED" exception when run."""
+
+    def __init__(self, test_step, cluster, command):
+        super().__init__(test_step)
+        self.cluster = cluster
+        self.command = command
+
+    def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
+        raise Exception(f"NOT YET IMPLEMENTED: {self.cluster}::{self.command}")
+
+
 class ReplTestRunner:
     '''Test runner to encode/decode values from YAML test Parser for executing the TestStep.
 
-    Uses ChipDeviceCtrl from chip-repl to execute parsed YAML TestSteps.
+    Uses ChipDeviceController from chip-repl to execute parsed YAML TestSteps.
     '''
 
     def __init__(self, test_spec_definition, certificate_authority_manager, alpha_dev_ctrl):
@@ -624,7 +692,9 @@ def encode(self, request) -> BaseAction:
         # Some of the tests contain 'cluster over-rides' that refer to a different
         # cluster than that specified in 'config'.
 
-        if cluster == 'DelayCommands' and command == 'WaitForCommissionee':
+        elif cluster == 'DiscoveryCommands':
+            return DiscoveryCommandAction(request)
+        elif cluster == 'DelayCommands' and command == 'WaitForCommissionee':
             action = self._wait_for_commissionee_action_factory(request)
         elif command == 'writeAttribute':
             action = self._attribute_write_action_factory(request, cluster)
@@ -668,6 +738,51 @@ def decode(self, result: _ActionResult):
             decoded_response['error'] = stringcase.snakecase(response.name).upper()
             return decoded_response
 
+        if isinstance(response, chip.discovery.CommissionableNode):
+            # CommissionableNode(
+            #    instanceName='04DD55352DD2AC53',
+            #    hostName='E6A32C6DBA8D0000',
+            #    port=5540,
+            #    longDiscriminator=3840,
+            #    vendorId=65521,
+            #    productId=32769,
+            #    commissioningMode=1,
+            #    deviceType=0,
+            #    deviceName='',
+            #    pairingInstruction='',
+            #    pairingHint=36,
+            #    mrpRetryIntervalIdle=None,
+            #    mrpRetryIntervalActive=None,
+            #    supportsTcp=True,
+            #    addresses=['fd00:0:1:1::3', '10.10.10.1']
+            # ), ...
+            decoded_response['value'] = {
+                'instanceName': response.instanceName,
+                'hostName': response.hostName,
+                'port': response.port,
+                'longDiscriminator': response.longDiscriminator,
+                'vendorId': response.vendorId,
+                'productId': response.productId,
+                'commissioningMode': response.commissioningMode,
+                'deviceType': response.deviceType,
+                'deviceName': response.deviceName,
+                'pairingInstruction': response.pairingInstruction,
+                'pairingHint': response.pairingHint,
+                'mrpRetryIntervalIdle': response.mrpRetryIntervalIdle,
+                'mrpRetryIntervalActive': response.mrpRetryIntervalActive,
+                'supportsTcp': response.supportsTcp,
+                'addresses': response.addresses,
+
+                # TODO: NOT AVAILABLE
+                'rotatingIdLen': 0,
+
+                # derived values
+                'numIPs': len(response.addresses),
+
+            }
+
+            return decoded_response
+
         if isinstance(response, ChipStackError):
             decoded_response['error'] = 'FAILURE'
             return decoded_response