From 5ae3c657a8f17582d1af0c8abc9a8642c7185ad2 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 8 Sep 2023 13:19:40 -0400 Subject: [PATCH] Actually enable our YAML tests in the python-calls-chip-tool runner. (#29096) * Actually enable our YAML tests in the python-calls-chip-tool runner. We were now getting the right test list, but for each test we were not finding the test file. And the harness did not treat that as an error. * Address review comments. * Fix chip-repl CI --- .github/workflows/tests.yaml | 1 + scripts/tests/chiptest/__init__.py | 37 ++++++- scripts/tests/chiptest/test_definition.py | 1 + scripts/tests/run_test_suite.py | 1 + scripts/tests/yaml/runner.py | 6 +- .../tests/suites/TestClusterMultiFabric.yaml | 98 +++++++------------ src/app/tests/suites/TestEventsById.yaml | 18 ++-- 7 files changed, 87 insertions(+), 75 deletions(-) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 15057ec27b2b4d..ce0881ae6c1cec 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -223,6 +223,7 @@ jobs: --exclude-tags MANUAL \ --exclude-tags FLAKY \ --exclude-tags IN_DEVELOPMENT \ + --exclude-tags EXTRA_SLOW \ --exclude-tags SLOW \ run \ --iterations 1 \ diff --git a/scripts/tests/chiptest/__init__.py b/scripts/tests/chiptest/__init__.py index ce312d492beabd..6267ee037db863 100644 --- a/scripts/tests/chiptest/__init__.py +++ b/scripts/tests/chiptest/__init__.py @@ -47,6 +47,9 @@ class ManualTest: "PICS_Example.yaml", "Response_Example.yaml", "Test_Example.yaml", + "Test_Example_1.yaml", + "Test_Example_2.yaml", + "Test_Example_3.yaml", } @@ -127,6 +130,16 @@ def _GetSlowTests() -> Set[str]: } +def _GetExtraSlowTests() -> Set[str]: + """Generally tests using sleep() so much they should never run in CI. + + 1 minute seems like a good threshold to consider something extra slow + """ + return { + "Test_TC_DGGEN_2_1.yaml", # > 2 hours + } + + def _GetInDevelopmentTests() -> Set[str]: """Tests that fail in YAML for some reason.""" return { @@ -145,6 +158,8 @@ def _GetInDevelopmentTests() -> Set[str]: # TestEventTriggersEnabled is true, which it's not in CI. "Test_TC_SMOKECO_2_6.yaml", # chip-repl does not support local timeout (07/20/2023) and test assumes # TestEventTriggersEnabled is true, which it's not in CI. + "Test_TC_IDM_1_2.yaml", # Broken harness: https://github.com/project-chip/connectedhomeip/issues/29115 + "Test_TC_S_2_4.yaml", # https://github.com/project-chip/connectedhomeip/issues/29117 } @@ -177,6 +192,8 @@ def _GetChipReplUnsupportedTests() -> Set[str]: "Test_TC_G_2_4.yaml", # chip-repl does not support EqualityCommands pseudo-cluster "Test_TC_RVCRUNM_3_1.yaml", # chip-repl does not support EqualityCommands pseudo-cluster "Test_TC_RVCCLEANM_3_1.yaml", # chip-repl does not support EqualityCommands pseudo-cluster + # chip-repl and chip-tool disagree on what the YAML here should look like: https://github.com/project-chip/connectedhomeip/issues/29110 + "TestClusterMultiFabric.yaml", } @@ -243,10 +260,14 @@ def tests_with_command(chip_tool: str, is_manual: bool): ) -def _AllFoundYamlTests(treat_repl_unsupported_as_in_development: bool): +def _AllFoundYamlTests(treat_repl_unsupported_as_in_development: bool, use_short_run_name: bool): + """ + use_short_run_name should be true if we want the run_name to be "Test_ABC" instead of "some/path/Test_ABC.yaml" + """ manual_tests = _GetManualTests() flaky_tests = _GetFlakyTests() slow_tests = _GetSlowTests() + extra_slow_tests = _GetExtraSlowTests() in_development_tests = _GetInDevelopmentTests() chip_repl_unsupported_tests = _GetChipReplUnsupportedTests() @@ -264,14 +285,22 @@ def _AllFoundYamlTests(treat_repl_unsupported_as_in_development: bool): if path.name in slow_tests: tags.add(TestTag.SLOW) + if path.name in extra_slow_tests: + tags.add(TestTag.EXTRA_SLOW) + if path.name in in_development_tests: tags.add(TestTag.IN_DEVELOPMENT) if treat_repl_unsupported_as_in_development and path.name in chip_repl_unsupported_tests: tags.add(TestTag.IN_DEVELOPMENT) + if use_short_run_name: + run_name = path.stem # `path.stem` converts "some/path/Test_ABC_1.2.yaml" to "Test_ABC.1.2" + else: + run_name = str(path) + yield TestDefinition( - run_name=str(path), + run_name=run_name, name=path.stem, # `path.stem` converts "some/path/Test_ABC_1.2.yaml" to "Test_ABC.1.2" target=target_for_name(path.name), tags=tags, @@ -279,12 +308,12 @@ def _AllFoundYamlTests(treat_repl_unsupported_as_in_development: bool): def AllReplYamlTests(): - for test in _AllFoundYamlTests(treat_repl_unsupported_as_in_development=True): + for test in _AllFoundYamlTests(treat_repl_unsupported_as_in_development=True, use_short_run_name=False): yield test def AllChipToolYamlTests(): - for test in _AllFoundYamlTests(treat_repl_unsupported_as_in_development=False): + for test in _AllFoundYamlTests(treat_repl_unsupported_as_in_development=False, use_short_run_name=True): yield test diff --git a/scripts/tests/chiptest/test_definition.py b/scripts/tests/chiptest/test_definition.py index 08f97c98a2ea4e..68f2323a3302fc 100644 --- a/scripts/tests/chiptest/test_definition.py +++ b/scripts/tests/chiptest/test_definition.py @@ -218,6 +218,7 @@ class TestTag(Enum): FLAKY = auto() # test is considered flaky (usually a bug/time dependent issue) IN_DEVELOPMENT = auto() # test may not pass or undergoes changes CHIP_TOOL_PYTHON_ONLY = auto() # test uses YAML features only supported by the CHIP_TOOL_PYTHON runner. + EXTRA_SLOW = auto() # test uses Sleep and is generally _very_ slow (>= 60s is a typical threshold) def to_s(self): for (k, v) in TestTag.__members__.items(): diff --git a/scripts/tests/run_test_suite.py b/scripts/tests/run_test_suite.py index 643f13ce4d92af..ae5e569b36dd59 100755 --- a/scripts/tests/run_test_suite.py +++ b/scripts/tests/run_test_suite.py @@ -174,6 +174,7 @@ def main(context, dry_run, log_level, target, target_glob, target_skip_glob, TestTag.MANUAL, TestTag.IN_DEVELOPMENT, TestTag.FLAKY, + TestTag.EXTRA_SLOW } if runtime != TestRunTime.CHIP_TOOL_PYTHON: diff --git a/scripts/tests/yaml/runner.py b/scripts/tests/yaml/runner.py index 073726fa7e4813..746fc01656f135 100755 --- a/scripts/tests/yaml/runner.py +++ b/scripts/tests/yaml/runner.py @@ -260,8 +260,12 @@ def runner_base(ctx, configuration_directory: str, test_name: str, configuration specifications = SpecDefinitionsFromPaths(specifications_paths.split(','), pseudo_clusters) tests_finder = TestsFinder(configuration_directory, configuration_name) + test_list = tests_finder.get(test_name) + if len(test_list) == 0: + raise Exception(f"No tests found for test name '{test_name}'") + parser_config = TestParserConfig(pics, specifications, kwargs) - parser_builder_config = TestParserBuilderConfig(tests_finder.get(test_name), parser_config, hooks=TestParserLogger()) + parser_builder_config = TestParserBuilderConfig(test_list, parser_config, hooks=TestParserLogger()) parser_builder_config.options.stop_on_error = stop_on_error while ctx: ctx.obj = ParserGroup(parser_builder_config, pseudo_clusters) diff --git a/src/app/tests/suites/TestClusterMultiFabric.yaml b/src/app/tests/suites/TestClusterMultiFabric.yaml index 97b38ee103df85..8d3febcf410034 100644 --- a/src/app/tests/suites/TestClusterMultiFabric.yaml +++ b/src/app/tests/suites/TestClusterMultiFabric.yaml @@ -284,8 +284,7 @@ tests: # # TODO: This can be fixed using the `saveAs` function. # - value: - [ + value: [ { FabricIndex: 1, fabricSensitiveInt8u: 33, @@ -328,39 +327,29 @@ tests: }, { FabricIndex: 2, - fabricSensitiveInt8u: 0, + # These should actually be missing, not null, but right + # now our harness treats those the same, and we have no + # way to indicate "missing" in the YAML. + # https://github.com/project-chip/connectedhomeip/issues/29110 + fabricSensitiveInt8u: null, + optionalFabricSensitiveInt8u: null, nullableFabricSensitiveInt8u: null, - fabricSensitiveCharString: "", - fabricSensitiveStruct: - { - a: 0, - b: false, - c: 0, - d: "", - e: "", - f: 0, - g: 0, - h: 0, - }, - fabricSensitiveInt8uList: [], + fabricSensitiveCharString: null, + fabricSensitiveStruct: null, + fabricSensitiveInt8uList: null, }, { FabricIndex: 2, - fabricSensitiveInt8u: 0, + # These should actually be missing, not null, but right + # now our harness treats those the same, and we have no + # way to indicate "missing" in the YAML. + # https://github.com/project-chip/connectedhomeip/issues/29110 + fabricSensitiveInt8u: null, + optionalFabricSensitiveInt8u: null, nullableFabricSensitiveInt8u: null, - fabricSensitiveCharString: "", - fabricSensitiveStruct: - { - a: 0, - b: false, - c: 0, - d: "", - e: "", - f: 0, - g: 0, - h: 0, - }, - fabricSensitiveInt8uList: [], + fabricSensitiveCharString: null, + fabricSensitiveStruct: null, + fabricSensitiveInt8uList: null, }, ] @@ -371,43 +360,32 @@ tests: fabricFiltered: false attribute: "list_fabric_scoped" response: - value: - [ + value: [ { FabricIndex: 1, - fabricSensitiveInt8u: 0, + # These should actually be missing, not null, but right + # now our harness treats those the same, and we have no + # way to indicate "missing" in the YAML. + # https://github.com/project-chip/connectedhomeip/issues/29110 + fabricSensitiveInt8u: null, + optionalFabricSensitiveInt8u: null, nullableFabricSensitiveInt8u: null, - fabricSensitiveCharString: "", - fabricSensitiveStruct: - { - a: 0, - b: false, - c: 0, - d: "", - e: "", - f: 0, - g: 0, - h: 0, - }, - fabricSensitiveInt8uList: [], + fabricSensitiveCharString: null, + fabricSensitiveStruct: null, + fabricSensitiveInt8uList: null, }, { FabricIndex: 1, - fabricSensitiveInt8u: 0, + # These should actually be missing, not null, but right + # now our harness treats those the same, and we have no + # way to indicate "missing" in the YAML. + # https://github.com/project-chip/connectedhomeip/issues/29110 + fabricSensitiveInt8u: null, + optionalFabricSensitiveInt8u: null, nullableFabricSensitiveInt8u: null, - fabricSensitiveCharString: "", - fabricSensitiveStruct: - { - a: 0, - b: false, - c: 0, - d: "", - e: "", - f: 0, - g: 0, - h: 0, - }, - fabricSensitiveInt8uList: [], + fabricSensitiveCharString: null, + fabricSensitiveStruct: null, + fabricSensitiveInt8uList: null, }, { FabricIndex: 2, diff --git a/src/app/tests/suites/TestEventsById.yaml b/src/app/tests/suites/TestEventsById.yaml index 29224f92e3e7ac..fa772eacb1d56e 100644 --- a/src/app/tests/suites/TestEventsById.yaml +++ b/src/app/tests/suites/TestEventsById.yaml @@ -48,7 +48,8 @@ tests: value: ReadRequestMessage.Cluster - name: "EventId" value: ReadRequestMessage.Event - response: [] + response: + error: UNSUPPORTED_ENDPOINT - label: "Read Request Message with a path that indicates a specific cluster @@ -62,11 +63,12 @@ tests: value: UnsupportedCluster - name: "EventId" value: ReadRequestMessage.Event - response: [] + response: + error: UNSUPPORTED_CLUSTER - label: - "Read Request Message with a path that indicates a specific endpoint - that is unsupported" + "Read Request Message with a path that indicates a specific event that + is unsupported" cluster: "AnyCommands" command: "ReadEventById" endpoint: ReadRequestMessage.EndPoint @@ -256,9 +258,5 @@ tests: - name: "EventId" value: ReadRequestMessage.EndPoint response: - - values: - - value: { arg1: 1, arg2: 2, arg3: true } - - values: - - value: { arg1: 3, arg2: 1, arg3: false } - - values: - - value: { arg1: 4, arg2: 3, arg3: true } + # Not a valid event request path. + error: FAILURE