Use task names directly in all report comments

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,27 +80,43 @@ 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__
+            sorted(issues, key=lambda i: i.analyzer), lambda i: i.analyzer
         )
 
-        def stats(items):
+        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):
+                        assert (
+                            _help is None
+                        ), f"Duplicate help command found for {analyzer}"
+                        _help = help_value
+
+            # Strip source-test- to get cleaner names
+            if analyzer.startswith("source-test-"):
+                analyzer = analyzer[12:]
+
             return {
+                "analyzer": analyzer,
+                "help": _help,
                 "total": len(_items),
                 "publishable": sum([i.is_publishable() for i in _items]),
                 "publishable_paths": list(
                     {i.path for i in _items if i.is_publishable()}
                 ),
             }
 
-        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 +135,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

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)
             ]
         )
 

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)

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(

bot/tests/test_reporter_phabricator.py

@@ -32,6 +32,17 @@
 """  # noqa
 
 
+VALID_FLAKE8_MESSAGE = """
+Code analysis found 1 defect in the diff 42:
+ - 1 defect found by 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",