diff --git a/bot/code_review_bot/__init__.py b/bot/code_review_bot/__init__.py index e79badc37..92b4d8a7b 100644 --- a/bot/code_review_bot/__init__.py +++ b/bot/code_review_bot/__init__.py @@ -14,7 +14,7 @@ # Workaround https://github.com/taskcluster/taskcluster/issues/9172 import taskcluster.download -from libmozdata.phabricator import LintResult, UnitResult, UnitResultState +from libmozdata.phabricator import LintResult from taskcluster.helper import TaskclusterConfig from code_review_bot.config import settings @@ -66,11 +66,134 @@ class Level(enum.Enum): Warning = "warning" -class Issue(abc.ABC): +class IssueType(enum.Enum): + Lint = 1 + + +class BaseIssue(abc.ABC): + type_: IssueType + + def __init__( + self, analyzer, revision, level: Level = Level.Warning, message: str = "" + ): + # Check while avoiding circular dependencies + from code_review_bot.revisions import Revision + + assert isinstance(revision, Revision) + assert isinstance(analyzer, AnalysisTask) + + self.analyzer = analyzer + self.revision = revision + self.level = level + self.message = message + # Mark the issue as known by default, so only errors are reported + # The before/after feature may tag some issues as new, so they are reported + self.new_issue = False + # Reserved payload for backend + self.on_backend = None + + @property + def display_name(self): + """ + Issue's base name (by default analyzer's name) + But can be overridden by subclasses + """ + return self.analyzer.display_name + + def build_extra_identifiers(self): + """ + Used to add information when building an issue unique hash + """ + return {} + + @property + def allow_before_and_after_publish(self): + """ + Allow the possibility for an issue to avoid being published based on before/after. + This allow publishing issues based on other criteria, like in_patch. + """ + if taskcluster.secrets.get( + f"{self.analyzer.name.upper()}_DISABLE_PUBLICATION_BEFORE_AFTER", False + ): + return False + + return self.revision.before_after_feature + + @cached_property + def hash(self): + raise NotImplementedError + + @abc.abstractmethod + def is_publishable(self): + raise NotImplementedError + + @abc.abstractmethod + def validates(self): + """ + Is this issue publishable on reporters using IN_PATCH publication ? + Should check specific rules and return a boolean + """ + raise NotImplementedError + + def as_dict(self): + """ + Build the serializable dict representation of the issue + Used by debugging tools + """ + issue_hash = None + try: + issue_hash = self.hash + except Exception as e: + logger.warn("Failed to build issue hash", error=str(e), issue=str(self)) + + return { + "analyzer": self.analyzer.name, + "level": self.level.value, + "message": self.message, + "validates": self.validates(), + "publishable": self.is_publishable(), + "hash": issue_hash, + } + + @abc.abstractmethod + def as_text(self): + """ + Build the text content for reporters + """ + raise NotImplementedError + + @abc.abstractmethod + def as_markdown(self): + """ + Build the Markdown content for debug email + """ + raise NotImplementedError + + def as_error(self): + """ + Build the Markdown content for for build error issues + """ + raise NotImplementedError + + @abc.abstractmethod + def is_build_error(self) -> bool: + """ + Is this issue a build error? + Default is False + """ + raise NotImplementedError + + @abc.abstractmethod + def as_phabricator_issue(self): + raise NotImplementedError + + +class Issue(BaseIssue): """ Common reported issue interface """ + type_: IssueType = IssueType.Lint revision = None def __init__( @@ -82,21 +205,15 @@ def __init__( nb_lines: int, check: str, column: int = None, - message: str = None, + message: str = "", level: Level = Level.Warning, fix: str = None, language: str = None, ): - # Check while avoiding circular dependencies - from code_review_bot.revisions import Revision - - assert isinstance(revision, Revision) - assert isinstance(analyzer, AnalysisTask) + super().__init__(analyzer, revision, level, message) # 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 self.path = path self.line = positive_int("line", line) @@ -109,11 +226,6 @@ def __init__( # Optional common fields self.column = column - self.message = message - self.level = level - - # Reserved payload for backend - self.on_backend = None # Store information when a fix is available self.fix = fix @@ -121,40 +233,17 @@ def __init__( if self.fix is not None: assert self.language is not None, "Missing fix language" - # Mark the issue as known by default, so only errors are reported - # The before/after feature may tag some issues as new, so they are reported - self.new_issue = False - def __str__(self): line = f"line {self.line}" if self.line is not None else "full file" return f"{self.analyzer.name} issue {self.check}@{self.level.value} {self.path} {line}" @property - def display_name(self): - """ - Issue's base name (by default analyzer's name) - But can be overridden by subclasses - """ - return self.analyzer.display_name - - def build_extra_identifiers(self): - """ - Used to add information when building an issue unique hash - """ - return {} + def in_patch(self): + return self.revision.contains(self) @property - def allow_before_and_after_publish(self): - """ - Allow the possibility for an issue to avoid being published based on before/after. - This allow publishing issues based on other criteria, like in_patch. - """ - if taskcluster.secrets.get( - f"{self.analyzer.name.upper()}_DISABLE_PUBLICATION_BEFORE_AFTER", False - ): - return False - - return self.revision.before_after_feature + def in_touched_files(self): + return self.revision.in_touched_files(self) def is_publishable(self): """ @@ -181,14 +270,6 @@ def is_publishable(self): # Fallback to in_patch detection return self.in_patch - @property - def in_patch(self): - return self.revision.contains(self) - - @property - def in_touched_files(self): - return self.revision.in_touched_files(self) - @cached_property def hash(self): """ @@ -282,62 +363,23 @@ def file_exists(self): raise e return False - @abc.abstractmethod - def validates(self): - """ - Is this issue publishable on reporters using IN_PATCH publication ? - Should check specific rules and return a boolean - """ - raise NotImplementedError - - @abc.abstractmethod - def as_text(self): - """ - Build the text content for reporters - """ - raise NotImplementedError - - @abc.abstractmethod - def as_markdown(self): - """ - Build the Markdown content for debug email - """ - raise NotImplementedError - - def as_error(self): - """ - Build the Markdown content for for build error issues - """ - raise NotImplementedError - def as_dict(self): - """ - Build the serializable dict representation of the issue - Used by debugging tools - """ - issue_hash = None - try: - issue_hash = self.hash - except Exception as e: - logger.warn("Failed to build issue hash", error=str(e), issue=str(self)) + dict_repr = super().as_dict() + dict_repr.update( + { + "path": self.path, + "line": self.line, + "nb_lines": self.nb_lines, + "column": self.column, + "check": self.check, + "in_patch": self.in_patch, + "fix": self.fix, + } + ) - return { - "analyzer": self.analyzer.name, - "path": self.path, - "line": self.line, - "nb_lines": self.nb_lines, - "column": self.column, - "check": self.check, - "level": self.level.value, - "message": self.message, - "in_patch": self.in_patch, - "validates": self.validates(), - "publishable": self.is_publishable(), - "hash": issue_hash, - "fix": self.fix, - } + return dict_repr - def as_phabricator_lint(self): + def as_phabricator_issue(self): """ Build the Phabricator LintResult instance """ @@ -367,27 +409,7 @@ def as_phabricator_lint(self): char=self.column, ) - def as_phabricator_unitresult(self): - """ - Build a Phabricator UnitResult for build errors - """ - assert ( - self.is_build_error() - ), "Only build errors may be published as unit results" - - return UnitResult( - namespace="code-review", - name="general", - result=UnitResultState.Fail, - details=f"Code review bot found a **build error**: \n{self.message}", - format="remarkup", - ) - def is_build_error(self): - """ - Is this issue a build error? - Default is False - """ return False diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index f859b2d3a..da0ce3945 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -9,12 +9,10 @@ logger = structlog.get_logger(__name__) -LANDO_WARNING_MESSAGE = "Static analysis and linting are still in progress." -LANDO_FAILURE_MESSAGE = ( - "Static analysis and linting did not run due to a generic failure." -) +LANDO_WARNING_MESSAGE = "{test_mode_string} are still in progress." +LANDO_FAILURE_MESSAGE = "{test_mode_string} did not run due to a generic failure." LANDO_FAILURE_HG_MESSAGE = ( - "Static analysis and linting did not run due to failure in applying the patch." + "{test_mode_string} did not run due to failure in applying the patch." ) @@ -22,6 +20,13 @@ class AnalysisMode(enum.Enum): Lint = 1 +def get_test_mode_string(analysis_mode: AnalysisMode): + if analysis_mode == AnalysisMode.Lint: + return "Static analysis and linting" + else: + raise NotImplementedError + + class PhabricatorRevisionBuild(PhabricatorBuild): """ Convert the bot revision into a libmozevent compatible build @@ -175,7 +180,7 @@ def publish_analysis_phabricator(payload, phabricator_api): logger.warning("Unsupported publication", mode=mode, build=build) -def publish_analysis_lando(payload, lando_warnings): +def publish_analysis_lando(payload, lando_warnings, analysis_mode: AnalysisMode): """ Publish result of patch application and push to try on Lando """ @@ -183,6 +188,8 @@ def publish_analysis_lando(payload, lando_warnings): assert isinstance(build, PhabricatorRevisionBuild), "Not a PhabricatorRevisionBuild" logger.debug("Publishing a Lando build update", mode=mode, build=str(build)) + test_mode_string = get_test_mode_string(analysis_mode) + if mode == "fail:general": # Send general failure message to Lando logger.info( @@ -192,7 +199,9 @@ def publish_analysis_lando(payload, lando_warnings): ) try: lando_warnings.add_warning( - LANDO_FAILURE_MESSAGE, build.revision["id"], build.diff_id + LANDO_FAILURE_MESSAGE.format(test_mode_string=test_mode_string), + build.revision["id"], + build.diff_id, ) except Exception as ex: logger.error(str(ex), exc_info=True) @@ -206,7 +215,9 @@ def publish_analysis_lando(payload, lando_warnings): ) try: lando_warnings.add_warning( - LANDO_FAILURE_HG_MESSAGE, build.revision["id"], build.diff_id + LANDO_FAILURE_HG_MESSAGE.format(test_mode_string=test_mode_string), + build.revision["id"], + build.diff_id, ) except Exception as ex: logger.error(str(ex), exc_info=True) @@ -219,7 +230,9 @@ def publish_analysis_lando(payload, lando_warnings): ) try: lando_warnings.add_warning( - LANDO_WARNING_MESSAGE, build.revision["id"], build.diff_id + LANDO_WARNING_MESSAGE.format(test_mode_string=test_mode_string), + build.revision["id"], + build.diff_id, ) except Exception as ex: logger.error(str(ex), exc_info=True) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 88cefbffc..4b89393a3 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -25,7 +25,7 @@ stats, taskcluster, ) -from code_review_bot.analysis import AnalysisMode +from code_review_bot.analysis import AnalysisMode, get_test_mode_string from code_review_bot.config import settings from code_review_bot.report import get_reporters from code_review_bot.revisions import PhabricatorRevision, Revision @@ -36,9 +36,7 @@ logger = structlog.get_logger(__name__) -LANDO_FAILURE_MESSAGE = ( - "Static analysis and linting did not run due to a generic failure." -) +LANDO_FAILURE_MESSAGE = "{test_mode_string} did not run due to a generic failure." def parse_cli(): @@ -186,6 +184,7 @@ def main(): ) revision = None + analysis_mode = None # Load unique revision try: if settings.generic_group_id: @@ -217,7 +216,6 @@ def main(): phabricator_api, ) - analysis_mode = None if parameters["target_tasks_method"] == "codereview": analysis_mode = AnalysisMode.Lint @@ -287,10 +285,22 @@ def main(): ) elif lando_publish_generic_failure: try: - lando_api.del_all_warnings(revision.id, revision.diff["id"]) - lando_api.add_warning( - LANDO_FAILURE_MESSAGE, revision.id, revision.diff["id"] - ) + if analysis_mode: + test_mode_string = get_test_mode_string(analysis_mode) + warnings = lando_api.get_warnings(revision.id, revision.diff["id"]) + to_delete = [ + w + for w in warnings + if w["data"]["message"].startswith(test_mode_string) + ] + if to_delete: + lando_api.del_warnings(to_delete) + + lando_api.add_warning( + LANDO_FAILURE_MESSAGE.format(test_mode_string=test_mode_string), + revision.id, + revision.diff["id"], + ) except Exception as ex: logger.error(str(ex), exc_info=True) diff --git a/bot/code_review_bot/report/base.py b/bot/code_review_bot/report/base.py index dc5b75239..1a14cbcaa 100644 --- a/bot/code_review_bot/report/base.py +++ b/bot/code_review_bot/report/base.py @@ -4,7 +4,7 @@ import itertools -from code_review_bot import Level +from code_review_bot import IssueType, Level class Reporter: @@ -52,7 +52,13 @@ def calc_stats(self, issues): def stats(analyzer, items): _items = list(items) - paths = list({i.path for i in _items if i.is_publishable()}) + paths = list( + { + i.path + for i in _items + if i.is_publishable() and i.type_ == IssueType.Lint + } + ) publishable = sum(i.is_publishable() for i in _items) build_errors = sum(i.is_build_error() for i in _items) diff --git a/bot/code_review_bot/report/builderrors.py b/bot/code_review_bot/report/builderrors.py index 0dfbeaf83..037745600 100644 --- a/bot/code_review_bot/report/builderrors.py +++ b/bot/code_review_bot/report/builderrors.py @@ -107,7 +107,7 @@ def publish_phabricator( } ) - def publish(self, issues, revision, task_failures, links, reviewers): + def publish(self, issues, revision, task_failures, links, reviewers, analysis_mode): build_errors = [issue for issue in issues if issue.is_build_error()] if not build_errors: diff --git a/bot/code_review_bot/report/debug.py b/bot/code_review_bot/report/debug.py index 4855b05c4..46072a4e2 100644 --- a/bot/code_review_bot/report/debug.py +++ b/bot/code_review_bot/report/debug.py @@ -23,7 +23,7 @@ def __init__(self, output_dir): assert os.path.isdir(output_dir), "Invalid output dir" self.report_path = os.path.join(output_dir, "report.json") - def publish(self, issues, revision, task_failures, links, reviewers): + def publish(self, issues, revision, task_failures, links, reviewers, analysis_mode): """ Display issues choices """ diff --git a/bot/code_review_bot/report/github.py b/bot/code_review_bot/report/github.py index 7d436cd0a..dd1e9e46c 100644 --- a/bot/code_review_bot/report/github.py +++ b/bot/code_review_bot/report/github.py @@ -26,7 +26,9 @@ def __init__(self, configuration={}, *args, **kwargs): self.analyzers_skipped, list ), "analyzers_skipped must be a list" - def publish(self, issues, revision, task_failures, notices, reviewers): + def publish( + self, issues, revision, task_failures, notices, reviewers, analysis_mode + ): """ Publish issues on a Github pull request. """ diff --git a/bot/code_review_bot/report/lando.py b/bot/code_review_bot/report/lando.py index 331a012b0..891b96324 100644 --- a/bot/code_review_bot/report/lando.py +++ b/bot/code_review_bot/report/lando.py @@ -5,13 +5,16 @@ import structlog from code_review_bot import Level +from code_review_bot.analysis import get_test_mode_string from code_review_bot.report.base import Reporter from code_review_bot.revisions import PhabricatorRevision logger = structlog.get_logger(__name__) -LANDO_MESSAGE = "The code review bot found {errors} {errors_noun} which should be fixed to avoid backout and {warnings} {warnings_noun}." -LANDO_MESSAGE_WARNINGS_ONLY = "The code review bot found {warnings} {warnings_noun}." +LANDO_MESSAGE = "{test_mode_string}: The code review bot found {errors} {errors_noun} which should be fixed to avoid backout and {warnings} {warnings_noun}." +LANDO_MESSAGE_WARNINGS_ONLY = ( + "{test_mode_string}: The code review bot found {warnings} {warnings_noun}." +) class LandoReporter(Reporter): @@ -26,7 +29,7 @@ def setup_api(self, lando_api): logger.info("Publishing warnings to lando is enabled by the bot!") self.lando_api = lando_api - def publish(self, issues, revision, task_failures, links, reviewers): + def publish(self, issues, revision, task_failures, links, reviewers, analysis_mode): """ Send an email to administrators """ @@ -61,9 +64,15 @@ def publish(self, issues, revision, task_failures, links, reviewers): try: # code-review.events sends an initial warning message to lando to specify that the analysis is in progress, # we should remove it - self.lando_api.del_all_warnings( + warnings = self.lando_api.get_warnings( revision.phabricator_id, revision.diff["id"] ) + test_mode_string = get_test_mode_string(analysis_mode) + to_delete = [ + w for w in warnings if w["data"]["message"].startswith(test_mode_string) + ] + if to_delete: + self.lando_api.del_warnings(to_delete) if nb_publishable > 0: if nb_publishable_errors >= 1: @@ -77,6 +86,7 @@ def publish(self, issues, revision, task_failures, links, reviewers): warnings_noun="warning" if nb_publishable_warnings == 1 else "warnings", + test_mode_string=test_mode_string, ), revision.phabricator_id, revision.diff["id"], @@ -89,6 +99,7 @@ def publish(self, issues, revision, task_failures, links, reviewers): warnings_noun="warning" if nb_publishable_warnings == 1 else "warnings", + test_mode_string=test_mode_string, ), revision.phabricator_id, revision.diff["id"], diff --git a/bot/code_review_bot/report/mail.py b/bot/code_review_bot/report/mail.py index eb4c01b51..a07977601 100644 --- a/bot/code_review_bot/report/mail.py +++ b/bot/code_review_bot/report/mail.py @@ -38,7 +38,7 @@ def __init__(self, configuration): logger.info("Mail report enabled", emails=self.emails) - def publish(self, issues, revision, task_failures, links, reviewers): + def publish(self, issues, revision, task_failures, links, reviewers, analysis_mode): """ Send an email to administrators """ diff --git a/bot/code_review_bot/report/phabricator.py b/bot/code_review_bot/report/phabricator.py index 57cf235e9..567ef6480 100644 --- a/bot/code_review_bot/report/phabricator.py +++ b/bot/code_review_bot/report/phabricator.py @@ -9,7 +9,8 @@ import structlog from libmozdata.phabricator import BuildState, PhabricatorAPI -from code_review_bot import Issue, Level, stats +from code_review_bot import BaseIssue, IssueType, Level, stats +from code_review_bot.analysis import AnalysisMode from code_review_bot.backend import BackendAPI from code_review_bot.report.base import Reporter from code_review_bot.revisions import PhabricatorRevision @@ -70,7 +71,7 @@ logger = structlog.get_logger(__name__) -Issues = List[Issue] +Issues = List[BaseIssue] class PhabricatorReporter(Reporter): @@ -163,7 +164,9 @@ def compare_issues(self, former_diff_id, issues): return unresolved, closed - def publish(self, issues, revision, task_failures, notices, reviewers): + def publish( + self, issues, revision, task_failures, notices, reviewers, analysis_mode + ): """ Publish issues on Phabricator: * publishable issues use lint results @@ -213,11 +216,13 @@ def publish(self, issues, revision, task_failures, notices, reviewers): # Use only new and publishable issues and patches # Avoid publishing a patch from a de-activated analyzer + issue_type = IssueType.Lint publishable_issues = [ issue for issue in issues if issue.is_publishable() and issue.analyzer.name not in self.analyzers_skipped + and issue.type_ == issue_type ] patches = [ patch @@ -243,35 +248,37 @@ def publish(self, issues, revision, task_failures, notices, reviewers): diff["id"] for diff in rev_diffs if diff["id"] < revision.diff_id ] former_diff_id = sorted(older_diff_ids)[-1] if older_diff_ids else None - unresolved_issues, closed_issues = self.compare_issues( - former_diff_id, publishable_issues - ) - if ( - len(unresolved_issues) == len(publishable_issues) - and not closed_issues - and not task_failures - and not notices - ): - # Nothing changed, no issue have been opened or closed - logger.info( - "No new issues nor failures/notices were detected. " - "Skipping comment publication (some issues are unresolved)", - unresolved_count=len(unresolved_issues), + if analysis_mode == AnalysisMode.Lint: + unresolved_issues, closed_issues = self.compare_issues( + former_diff_id, publishable_issues ) - return publishable_issues, patches - # Publish comment summarizing detected, unresolved and closed issues - self.publish_summary( - revision, - publishable_issues, - patches, - task_failures, - notices, - former_diff_id=former_diff_id, - unresolved_count=len(unresolved_issues), - closed_count=len(closed_issues), - ) + if ( + len(unresolved_issues) == len(publishable_issues) + and not closed_issues + and not task_failures + and not notices + ): + # Nothing changed, no issue have been opened or closed + logger.info( + "No new issues nor failures/notices were detected. " + "Skipping comment publication (some issues are unresolved)", + unresolved_count=len(unresolved_issues), + ) + return publishable_issues, patches + + # Publish comment summarizing detected, unresolved and closed issues + self.publish_summary( + revision, + publishable_issues, + patches, + task_failures, + notices, + former_diff_id=former_diff_id, + unresolved_count=len(unresolved_issues), + closed_count=len(closed_issues), + ) # Publish statistics stats.add_metric("report.phabricator.issues", len(issues)) @@ -279,20 +286,21 @@ def publish(self, issues, revision, task_failures, notices, reviewers): return publishable_issues, patches - def publish_harbormaster( - self, revision, lint_issues: Issues = [], unit_issues: Issues = [] - ): + def publish_harbormaster(self, revision, issues: Issues = []): """ Publish issues through HarborMaster either as lint results or unit tests results """ - assert lint_issues or unit_issues, "No issues to publish" + assert issues, "No issues to publish" + + lint_issues = [i for i in issues if i.type_ == IssueType.Lint] + unit_issues = [] self.api.update_build_target( revision.build_target_phid, state=BuildState.Work, - lint=[issue.as_phabricator_lint() for issue in lint_issues], - unit=[issue.as_phabricator_unitresult() for issue in unit_issues], + lint=[issue.as_phabricator_issue() for issue in lint_issues], + unit=[issue.as_phabricator_issue() for issue in unit_issues], ) logger.info( "Updated Harbormaster build state with issues", diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py index 3575d6194..24fa2e404 100644 --- a/bot/code_review_bot/revisions/base.py +++ b/bot/code_review_bot/revisions/base.py @@ -83,6 +83,10 @@ def __init__( self.files = [] self.lines = {} + def persistent_id(self): + """An identifier that is constant across all diffs for the same revision.""" + raise NotImplementedError + @property def namespaces(self): raise NotImplementedError @@ -93,17 +97,13 @@ def before_after_feature(self): Randomly run the before/after feature depending on a configured ratio. All the diffs of a revision must be analysed with or without the feature. """ - if getattr(self, "id", None) is None: - logger.debug( - "Backend ID must be set to determine if using the before/after feature. Skipping." - ) - return False # Set random module pseudo-random seed based on the revision ID to # ensure that successive calls to random.random will return deterministic values - random.seed(self.id) - return random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0) + random.seed(f"before-after-{self.persistent_id()}") + ret = random.random() < taskcluster.secrets.get("BEFORE_AFTER_RATIO", 0) # Reset random module seed to prevent deterministic values after calling that function random.seed(os.urandom(128)) + return ret def __repr__(self): raise NotImplementedError diff --git a/bot/code_review_bot/revisions/github.py b/bot/code_review_bot/revisions/github.py index 249b34758..b68eb6194 100644 --- a/bot/code_review_bot/revisions/github.py +++ b/bot/code_review_bot/revisions/github.py @@ -45,6 +45,9 @@ def __str__(self): def __repr__(self): return f"GithubRevision base_repo={self.base_repository} head_repo={self.head_repository} pull_number={self.pull_number} head={self.head_changeset}" + def persistent_id(self): + return self.pull_number + @property def repo_name(self): """ diff --git a/bot/code_review_bot/revisions/phabricator.py b/bot/code_review_bot/revisions/phabricator.py index 89ff1519e..dd2fdf960 100644 --- a/bot/code_review_bot/revisions/phabricator.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -82,6 +82,9 @@ def __init__( # Patch analysis self.patch = patch + def persistent_id(self): + return self.phabricator_id + @property def namespaces(self): # Simplify repository names diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 8915c120d..40b9b0d4e 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -114,7 +114,7 @@ def run(self, revision, analysis_mode: AnalysisMode): def _run_lint(self, revision): # Index ASAP Taskcluster task for this revision - self.index(revision, state="started") + self.index(revision, state="started", namespace_suffixes=["", "lint"]) # Set the Phabricator build as running self.update_status(revision, state=BuildState.Work) @@ -173,7 +173,15 @@ def _run_lint(self, revision): logger.info("No issues nor notices, stopping there.") # Publish all issues - self.publish(revision, issues, task_failures, notices, reviewers) + self.publish( + revision, + issues, + task_failures, + notices, + reviewers, + AnalysisMode.Lint, + ["", "lint"], + ) return issues @@ -286,8 +294,9 @@ def start_analysis( """ logger.info("Starting revision analysis", revision=revision) + namespace_suffixes = ["", "lint"] # Index ASAP Taskcluster task for this revision - self.index(revision, state="analysis") + self.index(revision, state="analysis", namespace_suffixes=namespace_suffixes) # Do not process revisions from black-listed users if revision.is_blacklisted: @@ -394,7 +403,9 @@ def start_analysis( self.cancel_previous(revision) # Update index when the patch has been pushed to try - self.index(revision, state="pushed_to_try") + self.index( + revision, state="pushed_to_try", namespace_suffixes=namespace_suffixes + ) # Update final state using worker output if self.update_build and isinstance(revision, PhabricatorRevision): @@ -405,7 +416,7 @@ def start_analysis( # Send Build in progress or errors to Lando lando_reporter = self.reporters.get("lando") if lando_reporter is not None: - publish_analysis_lando(output, lando_reporter.lando_api) + publish_analysis_lando(output, lando_reporter.lando_api, analysis_mode) else: logger.info("Skipping Lando publication") @@ -463,7 +474,16 @@ def clone_repository(self, revision): self.clone_available = True - def publish(self, revision, issues, task_failures, notices, reviewers): + def publish( + self, + revision, + issues, + task_failures, + notices, + reviewers, + analysis_mode: AnalysisMode, + namespace_suffixes, + ): """ Publish issues on selected reporters """ @@ -492,16 +512,23 @@ def publish(self, revision, issues, task_failures, notices, reviewers): state="analyzed", issues=nb_issues, issues_publishable=nb_publishable, + namespace_suffixes=namespace_suffixes, ) stats.add_metric("analysis.issues.publishable", nb_publishable) # Publish reports about these issues with stats.timer("runtime.reports"): for reporter in self.reporters.values(): - reporter.publish(issues, revision, task_failures, notices, reviewers) + reporter.publish( + issues, revision, task_failures, notices, reviewers, analysis_mode + ) self.index( - revision, state="done", issues=nb_issues, issues_publishable=nb_publishable + revision, + state="done", + issues=nb_issues, + issues_publishable=nb_publishable, + namespace_suffixes=namespace_suffixes, ) # Publish final HarborMaster state @@ -750,7 +777,7 @@ def find_try_decision_task(self, publication_task_id): except Exception as e: logger.warn("Failed to find a decision task", route=route, error=str(e)) - def index(self, revision, **kwargs): + def index(self, revision, namespace_suffixes=None, **kwargs): """ Index current task on Taskcluster index """ @@ -782,11 +809,23 @@ def index(self, revision, **kwargs): "error_code" ) in ("watchdog", "mercurial") + # Apply namespace suffixes if supplied + if namespace_suffixes: + namespaces = [] + for suffix in namespace_suffixes: + if suffix != "": + namespaces.extend( + [f"{namespace}.{suffix}" for namespace in revision.namespaces] + ) + else: + namespaces.extend(revision.namespaces) + else: + namespaces = revision.namespaces + # Add a sub namespace with the task id to be able to list # tasks from the parent namespace - namespaces = revision.namespaces + [ - f"{namespace}.{settings.taskcluster.task_id}" - for namespace in revision.namespaces + namespaces = namespaces + [ + f"{namespace}.{settings.taskcluster.task_id}" for namespace in namespaces ] # Build complete namespaces list, with monitoring update diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index a6b3d6395..1cfabc3d8 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -22,7 +22,7 @@ import responses from libmozdata.phabricator import PhabricatorAPI -from code_review_bot import Level, stats +from code_review_bot import IssueType, Level, stats from code_review_bot.backend import BackendAPI from code_review_bot.config import GetAppUserAgent, settings from code_review_bot.mercurial import MercurialRepository @@ -94,6 +94,8 @@ def mock_issues(mock_task): task = mock_task(DefaultTask, "mock-analyzer") class MockIssue: + type_ = IssueType.Lint + def __init__(self, nb): self.nb = nb self.path = "/path/to/file" diff --git a/bot/tests/test_clang.py b/bot/tests/test_clang.py index 40a1d1b59..6d02fbe38 100644 --- a/bot/tests/test_clang.py +++ b/bot/tests/test_clang.py @@ -144,7 +144,7 @@ def test_as_markdown(mock_revision, mock_task): """ ) - assert issue.as_phabricator_lint() == { + assert issue.as_phabricator_issue() == { "char": 51, "code": "dummy-check", "line": 42, diff --git a/bot/tests/test_coverage.py b/bot/tests/test_coverage.py index efb77291b..f188a064a 100644 --- a/bot/tests/test_coverage.py +++ b/bot/tests/test_coverage.py @@ -52,7 +52,7 @@ def test_coverage( "hash": "c43a516d74b257b21495bc3183f0543e", "fix": None, } - assert issue.as_phabricator_lint() == { + assert issue.as_phabricator_issue() == { "code": "no-coverage", "line": 1, "name": "code coverage analysis", @@ -101,7 +101,7 @@ def test_coverage( "hash": "79a322a555198ec03ccce3daf086ef81", "fix": None, } - assert issue.as_phabricator_lint() == { + assert issue.as_phabricator_issue() == { "code": "no-coverage", "line": 1, "name": "code coverage analysis", @@ -147,7 +147,7 @@ def test_coverage( "hash": "db03e65242d5bf27abf532026f782d2d", "fix": None, } - assert issue.as_phabricator_lint() == { + assert issue.as_phabricator_issue() == { "code": "no-coverage", "line": 1, "name": "code coverage analysis", diff --git a/bot/tests/test_index.py b/bot/tests/test_index.py index 63f984aae..d5169b220 100644 --- a/bot/tests/test_index.py +++ b/bot/tests/test_index.py @@ -69,6 +69,118 @@ def test_taskcluster_index(mock_config, mock_workflow): assert "indexed" in args["data"] +def test_taskcluster_index_with_suffix(mock_config, mock_workflow): + """ + Test the Taskcluster indexing API + by mocking an online taskcluster state + """ + + mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) + mock_workflow.index_service = mock.Mock() + rev = MockPhabricatorRevision( + namespaces=["mock.1234"], + details={"id": "1234", "someData": "mock", "state": "done"}, + repository="test-repo", + ) + mock_workflow.index(rev, namespace_suffixes=["suffixtest"], test="dummy") + + assert mock_workflow.index_service.insertTask.call_count == 2 + calls = mock_workflow.index_service.insertTask.call_args_list + + # First call with namespace + namespace, args = calls[0][0] + assert namespace == "project.relman.test.code-review.mock.1234.suffixtest" + assert args["taskId"] == "12345deadbeef" + assert args["data"]["test"] == "dummy" + assert args["data"]["id"] == "1234" + assert args["data"]["source"] == "try" + assert args["data"]["try_group_id"] == "remoteTryGroup" + assert args["data"]["repository"] == "test-repo" + assert args["data"]["someData"] == "mock" + assert "indexed" in args["data"] + + # Second call with sub namespace + namespace, args = calls[1][0] + assert ( + namespace + == "project.relman.test.code-review.mock.1234.suffixtest.12345deadbeef" + ) + assert args["taskId"] == "12345deadbeef" + assert args["data"]["test"] == "dummy" + assert args["data"]["id"] == "1234" + assert args["data"]["source"] == "try" + assert args["data"]["try_group_id"] == "remoteTryGroup" + assert args["data"]["repository"] == "test-repo" + assert args["data"]["someData"] == "mock" + assert "indexed" in args["data"] + + +def test_taskcluster_index_with_multiple_suffixes(mock_config, mock_workflow): + """ + Test the Taskcluster indexing API + by mocking an online taskcluster state + """ + + mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) + mock_workflow.index_service = mock.Mock() + rev = MockPhabricatorRevision( + namespaces=["mock.1234"], + details={"id": "1234", "someData": "mock", "state": "done"}, + repository="test-repo", + ) + mock_workflow.index(rev, namespace_suffixes=["", "suffixtest"], test="dummy") + + assert mock_workflow.index_service.insertTask.call_count == 4 + calls = mock_workflow.index_service.insertTask.call_args_list + + namespace, args = calls[0][0] + assert namespace == "project.relman.test.code-review.mock.1234" + assert args["taskId"] == "12345deadbeef" + assert args["data"]["test"] == "dummy" + assert args["data"]["id"] == "1234" + assert args["data"]["source"] == "try" + assert args["data"]["try_group_id"] == "remoteTryGroup" + assert args["data"]["repository"] == "test-repo" + assert args["data"]["someData"] == "mock" + assert "indexed" in args["data"] + + namespace, args = calls[1][0] + assert namespace == "project.relman.test.code-review.mock.1234.suffixtest" + assert args["taskId"] == "12345deadbeef" + assert args["data"]["test"] == "dummy" + assert args["data"]["id"] == "1234" + assert args["data"]["source"] == "try" + assert args["data"]["try_group_id"] == "remoteTryGroup" + assert args["data"]["repository"] == "test-repo" + assert args["data"]["someData"] == "mock" + assert "indexed" in args["data"] + + namespace, args = calls[2][0] + assert namespace == "project.relman.test.code-review.mock.1234.12345deadbeef" + assert args["taskId"] == "12345deadbeef" + assert args["data"]["test"] == "dummy" + assert args["data"]["id"] == "1234" + assert args["data"]["source"] == "try" + assert args["data"]["try_group_id"] == "remoteTryGroup" + assert args["data"]["repository"] == "test-repo" + assert args["data"]["someData"] == "mock" + assert "indexed" in args["data"] + + namespace, args = calls[3][0] + assert ( + namespace + == "project.relman.test.code-review.mock.1234.suffixtest.12345deadbeef" + ) + assert args["taskId"] == "12345deadbeef" + assert args["data"]["test"] == "dummy" + assert args["data"]["id"] == "1234" + assert args["data"]["source"] == "try" + assert args["data"]["try_group_id"] == "remoteTryGroup" + assert args["data"]["repository"] == "test-repo" + assert args["data"]["someData"] == "mock" + assert "indexed" in args["data"] + + def test_index_autoland( mock_autoland_task, mock_phabricator, diff --git a/bot/tests/test_lint.py b/bot/tests/test_lint.py index db6862076..6fff8ecc4 100644 --- a/bot/tests/test_lint.py +++ b/bot/tests/test_lint.py @@ -83,7 +83,7 @@ def test_as_text(mock_config, mock_revision, mock_hgmo, mock_task): issue.as_text() == "Error: Dummy test withUppercaseChars [flake8: dummy rule]" ) - assert issue.as_phabricator_lint() == { + assert issue.as_phabricator_issue() == { "char": 1, "code": "dummy rule", "line": 1, diff --git a/bot/tests/test_remote.py b/bot/tests/test_remote.py index c119cb58b..61f5f2a3c 100644 --- a/bot/tests/test_remote.py +++ b/bot/tests/test_remote.py @@ -561,7 +561,7 @@ def test_clang_format_task( "hash": "9a517e384b1dbbe92025c48ea6b7eab9", "fix": "Multi\nlines", } - assert issue.as_phabricator_lint() == { + assert issue.as_phabricator_issue() == { "code": "invalid-styling", "description": """WARNING: The change does not follow the C/C++ coding style, please reformat diff --git a/bot/tests/test_reporter_builderrors.py b/bot/tests/test_reporter_builderrors.py index 123c90b5f..b1b473273 100644 --- a/bot/tests/test_reporter_builderrors.py +++ b/bot/tests/test_reporter_builderrors.py @@ -10,6 +10,7 @@ from conftest import FIXTURES_DIR from responses import matchers +from code_review_bot.analysis import AnalysisMode from code_review_bot.report.builderrors import BuildErrorsReporter MAIL_CONTENT_BUILD_ERRORS = """ @@ -52,7 +53,7 @@ def _check_email(request): conf = {"emails": ["test@mozilla.com"]} r = BuildErrorsReporter(conf) - r.publish(mock_clang_tidy_issues, mock_revision, [], [], []) + r.publish(mock_clang_tidy_issues, mock_revision, [], [], [], AnalysisMode.Lint) assert log.has("Send build error email", to="test@mozilla.com") @@ -96,4 +97,6 @@ def test_builderrors_github( ], ) r = BuildErrorsReporter({}) - r.publish(mock_clang_tidy_issues, mock_github_revision, [], [], []) + r.publish( + mock_clang_tidy_issues, mock_github_revision, [], [], [], AnalysisMode.Lint + ) diff --git a/bot/tests/test_reporter_debug.py b/bot/tests/test_reporter_debug.py index f35d01575..89019733d 100644 --- a/bot/tests/test_reporter_debug.py +++ b/bot/tests/test_reporter_debug.py @@ -4,6 +4,7 @@ import json import os.path +from code_review_bot.analysis import AnalysisMode from code_review_bot.tasks.clang_tidy import ClangTidyTask @@ -21,7 +22,7 @@ def test_publication(tmpdir, mock_issues, mock_revision): task = ClangTidyTask("someTaskId", status) r = DebugReporter(report_dir) - r.publish(mock_issues, mock_revision, [task], [], []) + r.publish(mock_issues, mock_revision, [task], [], [], AnalysisMode.Lint) assert os.path.exists(report_path) with open(report_path) as f: diff --git a/bot/tests/test_reporter_github.py b/bot/tests/test_reporter_github.py index 51c921ccd..4a2e38c4a 100644 --- a/bot/tests/test_reporter_github.py +++ b/bot/tests/test_reporter_github.py @@ -11,6 +11,7 @@ from conftest import FIXTURES_DIR from code_review_bot import Level +from code_review_bot.analysis import AnalysisMode from code_review_bot.report.github import GithubReporter from code_review_bot.revisions import GithubRevision, Revision from code_review_bot.tasks.clang_tidy import ClangTidyIssue, ClangTidyTask @@ -110,7 +111,12 @@ def test_github_review( ) reporter.publish( - [issue_clang_tidy, issue_on_touched_file, issue_coverage], revision, [], [], [] + [issue_clang_tidy, issue_on_touched_file, issue_coverage], + revision, + [], + [], + [], + AnalysisMode.Lint, ) assert [(call.request.method, call.request.url) for call in responses.calls] == [ ("GET", "https://github.com/owner/repo-name/pull/1.diff"), @@ -214,7 +220,7 @@ def test_github_review_cleanup( json={}, ) - reporter.publish([], revision, [], [], []) + reporter.publish([], revision, [], [], [], AnalysisMode.Lint) assert [(call.request.method, call.request.url) for call in responses.calls] == [ ("GET", "https://github.com/owner/repo-name/pull/1.diff"), ("GET", "https://api.github.com:443/app/installations"), diff --git a/bot/tests/test_reporter_lando.py b/bot/tests/test_reporter_lando.py index 9f90884b0..bd20670ce 100644 --- a/bot/tests/test_reporter_lando.py +++ b/bot/tests/test_reporter_lando.py @@ -2,6 +2,7 @@ # 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/. +from code_review_bot.analysis import AnalysisMode from code_review_bot.report.lando import LANDO_MESSAGE, LandoReporter MOCK_LANDO_API_URL = "http://api.lando.test" @@ -17,18 +18,20 @@ def __init__(self, api_url, api_key): self.api_url = MOCK_LANDO_API_URL self.api_key = MOCK_LANDO_TOKEN + def get_warnings(self, revision_id, diff_id): + self.revision_id = revision_id + self.diff_id = diff_id + + return [] + def del_warnings(self, warnings): - pass + self.warnings = warnings def add_warning(self, warning, revision_id, diff_id): self.revision_id = revision_id self.diff_id = diff_id self.warning = warning - def del_all_warnings(self, revision_id, diff_id): - self.revision_id = revision_id - self.diff_id = diff_id - def test_lando(log, mock_clang_tidy_issues, mock_revision): """ @@ -48,12 +51,16 @@ def test_lando(log, mock_clang_tidy_issues, mock_revision): assert log.has("Publishing warnings to lando is enabled by the bot!") - r.publish(mock_clang_tidy_issues, mock_revision, [], [], []) + r.publish(mock_clang_tidy_issues, mock_revision, [], [], [], AnalysisMode.Lint) assert lando_api.revision_id == mock_revision.revision["id"] assert lando_api.diff_id == mock_revision.diff_id assert lando_api.warning == LANDO_MESSAGE.format( - errors=1, errors_noun="error", warnings=0, warnings_noun="warnings" + errors=1, + errors_noun="error", + warnings=0, + warnings_noun="warnings", + test_mode_string="Static analysis and linting", ) assert log.has("Publishing warnings to lando for 1 errors and 0 warnings") diff --git a/bot/tests/test_reporter_mail.py b/bot/tests/test_reporter_mail.py index 58e1af4de..843a69480 100644 --- a/bot/tests/test_reporter_mail.py +++ b/bot/tests/test_reporter_mail.py @@ -7,6 +7,7 @@ import pytest import responses +from code_review_bot.analysis import AnalysisMode from code_review_bot.tasks.clang_format import ClangFormatTask from code_review_bot.tasks.clang_tidy import ClangTidyTask @@ -108,7 +109,7 @@ def _check_email(request): list( map(lambda p: p.write(), mock_revision.improvement_patches) ) # trigger local write - r.publish(mock_issues, mock_revision, [], [], []) + r.publish(mock_issues, mock_revision, [], [], [], AnalysisMode.Lint) # Check stats assert r.calc_stats(mock_issues) == [ diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index f36566522..1782d09f3 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -12,6 +12,7 @@ from structlog.testing import capture_logs from code_review_bot import Level +from code_review_bot.analysis import AnalysisMode from code_review_bot.report.phabricator import PhabricatorReporter from code_review_bot.revisions import ImprovementPatch, PhabricatorRevision, Revision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask @@ -291,7 +292,7 @@ def test_phabricator_clang_tidy(mock_phabricator, phab, mock_decision_task, mock ) assert issue.is_publishable() - issues, patches = reporter.publish([issue], revision, [], [], []) + issues, patches = reporter.publish([issue], revision, [], [], [], AnalysisMode.Lint) assert len(issues) == 1 assert len(patches) == 0 @@ -333,7 +334,7 @@ def test_phabricator_clang_format( ] list(map(lambda p: p.write(), revision.improvement_patches)) # trigger local write - issues, patches = reporter.publish([issue], revision, [], [], []) + issues, patches = reporter.publish([issue], revision, [], [], [], AnalysisMode.Lint) assert len(issues) == 1 assert len(patches) == 1 @@ -393,7 +394,7 @@ def test_phabricator_mozlint( assert issue_eslint.is_publishable() issues, patches = reporter.publish( - [issue_flake, issue_eslint], revision, [], [], [] + [issue_flake, issue_eslint], revision, [], [], [], AnalysisMode.Lint ) assert len(issues) == 2 assert len(patches) == 0 @@ -466,7 +467,7 @@ def test_phabricator_coverage( ) assert issue.is_publishable() - issues, patches = reporter.publish([issue], revision, [], [], []) + issues, patches = reporter.publish([issue], revision, [], [], [], AnalysisMode.Lint) assert len(issues) == 1 assert len(patches) == 0 @@ -585,7 +586,7 @@ def test_phabricator_clang_tidy_and_coverage( assert issue_coverage.is_publishable() issues, patches = reporter.publish( - [issue_clang_tidy, issue_coverage], revision, [], [], [] + [issue_clang_tidy, issue_coverage], revision, [], [], [], AnalysisMode.Lint ) assert len(issues) == 2 assert len(patches) == 0 @@ -741,7 +742,7 @@ def test_phabricator_analyzers( ] list(map(lambda p: p.write(), revision.improvement_patches)) # trigger local write - issues, patches = reporter.publish(issues, revision, [], [], []) + issues, patches = reporter.publish(issues, revision, [], [], [], AnalysisMode.Lint) # Check issues & patches analyzers assert len(issues) == len(valid_issues) @@ -787,7 +788,9 @@ def test_phabricator_clang_tidy_build_error( assert issue.is_publishable() - issues, patches = reporter.publish([issue], revision, [], [], []) + issues, patches = reporter.publish( + [issue], revision, [], [], [], AnalysisMode.Lint + ) assert len(issues) == 1 assert len(patches) == 0 @@ -848,7 +851,7 @@ def test_full_file(mock_config, mock_phabricator, phab, mock_decision_task, mock assert revision.has_file(issue.path) assert revision.contains(issue) - issues, patches = reporter.publish([issue], revision, [], [], []) + issues, patches = reporter.publish([issue], revision, [], [], [], AnalysisMode.Lint) assert len(issues) == 1 assert len(patches) == 0 @@ -896,7 +899,7 @@ def test_task_failures(mock_phabricator, phab, mock_decision_task): "status": {"runs": [{"runId": 0}]}, } task = ClangTidyTask("ab3NrysvSZyEwsOHL2MZfw", status) - issues, patches = reporter.publish([], revision, [task], [], []) + issues, patches = reporter.publish([], revision, [task], [], [], AnalysisMode.Lint) assert len(issues) == 0 assert len(patches) == 0 @@ -959,7 +962,9 @@ def test_extra_errors(mock_phabricator, mock_decision_task, phab, mock_task): ), ] - published_issues, patches = reporter.publish(all_issues, revision, [], [], []) + published_issues, patches = reporter.publish( + all_issues, revision, [], [], [], AnalysisMode.Lint + ) assert len(published_issues) == 2 assert len(patches) == 0 @@ -1023,6 +1028,7 @@ def test_phabricator_notices(mock_phabricator, phab, mock_decision_task): [], notices, [], + AnalysisMode.Lint, ) # Check the comment has been posted @@ -1041,6 +1047,7 @@ def test_phabricator_notices(mock_phabricator, phab, mock_decision_task): [], notices, [], + AnalysisMode.Lint, ) # Check the comment has been posted @@ -1075,6 +1082,7 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_decision_task): [], [doc_notice], [], + AnalysisMode.Lint, ) # Check the comment has been posted @@ -1126,7 +1134,12 @@ def test_phabricator_external_tidy( assert not issue_clang_diagnostic.is_publishable() issues, patches = reporter.publish( - [issue_civet_warning, issue_clang_diagnostic], revision, [], [], [] + [issue_civet_warning, issue_clang_diagnostic], + revision, + [], + [], + [], + AnalysisMode.Lint, ) assert len(issues) == 1 assert len(patches) == 0 @@ -1168,7 +1181,9 @@ def test_phabricator_newer_diff( with capture_logs() as cap_logs: os.environ["SPECIAL_NAME"] = "PHID-DREV-zzzzz-updated" - issues, patches = reporter.publish([issue], revision, [], [], []) + issues, patches = reporter.publish( + [issue], revision, [], [], [], AnalysisMode.Lint + ) assert cap_logs == [ # Log from PhabricatorReporter.publish_harbormaster(), it was still called @@ -1302,7 +1317,9 @@ def test_phabricator_former_diff_comparison( os.environ["SPECIAL_NAME"] = "PHID-DREV-zzzzz-updated" with capture_logs() as cap_logs: - issues, patches = reporter.publish(issues, revision, [], [], []) + issues, patches = reporter.publish( + issues, revision, [], [], [], AnalysisMode.Lint + ) assert cap_logs == [ # Log from PhabricatorReporter.publish_harbormaster(), it was still called @@ -1416,7 +1433,7 @@ def test_phabricator_before_after_comment( with capture_logs() as cap_logs: issues, patches = reporter.publish( - [cov_issue, clang_issue], revision, [], [], [] + [cov_issue, clang_issue], revision, [], [], [], AnalysisMode.Lint ) assert cap_logs == [ diff --git a/bot/tests/test_revisions.py b/bot/tests/test_revisions.py index 8cc48e24e..11b18b100 100644 --- a/bot/tests/test_revisions.py +++ b/bot/tests/test_revisions.py @@ -136,7 +136,7 @@ def as_text(): def validates(): return True - def as_phabricator_lint(): + def as_phabricator_issue(): return {} issue_in_new_file = MyIssue("new.txt", 1) @@ -225,7 +225,7 @@ def test_revision_before_after(mock_config, mock_revision, mock_taskcluster_conf Ensure before/after feature is always run on specific revisions """ mock_taskcluster_config.secrets["BEFORE_AFTER_RATIO"] = 0.5 - mock_revision.id = 51 + mock_revision.phabricator_id = 51 assert mock_revision.before_after_feature is True - mock_revision.id = 42 + mock_revision.phabricator_id = 42 assert mock_revision.before_after_feature is False diff --git a/bot/tests/test_testing_policy.py b/bot/tests/test_testing_policy.py index abcabc24f..4b58d0e21 100644 --- a/bot/tests/test_testing_policy.py +++ b/bot/tests/test_testing_policy.py @@ -6,6 +6,7 @@ import pytest +from code_review_bot.analysis import AnalysisMode from code_review_bot.testing_policy import ( NEEDS_TESTING_TAG_PHID, TESTING_APPROVED_PHID, @@ -409,7 +410,7 @@ def test_phabricator_reporter_sets_tag( revision.id = 52 reporter = PhabricatorReporter({}, api=api) - reporter.publish([], revision, [], [], []) + reporter.publish([], revision, [], [], [], AnalysisMode.Lint) # A single transaction adding the tag has been sent assert phab.transactions == { @@ -435,6 +436,6 @@ def test_phabricator_reporter_skips_code_changes( revision.id = 52 reporter = PhabricatorReporter({}, api=api) - reporter.publish([], revision, [], [], []) + reporter.publish([], revision, [], [], [], AnalysisMode.Lint) assert phab.transactions == {} diff --git a/bot/tests/test_workflow.py b/bot/tests/test_workflow.py index 406ef0aba..f69c6f8d0 100644 --- a/bot/tests/test_workflow.py +++ b/bot/tests/test_workflow.py @@ -288,6 +288,8 @@ def test_before_after(mock_taskcluster_config, mock_workflow, mock_task, mock_re [], [], [], + AnalysisMode.Lint, + ["", "lint"], ) ] assert issues[0].new_issue is True