diff --git a/bot/code_review_bot/report/phabricator.py b/bot/code_review_bot/report/phabricator.py index 5358589e9..7779c3336 100644 --- a/bot/code_review_bot/report/phabricator.py +++ b/bot/code_review_bot/report/phabricator.py @@ -39,7 +39,9 @@ def __init__(self, configuration={}, *args, **kwargs): self.mode = configuration.get("mode", MODE_COMMENT) self.publish_build_errors = configuration.get("publish_build_errors", False) assert self.mode in (MODE_COMMENT, MODE_HARBORMASTER), "Invalid mode" - logger.info("Will publish using", mode=self.mode) + logger.info( + "Will publish using", mode=self.mode, build_errors=self.publish_build_errors + ) def setup_api(self, api): assert isinstance(api, PhabricatorAPI) @@ -71,17 +73,20 @@ def publish(self, issues, revision): for patch in revision.improvement_patches if patch.analyzer in analyzers_available ] + # List of issues without possible build errors + issues_only = [issue for issue in issues if not issue.is_build_error()] + build_errors = [issue for issue in issues if issue.is_build_error()] - if issues: + if issues_only or build_errors: if self.mode == MODE_COMMENT: - self.publish_comment(revision, issues, patches) + self.publish_comment(revision, issues_only, patches) elif self.mode == MODE_HARBORMASTER: - self.publish_harbormaster(revision, issues) + self.publish_harbormaster(revision, issues_only) else: raise Exception("Unsupported mode {}".format(self.mode)) # Also publish build issues if self.publish_build_errors: - self.publish_harbormaster_build_errors(revision, issues) + self.publish_harbormaster_build_errors(revision, build_errors) else: logger.info("No issues to publish on phabricator") @@ -91,17 +96,13 @@ def publish_harbormaster_build_errors(self, revision, issues): """ Publish build errors through Phabricator UnitResult """ - build_errors = [ - issue.as_phabricator_unitresult() - for issue in issues - if issue.is_build_error() - ] - - if not build_errors: + if not issues: logger.info("No build errors encountered") return self.api.update_build_target( - revision.build_target_phid, BuildState.Fail, unit=build_errors + revision.build_target_phid, + BuildState.Fail, + unit=[issue.as_phabricator_unitresult() for issue in issues], ) def publish_comment(self, revision, issues, patches): @@ -124,7 +125,6 @@ def publish_comment(self, revision, issues, patches): self.comment_inline(revision, issue, existing_comments) for issue in issues if issue.ANALYZER not in ANALYZERS_WITHOUT_INLINES - and not issue.is_build_error() ], ) ) diff --git a/bot/code_review_bot/tasks/coverity.py b/bot/code_review_bot/tasks/coverity.py index 5158e5955..46bb632dc 100755 --- a/bot/code_review_bot/tasks/coverity.py +++ b/bot/code_review_bot/tasks/coverity.py @@ -111,7 +111,7 @@ def validates(self): """ Publish only local Coverity issues """ - return self.is_local() and not self.is_clang_error() + return self.is_local() def as_text(self): """ @@ -188,11 +188,13 @@ def as_phabricator_unitresult(self): if not self.build_error: raise Exception("Current issue is not a build error: {}".format(self)) + message = f"Code review bot found a **build error**: \n{self.message}" + return UnitResult( namespace="code-review", name="general", result=UnitResultState.Fail, - details=self.message, + details=message, format="remarkup", ) diff --git a/bot/tests/test_remote.py b/bot/tests/test_remote.py index 7db6ea554..efc444d18 100644 --- a/bot/tests/test_remote.py +++ b/bot/tests/test_remote.py @@ -594,7 +594,7 @@ def test_coverity_task(mock_config, mock_revision, mock_workflow, mock_stats): namespace="code-review", name="general", result=UnitResultState.Fail, - details=issue.message, + details=f"Code review bot found a **build error**: \n{issue.message}", format="remarkup", ) diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 5b43ac6d2..4faac95e7 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -559,17 +559,7 @@ def _check_message(request): details = json.loads(payload["params"][0]) assert details == { "buildTargetPHID": "PHID-HMBD-deadbeef12456", - "lint": [ - { - "code": "coverity.NULL_RETURNS", - "name": "Checker reliability is medium, meaning that the false " - "positive ratio is medium.\n" - 'Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".', - "line": 41, - "path": "test.cpp", - "severity": "error", - } - ], + "lint": [], "unit": [], "type": "work", "__conduit__": {"token": "deadbeef"}, @@ -594,7 +584,7 @@ def _check_unitresult(request): "lint": [], "unit": [ { - "details": 'Dereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".', + "details": 'Code review bot found a **build error**: \nDereferencing a pointer that might be "nullptr" "env" when calling "lookupImport".', "format": "remarkup", "name": "general", "namespace": "code-review",