Ensure we don't run slow YAML tests in CI. (#19975)

* Move some existing slow YAML tests to the "manual" list.
* Modify the test runner script so it can run manual tests if you explicitly
  tell it to.
* Add a way to specify a per-YAML-file timeout to the runner script.
* Use a 2-minute timeout in CI so that any YAML file that takes longer than
  that will fail.

Author: bzbarsky-apple

.github/workflows/darwin-tests.yaml

@@ -104,6 +104,7 @@ jobs:
                      --target-skip-glob '{TestGroupMessaging}' \
                      run \
                      --iterations 1 \
+                     --test-timeout-seconds 120 \
                      --all-clusters-app ./out/darwin-x64-all-clusters-${BUILD_VARIANT}/chip-all-clusters-app \
                      --lock-app ./out/darwin-x64-lock-${BUILD_VARIANT}/chip-lock-app \
                      --ota-provider-app ./out/darwin-x64-ota-provider-${BUILD_VARIANT}/chip-ota-provider-app \

.github/workflows/tests.yaml

@@ -97,6 +97,7 @@ jobs:
                      --chip-tool ./out/linux-x64-chip-tool${CHIP_TOOL_VARIANT}-${BUILD_VARIANT}/chip-tool \
                      run \
                      --iterations 1 \
+                     --test-timeout-seconds 120 \
                      --all-clusters-app ./out/linux-x64-all-clusters-${BUILD_VARIANT}/chip-all-clusters-app \
                      --lock-app ./out/linux-x64-lock-${BUILD_VARIANT}/chip-lock-app \
                      --ota-provider-app ./out/linux-x64-ota-provider-${BUILD_VARIANT}/chip-ota-provider-app \
@@ -198,6 +199,7 @@ jobs:
                      --target-skip-glob '{TestGroupMessaging,Test_TC_DIAG_TH_NW_1_1,Test_TC_DIAG_TH_NW_1_2,Test_TC_DIAG_TH_NW_2_2,Test_TC_DIAG_TH_NW_2_3,Test_TC_DIAG_TH_NW_2_6,Test_TC_DIAG_TH_NW_2_7,Test_TC_DIAG_TH_NW_2_8,Test_TC_DIAG_TH_NW_2_9}' \
                      run \
                      --iterations 1 \
+                     --test-timeout-seconds 120 \
                      --all-clusters-app ./out/darwin-x64-all-clusters-${BUILD_VARIANT}/chip-all-clusters-app \
                      --lock-app ./out/darwin-x64-lock-${BUILD_VARIANT}/chip-lock-app \
                      --ota-provider-app ./out/darwin-x64-ota-provider-${BUILD_VARIANT}/chip-ota-provider-app \

examples/darwin-framework-tool/templates/tests/tests.js

