tidy: A few small improvements and fixes (#30941)

1. Make the tidy output easier to follow
2. Integrate the WPT manifest cleanliness step into tidy
   itself and don't run it if nothing has changed in the WPT
   directory.
3. Fix an issue where Python test requirements were not installed,
   which could cause issues with some modules not being found.

Fixes #30002.

python/mach_bootstrap.py

@@ -14,8 +14,9 @@ from tempfile import TemporaryFile
 SCRIPT_PATH = os.path.abspath(os.path.dirname(__file__))
 TOP_DIR = os.path.abspath(os.path.join(SCRIPT_PATH, ".."))
 WPT_PATH = os.path.join(TOP_DIR, "tests", "wpt")
-WPT_RUNNER_PATH = os.path.join(WPT_PATH, "tests", "tools", "wptrunner")
+WPT_TOOLS_PATH = os.path.join(WPT_PATH, "tests", "tools")
-WPT_SERVE_PATH = os.path.join(WPT_PATH, "tests", "tools", "wptserve")
+WPT_RUNNER_PATH = os.path.join(WPT_TOOLS_PATH, "wptrunner")
+WPT_SERVE_PATH = os.path.join(WPT_TOOLS_PATH, "wptserve")
 
 SEARCH_PATHS = [
     os.path.join("python", "mach"),
@@ -162,6 +163,7 @@ def _activate_virtualenv(topdir):
     # and it will check for conflicts.
     requirements_paths = [
         os.path.join("python", "requirements.txt"),
+        os.path.join(WPT_TOOLS_PATH, "requirements_tests.txt",),
         os.path.join(WPT_RUNNER_PATH, "requirements.txt",),
     ]
 

python/servo/testing_commands.py

@@ -281,29 +281,27 @@ class MachCommands(CommandBase):
     @CommandArgument('--all', default=False, action="store_true", dest="all_files",
                      help="Check all files, and run the WPT lint in tidy, "
                           "even if unchanged")
-    @CommandArgument('--no-wpt', default=False, action="store_true", dest="no_wpt",
-                     help="Skip checking that web-platform-tests manifests are up to date")
     @CommandArgument('--no-progress', default=False, action="store_true",
                      help="Don't show progress for tidy")
-    @CommandArgument('--stylo', default=False, action="store_true",
+    def test_tidy(self, all_files, no_progress):
-                     help="Only handle files in the stylo tree")
+        tidy_failed = tidy.scan(not all_files, not no_progress)
-    def test_tidy(self, all_files, no_progress, stylo, no_wpt=False):
-        if no_wpt:
-            manifest_dirty = False
-        else:
-            manifest_dirty = wpt.manifestupdate.update(check_clean=True)
-        tidy_failed = tidy.scan(not all_files, not no_progress, stylo=stylo, no_wpt=no_wpt)
 
         call(["rustup", "install", "nightly-2023-03-18"])
         call(["rustup", "component", "add", "rustfmt", "--toolchain", "nightly-2023-03-18"])
         rustfmt_failed = call(["cargo", "+nightly-2023-03-18", "fmt", "--", "--check"])
-
         if rustfmt_failed:
             print("Run `./mach fmt` to fix the formatting")
 
         taplo_failed = format_toml_files_with_taplo()
 
-        return tidy_failed or manifest_dirty or rustfmt_failed or taplo_failed
+        tidy_failed = tidy_failed or rustfmt_failed or taplo_failed
+        print()
+        if tidy_failed:
+            print("\r ❌ test-tidy reported errors.")
+        else:
+            print("\r ✅ test-tidy reported no errors.")
+
+        tidy_failed
 
     @Command('test-scripts',
              description='Run tests for all build and support scripts.',

python/tidy/test.py

@@ -16,9 +16,12 @@ from . import tidy
 
 BASE_PATH = 'python/tidy/tests/'
 
+def test_file_path(name):
+    return os.path.join(BASE_PATH, name)
+
 
 def iterFile(name):
-    return iter([os.path.join(BASE_PATH, name)])
+    return iter([test_file_path(name)])
 
 
 class CheckTidiness(unittest.TestCase):
@@ -35,15 +38,6 @@ class CheckTidiness(unittest.TestCase):
         self.assertEqual("ignored directory './fake/dir' doesn't exist", next(errors)[2])
         self.assertNoMoreErrors(errors)
 
-    def test_non_existing_wpt_manifest_checks(self):
-        wrong_path = "/wrong/path.ini"
-        errors = tidy.check_manifest_dirs(wrong_path, print_text=False)
-        self.assertEqual("%s manifest file is required but was not found" % wrong_path, next(errors)[2])
-        self.assertNoMoreErrors(errors)
-        errors = tidy.check_manifest_dirs(os.path.join(BASE_PATH, 'manifest-include.ini'), print_text=False)
-        self.assertTrue(next(errors)[2].endswith("never_going_to_exist"))
-        self.assertNoMoreErrors(errors)
-
     def test_directory_checks(self):
         dirs = {
             os.path.join(BASE_PATH, "dir_check/webidl_plus"): ['webidl', 'test'],
@@ -180,7 +174,7 @@ class CheckTidiness(unittest.TestCase):
         self.assertNoMoreErrors(errors)
 
     def test_lock(self):
-        errors = tidy.collect_errors_for_files(iterFile('duplicated_package.lock'), [tidy.check_lock], [], print_text=False)
+        errors = tidy.check_cargo_lock_file(test_file_path('duplicated_package.lock'), print_text=False)
         msg = """duplicate versions for package `test`
 \t\x1b[93mThe following packages depend on version 0.4.9 from 'crates.io':\x1b[0m
 \t\ttest2 0.1.0
@@ -197,7 +191,7 @@ class CheckTidiness(unittest.TestCase):
 
     def test_lock_ignore_without_duplicates(self):
         tidy.config["ignore"]["packages"] = ["test", "test2", "test3", "test5"]
-        errors = tidy.collect_errors_for_files(iterFile('duplicated_package.lock'), [tidy.check_lock], [], print_text=False)
+        errors = tidy.check_cargo_lock_file(test_file_path('duplicated_package.lock'), print_text=False)
 
         msg = (
             "duplicates for `test2` are allowed, but only single version found"
@@ -215,7 +209,7 @@ class CheckTidiness(unittest.TestCase):
 
     def test_lock_exceptions(self):
         tidy.config["blocked-packages"]["rand"] = ["test_exception", "test_unneeded_exception"]
-        errors = tidy.collect_errors_for_files(iterFile('blocked_package.lock'), [tidy.check_lock], [], print_text=False)
+        errors = tidy.check_cargo_lock_file(test_file_path('blocked_package.lock'), print_text=False)
 
         msg = (
             "Package test_blocked 0.0.2 depends on blocked package rand."

python/tidy/tidy.py

@@ -7,39 +7,34 @@
 # option. This file may not be copied, modified, or distributed
 # except according to those terms.
 
+import configparser
 import fnmatch
 import glob
+import io
 import itertools
 import json
 import os
 import re
 import subprocess
 import sys
+from typing import List
 
 import colorama
 import toml
 
+import wpt.manifestupdate
+
 from .licenseck import OLD_MPL, MPL, APACHE, COPYRIGHT, licenses_toml, licenses_dep_toml
 
 TOPDIR = os.path.abspath(os.path.dirname(sys.argv[0]))
 WPT_PATH = os.path.join(".", "tests", "wpt")
-SUITES = ["tests", os.path.join("mozilla", "tests")]
-
-
-def wpt_path(*args):
-    return os.path.join(WPT_PATH, *args)
-
-
 CONFIG_FILE_PATH = os.path.join(".", "servo-tidy.toml")
-WPT_MANIFEST_PATH = wpt_path("include.ini")
+WPT_CONFIG_INI_PATH = os.path.join(WPT_PATH, "config.ini")
 # regex source https://stackoverflow.com/questions/6883049/
 URL_REGEX = re.compile(br'https?://(?:[-\w.]|(?:%[\da-fA-F]{2}))+')
 
-# Import wptmanifest only when we do have wpt in tree, i.e. we're not
+sys.path.append(os.path.join(WPT_PATH, "tests"))
-# inside a Firefox checkout.
+sys.path.append(os.path.join(WPT_PATH, "tests", "tools", "wptrunner"))
-if os.path.isfile(WPT_MANIFEST_PATH):
-    sys.path.append(wpt_path("tests", "tools", "wptrunner", "wptrunner"))
-    from wptmanifest import parser, node
 
 # Default configs
 config = {
@@ -188,8 +183,6 @@ def filter_file(file_name):
 def filter_files(start_dir, only_changed_files, progress):
     file_iter = FileList(start_dir, only_changed_files=only_changed_files,
                          exclude_dirs=config["ignore"]["directories"], progress=progress)
-    # always yield Cargo.lock so that the correctness of transitive dependencies is checked
-    yield "./Cargo.lock"
 
     for file_name in iter(file_iter):
         base_name = os.path.basename(file_name)
@@ -333,7 +326,13 @@ def check_flake8(file_name, contents):
         yield line_num, message.strip()
 
 
-def check_lock(file_name, contents):
+def check_cargo_lock_file(filename, print_text=True):
+    if print_text:
+        print(f"\r ➤  Checking cargo lock ({filename})...")
+
+    with open(filename) as cargo_lock_file:
+        content = toml.load(cargo_lock_file)
+
     def find_reverse_dependencies(name, content):
         for package in itertools.chain([content.get("root", {})], content["package"]):
             for dependency in package.get("dependencies", []):
@@ -342,14 +341,9 @@ def check_lock(file_name, contents):
                 if dependency[0] == name:
                     yield package["name"], package["version"], dependency
 
-    if not file_name.endswith(".lock"):
-        return
-
     # Package names to be neglected (as named by cargo)
     exceptions = config["ignore"]["packages"]
 
-    content = toml.loads(contents.decode("utf-8"))
-
     packages_by_name = {}
     for package in content.get("package", []):
         if "replace" in package:
@@ -361,7 +355,7 @@ def check_lock(file_name, contents):
 
     for name in exceptions:
         if name not in packages_by_name:
-            yield (1, "duplicates are allowed for `{}` but it is not a dependency".format(name))
+            yield (filename, 1, "duplicates are allowed for `{}` but it is not a dependency".format(name))
 
     for (name, packages) in packages_by_name.items():
         has_duplicates = len(packages) > 1
@@ -385,7 +379,7 @@ def check_lock(file_name, contents):
                 if (not dependency[1] or version in dependency[1]) and \
                    (not dependency[2] or short_source in dependency[2]):
                     message += "\n\t\t" + pname + " " + package_version
-        yield (1, message)
+        yield (filename, 1, message)
 
     # Check to see if we are transitively using any blocked packages
     blocked_packages = config["blocked-packages"]
@@ -403,7 +397,7 @@ def check_lock(file_name, contents):
                 if package_name not in whitelist:
                     fmt = "Package {} {} depends on blocked package {}."
                     message = fmt.format(package_name, package_version, dependency_name)
-                    yield (1, message)
+                    yield (filename, 1, message)
                 else:
                     visited_whitelisted_packages[dependency_name][package_name] = True
 
@@ -413,7 +407,7 @@ def check_lock(file_name, contents):
             if not visited_whitelisted_packages[dependency_name].get(package_name):
                 fmt = "Package {} is not required to be an exception of blocked package {}."
                 message = fmt.format(package_name, dependency_name)
-                yield (1, message)
+                yield (filename, 1, message)
 
 
 def check_toml(file_name, lines):
@@ -479,38 +473,6 @@ def check_shell(file_name, lines):
                     yield (idx + 1, "variable substitutions should use the full \"${VAR}\" form")
 
 
-def rec_parse(current_path, root_node):
-    dirs = []
-    for item in root_node.children:
-        if isinstance(item, node.DataNode):
-            next_depth = os.path.join(current_path, item.data)
-            dirs.append(next_depth)
-            dirs += rec_parse(next_depth, item)
-    return dirs
-
-
-def check_manifest_dirs(config_file, print_text=True):
-    if not os.path.exists(config_file):
-        yield (config_file, 0, "%s manifest file is required but was not found" % config_file)
-        return
-
-    # Load configs from include.ini
-    with open(config_file, "rb") as content:
-        conf_file = content.read()
-        lines = conf_file.splitlines(True)
-
-    if print_text:
-        print('\rChecking the wpt manifest file...')
-
-    p = parser.parse(lines)
-    paths = rec_parse(wpt_path("tests"), p)
-    for idx, path in enumerate(paths):
-        if '_mozilla' in path or '_webgl' in path or '_webgpu' in path:
-            continue
-        if not os.path.isdir(path):
-            yield (config_file, idx + 1, "Path in manifest was not found: {}".format(path))
-
-
 def check_rust(file_name, lines):
     if not file_name.endswith(".rs") or \
        file_name.endswith(".mako.rs") or \
@@ -778,6 +740,78 @@ def check_json(filename, contents):
         yield (None, e.args[0])
 
 
+def check_that_manifests_exist():
+    # Determine the metadata and test directories from the configuration file.
+    metadata_dirs = []
+    config = configparser.ConfigParser()
+    config.read(WPT_CONFIG_INI_PATH)
+    for key in config:
+        if key.startswith("manifest:"):
+            metadata_dirs.append(os.path.join("./tests/wpt/", config[key]["metadata"]))
+
+    for directory in metadata_dirs:
+        manifest_path = os.path.join(TOPDIR, directory, "MANIFEST.json")
+        if not os.path.isfile(manifest_path):
+            yield (WPT_CONFIG_INI_PATH, "", f"Path in config was not found: {manifest_path}")
+
+
+def check_that_manifests_are_clean():
+    from wptrunner import wptlogging
+    print("\r ➤  Checking WPT manifests for cleanliness...")
+    output_stream = io.StringIO("")
+    logger = wptlogging.setup({}, {"mach": output_stream})
+    if wpt.manifestupdate.update(check_clean=True, logger=logger):
+        for line in output_stream.getvalue().splitlines():
+            if "ERROR" in line:
+                yield (WPT_CONFIG_INI_PATH, 0, line)
+        yield (WPT_CONFIG_INI_PATH, "", "WPT manifest is dirty. Run `./mach update-manifest`.")
+
+
+def lint_wpt_test_files():
+    from tools.lint import lint
+
+    # Override the logging function so that we can collect errors from
+    # the lint script, which doesn't allow configuration of the output.
+    messages: List[str] = []
+    lint.logger.error = lambda message: messages.append(message)
+
+    # We do not lint all WPT-like tests because they do not all currently have
+    # lint.ignore files.
+    LINTED_SUITES = ["tests", os.path.join("mozilla", "tests")]
+
+    for suite in LINTED_SUITES:
+        print(f"\r ➤  Linting WPT suite ({suite})...")
+
+        messages = []  # Clear any old messages.
+
+        suite_directory = os.path.abspath(os.path.join(WPT_PATH, suite))
+        tests_changed = FileList(suite_directory, only_changed_files=True, progress=False)
+        tests_changed = [os.path.relpath(file, suite_directory) for file in tests_changed]
+
+        if lint.lint(suite_directory, tests_changed, output_format="normal"):
+            for message in messages:
+                (filename, message) = message.split(":", maxsplit=1)
+                yield (filename, "", message)
+
+
+def run_wpt_lints(only_changed_files: bool):
+    if not os.path.exists(WPT_CONFIG_INI_PATH):
+        yield (WPT_CONFIG_INI_PATH, 0, f"{WPT_CONFIG_INI_PATH} is required but was not found")
+        return
+
+    if not list(FileList("./tests/wpt", only_changed_files=True, progress=False)):
+        print("\r ➤  Skipping WPT lint checks, because no relevant files changed.")
+        return
+
+    manifests_exist_errors = list(check_that_manifests_exist())
+    if manifests_exist_errors:
+        yield from manifests_exist_errors
+        return
+
+    yield from check_that_manifests_are_clean()
+    yield from lint_wpt_test_files()
+
+
 def check_spec(file_name, lines):
     if SPEC_BASE_PATH not in file_name:
         return
@@ -820,7 +854,7 @@ def check_spec(file_name, lines):
                     break
 
 
-def check_config_file(config_file, print_text=True, no_wpt=False):
+def check_config_file(config_file, print_text=True):
     # Check if config file exists
     if not os.path.exists(config_file):
         print("%s config file is required but was not found" % config_file)
@@ -832,7 +866,7 @@ def check_config_file(config_file, print_text=True, no_wpt=False):
         lines = conf_file.splitlines(True)
 
     if print_text:
-        print('\rChecking the config file...')
+        print(f"\r ➤  Checking config file ({config_file})...")
 
     config_content = toml.loads(conf_file)
     exclude = config_content.get("ignore", {})
@@ -845,11 +879,6 @@ def check_config_file(config_file, print_text=True, no_wpt=False):
     # Check for invalid listed ignored files
     invalid_files = [f for f in exclude.get("files", []) if not os.path.exists(f)]
 
-    # Do not check for the existense of ignored files under tests/wpts if --no-wpt is used
-    if no_wpt:
-        wpt_dir = './tests/wpt/'
-        invalid_files = [f for f in invalid_files if not os.path.commonprefix([wpt_dir, f]) == wpt_dir]
-
     current_table = ""
     for idx, line in enumerate(lines):
         # Ignore comment lines
@@ -925,7 +954,7 @@ def parse_config(config_file):
 
 def check_directory_files(directories, print_text=True):
     if print_text:
-        print('\rChecking directories for correct file extensions...')
+        print("\r ➤  Checking directories for correct file extensions...")
     for directory, file_extensions in directories.items():
         files = sorted(os.listdir(directory))
         for filename in files:
@@ -945,7 +974,7 @@ def collect_errors_for_files(files_to_check, checking_functions, line_checking_f
     if not has_element:
         return
     if print_text:
-        print('\rChecking files for tidiness...')
+        print("\r ➤  Checking files for tidiness")
 
     for filename in files_to_check:
         if not os.path.exists(filename):
@@ -987,99 +1016,33 @@ def check_dep_license_errors(filenames, progress=True):
                 yield (filename, 0, "dependency should contain a valid license.")
 
 
-class LintRunner(object):
+def scan(only_changed_files=False, progress=False):
-    def __init__(self, lint_path=None, only_changed_files=True,
-                 exclude_dirs=[], progress=True, stylo=False, no_wpt=False):
-        self.only_changed_files = only_changed_files
-        self.exclude_dirs = exclude_dirs
-        self.progress = progress
-        self.path = lint_path
-        self.stylo = stylo
-        self.no_wpt = no_wpt
-
-    def check(self, lint_cls):
-        lint = lint_cls(self.path, self.only_changed_files,
-                        self.exclude_dirs, self.progress,
-                        stylo=self.stylo, no_wpt=self.no_wpt)
-        for error in lint.run():
-            if type(error) is not tuple or (type(error) is tuple and len(error) != 3):
-                yield (self.path, 1, "errors should be a tuple of (path, line, reason)")
-                return
-            yield error
-
-    def get_files(self, path, **kwargs):
-        args = ['only_changed_files', 'exclude_dirs', 'progress']
-        kwargs = {k: kwargs.get(k, getattr(self, k)) for k in args}
-        return FileList(path, **kwargs)
-
-    def run(self):
-        yield (self.path, 0, "class 'Lint' should implement 'run' method")
-
-
-def run_lint_scripts(only_changed_files=False, progress=True, stylo=False, no_wpt=False):
-    runner = LintRunner(only_changed_files=only_changed_files, progress=progress, stylo=stylo, no_wpt=no_wpt)
-    for error in runner.check(WPTLint):
-        yield error
-
-
-def scan(only_changed_files=False, progress=True, stylo=False, no_wpt=False):
     # check config file for errors
-    config_errors = check_config_file(CONFIG_FILE_PATH, no_wpt=no_wpt)
+    config_errors = check_config_file(CONFIG_FILE_PATH)
-    # check ini directories exist
-    if not no_wpt and os.path.isfile(WPT_MANIFEST_PATH):
-        manifest_errors = check_manifest_dirs(WPT_MANIFEST_PATH)
-    else:
-        manifest_errors = ()
     # check directories contain expected files
     directory_errors = check_directory_files(config['check_ext'])
     # standard checks
-    files_to_check = filter_files('.', only_changed_files and not stylo, progress)
+    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_webidl_spec, check_json)
     line_checking_functions = (check_license, check_by_line, check_toml, check_shell,
                                check_rust, check_spec, check_modeline)
+    lock_errors = check_cargo_lock_file(os.path.join(TOPDIR, "Cargo.lock"))
     file_errors = collect_errors_for_files(files_to_check, checking_functions, line_checking_functions)
     # check dependecy licenses
     dep_license_errors = check_dep_license_errors(get_dep_toml_files(only_changed_files), progress)
-    # other lint checks
+
-    lint_errors = run_lint_scripts(only_changed_files, progress, stylo=stylo, no_wpt=no_wpt)
+    wpt_errors = run_wpt_lints(only_changed_files)
+
     # chain all the iterators
-    errors = itertools.chain(config_errors, manifest_errors, directory_errors, lint_errors,
+    errors = itertools.chain(config_errors, directory_errors, lock_errors, file_errors,
-                             file_errors, dep_license_errors)
+                             dep_license_errors, wpt_errors)
 
-    error = None
     colorama.init()
+    error = None
     for error in errors:
-        print("\r\033[94m{}\033[0m:\033[93m{}\033[0m: \033[91m{}\033[0m".format(*error))
+        print("\r  | "
-
+              + f"{colorama.Fore.BLUE}{error[0]}{colorama.Style.RESET_ALL}:"
-    print()
+              + f"{colorama.Fore.YELLOW}{error[1]}{colorama.Style.RESET_ALL}: "
-    if error is None:
+              + f"{colorama.Fore.RED}{error[2]}{colorama.Style.RESET_ALL}")
-        print("\033[92mtidy reported no errors.\033[0m")
 
     return int(error is not None)
-
-
-class WPTLint(LintRunner):
-    def _get_wpt_files(self, suite):
-        working_dir = os.path.join(WPT_PATH, suite, '')
-        file_iter = self.get_files(working_dir, exclude_dirs=[])
-        print('\nRunning the WPT lint on %s...' % working_dir)
-        for f in file_iter:
-            if filter_file(f):
-                yield f[len(working_dir):]
-
-    def run(self):
-        if self.stylo or self.no_wpt:
-            return
-
-        wpt_working_dir = os.path.abspath(os.path.join(WPT_PATH, "tests"))
-        for suite in SUITES:
-            files = list(self._get_wpt_files(suite))
-            if not files:
-                continue
-            sys.path.insert(0, wpt_working_dir)
-            from tools.lint import lint
-            file_dir = os.path.abspath(os.path.join(WPT_PATH, suite))
-            returncode = lint.lint(file_dir, files, output_format="json")
-            sys.path.remove(wpt_working_dir)
-            if returncode:
-                yield ("WPT Lint Tool", "", "lint error(s) in Web Platform Tests: exit status %s" % returncode)

python/wpt/manifestupdate.py

@@ -31,8 +31,9 @@ def create_parser():
     return p
 
 
-def update(check_clean=True, rebuild=False, **kwargs):
+def update(check_clean=True, rebuild=False, logger=None, **kwargs):
-    logger = wptlogging.setup(kwargs, {"mach": sys.stdout})
+    if not logger:
+        logger = wptlogging.setup(kwargs, {"mach": sys.stdout})
     kwargs = {"config": os.path.join(WPT_PATH, "config.ini"),
               "product": "servo",
               "manifest_path": os.path.join(WPT_PATH, "meta"),