Bug 1651991 [wpt PR 24556] - [Taskcluster] Make lint create a GitHub Checks output file, a=testonly
authorStephen McGruer <smcgruer@chromium.org>
Thu, 10 Sep 2020 15:53:14 +0000
changeset 548308 a6e4b389013941a0c67a2ed4b32796bfa59bcb23
parent 548307 3b5ac5b97a79d8ceeafcb9de4b74c43c1b668fc6
child 548309 b133e2d673e8ec7719036395a017c0b91ba15937
push id37776
push userbtara@mozilla.com
push dateFri, 11 Sep 2020 15:10:42 +0000
treeherdermozilla-central@b133e2d673e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstestonly
bugs1651991, 24556, 15412
milestone82.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1651991 [wpt PR 24556] - [Taskcluster] Make lint create a GitHub Checks output file, a=testonly Automatic update from web-platform-tests [Taskcluster] Make lint create a GitHub Checks output file (#24556) See https://github.com/web-platform-tests/wpt/issues/15412 -- wpt-commits: 8420fdfa2c9124b1f7b1eaf64517c5b3fc3f072b wpt-pr: 24556
testing/web-platform/tests/tools/ci/tc/github_checks_output.py
testing/web-platform/tests/tools/ci/tc/tasks/test.yml
testing/web-platform/tests/tools/lint/lint.py
testing/web-platform/tests/tools/lint/tests/test_lint.py
testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py
testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py
--- a/testing/web-platform/tests/tools/ci/tc/github_checks_output.py
+++ b/testing/web-platform/tests/tools/ci/tc/github_checks_output.py
@@ -1,29 +1,42 @@
+MYPY = False
+if MYPY:
+    # MYPY is set to True when run under Mypy.
+    from typing import Any
+    from typing import Optional
+    from typing import Text
+
 class GitHubChecksOutputter(object):
     """Provides a method to output data to be shown in the GitHub Checks UI.
 
     This can be useful to provide a summary of a given check (e.g. the lint)
     to enable developers to quickly understand what has gone wrong. The output
     supports markdown format.
 
     See https://docs.taskcluster.net/docs/reference/integrations/github/checks#custom-text-output-in-checks
     """
     def __init__(self, path):
+        # type: (Text) -> None
         self.path = path
 
     def output(self, line):
+        # type: (Any) -> None
+        # TODO(stephenmcgruer): Once mypy 0.790 is released, we can change this
+        # to AnyStr, as that release teaches mypy about the mode flags of open.
+        # See https://github.com/python/typeshed/pull/4146
         with open(self.path, 'a') as f:
             f.write(line)
             f.write('\n')
 
 
 __outputter = None
-def get_gh_checks_outputter(kwargs):
+def get_gh_checks_outputter(filepath):
+    # type: (Optional[Text]) -> Optional[GitHubChecksOutputter]
     """Return the outputter for GitHub Checks output, if enabled.
 
-    :param kwargs: The arguments passed to the program (to look for the
-                   github_checks_text_file field)
+    :param filepath: The filepath to write GitHub Check output information to,
+                     or None if not enabled.
     """
     global __outputter
-    if kwargs['github_checks_text_file'] and __outputter is None:
-        __outputter = GitHubChecksOutputter(kwargs['github_checks_text_file'])
+    if filepath and __outputter is None:
+        __outputter = GitHubChecksOutputter(filepath)
     return __outputter
--- a/testing/web-platform/tests/tools/ci/tc/tasks/test.yml
+++ b/testing/web-platform/tests/tools/ci/tc/tasks/test.yml
@@ -381,17 +381,17 @@ tasks:
 
   - lint:
       use:
         - wpt-base
         - trigger-master
         - trigger-pr
       description: >-
         Lint for wpt-specific requirements
-      command: "./wpt lint --all"
+      command: "./wpt lint --all --github-checks-text-file=/home/test/artifacts/checkrun.md"
 
   - update-built:
       use:
         - wpt-base
         - trigger-pr
       schedule-if:
         run-job:
           - update_built
--- a/testing/web-platform/tests/tools/lint/lint.py
+++ b/testing/web-platform/tests/tools/lint/lint.py
@@ -12,29 +12,31 @@ import subprocess
 import sys
 import tempfile
 
 from collections import defaultdict
 
 from . import fnmatch
 from . import rules
 from .. import localpaths
+from ..ci.tc.github_checks_output import get_gh_checks_outputter, GitHubChecksOutputter
 from ..gitignore.gitignore import PathFilter
 from ..wpt import testfiles
 from ..manifest.vcs import walk
 
 from ..manifest.sourcefile import SourceFile, js_meta_re, python_meta_re, space_chars, get_any_variants
 from six import binary_type, ensure_binary, ensure_text, iteritems, itervalues, with_metaclass
 from six.moves import range
 from six.moves.urllib.parse import urlsplit, urljoin
 
 MYPY = False
 if MYPY:
     # MYPY is set to True when run under Mypy.
     from typing import Any
+    from typing import Callable
     from typing import Dict
     from typing import IO
     from typing import Iterable
     from typing import List
     from typing import Optional
     from typing import Sequence
     from typing import Set
     from typing import Text
@@ -804,49 +806,69 @@ def check_file_contents(repo_root, path,
 
     errors = []
     for file_fn in file_lints:
         errors.extend(file_fn(repo_root, path, f))
         f.seek(0)
     return errors
 
 
-def output_errors_text(errors):
-    # type: (List[rules.Error]) -> None
-    assert logger is not None
+def output_errors_text(log, errors):
+    # type: (Callable[[Any], None], List[rules.Error]) -> None
     for error_type, description, path, line_number in errors:
         pos_string = path
         if line_number:
             pos_string += ":%s" % line_number
-        logger.error("%s: %s (%s)" % (pos_string, description, error_type))
+        log("%s: %s (%s)" % (pos_string, description, error_type))
 
 
-def output_errors_markdown(errors):
-    # type: (List[rules.Error]) -> None
+def output_errors_markdown(log, errors):
+    # type: (Callable[[Any], None], List[rules.Error]) -> None
     if not errors:
         return
-    assert logger is not None
     heading = """Got lint errors:
 
 | Error Type | Position | Message |
 |------------|----------|---------|"""
     for line in heading.split("\n"):
-        logger.error(line)
+        log(line)
     for error_type, description, path, line_number in errors:
         pos_string = path
         if line_number:
             pos_string += ":%s" % line_number
-        logger.error("%s | %s | %s |" % (error_type, pos_string, description))
+        log("%s | %s | %s |" % (error_type, pos_string, description))
+
+
+def output_errors_json(log, errors):
+    # type: (Callable[[Any], None], List[rules.Error]) -> None
+    for error_type, error, path, line_number in errors:
+        # We use 'print' rather than the log function to ensure that the output
+        # is valid JSON (e.g. with no logger preamble).
+        print(json.dumps({"path": path, "lineno": line_number,
+                          "rule": error_type, "message": error}))
 
 
-def output_errors_json(errors):
-    # type: (List[rules.Error]) -> None
-    for error_type, error, path, line_number in errors:
-        print(json.dumps({"path": path, "lineno": line_number,
-                          "rule": error_type, "message": error}))
+def output_errors_github_checks(outputter, errors, first_reported):
+    # type: (GitHubChecksOutputter, List[rules.Error], bool) -> None
+    """Output errors to the GitHub Checks output markdown format.
+
+    :param outputter: the GitHub Checks outputter
+    :param errors: a list of error tuples (error type, message, path, line number)
+    :param first_reported: True if these are the first reported errors
+    """
+    if first_reported:
+        outputter.output(
+            "\nChanges in this PR contain lint errors, listed below. These "
+            "errors must either be fixed or added to the list of ignored "
+            "errors; see [the documentation]("
+            "https://web-platform-tests.org/writing-tests/lint-tool.html). "
+            "For help, please tag `@web-platform-tests/wpt-core-team` in a "
+            "comment.\n")
+        outputter.output("```")
+    output_errors_text(outputter.output, errors)
 
 
 def output_error_count(error_count):
     # type: (Dict[Text, int]) -> None
     if not error_count:
         return
 
     assert logger is not None
@@ -905,16 +927,18 @@ def create_parser():
                         help="The WPT directory. Use this "
                         "option if the lint script exists outside the repository")
     parser.add_argument("--ignore-glob", type=ensure_text, action="append",
                         help="Additional file glob to ignore (repeat to add more). "
                         "Globs are matched against paths relative to REPO_ROOT "
                         "using fnmatch, except that path separators are normalized.")
     parser.add_argument("--all", action="store_true", help="If no paths are passed, try to lint the whole "
                         "working directory, not just files that changed")
+    parser.add_argument("--github-checks-text-file", type=ensure_text,
+                        help="Path to GitHub checks output file for Taskcluster runs")
     return parser
 
 
 def main(**kwargs_str):
     # type: (**Any) -> int
     kwargs = {ensure_text(key): value for key, value in iteritems(kwargs_str)}
 
     assert logger is not None
@@ -930,21 +954,23 @@ def main(**kwargs_str):
 
     if output_format == "markdown":
         setup_logging(True)
 
     paths = lint_paths(kwargs, repo_root)
 
     ignore_glob = kwargs.get("ignore_glob", [])
 
-    return lint(repo_root, paths, output_format, ignore_glob)
+    github_checks_outputter = get_gh_checks_outputter(kwargs["github_checks_text_file"])
+
+    return lint(repo_root, paths, output_format, ignore_glob, github_checks_outputter)
 
 
-def lint(repo_root, paths, output_format, ignore_glob=None):
-    # type: (Text, List[Text], Text, Optional[List[Text]]) -> int
+def lint(repo_root, paths, output_format, ignore_glob=None, github_checks_outputter=None):
+    # type: (Text, List[Text], Text, Optional[List[Text]], Optional[GitHubChecksOutputter]) -> int
     error_count = defaultdict(int)  # type: Dict[Text, int]
     last = None
 
     with io.open(os.path.join(repo_root, "lint.ignore"), "r") as f:
         ignorelist, skipped_files = parse_ignorelist(f)
 
     if ignore_glob:
         skipped_files |= set(ignore_glob)
@@ -959,21 +985,26 @@ def lint(repo_root, paths, output_format
         Filters and prints the errors, and updates the ``error_count`` object.
 
         :param errors: a list of error tuples (error type, message, path, line number)
         :returns: ``None`` if there were no errors, or
                   a tuple of the error type and the path otherwise
         """
 
         errors = filter_ignorelist_errors(ignorelist, errors)
-
         if not errors:
             return None
 
-        output_errors(errors)
+        assert logger is not None
+        output_errors(logger.error, errors)
+
+        if github_checks_outputter:
+            first_output = len(error_count) == 0
+            output_errors_github_checks(github_checks_outputter, errors, first_output)
+
         for error_type, error, path, line in errors:
             error_count[error_type] += 1
 
         return (errors[-1][0], path)
 
     for path in paths[:]:
         abs_path = os.path.join(repo_root, path)
         if not os.path.exists(abs_path):
@@ -997,16 +1028,20 @@ def lint(repo_root, paths, output_format
 
     if output_format in ("normal", "markdown"):
         output_error_count(error_count)
         if error_count:
             assert last is not None
             assert logger is not None
             for line in (ERROR_MSG % (last[0], last[1], last[0], last[1])).split("\n"):
                 logger.info(line)
+
+    if error_count and github_checks_outputter:
+        github_checks_outputter.output("```")
+
     return sum(itervalues(error_count))
 
 
 path_lints = [check_file_type, check_path_length, check_worker_collision, check_ahem_copy,
               check_mojom_js, check_tentative_directories, check_gitignore_file]
 all_paths_lints = [check_css_globally_unique, check_unique_testharness_basenames]
 file_lints = [check_regexp_line, check_parsed, check_python_ast, check_script_metadata,
               check_ahem_system_font]
--- a/testing/web-platform/tests/tools/lint/tests/test_lint.py
+++ b/testing/web-platform/tests/tools/lint/tests/test_lint.py
@@ -525,35 +525,36 @@ def test_main_with_args():
         with mock.patch(lint_mod.__name__ + ".os.path.isfile") as mock_isfile:
             mock_isfile.return_value = True
             with _mock_lint('lint', return_value=True) as m:
                 lint_mod.main(**vars(create_parser().parse_args()))
                 m.assert_called_once_with(repo_root,
                                           [os.path.relpath(os.path.join(os.getcwd(), x), repo_root)
                                            for x in ['a', 'b', 'c']],
                                           "normal",
+                                          None,
                                           None)
     finally:
         sys.argv = orig_argv
 
 
 def test_main_no_args():
     orig_argv = sys.argv
     try:
         sys.argv = ['./lint']
         with _mock_lint('lint', return_value=True) as m:
             with _mock_lint('changed_files', return_value=['foo', 'bar']):
                 lint_mod.main(**vars(create_parser().parse_args()))
-                m.assert_called_once_with(repo_root, ['foo', 'bar'], "normal", None)
+                m.assert_called_once_with(repo_root, ['foo', 'bar'], "normal", None, None)
     finally:
         sys.argv = orig_argv
 
 
 def test_main_all():
     orig_argv = sys.argv
     try:
         sys.argv = ['./lint', '--all']
         with _mock_lint('lint', return_value=True) as m:
             with _mock_lint('all_filesystem_paths', return_value=['foo', 'bar']):
                 lint_mod.main(**vars(create_parser().parse_args()))
-                m.assert_called_once_with(repo_root, ['foo', 'bar'], "normal", None)
+                m.assert_called_once_with(repo_root, ['foo', 'bar'], "normal", None, None)
     finally:
         sys.argv = orig_argv
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py
@@ -342,17 +342,17 @@ def check_stability(logger, repeat_loop=
     if chaos_mode and kwargs["product"] == "firefox":
         kwargs_extras.append({"chaos_mode_flags": "0xfb"})
 
     steps = get_steps(logger, repeat_loop, repeat_restart, kwargs_extras)
 
     start_time = datetime.now()
     step_results = []
 
-    github_checks_outputter = get_gh_checks_outputter(kwargs)
+    github_checks_outputter = get_gh_checks_outputter(kwargs["github_checks_text_file"])
 
     for desc, step_func in steps:
         if max_time and datetime.now() - start_time > max_time:
             logger.info("::: Test verification is taking too long: Giving up!")
             logger.info("::: So far, all checks passed, but not all checks were run.")
             write_summary(logger, step_results, "TIMEOUT")
             return 2
 
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py
@@ -1,16 +1,16 @@
 from __future__ import absolute_import, print_function
 import argparse
 import os
 import sys
 from collections import OrderedDict
 from distutils.spawn import find_executable
 from datetime import timedelta
-from six import iterkeys, itervalues, iteritems
+from six import ensure_text, iterkeys, itervalues, iteritems
 
 from . import config
 from . import wpttest
 from .formatters import chromium, wptreport, wptscreenshot
 
 def abs_path(path):
     return os.path.abspath(os.path.expanduser(path))
 
@@ -345,17 +345,17 @@ scheme host and port.""")
                                   "aborting")
     sauce_group.add_argument("--sauce-connect-arg", action="append",
                              default=[], dest="sauce_connect_args",
                              help="Command-line argument to forward to the "
                                   "Sauce Connect binary (repeatable)")
 
     taskcluster_group = parser.add_argument_group("Taskcluster-specific")
     taskcluster_group.add_argument("--github-checks-text-file",
-                                   dest="github_checks_text_file",
+                                   type=ensure_text,
                                    help="Path to GitHub checks output file")
 
     webkit_group = parser.add_argument_group("WebKit-specific")
     webkit_group.add_argument("--webkit-port", dest="webkit_port",
                               help="WebKit port")
 
     parser.add_argument("test_list", nargs="*",
                         help="List of URLs for tests to run, or paths including tests to run. "