@@ -58,13 +58,16 @@ function getTests() {
   tests.disable('Test_TC_DIAG_TH_NW_2_3');
 
   // TODO: Test_TC_CC_9_1 does not work on Darwin for now.
-  tests.disable('Test_TC_CC_9_1');
+  // But is disabled in CI, so we can't disable it here.
+  //tests.disable('Test_TC_CC_9_1');
 
   // TODO: Test_TC_CC_9_2 does not work on Darwin for now.
-  tests.disable('Test_TC_CC_9_2');
+  // But is disabled in CI, so we can't disable it here.
+  //tests.disable('Test_TC_CC_9_2');
 
   // TODO: Test_TC_CC_9_3 does not work on Darwin for now.
-  tests.disable('Test_TC_CC_9_3');
+  // But is disabled in CI, so we can't disable it here.
+  //tests.disable('Test_TC_CC_9_3');
 
   // TODO: Test_TC_MC_3_7 does not work on Darwin for now.
   tests.disable('Test_TC_MC_3_7');

scripts/tests/chiptest/__init__.py

@@ -20,26 +20,41 @@
 from .test_definition import ApplicationPaths, TestDefinition, TestTarget
 
 
-def AllTests(chip_tool: str):
-    """Executes `chip_tool` binary to see what tests are available.
+def target_for_name(name: str):
+    if name.startswith('TV_') or name.startswith('Test_TC_MC_'):
+        return TestTarget.TV
+    if name.startswith('DL_') or name.startswith('Test_TC_DL_'):
+        return TestTarget.LOCK
+    if name.startswith('OTA_'):
+        return TestTarget.OTA
+    return TestTarget.ALL_CLUSTERS
+
+
+def tests_with_command(chip_tool: str, is_manual: bool):
+    """Executes `chip_tool` binary to see what tests are available, using cmd
+       to get the list.
     """
+    cmd = 'list'
+    if is_manual:
+        cmd += '-manual'
 
-    result = subprocess.run([chip_tool, 'tests', 'list'], capture_output=True)
+    result = subprocess.run([chip_tool, 'tests', cmd], capture_output=True)
 
     for name in result.stdout.decode('utf8').split('\n'):
         if not name:
             continue
 
-        if name.startswith('TV_') or name.startswith('Test_TC_MC_'):
-            target = TestTarget.TV
-        elif name.startswith('DL_') or name.startswith('Test_TC_DL_'):
-            target = TestTarget.LOCK
-        elif name.startswith('OTA_'):
-            target = TestTarget.OTA
-        else:
-            target = TestTarget.ALL_CLUSTERS
+        target = target_for_name(name)
+
+        yield TestDefinition(run_name=name, name=name, target=target, is_manual=is_manual)
+
+
+def AllTests(chip_tool: str):
+    for test in tests_with_command(chip_tool, is_manual=False):
+        yield test
 
-        yield TestDefinition(run_name=name, name=name, target=target)
+    for test in tests_with_command(chip_tool, is_manual=True):
+        yield test
 
 
 __all__ = [

scripts/tests/chiptest/runner.py

@@ -19,6 +19,7 @@
 import re
 import subprocess
 import threading
+import typing
 
 
 class LogPipe(threading.Thread):
@@ -87,12 +88,24 @@ def close(self):
 
 
 class RunnerWaitQueue:
-
-    def __init__(self):
+    def __init__(self, timeout_seconds: typing.Optional[int]):
         self.queue = queue.Queue()
+        self.timeout_seconds = timeout_seconds
+        self.timed_out = False
 
     def __wait(self, process, userdata):
-        process.wait()
+        if userdata is None:
+            # We're the main process for this wait queue.
+            timeout = self.timeout_seconds
+        else:
+            timeout = None
+        try:
+            process.wait(timeout)
+        except subprocess.TimeoutExpired:
+            self.timed_out = True
+            process.kill()
+            # And wait for the kill() to kill it.
+            process.wait()
         self.queue.put((process, userdata))
 
     def add_process(self, process, userdata=None):
@@ -109,7 +122,7 @@ class Runner:
     def __init__(self, capture_delegate=None):
         self.capture_delegate = capture_delegate
 
-    def RunSubprocess(self, cmd, name, wait=True, dependencies=[]):
+    def RunSubprocess(self, cmd, name, wait=True, dependencies=[], timeout_seconds: typing.Optional[int] = None):
         outpipe = LogPipe(
             logging.DEBUG, capture_delegate=self.capture_delegate,
             name=name + ' OUT')
@@ -127,7 +140,7 @@ def RunSubprocess(self, cmd, name, wait=True, dependencies=[]):
         if not wait:
             return s, outpipe, errpipe
 
-        wait = RunnerWaitQueue()
+        wait = RunnerWaitQueue(timeout_seconds=timeout_seconds)
         wait.add_process(s)
 
         for dependency in dependencies:
@@ -143,6 +156,9 @@ def RunSubprocess(self, cmd, name, wait=True, dependencies=[]):
                             (process.returncode, userdata))
 
         if s.returncode != 0:
-            raise Exception('Command %r failed: %d' % (cmd, s.returncode))
+            if wait.timed_out:
+                raise Exception("Command %r exceeded test timeout (%d seconds)" % (cmd, wait.timeout_seconds))
+            else:
+                raise Exception('Command %r failed: %d' % (cmd, s.returncode))
 
         logging.debug('Command %r completed with error code 0', cmd)

scripts/tests/chiptest/test_definition.py

@@ -207,8 +207,9 @@ class TestDefinition:
     name: str
     run_name: str
     target: TestTarget
+    is_manual: bool
 
-    def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str):
+    def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str, timeout_seconds: typing.Optional[int]):
         """
         Executes the given test case using the provided runner for execution.
         """
@@ -269,7 +270,8 @@ def Run(self, runner, apps_register, paths: ApplicationPaths, pics_file: str):
             test_cmd = tool_cmd + ['tests', self.run_name] + ['--PICS', pics_file]
             runner.RunSubprocess(
                 test_cmd,
-                name='TEST', dependencies=[apps_register])
+                name='TEST', dependencies=[apps_register],
+                timeout_seconds=timeout_seconds)
 
         except Exception:
             logging.error("!!!!!!!!!!!!!!!!!!!! ERROR !!!!!!!!!!!!!!!!!!!!!!")

scripts/tests/run_test_suite.py

@@ -119,7 +119,9 @@ def main(context, log_level, target, target_glob, target_skip_glob,
 
     # Figures out selected test that match the given name(s)
     all_tests = [test for test in chiptest.AllTests(chip_tool)]
-    tests = all_tests
+
+    # Default to only non-manual tests unless explicit targets are specified.
+    tests = list(filter(lambda test: not test.is_manual, all_tests))
     if 'all' not in target:
         tests = []
         for name in target:
@@ -131,6 +133,7 @@ def main(context, log_level, target, target_glob, target_skip_glob,
 
     if target_glob:
         matcher = GlobMatcher(target_glob.lower())
+        # Globs ignore manual tests, because it's too easy to mess up otherwise.
         tests = [test for test in tests if matcher.matches(test.name.lower())]
 
     if len(tests) == 0:
@@ -185,8 +188,13 @@ def cmd_list(context):
     type=click.Path(exists=True),
     default="src/app/tests/suites/certification/ci-pics-values",
     help='PICS file to use for test runs.')
+@click.option(
+    '--test-timeout-seconds',
+    default=None,
+    type=int,
+    help='If provided, fail if a test runs for longer than this time')
 @click.pass_context
-def cmd_run(context, iterations, all_clusters_app, lock_app, ota_provider_app, ota_requestor_app, tv_app, pics_file):
+def cmd_run(context, iterations, all_clusters_app, lock_app, ota_provider_app, ota_requestor_app, tv_app, pics_file, test_timeout_seconds):
     runner = chiptest.runner.Runner()
 
     if all_clusters_app is None:
@@ -237,7 +245,7 @@ def cmd_run(context, iterations, all_clusters_app, lock_app, ota_provider_app, o
         for test in context.obj.tests:
             test_start = time.monotonic()
             try:
-                test.Run(runner, apps_register, paths, pics_file)
+                test.Run(runner, apps_register, paths, pics_file, test_timeout_seconds)
                 test_end = time.monotonic()
                 logging.info('%-20s - Completed in %0.2f seconds' %
                              (test.name, (test_end - test_start)))

src/app/tests/suites/tests.js

@@ -250,6 +250,12 @@ function getManualTests() {
     'Test_TC_MF_1_26',
     'Test_TC_MF_1_27',
     'Test_TC_MF_1_28',
+    // Slow tests that should not run in CI because they take many minutes each
+    'Test_TC_MF_1_5',
+    'Test_TC_MF_1_6',
+    'Test_TC_MF_1_9',
+    'Test_TC_MF_1_10',
+    'Test_TC_MF_1_15',
   ];
 
   const ModeSelect = [
@@ -365,6 +371,10 @@ function getManualTests() {
     'Test_TC_CC_6_4',
     'Test_TC_CC_7_5',
     'Test_TC_CC_9_4',
+    // Slow tests that should not run in CI because they take many minutes each
+    'Test_TC_CC_9_1',
+    'Test_TC_CC_9_2',
+    'Test_TC_CC_9_3',
   ];
 
   const DoorLock = [
@@ -558,9 +568,6 @@ function getTests() {
     'Test_TC_CC_7_3',
     'Test_TC_CC_7_4',
     'Test_TC_CC_8_1',
-    'Test_TC_CC_9_1',
-    'Test_TC_CC_9_2',
-    'Test_TC_CC_9_3',
   ];
 
   const DeviceManagement = [
@@ -677,11 +684,6 @@ function getTests() {
   const MultipleFabrics = [
     'Test_TC_MF_1_3',
     'Test_TC_MF_1_4',
-    'Test_TC_MF_1_5',
-    'Test_TC_MF_1_6',
-    'Test_TC_MF_1_9',
-    'Test_TC_MF_1_10',
-    'Test_TC_MF_1_15',
   ];
 
   const OTASoftwareUpdate = [