diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index 570d2c760..f6a9a6a7c 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -17,12 +17,12 @@ ) -class RevisionBuild(PhabricatorBuild): +class PhabricatorRevisionBuild(PhabricatorBuild): """ Convert the bot revision into a libmozevent compatible build """ - def __init__(self, revision, phabricator_api): + def __init__(self, phab_revision, phabricator_api): # State should be updated to Public self.state = PhabricatorBuildState.Queued @@ -32,18 +32,18 @@ def __init__(self, revision, phabricator_api): # Incremented on an unexpected failure during build's push to try self.retries = 0 - # Revision used by Phabricator updates + # PhabricatorRevision used by Phabricator updates # Direct output of Phabricator API (not the object passed here) - self.revision_id = revision.phabricator_id + self.revision_id = phab_revision.phabricator_id self.revision_url = None self.revision = None # Needed to update Phabricator Harbormaster - self.target_phid = revision.build_target_phid + self.target_phid = phab_revision.build_target_phid # Needed to load stack of patches - self.diff = revision.diff - self.diff_id = revision.diff_id + self.diff = phab_revision.diff + self.diff_id = phab_revision.diff_id # Needed to apply patch and communicate on Phabricator self.base_revision = None @@ -166,7 +166,7 @@ def publish_analysis_lando(payload, lando_warnings): Publish result of patch application and push to try on Lando """ mode, build, extras = payload - assert isinstance(build, RevisionBuild), "Not a RevisionBuild" + assert isinstance(build, PhabricatorRevisionBuild), "Not a PhabricatorRevisionBuild" logger.debug("Publishing a Lando build update", mode=mode, build=str(build)) if mode == "fail:general": diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 5e4c2ad7c..6e2a9a875 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -26,7 +26,7 @@ ) from code_review_bot.config import settings from code_review_bot.report import get_reporters -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision, Revision from code_review_bot.tools.libmozdata import setup as setup_libmozdata from code_review_bot.tools.log import init_logger from code_review_bot.workflow import Workflow @@ -162,11 +162,13 @@ def main(): # Load unique revision try: if settings.generic_group_id: - revision = Revision.from_decision_task( + # Only Phabricator revisions is supported from decision task + revision = PhabricatorRevision.from_decision_task( queue_service.task(settings.generic_group_id), phabricator_api ) elif settings.phabricator_build_target: - revision = Revision.from_phabricator_trigger( + # Only Phabricator revisions is supported from build target + revision = PhabricatorRevision.from_phabricator_trigger( settings.phabricator_build_target, phabricator_api, ) @@ -176,6 +178,7 @@ def main(): queue_service.task(settings.try_group_id), phabricator_api, ) + except InvalidTrigger as e: logger.info("Early stop analysis due to invalid trigger", error=str(e)) diff --git a/bot/code_review_bot/github.py b/bot/code_review_bot/github.py new file mode 100644 index 000000000..e69de29bb diff --git a/bot/code_review_bot/report/lando.py b/bot/code_review_bot/report/lando.py index 330bda4ab..21ed73771 100644 --- a/bot/code_review_bot/report/lando.py +++ b/bot/code_review_bot/report/lando.py @@ -6,6 +6,7 @@ from code_review_bot import Level from code_review_bot.report.base import Reporter +from code_review_bot.revisions import PhabricatorRevision logger = structlog.get_logger(__name__) @@ -29,6 +30,11 @@ def publish(self, issues, revision, task_failures, links, reviewers): """ Send an email to administrators """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) + assert ( revision.phabricator_id and revision.phabricator_phid and revision.diff ), "Revision must have a Phabricator ID, a PHID and a diff" diff --git a/bot/code_review_bot/report/mail_builderrors.py b/bot/code_review_bot/report/mail_builderrors.py index 621488264..d1879eb61 100644 --- a/bot/code_review_bot/report/mail_builderrors.py +++ b/bot/code_review_bot/report/mail_builderrors.py @@ -36,7 +36,7 @@ def publish(self, issues, revision, task_failures, links, reviewers): """ assert ( revision.phabricator_id and revision.phabricator_phid - ), "Revision must have a Phabricator ID and PHID" + ), "PhabricatorRevision must have a Phabricator ID and PHID" assert ( "attachments" in revision.diff ), f"Unable to find the commits for revision with phid {revision.phabricator_phid}." diff --git a/bot/code_review_bot/revisions/__init__.py b/bot/code_review_bot/revisions/__init__.py new file mode 100644 index 000000000..14de3f486 --- /dev/null +++ b/bot/code_review_bot/revisions/__init__.py @@ -0,0 +1,6 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# 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.revisions.base import ImprovementPatch, Revision # noqa +from code_review_bot.revisions.phabricator import PhabricatorRevision # noqa diff --git a/bot/code_review_bot/revisions/base.py b/bot/code_review_bot/revisions/base.py new file mode 100644 index 000000000..032f7dd70 --- /dev/null +++ b/bot/code_review_bot/revisions/base.py @@ -0,0 +1,257 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# 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 random +from abc import ABC +from datetime import timedelta + +import rs_parsepatch +import structlog +from libmozdata.phabricator import PhabricatorAPI + +from code_review_bot import Issue, stats, taskcluster +from code_review_bot.config import ( + settings, +) +from code_review_bot.tasks.base import AnalysisTask + +logger = structlog.get_logger(__name__) + + +class ImprovementPatch: + """ + An improvement patch built by the bot + """ + + def __init__(self, analyzer, patch_name, content): + assert isinstance(analyzer, AnalysisTask) + + # Build name from analyzer and revision + self.analyzer = analyzer + self.name = f"{self.analyzer.name}-{patch_name}.diff" + self.content = content + self.url = None + self.path = None + + def __str__(self): + return f"{self.analyzer.name}: {self.url or self.path or self.name}" + + def write(self): + """ + Write patch on local FS, for dev & tests only + """ + self.path = os.path.join(settings.taskcluster.results_dir, self.name) + with open(self.path, "w") as f: + length = f.write(self.content) + logger.info("Improvement patch saved", path=self.path, length=length) + + def publish(self, days_ttl=30): + """ + Push through Taskcluster API to setup the content-type header + so it displays nicely in browsers + """ + assert ( + not settings.taskcluster.local + ), "Only publish on online Taskcluster tasks" + self.url = taskcluster.upload_artifact( + f"public/patch/{self.name}", + self.content.encode(), + content_type="text/plain; charset=utf-8", # Displays instead of download + ttl=timedelta(days=days_ttl - 1), + ) + logger.info("Improvement patch published", url=self.url) + + +class Revision(ABC): + """ + A generic revision class to override using provider specific details + """ + + def __init__( + self, + ): + # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional) + self.issues_url = None + + # Patches built later on + self.improvement_patches = [] + + # Patch analysis + self.files = [] + self.lines = {} + + @property + def namespaces(self): + raise NotImplementedError + + @property + 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) + # Reset random module seed to prevent deterministic values after calling that function + random.seed(os.urandom(128)) + + def __repr__(self): + raise NotImplementedError + + def __str__(self): + raise NotImplementedError + + def analyze_patch(self): + """ + Analyze loaded patch to extract modified lines + and statistics + """ + assert self.patch is not None, "Missing patch" + assert isinstance(self.patch, str), "Invalid patch type" + + # List all modified lines from current revision changes + patch_stats = rs_parsepatch.get_lines(self.patch) + assert len(patch_stats) > 0, "Empty patch" + + self.lines = {stat["filename"]: stat["added_lines"] for stat in patch_stats} + + # Shortcut to files modified + self.files = self.lines.keys() + + # Report nb of files and lines analyzed + stats.add_metric("analysis.files", len(self.files)) + stats.add_metric( + "analysis.lines", sum(len(line) for line in self.lines.values()) + ) + + def load_file(self, path): + """ + Load a file content at current revision from remote HGMO + """ + raise NotImplementedError + + def has_file(self, path): + """ + Check if the path is in this patch + """ + assert isinstance(path, str) + return path in self.files + + def contains(self, issue): + """ + Check if the issue (path+lines) is in this patch + """ + assert isinstance(issue, Issue) + + # Get modified lines for this issue + modified_lines = self.lines.get(issue.path) + if modified_lines is None: + return False + + # Empty line means full file + if issue.line is None: + return True + + # Detect if this issue is in the patch + lines = set(range(issue.line, issue.line + issue.nb_lines)) + return not lines.isdisjoint(modified_lines) + + @property + def has_clang_files(self): + """ + Check if this revision has any file that might + be a C/C++ file + """ + + def _is_clang(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.cpp_extensions + + return any(_is_clang(f) for f in self.files) + + @property + def has_clang_header_files(self): + """ + Check if this revision has any file that might + be a C/C++ header file + """ + + def _is_clang_header(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.cpp_header_extensions + + return any(_is_clang_header(f) for f in self.files) + + @property + def has_idl_files(self): + """ + Check if this revision has any idl files + """ + + def _is_idl(filename): + _, ext = os.path.splitext(filename) + return ext.lower() in settings.idl_extensions + + return any(_is_idl(f) for f in self.files) + + @property + def is_blacklisted(self): + raise NotImplementedError + + def add_improvement_patch(self, analyzer, content): + """ + Save an improvement patch, and make it available + as a Taskcluster artifact + """ + assert isinstance(content, str) + assert len(content) > 0 + self.improvement_patches.append(ImprovementPatch(analyzer, repr(self), content)) + + def reset(self): + """ + Reset temporary data in BEFORE mode + * improvement patches + """ + self.improvement_patches = [] + + @property + def bugzilla_id(self): + raise NotImplementedError + + @property + def title(self): + raise NotImplementedError + + def as_dict(self): + """ + Outputs a serializable representation of this revision + """ + raise NotImplementedError + + @staticmethod + def from_try_task( + try_task: dict, decision_task: dict, phabricator: PhabricatorAPI = None + ): + """ + Load identifiers from Phabricator, using the remote task description + """ + from code_review_bot.revisions.phabricator import PhabricatorRevision + + # Load build target phid from the task env + code_review = try_task["extra"]["code-review"] + + if phabricator: + return PhabricatorRevision.from_try_task( + code_review, decision_task, phabricator + ) + else: + raise NotImplementedError diff --git a/bot/code_review_bot/revisions.py b/bot/code_review_bot/revisions/phabricator.py similarity index 71% rename from bot/code_review_bot/revisions.py rename to bot/code_review_bot/revisions/phabricator.py index d6b80ef3e..8970e19ae 100644 --- a/bot/code_review_bot/revisions.py +++ b/bot/code_review_bot/revisions/phabricator.py @@ -3,74 +3,27 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. import os -import random import time import urllib.parse -from datetime import timedelta from pathlib import Path import requests -import rs_parsepatch import structlog from libmozdata.phabricator import PhabricatorAPI -from code_review_bot import InvalidRepository, InvalidTrigger, Issue, stats, taskcluster +from code_review_bot import InvalidRepository, InvalidTrigger from code_review_bot.config import ( REPO_AUTOLAND, REPO_MOZILLA_CENTRAL, GetAppUserAgent, settings, ) -from code_review_bot.tasks.base import AnalysisTask +from code_review_bot.revisions.base import Revision logger = structlog.get_logger(__name__) -class ImprovementPatch: - """ - An improvement patch built by the bot - """ - - def __init__(self, analyzer, patch_name, content): - assert isinstance(analyzer, AnalysisTask) - - # Build name from analyzer and revision - self.analyzer = analyzer - self.name = f"{self.analyzer.name}-{patch_name}.diff" - self.content = content - self.url = None - self.path = None - - def __str__(self): - return f"{self.analyzer.name}: {self.url or self.path or self.name}" - - def write(self): - """ - Write patch on local FS, for dev & tests only - """ - self.path = os.path.join(settings.taskcluster.results_dir, self.name) - with open(self.path, "w") as f: - length = f.write(self.content) - logger.info("Improvement patch saved", path=self.path, length=length) - - def publish(self, days_ttl=30): - """ - Push through Taskcluster API to setup the content-type header - so it displays nicely in browsers - """ - assert ( - not settings.taskcluster.local - ), "Only publish on online Taskcluster tasks" - self.url = taskcluster.upload_artifact( - f"public/patch/{self.name}", - self.content.encode(), - content_type="text/plain; charset=utf-8", # Displays instead of download - ttl=timedelta(days=days_ttl - 1), - ) - logger.info("Improvement patch published", url=self.url) - - -class Revision: +class PhabricatorRevision(Revision): """ A Phabricator revision to analyze and report on """ @@ -91,9 +44,11 @@ def __init__( base_repository=None, base_repository_conf=None, phabricator_repository=None, - url=None, patch=None, + url=None, ): + super().__init__() + # Identification self.phabricator_id = phabricator_id self.phabricator_phid = phabricator_phid @@ -121,16 +76,8 @@ def __init__( # the phabricator repository payload for later identification self.phabricator_repository = phabricator_repository - # backend's returned URL to list or create issues linked to the revision in bulk (diff is optional) - self.issues_url = None - - # Patches built later on - self.improvement_patches = [] - # Patch analysis self.patch = patch - self.files = [] - self.lines = {} @property def namespaces(self): @@ -152,7 +99,7 @@ def repo_slug(url): if self.diff_phid: out.append(f"phabricator.diffphid.{self.diff_phid}") - # Revision indexes + # Phabricator revision indexes # Only head changeset is useful to uniquely identify the revision if self.head_repository and self.head_changeset: repo = repo_slug(self.head_repository) @@ -168,24 +115,6 @@ def from_autoland(self): def from_mozilla_central(self): return self.head_repository == REPO_MOZILLA_CENTRAL - @property - 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) - # Reset random module seed to prevent deterministic values after calling that function - random.seed(os.urandom(128)) - def __repr__(self): if self.diff_phid: # Most revisions have a Diff from Phabricator @@ -205,7 +134,6 @@ def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorA """ Load identifiers from Phabricator, using the remote task description """ - # Load build target phid from the task env code_review = try_task["extra"]["code-review"] build_target_phid = code_review.get("phabricator-diff") or code_review.get( @@ -289,7 +217,7 @@ def from_try_task(try_task: dict, decision_task: dict, phabricator: PhabricatorA # Build a revision without repositories as they are retrieved later # when analyzing the full task group - return Revision( + return PhabricatorRevision( phabricator_id=revision["id"], phabricator_phid=phid, diff_id=diff_id, @@ -334,7 +262,7 @@ def from_decision_task(task: dict, phabricator: PhabricatorAPI): head_changeset = task["payload"]["env"]["GECKO_HEAD_REV"] base_changeset = task["payload"]["env"]["GECKO_BASE_REV"] - return Revision( + return PhabricatorRevision( head_changeset=head_changeset, base_changeset=base_changeset, head_repository=head_repository, @@ -397,7 +325,7 @@ def from_phabricator_trigger(build_target_phid: str, phabricator: PhabricatorAPI ) logger.info("Found repository", name=repo_name, phid=repo_phid) - return Revision( + return PhabricatorRevision( phabricator_id=revision["id"], phabricator_phid=revision_phid, diff_id=diff["id"], @@ -412,29 +340,6 @@ def from_phabricator_trigger(build_target_phid: str, phabricator: PhabricatorAPI repository_try_name=repository.try_name, ) - def analyze_patch(self): - """ - Analyze loaded patch to extract modified lines - and statistics - """ - assert self.patch is not None, "Missing patch" - assert isinstance(self.patch, str), "Invalid patch type" - - # List all modified lines from current revision changes - patch_stats = rs_parsepatch.get_lines(self.patch) - assert len(patch_stats) > 0, "Empty patch" - - self.lines = {stat["filename"]: stat["added_lines"] for stat in patch_stats} - - # Shortcut to files modified - self.files = self.lines.keys() - - # Report nb of files and lines analyzed - stats.add_metric("analysis.files", len(self.files)) - stats.add_metric( - "analysis.lines", sum(len(line) for line in self.lines.values()) - ) - def load_file(self, path): """ Load a file content at current revision from remote HGMO @@ -467,70 +372,6 @@ def load_file(self, path): return content - def has_file(self, path): - """ - Check if the path is in this patch - """ - assert isinstance(path, str) - return path in self.files - - def contains(self, issue): - """ - Check if the issue (path+lines) is in this patch - """ - assert isinstance(issue, Issue) - - # Get modified lines for this issue - modified_lines = self.lines.get(issue.path) - if modified_lines is None: - return False - - # Empty line means full file - if issue.line is None: - return True - - # Detect if this issue is in the patch - lines = set(range(issue.line, issue.line + issue.nb_lines)) - return not lines.isdisjoint(modified_lines) - - @property - def has_clang_files(self): - """ - Check if this revision has any file that might - be a C/C++ file - """ - - def _is_clang(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.cpp_extensions - - return any(_is_clang(f) for f in self.files) - - @property - def has_clang_header_files(self): - """ - Check if this revision has any file that might - be a C/C++ header file - """ - - def _is_clang_header(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.cpp_header_extensions - - return any(_is_clang_header(f) for f in self.files) - - @property - def has_idl_files(self): - """ - Check if this revision has any idl files - """ - - def _is_idl(filename): - _, ext = os.path.splitext(filename) - return ext.lower() in settings.idl_extensions - - return any(_is_idl(f) for f in self.files) - @property def is_blacklisted(self): """Check if the revision author is in the black-list""" @@ -538,25 +379,11 @@ def is_blacklisted(self): if author is None: return False - logger.info("Revision from a blacklisted user", revision=self, author=author) + logger.info( + "Phabricator revision from a blacklisted user", revision=self, author=author + ) return True - def add_improvement_patch(self, analyzer, content): - """ - Save an improvement patch, and make it available - as a Taskcluster artifact - """ - assert isinstance(content, str) - assert len(content) > 0 - self.improvement_patches.append(ImprovementPatch(analyzer, repr(self), content)) - - def reset(self): - """ - Reset temporary data in BEFORE mode - * improvement patches - """ - self.improvement_patches = [] - @property def bugzilla_id(self): if self.revision is None: diff --git a/bot/code_review_bot/sources/phabricator.py b/bot/code_review_bot/sources/phabricator.py index 56087a12e..1e82b5c20 100644 --- a/bot/code_review_bot/sources/phabricator.py +++ b/bot/code_review_bot/sources/phabricator.py @@ -56,7 +56,7 @@ def __init__(self, request): self.actual_base_revision = None def __str__(self): - return f"Revision {self.revision_id} - {self.target_phid}" + return f"Phabricator revision {self.revision_id} - {self.target_phid}" class PhabricatorActions: @@ -121,17 +121,17 @@ def update_state(self, build): if self.is_visible(build): build.state = PhabricatorBuildState.Public build.revision_url = self.build_revision_url(build) - logger.info("Revision is public", build=str(build)) + logger.info("Phabricator revision is public", build=str(build)) # Check if the build has not expired if self.is_expired_build(build): build.state = PhabricatorBuildState.Expired - logger.info("Revision has expired", build=str(build)) + logger.info("Phabricator revision has expired", build=str(build)) elif retries_left <= 0: # Mark as secured when no retries are left build.state = PhabricatorBuildState.Secured - logger.info("Revision is marked as secure", build=str(build)) + logger.info("Phabricator revision is marked as secure", build=str(build)) else: # Enqueue back to retry later @@ -157,7 +157,9 @@ def is_visible(self, build): if projects.intersection(self.secure_projects): raise Exception("Secure revision") except Exception as e: - logger.info("Revision not accessible", build=str(build), error=str(e)) + logger.info( + "Phabricator revision not accessible", build=str(build), error=str(e) + ) return False return True diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index d4ded08e5..260a638e3 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -12,7 +12,7 @@ from code_review_bot import Level, stats from code_review_bot.analysis import ( - RevisionBuild, + PhabricatorRevisionBuild, publish_analysis_lando, publish_analysis_phabricator, ) @@ -20,7 +20,7 @@ from code_review_bot.config import settings from code_review_bot.mercurial import MercurialWorker, Repository, robust_checkout from code_review_bot.report.debug import DebugReporter -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision, Revision from code_review_bot.sources.phabricator import ( PhabricatorActions, PhabricatorBuildState, @@ -160,6 +160,10 @@ def ingest_revision(self, revision, group_id): """ Simpler workflow to ingest a revision """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) assert ( revision.from_autoland or revision.from_mozilla_central ), "Need a revision from autoland or mozilla-central" @@ -248,6 +252,10 @@ def start_analysis(self, revision): Apply a patch on a local clone and push to try to trigger a new Code review analysis """ logger.info("Starting revision analysis", revision=revision) + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) # Index ASAP Taskcluster task for this revision self.index(revision, state="analysis") @@ -276,7 +284,7 @@ def start_analysis(self, revision): ) # Initialize Phabricator build using revision - build = RevisionBuild(revision, self.phabricator) + build = PhabricatorRevisionBuild(revision, self.phabricator) # Copy internal Phabricator credentials to setup libmozevent phabricator = PhabricatorActions( @@ -428,7 +436,7 @@ def index(self, revision, **kwargs): """ Index current task on Taskcluster index """ - assert isinstance(revision, Revision) + assert isinstance(revision, Revision), "Must be a Revision instance" if settings.taskcluster.local or self.index_service is None: logger.info("Skipping taskcluster indexing", rev=str(revision), **kwargs) @@ -447,9 +455,10 @@ def index(self, revision, **kwargs): payload["try_task_id"] = settings.try_task_id payload["try_group_id"] = settings.try_group_id - # Always add the repository we are working on + # Add the repository we are working on for Phabricator revisions # This is mainly used by the frontend to list & filter diffs - payload["repository"] = revision.base_repository + if isinstance(revision, PhabricatorRevision): + payload["repository"] = revision.base_repository # Add restartable flag for monitoring payload["monitoring_restart"] = payload["state"] == "error" and payload.get( @@ -664,6 +673,10 @@ def update_status(self, revision, state): """ Update build status on HarborMaster """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) assert isinstance(state, BuildState) if not revision.build_target_phid: logger.info( @@ -684,6 +697,10 @@ def publish_link(self, revision: Revision, slug: str, name: str, url: str): """ Publish a link as a HarborMaster artifact """ + if not isinstance(revision, PhabricatorRevision): + raise NotImplementedError( + "Only Phabricator revisions are supported for now" + ) if not revision.build_target_phid: logger.info( "No build target found, skipping HarborMaster link creation", diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 30e194e2a..9d75614dc 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -332,10 +332,10 @@ def mock_revision(mock_phabricator, mock_try_task, mock_decision_task, mock_conf """ Mock a mercurial revision """ - from code_review_bot.revisions import Revision + from code_review_bot.revisions import PhabricatorRevision with mock_phabricator as api: - return Revision.from_try_task(mock_try_task, mock_decision_task, api) + return PhabricatorRevision.from_try_task(mock_try_task, mock_decision_task, api) @pytest.fixture @@ -343,10 +343,10 @@ def mock_revision_autoland(mock_phabricator, mock_autoland_task): """ Mock a mercurial revision from autoland repo """ - from code_review_bot.revisions import Revision + from code_review_bot.revisions import PhabricatorRevision with mock_phabricator as api: - return Revision.from_decision_task(mock_autoland_task, api) + return PhabricatorRevision.from_decision_task(mock_autoland_task, api) class Response: diff --git a/bot/tests/test_autoland.py b/bot/tests/test_autoland.py index e7a2275e4..7ca46bc41 100644 --- a/bot/tests/test_autoland.py +++ b/bot/tests/test_autoland.py @@ -2,7 +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.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision def test_revision(mock_autoland_task, mock_phabricator, mock_hgmo): @@ -11,7 +11,7 @@ def test_revision(mock_autoland_task, mock_phabricator, mock_hgmo): """ with mock_phabricator as api: - revision = Revision.from_decision_task(mock_autoland_task, api) + revision = PhabricatorRevision.from_decision_task(mock_autoland_task, api) assert revision.as_dict() == { "bugzilla_id": None, diff --git a/bot/tests/test_index.py b/bot/tests/test_index.py index 277c3b7c3..768bf06d8 100644 --- a/bot/tests/test_index.py +++ b/bot/tests/test_index.py @@ -5,10 +5,10 @@ from unittest import mock from code_review_bot.config import TaskCluster -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision -class MockRevision(Revision): +class MockPhabricatorRevision(PhabricatorRevision): """ Fake revision to easily set properties """ @@ -34,7 +34,7 @@ def test_taskcluster_index(mock_config, mock_workflow, mock_try_task): mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) mock_workflow.index_service = mock.Mock() - rev = MockRevision( + rev = MockPhabricatorRevision( namespaces=["mock.1234"], details={"id": "1234", "someData": "mock", "state": "done"}, repository="test-repo", @@ -84,7 +84,7 @@ def test_index_autoland( """ with mock_phabricator as api: - revision = Revision.from_decision_task(mock_autoland_task, api) + revision = PhabricatorRevision.from_decision_task(mock_autoland_task, api) mock_workflow.index_service = mock.Mock() mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) @@ -134,7 +134,7 @@ def test_index_phabricator( """ with mock_phabricator as api: - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( build_target_phid="PHID-HMBT-test", phabricator=api, ) @@ -199,7 +199,9 @@ def test_index_from_try( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) mock_workflow.index_service = mock.Mock() mock_config.taskcluster = TaskCluster("/tmp/dummy", "12345deadbeef", 0, False) diff --git a/bot/tests/test_analysis.py b/bot/tests/test_phabricator_analysis.py similarity index 97% rename from bot/tests/test_analysis.py rename to bot/tests/test_phabricator_analysis.py index da0a50b7e..9eb499df8 100644 --- a/bot/tests/test_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -7,7 +7,7 @@ from code_review_bot import mercurial from code_review_bot.config import RepositoryConf -from code_review_bot.revisions import Revision +from code_review_bot.revisions import PhabricatorRevision from code_review_bot.sources.phabricator import PhabricatorActions @@ -18,7 +18,7 @@ def test_revision(mock_phabricator): """ with mock_phabricator as api: - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( build_target_phid="PHID-HMBT-test", phabricator=api, ) @@ -93,7 +93,7 @@ def mock_hgrun(cmd): with mock_phabricator as api: mock_workflow.phabricator = api - revision = Revision.from_phabricator_trigger( + revision = PhabricatorRevision.from_phabricator_trigger( build_target_phid="PHID-HMBT-test", phabricator=api, ) diff --git a/bot/tests/test_reporter_phabricator.py b/bot/tests/test_reporter_phabricator.py index 3a9438f75..9fa67ccea 100644 --- a/bot/tests/test_reporter_phabricator.py +++ b/bot/tests/test_reporter_phabricator.py @@ -13,7 +13,7 @@ from code_review_bot import Level from code_review_bot.report.phabricator import PhabricatorReporter -from code_review_bot.revisions import ImprovementPatch, Revision +from code_review_bot.revisions import ImprovementPatch, PhabricatorRevision from code_review_bot.tasks.clang_format import ClangFormatIssue, ClangFormatTask from code_review_bot.tasks.clang_tidy import ClangTidyIssue, ClangTidyTask from code_review_bot.tasks.clang_tidy_external import ( @@ -270,7 +270,9 @@ def test_phabricator_clang_tidy( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "another_test.cpp": [41, 42, 43] @@ -306,7 +308,9 @@ def test_phabricator_clang_format( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.cpp": [41, 42, 43], @@ -348,7 +352,9 @@ def test_phabricator_mozlint( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "python/test.py": [41, 42, 43], @@ -437,7 +443,9 @@ def test_phabricator_coverage( Test Phabricator reporter publication on a mock coverage issue """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -503,7 +511,9 @@ def raise_404(*args, **kwargs): raise HTTPError(response=resp_mock) with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -537,7 +547,9 @@ def test_phabricator_clang_tidy_and_coverage( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -661,7 +673,9 @@ def test_phabricator_analyzers( api.comment = unittest.mock.Mock(return_value=True) # Always use the same setup, only varies the analyzers - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = {"test.cpp": [0, 41, 42, 43], "dom/test.cpp": [42]} revision.id = 52 reporter = PhabricatorReporter( @@ -745,7 +759,9 @@ def test_phabricator_clang_tidy_build_error( from code_review_bot import Level with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.cpp": [41, 42, 43] @@ -803,7 +819,9 @@ def test_full_file( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "xx.cpp": [123, 124, 125] @@ -865,7 +883,9 @@ def test_task_failures(mock_phabricator, phab, mock_try_task, mock_decision_task """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.id = 52 reporter = PhabricatorReporter({"analyzers": ["clang-tidy"]}, api=api) @@ -890,7 +910,9 @@ def test_extra_errors( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = {"path/to/file.py": [1, 2, 3]} revision.files = ["path/to/file.py"] revision.id = 52 @@ -981,7 +1003,9 @@ def test_phabricator_notices(mock_phabricator, phab, mock_try_task, mock_decisio """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.rst": [41, 42, 43], @@ -1029,7 +1053,9 @@ def test_phabricator_tgdiff(mock_phabricator, phab, mock_try_task, mock_decision """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.rst": [41, 42, 43], @@ -1065,7 +1091,9 @@ def test_phabricator_external_tidy( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "another_test.cpp": [41, 42, 43] @@ -1116,7 +1144,9 @@ def test_phabricator_newer_diff( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -1194,7 +1224,9 @@ def test_phabricator_former_diff_comparison( """ with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0], @@ -1342,7 +1374,9 @@ def test_phabricator_before_after_comment( mock_taskcluster_config.secrets = {"BEFORE_AFTER_RATIO": 1} with mock_phabricator as api: - revision = Revision.from_try_task(mock_try_task, mock_decision_task, api) + revision = PhabricatorRevision.from_try_task( + mock_try_task, mock_decision_task, api + ) revision.lines = { # Add dummy lines diff "test.txt": [0],