Do not clean issues absolute paths

bot/code_review_bot/__init__.py

@@ -7,6 +7,7 @@
 import enum
 import hashlib
 import json
+import os
 
 import requests
 import structlog
@@ -72,6 +73,7 @@ def __init__(
         assert isinstance(revision, Revision)
 
         # Base required fields for all issues
+        assert not os.path.isabs(path), f"Issue path can not be absolute {path}"
         self.revision = revision
         self.analyzer = analyzer
         self.check = check

bot/code_review_bot/stats.py

@@ -94,9 +94,6 @@ def report_task(self, task, issues):
         # Report total paths
         self.add_metric("issues.paths", len({i.path for i in issues}), tags)
 
-        # Report cleaned paths
-        self.add_metric("issues.cleaned_paths", len(task.cleaned_paths), tags)
-
     @contextmanager
     def timer(self, name):
         """

bot/code_review_bot/tasks/base.py

@@ -3,18 +3,10 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-import os
-
 import structlog
 
 logger = structlog.get_logger(__name__)
 
-WORKER_CHECKOUTS = (
-    "/builds/worker/checkouts/gecko",
-    "/home/worker/nss",
-    "/home/worker/nspr",
-)
-
 
 class AnalysisTask(object):
     """
@@ -32,7 +24,6 @@ def __init__(self, task_id, task_status):
         assert "status" in task_status, "No status data for {}".format(self.id)
         self.task = task_status["task"]
         self.status = task_status["status"]
-        self.cleaned_paths = set()
 
     @property
     def run_id(self):
@@ -114,22 +105,6 @@ def load_artifacts(self, queue_service):
 
         return out
 
-    def clean_path(self, path):
-        """
-        Helper to clean issues path from remote tasks
-        """
-        if not os.path.isabs(path):
-            # Never alter a relative path
-            return path
-
-        for checkout in WORKER_CHECKOUTS:
-            if path.startswith(checkout):
-                self.cleaned_paths.add(path)
-                logger.warning("Cleaned issue absolute path", path=path, name=self.name)
-                return os.path.relpath(path, checkout)
-
-        return path
-
     def build_patches(self, artifacts):
         """
         Some analyzers can provide a patch applicable by developers

bot/code_review_bot/tasks/clang_format.py

@@ -103,7 +103,7 @@ def parse_issues(self, artifacts, revision):
         return [
             ClangFormatIssue(
                 analyzer=self.name,
-                path=self.clean_path(path),
+                path=path,
                 line=issue["line"],
                 nb_lines=issue["lines_modified"],
                 column=issue["line_offset"],

bot/code_review_bot/tasks/clang_tidy.py

@@ -197,7 +197,7 @@ def parse_issues(self, artifacts, revision):
             ClangTidyIssue(
                 analyzer=self.name,
                 revision=revision,
-                path=self.clean_path(path),
+                path=path,
                 line=warning["line"],
                 column=warning["column"],
                 check=warning["flag"],

bot/code_review_bot/tasks/coverage.py

@@ -98,7 +98,7 @@ def parse_issues(self, artifacts, revision):
         }
 
         return [
-            CoverageIssue(self.clean_path(path), 0, "This file is uncovered", revision)
+            CoverageIssue(path, 0, "This file is uncovered", revision)
             for path in revision.files
             if path in zero_coverage_files
         ]

bot/code_review_bot/tasks/coverity.py

@@ -200,10 +200,7 @@ def parse_issues(self, artifacts, revision):
         assert isinstance(artifacts, dict)
         return [
             CoverityIssue(
-                analyzer=self.name,
-                revision=revision,
-                issue=warning,
-                file_path=self.clean_path(path),
+                analyzer=self.name, revision=revision, issue=warning, file_path=path
             )
             for artifact in artifacts.values()
             for path, items in artifact["files"].items()

bot/code_review_bot/tasks/lint.py

@@ -143,7 +143,7 @@ def parse_issues(self, artifacts, revision):
             MozLintIssue(
                 analyzer=self.name,
                 revision=revision,
-                path=issue.get("relpath", self.clean_path(issue["path"])),
+                path=issue.get("relpath", issue["path"]),
                 column=issue["column"],
                 level=issue["level"],
                 lineno=issue["lineno"],

bot/tests/fixtures/mozlint_license_no_check.json

@@ -1,12 +1,12 @@
 {
-    "/builds/worker/checkouts/gecko/intl/locale/rust/unic-langid-ffi/src/lib.rs": [
+    "intl/locale/rust/unic-langid-ffi/src/lib.rs": [
         {
             "rule": null,
             "linter": "license",
             "column": null,
             "lineoffset": null,
             "lineno": 0,
-            "path": "/builds/worker/checkouts/gecko/intl/locale/rust/unic-langid-ffi/src/lib.rs",
+            "path": "intl/locale/rust/unic-langid-ffi/src/lib.rs",
             "relpath": "intl/locale/rust/unic-langid-ffi/src/lib.rs",
             "level": "error",
             "diff": null,

bot/tests/mocks/coverity_simple.json

@@ -1 +1 @@
-{"files": {"/builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp": {"warnings": [{"line": 3703, "reliability": "medium", "message": "Dereferencing a pointer that might be \"nullptr\" \"env\" when calling \"lookupImport\".", "flag": "NULL_RETURNS", "extra": {"category": "Null pointer dereferences", "stateOnServer": {"ownerLdapServerName": "local", "stream": "Firefox", "cid": 95687, "cached": false, "retrievalDateTime": "2019-05-13T10:20:22+00:00", "firstDetectedDateTime": "2019-04-08T12:57:07+00:00", "presentInReferenceSnapshot": false, "components": ["js"], "customTriage": {}, "triage": {"fixTarget": "Untargeted", "severity": "Unspecified", "classification": "Unclassified", "owner": "try", "legacy": "False", "action": "Undecided", "externalReference": ""}}, "stack": [{"line_number": 3697, "description": "\"GetModuleEnvironmentForScript\" returns \"nullptr\" (checked 2 out of 2 times).", "file_path": "/builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp", "path_type": "returned_null"}, {"line_number": 3697, "description": "Assigning: \"env\" = \"nullptr\" return value from \"GetModuleEnvironmentForScript\".", "file_path": "/builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp", "path_type": "var_assigned"}, {"line_number": 3703, "description": "Dereferencing a pointer that might be \"nullptr\" \"env\" when calling \"lookupImport\".", "file_path": "/builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp", "path_type": "dereference"}]}}]}}}
+{"files": {"js/src/jit/BaselineCompiler.cpp": {"warnings": [{"line": 3703, "reliability": "medium", "message": "Dereferencing a pointer that might be \"nullptr\" \"env\" when calling \"lookupImport\".", "flag": "NULL_RETURNS", "extra": {"category": "Null pointer dereferences", "stateOnServer": {"ownerLdapServerName": "local", "stream": "Firefox", "cid": 95687, "cached": false, "retrievalDateTime": "2019-05-13T10:20:22+00:00", "firstDetectedDateTime": "2019-04-08T12:57:07+00:00", "presentInReferenceSnapshot": false, "components": ["js"], "customTriage": {}, "triage": {"fixTarget": "Untargeted", "severity": "Unspecified", "classification": "Unclassified", "owner": "try", "legacy": "False", "action": "Undecided", "externalReference": ""}}, "stack": [{"line_number": 3697, "description": "\"GetModuleEnvironmentForScript\" returns \"nullptr\" (checked 2 out of 2 times).", "file_path": "js/src/jit/BaselineCompiler.cpp", "path_type": "returned_null"}, {"line_number": 3697, "description": "Assigning: \"env\" = \"nullptr\" return value from \"GetModuleEnvironmentForScript\".", "file_path": "js/src/jit/BaselineCompiler.cpp", "path_type": "var_assigned"}, {"line_number": 3703, "description": "Dereferencing a pointer that might be \"nullptr\" \"env\" when calling \"lookupImport\".", "file_path": "js/src/jit/BaselineCompiler.cpp", "path_type": "dereference"}]}}]}}}

bot/tests/test_coverity.py

@@ -13,7 +13,6 @@ def __init__(self):
         """
         Simply skip task loading
         """
-        self.cleaned_paths = set()
         self.task = {"metadata": {"name": "mock-coverity"}}
 
 
@@ -50,13 +49,13 @@ def test_simple(mock_revision, mock_config, log, mock_hgmo):
         == """Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".
 The path that leads to this defect is:
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3697//:
+- //js/src/jit/BaselineCompiler.cpp:3697//:
 -- `returned_null: "GetModuleEnvironmentForScript" returns "nullptr" (checked 2 out of 2 times).`.
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3697//:
+- //js/src/jit/BaselineCompiler.cpp:3697//:
 -- `var_assigned: Assigning: "env" = "nullptr" return value from "GetModuleEnvironmentForScript".`.
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3703//:
+- //js/src/jit/BaselineCompiler.cpp:3703//:
 -- `dereference: Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".`.
 """
     )
@@ -82,28 +81,21 @@ def test_simple(mock_revision, mock_config, log, mock_hgmo):
     }
     assert issue.nb_lines == 1
 
-    # Check the issue's absolute path has been cleaned
-    assert log.has(
-        "Cleaned issue absolute path",
-        path="/builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp",
-        name="mock-coverity",
-        level="warning",
-    )
-
+    assert issue.path == "js/src/jit/BaselineCompiler.cpp"
     assert issue.validates()
     assert not issue.is_publishable()
 
     checker_desc = """Checker reliability is medium, meaning that the false positive ratio is medium.
 Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".
 The path that leads to this defect is:
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3697//:
+- //js/src/jit/BaselineCompiler.cpp:3697//:
 -- `returned_null: "GetModuleEnvironmentForScript" returns "nullptr" (checked 2 out of 2 times).`.
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3697//:
+- //js/src/jit/BaselineCompiler.cpp:3697//:
 -- `var_assigned: Assigning: "env" = "nullptr" return value from "GetModuleEnvironmentForScript".`.
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3703//:
+- //js/src/jit/BaselineCompiler.cpp:3703//:
 -- `dereference: Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".`.
 """
     assert issue.as_phabricator_lint() == {
@@ -126,13 +118,13 @@ def test_simple(mock_revision, mock_config, log, mock_hgmo):
         "message": """Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".
 The path that leads to this defect is:
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3697//:
+- //js/src/jit/BaselineCompiler.cpp:3697//:
 -- `returned_null: "GetModuleEnvironmentForScript" returns "nullptr" (checked 2 out of 2 times).`.
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3697//:
+- //js/src/jit/BaselineCompiler.cpp:3697//:
 -- `var_assigned: Assigning: "env" = "nullptr" return value from "GetModuleEnvironmentForScript".`.
 
-- ///builds/worker/checkouts/gecko/js/src/jit/BaselineCompiler.cpp:3703//:
+- //js/src/jit/BaselineCompiler.cpp:3703//:
 -- `dereference: Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".`.
 """,
         "nb_lines": 1,

bot/tests/test_issues.py

@@ -3,9 +3,6 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-import pytest
-
-from code_review_bot.tasks.base import AnalysisTask
 from code_review_bot.tasks.clang_format import ClangFormatIssue
 
 
@@ -32,36 +29,3 @@ def _allowed(path):
     }
     for path, result in checks.items():
         assert _allowed(path) is result
-
-
-@pytest.mark.parametrize(
-    "path, cleaned_path",
-    [
-        ("myfile.cpp", "myfile.cpp"),
-        # Unknown full path
-        ("/absolute/path/file.rs", "/absolute/path/file.rs"),
-        # Known full paths
-        ("/builds/worker/checkouts/gecko/js/xx.h", "js/xx.h"),
-        ("/home/worker/nss/something.py", "something.py"),
-        ("/home/worker/nspr/Test.c", "Test.c"),
-    ],
-)
-def test_cleaned_paths(log, path, cleaned_path):
-    """
-    Test cleaning a path using a known worker's checkout
-    """
-    assert len(log.events) == 0
-
-    task = AnalysisTask(
-        "testTask", {"task": {"metadata": {"name": "test-task"}}, "status": None}
-    )
-    assert task.clean_path(path) == cleaned_path
-
-    # Check a warning is sent when path is cleaned
-    if path == cleaned_path:
-        assert len(log.events) == 0
-    else:
-        assert len(log.events) == 1
-        assert log.has(
-            "Cleaned issue absolute path", path=path, name="test-task", level="warning"
-        )

bot/tests/test_remote.py

@@ -140,11 +140,9 @@ def test_baseline(mock_config, mock_revision, mock_workflow):
             ("code-review.issues", "source-test-mozlint-flake8", 1),
             ("code-review.issues.publishable", "source-test-mozlint-flake8", 0),
             ("code-review.issues.paths", "source-test-mozlint-flake8", 1),
-            ("code-review.issues.cleaned_paths", "source-test-mozlint-flake8", 0),
             ("code-review.issues", "source-test-mozlint-zero-cov", 1),
             ("code-review.issues.publishable", "source-test-mozlint-zero-cov", 1),
             ("code-review.issues.paths", "source-test-mozlint-zero-cov", 1),
-            ("code-review.issues.cleaned_paths", "source-test-mozlint-zero-cov", 0),
             ("code-review.analysis.issues.publishable", None, 1),
             ("code-review.runtime.reports", None, "runtime"),
         ]
@@ -361,7 +359,6 @@ def test_mozlint_task(mock_config, mock_revision, mock_workflow):
             ("code-review.issues", "source-test-mozlint-dummy", 1),
             ("code-review.issues.publishable", "source-test-mozlint-dummy", 0),
             ("code-review.issues.paths", "source-test-mozlint-dummy", 1),
-            ("code-review.issues.cleaned_paths", "source-test-mozlint-dummy", 0),
             ("code-review.analysis.issues.publishable", None, 0),
             ("code-review.runtime.reports", None, "runtime"),
         ]
@@ -445,7 +442,6 @@ def test_clang_tidy_task(mock_config, mock_revision, mock_workflow):
             ("code-review.issues", "source-test-clang-tidy", 2),
             ("code-review.issues.publishable", "source-test-clang-tidy", 0),
             ("code-review.issues.paths", "source-test-clang-tidy", 1),
-            ("code-review.issues.cleaned_paths", "source-test-clang-tidy", 0),
             ("code-review.analysis.issues.publishable", None, 0),
             ("code-review.runtime.reports", None, "runtime"),
         ]
@@ -529,7 +525,6 @@ def test_clang_format_task(mock_config, mock_revision, mock_workflow, mock_hgmo)
             ("code-review.issues", "source-test-clang-format", 1),
             ("code-review.issues.publishable", "source-test-clang-format", 0),
             ("code-review.issues.paths", "source-test-clang-format", 1),
-            ("code-review.issues.cleaned_paths", "source-test-clang-format", 0),
             ("code-review.analysis.issues.publishable", None, 0),
             ("code-review.runtime.reports", None, "runtime"),
         ]
@@ -595,7 +590,7 @@ def test_coverity_task(mock_config, mock_revision, mock_workflow):
                                     }
                                 ]
                             },
-                            "/builds/worker/checkouts/gecko/dom/something.cpp": {
+                            "dom/something.cpp": {
                                 "warnings": [
                                     {
                                         "line": 123,
@@ -678,7 +673,6 @@ def test_coverity_task(mock_config, mock_revision, mock_workflow):
             ("code-review.issues", "source-test-coverity-coverity", 2),
             ("code-review.issues.publishable", "source-test-coverity-coverity", 0),
             ("code-review.issues.paths", "source-test-coverity-coverity", 2),
-            ("code-review.issues.cleaned_paths", "source-test-coverity-coverity", 1),
             ("code-review.analysis.issues.publishable", None, 0),
             ("code-review.runtime.reports", None, "runtime"),
         ]