presubmit: Validate testcase.yaml files

Add a helper script that examines testcase.yaml files and call it during
the presubmit hook. Currently the script scans for extra_args fields in
the testcase.yaml files that reference the deprecated CONF_FILE,
OVERLAY_CONFIG, or DTC_OVERLAY_FILE vars and prints a message to use the
corresponding extra_conf_files, extra_overlay_confs, or
extra_dtc_overlay_files YAML field.

BUG=None
BRANCH=None
TEST=Tried submitting some invalid testcase.yaml files

Change-Id: I0a08c2bec156eb4e43acca255377c7563757c53b
Signed-off-by: Tristan Honscheid <honscheid@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/4428298
Reviewed-by: Al Semjonovs <asemjonovs@google.com>

3 files changed, 138 insertions, 4 deletions

diff --git a/util/presubmit_check.sh b/util/presubmit_check.sh
index b6ffa80a35..08b548b631 100755
--- a/util/presubmit_check.sh
+++ b/util/presubmit_check.sh
@@ -5,7 +5,7 @@
 # found in the LICENSE file.
 
 # Verify there is no CPRINTS("....\n", ...) statements added to the code.
-upstream_branch="$(git rev-parse --abbrev-ref --symbolic-full-name @{u} \
+upstream_branch="$(git rev-parse --abbrev-ref --symbolic-full-name @\{u\} \
     2>/dev/null)"
 if [[ -z ${upstream_branch} ]]; then
   echo "Current branch does not have an upstream branch" >&2
@@ -27,3 +27,10 @@ if git diff --no-ext-diff "${upstream_branch}" HEAD |
   echo "error: 'test_' prefix missing from test function name" >&2
   exit 1
 fi
+
+# Validate testcase.yaml files
+if ! git diff --no-ext-diff --name-only "${upstream_branch}" HEAD |
+     util/zephyr_check_testcase_yaml.py - ; then
+  echo "Errors detected while validating testcase.yaml files"
+  exit 1
+fi
diff --git a/util/twister_launcher.py b/util/twister_launcher.py
index 4cd35a6ef7..aa0665d8a9 100755
--- a/util/twister_launcher.py
+++ b/util/twister_launcher.py
@@ -93,6 +93,19 @@ import tempfile
 import time
 
 
+# Paths under the EC base dir that contain tests. This is used to define
+# Twister's search scope.
+EC_TEST_PATHS = [
+    Path("common"),
+    Path("zephyr/test"),
+]
+
+# Paths under ZEPHYR_BASE that we also wish to search for test cases.
+ZEPHYR_TEST_PATHS = [
+    Path("tests/subsys/shell"),
+]
+
+
 def find_checkout() -> Path:
     """Find the location of the source checkout or return None."""
     cros_checkout = os.environ.get("CROS_WORKON_SRCROOT")
@@ -336,9 +349,12 @@ def main():
         # should encompass all current Zephyr EC tests. The paths are meant to
         # be as specific as possible to limit Twister's search scope. This saves
         # significant time when starting Twister.
-        twister_cli.extend(["-T", str(ec_base / "common")])
-        twister_cli.extend(["-T", str(ec_base / "zephyr/test")])
-        twister_cli.extend(["-T", str(zephyr_base / "tests/subsys/shell")])
+        for path in EC_TEST_PATHS:
+            twister_cli.extend(["-T", str(ec_base / path)])
+
+        # Upstream tests we also wish to run:
+        for path in ZEPHYR_TEST_PATHS:
+            twister_cli.extend(["-T", str(zephyr_base / path)])
 
     if intercepted_args.platform:
         # Pass user-provided -p args when present.
diff --git a/util/zephyr_check_testcase_yaml.py b/util/zephyr_check_testcase_yaml.py
new file mode 100755
index 0000000000..82a8320a72
--- /dev/null
+++ b/util/zephyr_check_testcase_yaml.py
@@ -0,0 +1,111 @@
+#!/usr/bin/env vpython3
+# Copyright 2023 The ChromiumOS Authors
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Perform pre-submit checks on testcase.yaml files"""
+
+# [VPYTHON:BEGIN]
+# python_version: "3.8"
+# wheel: <
+#   name: "infra/python/wheels/pyyaml-py3"
+#   version: "version:5.3.1"
+# >
+# [VPYTHON:END]
+
+import argparse
+import glob
+from pathlib import Path
+import sys
+from typing import List
+
+from twister_launcher import EC_TEST_PATHS
+import yaml  # pylint: disable=import-error
+
+
+def check_extra_args(filepath: Path):
+    """Check if extra_args references blocked fields"""
+    errors = []
+
+    blocked_fields = {
+        "CONF_FILE": "extra_conf_files",
+        "OVERLAY_CONFIG": "extra_overlay_confs",
+        "DTC_OVERLAY_FILE": "extra_dtc_overlay_files",
+    }
+
+    with open(filepath, "r") as file:
+        data = yaml.load(file, Loader=yaml.SafeLoader)
+
+    def scan_section(section):
+        if "extra_args" in section:
+            for field in blocked_fields:
+                if field in section["extra_args"]:
+                    errors.append(
+                        f" * Don't specify {field} in `extra_args`. "
+                        f"Use `{blocked_fields[field]}` ({filepath})"
+                    )
+
+    if "common" in data:
+        scan_section(data["common"])
+    if "tests" in data:
+        for section in data["tests"]:
+            scan_section(data["tests"][section])
+
+    return errors
+
+
+def validate_files(files: List[Path]) -> List[str]:
+    """Run checks on a list of file paths."""
+    errors = []
+
+    checkers = [check_extra_args]
+
+    for file in files:
+        if not file.name == "testcase.yaml":
+            continue
+        for check in checkers:
+            errors.extend(check(file))
+
+    return errors
+
+
+if __name__ == "__main__":
+    parser = argparse.ArgumentParser()
+    parser.add_argument(
+        "files",
+        nargs="*",
+        type=Path,
+        help="List of files to validate. If blank, scan entire EC repo. "
+        "If '-', read a newline-separated list of files from stdin",
+    )
+
+    args = parser.parse_args()
+
+    if len(args.files) == 0:
+        ec_dir = Path(__file__).resolve().parent.parent
+        file_list = []
+
+        for p in EC_TEST_PATHS:
+            file_list.extend(
+                [
+                    Path(f)
+                    for f in glob.glob(
+                        str(ec_dir / p / "**/testcase.yaml"),
+                    )
+                ]
+            )
+
+    elif args.files[0] == Path("-"):
+        # Read from stdin
+        file_list = [Path(line.strip()) for line in sys.stdin.readlines()]
+        file_list.extend(args.files[1:])
+    else:
+        file_list = args.files
+
+    all_errors = validate_files(file_list)
+
+    if all_errors:
+        for error in all_errors:
+            sys.stderr.write(error)
+            sys.stderr.write("\n")
+        sys.exit(1)