From 22046a3554a9381f9f9a10b0da79f7d56574f7b1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 2 Feb 2023 10:08:51 -0500 Subject: [PATCH] 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 --- scripts/tests/chiptest/__init__.py | 10 +- .../yamltest_with_chip_repl_tester.py | 8 +- src/app/tests/suites/TestDiscovery.yaml | 5 +- .../commands/discovery/DiscoveryCommands.cpp | 3 +- src/controller/python/chip/yaml/runner.py | 147 ++++++++++++++++-- .../chip-tool/zap-generated/test/Commands.h | 4 +- 6 files changed, 152 insertions(+), 25 deletions(-) diff --git a/scripts/tests/chiptest/__init__.py b/scripts/tests/chiptest/__init__.py index d8698e7a110a34..08153207f1420e 100644 --- a/scripts/tests/chiptest/__init__.py +++ b/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")) diff --git a/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py b/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py index f852d30c9b4a53..f1d5583feb865c 100644 --- a/scripts/tests/chiptest/yamltest_with_chip_repl_tester.py +++ b/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()) diff --git a/src/app/tests/suites/TestDiscovery.yaml b/src/app/tests/suites/TestDiscovery.yaml index fc01f5c1cfef8b..474bfb02945884 100644 --- a/src/app/tests/suites/TestDiscovery.yaml +++ b/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" diff --git a/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp b/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp index 7cf0dde53f525e..c3ba7b4ca0e65e 100644 --- a/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp +++ b/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp @@ -37,8 +37,7 @@ CHIP_ERROR DiscoveryCommands::FindCommissionableByShortDiscriminator( { ReturnErrorOnFailure(SetupDiscoveryCommands()); - uint64_t shortDiscriminator = static_cast((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); } diff --git a/src/controller/python/chip/yaml/runner.py b/src/controller/python/chip/yaml/runner.py index 0318d5262b9b79..8d8f6c970d8b7a 100644 --- a/src/controller/python/chip/yaml/runner.py +++ b/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 diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index c69097fde2ab1f..995f4a5e66bdff 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -65274,6 +65274,7 @@ class TestDiscoverySuite : public TestCommand AddArgument("nodeId", 0, UINT64_MAX, &mNodeId); AddArgument("endpoint", 0, UINT16_MAX, &mEndpoint); AddArgument("discriminator", 0, UINT16_MAX, &mDiscriminator); + AddArgument("shortDiscriminator", 0, UINT16_MAX, &mShortDiscriminator); AddArgument("vendorId", 0, UINT16_MAX, &mVendorId); AddArgument("productId", 0, UINT16_MAX, &mProductId); AddArgument("deviceType", 0, UINT16_MAX, &mDeviceType); @@ -65298,6 +65299,7 @@ class TestDiscoverySuite : public TestCommand chip::Optional mNodeId; chip::Optional mEndpoint; chip::Optional mDiscriminator; + chip::Optional mShortDiscriminator; chip::Optional mVendorId; chip::Optional mProductId; chip::Optional mDeviceType; @@ -65619,7 +65621,7 @@ class TestDiscoverySuite : public TestCommand LogStep(8, "Check Short Discriminator (_S)"); ListFreer listFreer; chip::app::Clusters::DiscoveryCommands::Commands::FindCommissionableByShortDiscriminator::Type value; - value.value = mDiscriminator.HasValue() ? mDiscriminator.Value() : 3840ULL; + value.value = mShortDiscriminator.HasValue() ? mShortDiscriminator.Value() : 15ULL; return FindCommissionableByShortDiscriminator(kIdentityAlpha, value); } case 9: {