From 0491bbbb524b024d169152c3b6fd7969127da4fd Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Wed, 23 Oct 2019 12:51:33 +0200 Subject: [PATCH 1/2] bot: Do not clean absolute paths, fixes #45 --- bot/code_review_bot/__init__.py | 2 ++ bot/code_review_bot/stats.py | 3 --- bot/code_review_bot/tasks/base.py | 25 ----------------------- bot/code_review_bot/tasks/clang_format.py | 2 +- bot/code_review_bot/tasks/clang_tidy.py | 2 +- bot/code_review_bot/tasks/coverage.py | 2 +- bot/code_review_bot/tasks/coverity.py | 5 +---- bot/code_review_bot/tasks/lint.py | 2 +- 8 files changed, 7 insertions(+), 36 deletions(-) diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py index ecb3ccf90..5caafee7e 100644 --- a/bot/code_review_bot/__init__.py +++ b/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 diff --git a/bot/code_review_bot/stats.py b/bot/code_review_bot/stats.py index 42c805209..b5fdd1a91 100644 --- a/bot/code_review_bot/stats.py +++ b/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): """ diff --git a/bot/code_review_bot/tasks/base.py b/bot/code_review_bot/tasks/base.py index ed558dbcc..e9ff1a009 100644 --- a/bot/code_review_bot/tasks/base.py +++ b/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 diff --git a/bot/code_review_bot/tasks/clang_format.py b/bot/code_review_bot/tasks/clang_format.py index a7da0711d..386a19126 100644 --- a/bot/code_review_bot/tasks/clang_format.py +++ b/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"], diff --git a/bot/code_review_bot/tasks/clang_tidy.py b/bot/code_review_bot/tasks/clang_tidy.py index 3069529e0..5229cb96e 100755 --- a/bot/code_review_bot/tasks/clang_tidy.py +++ b/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"], diff --git a/bot/code_review_bot/tasks/coverage.py b/bot/code_review_bot/tasks/coverage.py index 4ef45f88d..96e44db15 100644 --- a/bot/code_review_bot/tasks/coverage.py +++ b/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 ] diff --git a/bot/code_review_bot/tasks/coverity.py b/bot/code_review_bot/tasks/coverity.py index 2fed402ea..fd5b0db31 100755 --- a/bot/code_review_bot/tasks/coverity.py +++ b/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() diff --git a/bot/code_review_bot/tasks/lint.py b/bot/code_review_bot/tasks/lint.py index d366465be..65d0144e7 100644 --- a/bot/code_review_bot/tasks/lint.py +++ b/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"], From 0911fd16eab1eb6aa83bd5b41cc3026d0fc6fe22 Mon Sep 17 00:00:00 2001 From: Bastien Abadie Date: Wed, 23 Oct 2019 12:52:12 +0200 Subject: [PATCH 2/2] Fix unit tests --- .../fixtures/mozlint_license_no_check.json | 4 +-- bot/tests/mocks/coverity_simple.json | 2 +- bot/tests/test_coverity.py | 28 ++++++--------- bot/tests/test_issues.py | 36 ------------------- bot/tests/test_remote.py | 8 +---- 5 files changed, 14 insertions(+), 64 deletions(-) diff --git a/bot/tests/fixtures/mozlint_license_no_check.json b/bot/tests/fixtures/mozlint_license_no_check.json index e04cb6789..4998e12c7 100644 --- a/bot/tests/fixtures/mozlint_license_no_check.json +++ b/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, diff --git a/bot/tests/mocks/coverity_simple.json b/bot/tests/mocks/coverity_simple.json index a0f481ae7..296a7466b 100644 --- a/bot/tests/mocks/coverity_simple.json +++ b/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"}]}}]}}} \ No newline at end of file +{"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"}]}}]}}} \ No newline at end of file diff --git a/bot/tests/test_coverity.py b/bot/tests/test_coverity.py index 37b132ce4..1f421be6c 100644 --- a/bot/tests/test_coverity.py +++ b/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,14 +81,7 @@ 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() @@ -97,13 +89,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".`. """ 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, diff --git a/bot/tests/test_issues.py b/bot/tests/test_issues.py index d7d291f48..f09f84fa2 100644 --- a/bot/tests/test_issues.py +++ b/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" - ) diff --git a/bot/tests/test_remote.py b/bot/tests/test_remote.py index 902a68ff1..721f0feb3 100644 --- a/bot/tests/test_remote.py +++ b/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"), ]