Add tidy linting checks for buildbot_steps.yml

This commit adds tidy checks for buildbot_steps.yml, as well as unit
tests. These checks include:

* Checking buildbot_steps.yml can be parsed by a YAML loader
* buildbot_steps.yml does not contain duplicate keys
* buildbot_steps.yml keys map to a list of strings

1 files changed, 58 insertions, 4 deletions

diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index 45ad2f44638..e96e4e82a55 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -20,6 +20,7 @@ import sys
 from licenseck import MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
 import colorama
 import toml
+import yaml
 
 CONFIG_FILE_PATH = os.path.join(".", "servo-tidy.toml")
 
@@ -45,7 +46,8 @@ COMMENTS = ["// ", "# ", " *", "/* "]
 # File patterns to include in the non-WPT tidy check.
 FILE_PATTERNS_TO_CHECK = ["*.rs", "*.rc", "*.cpp", "*.c",
                           "*.h", "Cargo.lock", "*.py", "*.sh",
-                          "*.toml", "*.webidl", "*.json", "*.html"]
+                          "*.toml", "*.webidl", "*.json", "*.html",
+                          "*.yml"]
 
 # File patterns that are ignored for all tidy and lint checks.
 FILE_PATTERNS_TO_IGNORE = ["*.#*", "*.pyc", "fake-ld.sh"]
@@ -137,7 +139,7 @@ def is_apache_licensed(header):
 
 
 def check_license(file_name, lines):
-    if any(file_name.endswith(ext) for ext in (".toml", ".lock", ".json", ".html")) or \
+    if any(file_name.endswith(ext) for ext in (".yml", ".toml", ".lock", ".json", ".html")) or \
        config["skip-check-licenses"]:
         raise StopIteration
 
@@ -175,7 +177,7 @@ def check_modeline(file_name, lines):
 
 
 def check_length(file_name, idx, line):
-    if any(file_name.endswith(ext) for ext in (".lock", ".json", ".html", ".toml")) or \
+    if any(file_name.endswith(ext) for ext in (".yml", ".lock", ".json", ".html", ".toml")) or \
        config["skip-check-length"]:
         raise StopIteration
 
@@ -632,6 +634,58 @@ def check_webidl_spec(file_name, contents):
     yield (0, "No specification link found.")
 
 
+def duplicate_key_yaml_constructor(loader, node, deep=False):
+    mapping = {}
+    for key_node, value_node in node.value:
+        key = loader.construct_object(key_node, deep=deep)
+        if key in mapping:
+            raise KeyError(key)
+        value = loader.construct_object(value_node, deep=deep)
+        mapping[key] = value
+    return loader.construct_mapping(node, deep)
+
+
+def lint_buildbot_steps_yaml(mapping):
+    # Check for well-formedness of contents
+    # A well-formed buildbot_steps.yml should be a map to list of strings
+    for k in mapping.keys():
+        if not isinstance(mapping[k], list):
+            raise ValueError("Key '{}' maps to type '{}', but list expected".format(k, type(mapping[k]).__name__))
+
+        # check if value is a list of strings
+        for item in itertools.ifilter(lambda i: not isinstance(i, str), mapping[k]):
+            raise ValueError("List mapped to '{}' contains non-string element".format(k))
+
+
+class SafeYamlLoader(yaml.SafeLoader):
+    """Subclass of yaml.SafeLoader to avoid mutating the global SafeLoader."""
+    pass
+
+
+def check_yaml(file_name, contents):
+    if not file_name.endswith("buildbot_steps.yml"):
+        raise StopIteration
+
+    # YAML specification doesn't explicitly disallow
+    # duplicate keys, but they shouldn't be allowed in
+    # buildbot_steps.yml as it could lead to confusion
+    SafeYamlLoader.add_constructor(
+        yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
+        duplicate_key_yaml_constructor
+    )
+
+    try:
+        contents = yaml.load(contents, Loader=SafeYamlLoader)
+        lint_buildbot_steps_yaml(contents)
+    except yaml.YAMLError as e:
+        line = e.problem_mark.line + 1 if hasattr(e, 'problem_mark') else None
+        yield (line, e)
+    except KeyError as e:
+        yield (None, "Duplicated Key ({})".format(e.message))
+    except ValueError as e:
+        yield (None, e.message)
+
+
 def check_for_possible_duplicate_json_keys(key_value_pairs):
     keys = [x[0] for x in key_value_pairs]
     seen_keys = set()
@@ -906,7 +960,7 @@ def scan(only_changed_files=False, progress=True):
     directory_errors = check_directory_files(config['check_ext'])
     # standard checks
     files_to_check = filter_files('.', only_changed_files, progress)
-    checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json)
+    checking_functions = (check_flake8, check_lock, check_webidl_spec, check_json, check_yaml)
     line_checking_functions = (check_license, check_by_line, check_toml, check_shell,
                                check_rust, check_spec, check_modeline)
     file_errors = collect_errors_for_files(files_to_check, checking_functions, line_checking_functions)