From 26dd63c89097bfaedcf28379ccc1cf53e840283b Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Fri, 6 Dec 2019 11:20:29 +0100 Subject: [PATCH 1/2] bot: Use task names directly in all report comments, fixes #233. --- bot/code_review_bot/report/base.py | 80 +++++++++++----------- bot/code_review_bot/report/mail.py | 6 +- bot/tests/conftest.py | 1 + bot/tests/test_reporter_mail.py | 13 ++-- bot/tests/test_reporter_phabricator.py | 92 ++++++++++++++++++++++++-- 5 files changed, 136 insertions(+), 56 deletions(-) diff --git a/bot/code_review_bot/report/base.py b/bot/code_review_bot/report/base.py index 9ccb76116..7efa55f88 100644 --- a/bot/code_review_bot/report/base.py +++ b/bot/code_review_bot/report/base.py @@ -4,34 +4,17 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. import itertools +import re import urllib.parse - -from code_review_bot.tasks.clang_format import ClangFormatIssue -from code_review_bot.tasks.clang_tidy import ClangTidyIssue -from code_review_bot.tasks.coverity import CoverityIssue -from code_review_bot.tasks.default import DefaultIssue -from code_review_bot.tasks.infer import InferIssue -from code_review_bot.tasks.lint import MozLintIssue - -COMMENT_PARTS = { - ClangTidyIssue: { - "defect": " - {nb} found by clang-tidy", - "analyzer": " - `./mach static-analysis check {files}` (C/C++)", - }, - InferIssue: { - "defect": " - {nb} found by infer", - "analyzer": " - `./mach static-analysis check-java path/to/file.java` (Java)", - }, - CoverityIssue: {"defect": " - {nb} found by Coverity"}, - ClangFormatIssue: { - "defect": " - {nb} found by clang-format", - "analyzer": " - `./mach clang-format -s -p {files}` (C/C++)", - }, - MozLintIssue: { - "defect": " - {nb} found by mozlint", - "analyzer": " - `./mach lint --warnings path/to/file` (JS/Python/etc)", - }, - DefaultIssue: {"defect": " - {nb} found by a generic analyzer"}, +from typing import Pattern + +HELP_COMMANDS = { + "source-test-clang-tidy": " - `./mach static-analysis check {files}` (C/C++)", + "source-test-infer-infer": " - `./mach static-analysis check-java path/to/file.java` (Java)", + "source-test-clang-format": " - `./mach clang-format -s -p {files}` (C/C++)", + re.compile( + "^source-test-mozlint-.*" + ): " - `./mach lint --warnings path/to/file` (JS/Python/etc)", } COMMENT_FAILURE = """ Code analysis found {defects_total} in the diff {diff_id}: @@ -97,17 +80,30 @@ def requires(self, configuration, *keys): def calc_stats(self, issues): """ Calc stats about issues: - * group issues by class name + * group issues by analyzer * count their total number * count their publishable number """ - groups = itertools.groupby( - sorted(issues, key=lambda x: str(x.__class__)), lambda x: x.__class__ - ) - def stats(items): + def by_analyzer(i): + return i.analyzer + + groups = itertools.groupby(sorted(issues, key=by_analyzer), by_analyzer) + + def stats(analyzer, items): _items = list(items) + + # Lookup the help message, supporting regexes + _help = HELP_COMMANDS.get(analyzer) + if _help is None: + for regex, help_value in HELP_COMMANDS.items(): + if isinstance(regex, Pattern) and regex.match(analyzer): + _help = help_value + break + return { + "analyzer": analyzer, + "help": _help, "total": len(_items), "publishable": sum([i.is_publishable() for i in _items]), "publishable_paths": list( @@ -115,9 +111,7 @@ def stats(items): ), } - from collections import OrderedDict - - return OrderedDict([(cls, stats(items)) for cls, items in groups]) + return [stats(analyzer, items) for analyzer, items in groups] def build_comment( self, revision, issues, bug_report_url, patches=[], task_failures=[] @@ -136,17 +130,17 @@ def pluralize(word, nb): # Build parts depending on issues defects, analyzers = [], [] - for cls, cls_stats in stats.items(): - part = COMMENT_PARTS.get(cls) - assert part is not None, "Unsupported issue class {}".format(cls) + for stat in stats: defects.append( - part["defect"].format(nb=pluralize("defect", cls_stats["publishable"])) + " - {nb} found by {analyzer}".format( + analyzer=stat["analyzer"], + nb=pluralize("defect", stat["publishable"]), + ) ) - if "analyzer" in part: + _help = stat.get("help") + if _help is not None: analyzers.append( - part["analyzer"].format( - files=" ".join(cls_stats["publishable_paths"]) - ) + _help.format(files=" ".join(stat["publishable_paths"])) ) # Build top comment diff --git a/bot/code_review_bot/report/mail.py b/bot/code_review_bot/report/mail.py index b585783e9..730ca6bde 100644 --- a/bot/code_review_bot/report/mail.py +++ b/bot/code_review_bot/report/mail.py @@ -12,7 +12,7 @@ logger = structlog.get_logger(__name__) -EMAIL_STATS_LINE = "* **{source}**: {publishable} publishable ({total} total)" +EMAIL_STATS_LINE = "* **{analyzer}**: {publishable} publishable ({total} total)" EMAIL_HEADER = """ # Found {publishable} publishable issues ({total} total) @@ -49,11 +49,11 @@ def publish(self, issues, revision, task_failures): stats = "\n".join( [ EMAIL_STATS_LINE.format( - source=str(cls.__name__), + analyzer=stat["analyzer"], total=stat["total"], publishable=stat["publishable"], ) - for cls, stat in self.calc_stats(issues).items() + for stat in self.calc_stats(issues) ] ) diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index c54841791..298756ace 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -52,6 +52,7 @@ class MockIssue(object): def __init__(self, nb): self.nb = nb self.path = "/path/to/file" + self.analyzer = "mock-analyzer" def as_markdown(self): return "This is the mock issue n°{}".format(self.nb) diff --git a/bot/tests/test_reporter_mail.py b/bot/tests/test_reporter_mail.py index 0b361bda0..6ec12cd10 100644 --- a/bot/tests/test_reporter_mail.py +++ b/bot/tests/test_reporter_mail.py @@ -110,10 +110,15 @@ def _check_email(request): r.publish(mock_issues, mock_revision, []) # Check stats - mock_cls = mock_issues[0].__class__ - assert r.calc_stats(mock_issues) == { - mock_cls: {"total": 5, "publishable": 3, "publishable_paths": ["/path/to/file"]} - } + assert r.calc_stats(mock_issues) == [ + { + "analyzer": "mock-analyzer", + "help": None, + "total": 5, + "publishable": 3, + "publishable_paths": ["/path/to/file"], + } + ] def test_mail_builderrors( diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index fc238165a..19ecb3efe 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -11,7 +11,7 @@ VALID_CLANG_TIDY_MESSAGE = """ Code analysis found 1 defect in the diff 42: - - 1 defect found by clang-tidy + - 1 defect found by source-test-clang-tidy You can run this analysis locally with: - `./mach static-analysis check another_test.cpp` (C/C++) @@ -21,7 +21,7 @@ VALID_CLANG_FORMAT_MESSAGE = """ Code analysis found 1 defect in the diff 42: - - 1 defect found by clang-format + - 1 defect found by source-test-clang-format You can run this analysis locally with: - `./mach clang-format -s -p dom/test.cpp` (C/C++) @@ -32,6 +32,17 @@ """ # noqa +VALID_FLAKE8_MESSAGE = """ +Code analysis found 1 defect in the diff 42: + - 1 defect found by source-test-mozlint-py-flake8 + +You can run this analysis locally with: + - `./mach lint --warnings path/to/file` (JS/Python/etc) + +If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox+Build+System&component=Source+Code+Analysis&short_desc=[Automated+review]+UPDATE&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__). +""" # noqa + + VALID_COVERAGE_MESSAGE = """ In our previous code coverage analysis run, we found some files which had no coverage and are being modified in this patch: - [path/to/test.cpp](https://coverage.moz.tools/#revision=latest&path=path%2Fto%2Ftest.cpp&view=file) @@ -46,7 +57,7 @@ VALID_DEFAULT_MESSAGE = """ Code analysis found 1 defect in the diff 42: - - 1 defect found by a generic analyzer + - 1 defect found by full-file-analyzer If you see a problem in this automated review, [please report it here](https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox+Build+System&component=Source+Code+Analysis&short_desc=[Automated+review]+UPDATE&comment=**Phabricator+URL:**+https://phabricator.services.mozilla.com/...&format=__default__). """ # noqa @@ -106,7 +117,7 @@ def _check_comment(request): ) issue = ClangTidyIssue( - "mock-clang-tidy", + "source-test-clang-tidy", revision, "another_test.cpp", "42", @@ -169,7 +180,9 @@ def _check_comment(request): } reporter = PhabricatorReporter({"analyzers": ["clang-format"]}, api=api) - issue = ClangFormatIssue("mock-clang-format", "dom/test.cpp", 42, 1, revision) + issue = ClangFormatIssue( + "source-test-clang-format", "dom/test.cpp", 42, 1, revision + ) assert issue.is_publishable() revision.improvement_patches = [ @@ -188,6 +201,73 @@ def _check_comment(request): assert call.response.headers.get("unittest") == "clang-format" +def test_phabricator_mozlint(mock_config, mock_phabricator, mock_try_task): + """ + Test Phabricator reporter publication on a mock mozlint issue + """ + from code_review_bot.report.phabricator import PhabricatorReporter + from code_review_bot.revisions import Revision + from code_review_bot.tasks.lint import MozLintIssue + + def _check_comment(request): + # Check the Phabricator main comment is well formed + payload = urllib.parse.parse_qs(request.body) + assert payload["output"] == ["json"] + assert len(payload["params"]) == 1 + details = json.loads(payload["params"][0]) + assert details["message"] == VALID_FLAKE8_MESSAGE.format( + results=mock_config.taskcluster.results_dir + ) + + # Outputs dummy empty response + resp = {"error_code": None, "result": None} + return ( + 201, + {"Content-Type": "application/json", "unittest": "flake8"}, + json.dumps(resp), + ) + + responses.add_callback( + responses.POST, + "http://phabricator.test/api/differential.createcomment", + callback=_check_comment, + ) + + with mock_phabricator as api: + revision = Revision.from_try(mock_try_task, api) + revision.lines = { + # Add dummy lines diff + "python/test.py": [41, 42, 43], + "dom/test.cpp": [42], + } + revision.files = revision.lines.keys() + reporter = PhabricatorReporter({"analyzers": ["clang-format"]}, api=api) + + issue = MozLintIssue( + analyzer="source-test-mozlint-py-flake8", + path="python/test.py", + lineno=42, + column=1, + message="A bad bad error", + level="error", + revision=revision, + linter="flake8", + check="EXXX", + ) + assert issue.is_publishable() + + issues, patches = reporter.publish([issue], revision, []) + assert len(issues) == 1 + assert len(patches) == 0 + + # Check the callback has been used + assert len(responses.calls) > 0 + call = responses.calls[-1] + print(list(responses.calls)) + assert call.request.url == "http://phabricator.test/api/differential.createcomment" + assert call.response.headers.get("unittest") == "flake8" + + def test_phabricator_coverage(mock_config, mock_phabricator, mock_try_task): """ Test Phabricator reporter publication on a mock coverage issue @@ -316,7 +396,7 @@ def _check_comment_ccov(request): ) issue_clang_tidy = ClangTidyIssue( - "mock-clang-format", + "source-test-clang-tidy", revision, "another_test.cpp", "42", From e9f85a066f889788a300dab78f4adb2901741e8b Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Mon, 9 Dec 2019 10:15:36 +0100 Subject: [PATCH 2/2] Fix nits --- bot/code_review_bot/report/base.py | 15 ++++++++++----- bot/tests/test_reporter_phabricator.py | 6 +++--- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/bot/code_review_bot/report/base.py b/bot/code_review_bot/report/base.py index 7efa55f88..37ab271ac 100644 --- a/bot/code_review_bot/report/base.py +++ b/bot/code_review_bot/report/base.py @@ -85,10 +85,9 @@ def calc_stats(self, issues): * count their publishable number """ - def by_analyzer(i): - return i.analyzer - - groups = itertools.groupby(sorted(issues, key=by_analyzer), by_analyzer) + groups = itertools.groupby( + sorted(issues, key=lambda i: i.analyzer), lambda i: i.analyzer + ) def stats(analyzer, items): _items = list(items) @@ -98,8 +97,14 @@ def stats(analyzer, items): if _help is None: for regex, help_value in HELP_COMMANDS.items(): if isinstance(regex, Pattern) and regex.match(analyzer): + assert ( + _help is None + ), f"Duplicate help command found for {analyzer}" _help = help_value - break + + # Strip source-test- to get cleaner names + if analyzer.startswith("source-test-"): + analyzer = analyzer[12:] return { "analyzer": analyzer, diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 19ecb3efe..b37a8482e 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -11,7 +11,7 @@ VALID_CLANG_TIDY_MESSAGE = """ Code analysis found 1 defect in the diff 42: - - 1 defect found by source-test-clang-tidy + - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check another_test.cpp` (C/C++) @@ -21,7 +21,7 @@ VALID_CLANG_FORMAT_MESSAGE = """ Code analysis found 1 defect in the diff 42: - - 1 defect found by source-test-clang-format + - 1 defect found by clang-format You can run this analysis locally with: - `./mach clang-format -s -p dom/test.cpp` (C/C++) @@ -34,7 +34,7 @@ VALID_FLAKE8_MESSAGE = """ Code analysis found 1 defect in the diff 42: - - 1 defect found by source-test-mozlint-py-flake8 + - 1 defect found by mozlint-py-flake8 You can run this analysis locally with: - `./mach lint --warnings path/to/file` (JS/Python/etc)