From e6e7d96c7083dff33cf07028090242acf20dc5d6 Mon Sep 17 00:00:00 2001 From: Stephen McGruer Date: Thu, 10 Sep 2020 15:53:14 +0000 Subject: [PATCH] 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 --- .../tests/tools/ci/tc/github_checks_output.py | 23 +++++-- .../tests/tools/ci/tc/tasks/test.yml | 2 +- testing/web-platform/tests/tools/lint/lint.py | 67 ++++++++++++++----- .../tests/tools/lint/tests/test_lint.py | 5 +- .../tools/wptrunner/wptrunner/stability.py | 2 +- .../wptrunner/wptrunner/wptcommandline.py | 4 +- 6 files changed, 76 insertions(+), 27 deletions(-) diff --git a/testing/web-platform/tests/tools/ci/tc/github_checks_output.py b/testing/web-platform/tests/tools/ci/tc/github_checks_output.py index 16d399002984..b34e33dd9e81 100644 --- 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,3 +1,10 @@ +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. @@ -8,22 +15,28 @@ class GitHubChecksOutputter(object): 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 diff --git a/testing/web-platform/tests/tools/ci/tc/tasks/test.yml b/testing/web-platform/tests/tools/ci/tc/tasks/test.yml index b1534c9d230d..4c6cc324bff2 100644 --- a/testing/web-platform/tests/tools/ci/tc/tasks/test.yml +++ b/testing/web-platform/tests/tools/ci/tc/tasks/test.yml @@ -386,7 +386,7 @@ tasks: - 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: diff --git a/testing/web-platform/tests/tools/lint/lint.py b/testing/web-platform/tests/tools/lint/lint.py index 7bec438b773d..8965c1a0a129 100644 --- a/testing/web-platform/tests/tools/lint/lint.py +++ b/testing/web-platform/tests/tools/lint/lint.py @@ -17,6 +17,7 @@ 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 @@ -30,6 +31,7 @@ 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 @@ -809,41 +811,61 @@ def check_file_contents(repo_root, path, f): 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(errors): - # type: (List[rules.Error]) -> None +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_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: @@ -910,6 +932,8 @@ def create_parser(): "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 @@ -935,11 +959,13 @@ def main(**kwargs_str): 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 @@ -964,11 +990,16 @@ def process_errors(errors): """ 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 @@ -1002,6 +1033,10 @@ def process_errors(errors): 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)) diff --git a/testing/web-platform/tests/tools/lint/tests/test_lint.py b/testing/web-platform/tests/tools/lint/tests/test_lint.py index 5857f02b6fa0..e9925180f20a 100644 --- a/testing/web-platform/tests/tools/lint/tests/test_lint.py +++ b/testing/web-platform/tests/tools/lint/tests/test_lint.py @@ -530,6 +530,7 @@ def test_main_with_args(): [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 @@ -542,7 +543,7 @@ def test_main_no_args(): 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 @@ -554,6 +555,6 @@ def test_main_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 diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py index d6befa802fda..88d1c232bfb5 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/stability.py @@ -347,7 +347,7 @@ def check_stability(logger, repeat_loop=10, repeat_restart=5, chaos_mode=True, m 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: diff --git a/testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py b/testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py index ea5868256161..532c795cd3c5 100644 --- a/testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py +++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/wptcommandline.py @@ -5,7 +5,7 @@ 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 @@ -350,7 +350,7 @@ def create_parser(product_choices=None): 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")