diff --git a/bot/code_review_bot/mercurial.py b/bot/code_review_bot/mercurial.py index 8a0a764b2..ae0b37e99 100644 --- a/bot/code_review_bot/mercurial.py +++ b/bot/code_review_bot/mercurial.py @@ -5,7 +5,6 @@ import atexit import fcntl import io -import json import os import tempfile import time @@ -13,33 +12,19 @@ import hglib import requests -import rs_parsepatch import structlog from libmozdata.lando import LandoCommitMapAPI, LandoMissingCommit from libmozdata.phabricator import PhabricatorPatch -from code_review_bot.sources.phabricator import PhabricatorBuild +from code_review_bot.vcs import BaseRepository, BaseWorker logger = structlog.get_logger(__name__) -TREEHERDER_URL = "https://treeherder.mozilla.org/#/jobs?repo={}&revision={}" DEFAULT_AUTHOR = "code review bot " # On build failure, check try status until available every 5 minutes and up to 24h TRY_STATUS_URL = "https://treestatus.prod.lando.prod.cloudops.mozgcp.net/trees/try" TRY_STATUS_DELAY = 5 * 60 TRY_STATUS_MAX_WAIT = 24 * 60 * 60 -# Number of allowed retries on an unexpected push fail -MAX_PUSH_RETRIES = 4 -# Wait successive exponential delays: 6sec, 36sec, 3.6min, 21.6min -PUSH_RETRY_EXPONENTIAL_DELAY = 6 - -logger = structlog.get_logger(__name__) - - -class RetryNeeded(Exception): - """ - Raised when retrying a mercurial build is needed - """ def hg_run(cmd): @@ -146,32 +131,23 @@ def robust_checkout( hg_run(cmd) -class Repository: +class MercurialRepository(BaseRepository): """ A Mercurial repository with its try server credentials """ + DEFAULT_REVISION = "default" + def __init__(self, config, cache_root): - assert isinstance(config, dict) - self.name = config["name"] - self.url = config["url"] - self.dir = os.path.join(cache_root, config["name"]) + super().__init__(config, cache_root) self.share_base_dir = os.path.join(cache_root, f"{config['name']}-shared") self.checkout_mode = config.get("checkout", "batch") self.batch_size = config.get("batch_size", 10000) - self.try_url = config["try_url"] - self.try_name = config.get("try_name", "try") - self.default_revision = config.get("default_revision", "default") - - # Apply patches to the latest revision when `True`. - self.use_latest_revision = config.get("use_latest_revision", False) # Crash when configuration requests try syntax if config.get("try_mode") == "syntax": raise Exception("Try syntax mode is deprecated") - self._repo = None - # Write ssh key from secret _, self.ssh_key_path = tempfile.mkstemp(suffix=".key") with open(self.ssh_key_path, "w") as f: @@ -190,9 +166,6 @@ def __init__(self, config, cache_root): # Remove key when finished atexit.register(self.end_of_life) - def __str__(self): - return self.name - def end_of_life(self): os.unlink(self.ssh_key_path) logger.info("Removed ssh key") @@ -276,57 +249,11 @@ def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: # Base revision may reference a Git hash on new repositories return self.get_mercurial_base_hash(base_rev_hash) - def apply_build(self, build): - """ - Apply a stack of patches to mercurial repo - and commit them one by one - """ - assert isinstance(build, PhabricatorBuild) - assert len(build.stack) > 0, "No patches to apply" - assert all(map(lambda p: isinstance(p, PhabricatorPatch), build.stack)) - - # Find the first unknown base revision - needed_stack = [] - for patch in reversed(build.stack): - # Skip already merged patches - if patch.merged: - logger.info( - f"Skip applying patch {patch.id} as it's already been merged upstream" - ) - continue - - # Add the patch into the stack only if not already merged ! - needed_stack.insert(0, patch) - - # Stop as soon as a base revision is available - if self.has_revision(patch.base_revision): - logger.info(f"Stopping at revision {patch.base_revision}") - break - - if not needed_stack: - logger.info("All the patches are already applied") - return - - hg_base = self.get_base_identifier(needed_stack) - - # When base revision is missing, update to default revision - build.base_revision = hg_base - build.missing_base_revision = not self.has_revision(hg_base) - if build.missing_base_revision: - logger.warning( - "Missing base revision from Phabricator", - revision=hg_base, - fallback=self.default_revision, - ) - hg_base = self.default_revision - - # Store the actual base revision we used - build.actual_base_revision = hg_base - - # Update the repo to base revision + def checkout_base(self, base): + """Update the local checkout to the base revision""" try: - logger.info(f"Updating repo to revision {hg_base}") - self.repo.update(rev=hg_base, clean=True) + logger.info(f"Updating repo to revision {base}") + self.repo.update(rev=base, clean=True) # See if the repo is clean repo_status = self.repo.status( @@ -335,111 +262,79 @@ def apply_build(self, build): if len(repo_status) != 0: logger.warn( "Repo is dirty!", - revision=hg_base, + revision=base, repo=self.name, repo_status=repo_status, ) except hglib.error.CommandError: - raise Exception(f"Failed to update to revision {hg_base}") - - # In this case revision is `hg_base` - logger.info("Updated repo", revision=hg_base, repo=self.name) - - def get_author(commit): - """Helper to build a mercurial author from Phabricator data""" - author = commit.get("author") - if author is None: - return DEFAULT_AUTHOR - if author["name"] and author["email"]: - # Build clean version without quotes - return f"{author['name']} <{author['email']}>" - return author["raw"] - - for patch in needed_stack: - if patch.commits: - # Use the first commit only - commit = patch.commits[0] - message = "{}\n".format(commit["message"]) - user = get_author(commit) - else: - # We should always have some commits here - logger.warning("Missing commit on patch", id=patch.id) - message = "" - user = DEFAULT_AUTHOR - message += f"Differential Diff: {patch.phid}" - - logger.info("Applying patch", phid=patch.phid, message=message) - patches = io.BytesIO(patch.patch.encode("utf-8")) - try: - self.repo.import_( - patches=patches, + raise Exception(f"Failed to update to revision {base}") + + # In this case revision is `base` + logger.info("Updated repo", revision=base, repo=self.name) + + @staticmethod + def get_author(commit): + """Helper to build a mercurial author from Phabricator data""" + author = commit.get("author") + if author is None: + return DEFAULT_AUTHOR + if author["name"] and author["email"]: + # Build clean version without quotes + return f"{author['name']} <{author['email']}>" + return author["raw"] + + def apply_patch(self, patch, message, commit): + """Apply a single patch on the local checkout and commit it""" + user = self.get_author(commit) if commit else DEFAULT_AUTHOR + + patches = io.BytesIO(patch.patch.encode("utf-8")) + try: + self.repo.import_( + patches=patches, + message=message.encode("utf-8"), + user=user.encode("utf-8"), + similarity=95, + ) + except hglib.error.CommandError as e: + logger.warning( + ( + f"Mercurial command from hglib failed: {e}. " + "Retrying with --config ui.patch=patch." + ), + phid=patch.phid, + exc_info=True, + ) + patches.seek(0) + # Same method as repo.import_() but with the extra argument "--config ui.patch=patch". + # https://repo.mercurial-scm.org/python-hglib/file/484b56ac4aec/hglib/client.py#l959 + self.repo.rawcommand( + hglib.util.cmdbuilder( + b"import", message=message.encode("utf-8"), user=user.encode("utf-8"), similarity=95, + config="ui.patch=patch", + *patches, ) - except hglib.error.CommandError as e: - logger.warning( - ( - f"Mercurial command from hglib failed: {e}. " - "Retrying with --config ui.patch=patch." - ), - phid=patch.phid, - exc_info=True, - ) - patches.seek(0) - # Same method as repo.import_() but with the extra argument "--config ui.patch=patch". - # https://repo.mercurial-scm.org/python-hglib/file/484b56ac4aec/hglib/client.py#l959 - self.repo.rawcommand( - hglib.util.cmdbuilder( - b"import", - message=message.encode("utf-8"), - user=user.encode("utf-8"), - similarity=95, - config="ui.patch=patch", - *patches, - ) - ) - # When using an external patch util mercurial won't automatically handle add/remove/renames - self.repo.rawcommand( - hglib.util.cmdbuilder( - b"addremove", - similarity=95, - ) - ) - except Exception as e: - logger.info( - f"Failed to apply patch: {e}", - phid=patch.phid, - exc_info=True, + ) + # When using an external patch util mercurial won't automatically handle add/remove/renames + self.repo.rawcommand( + hglib.util.cmdbuilder( + b"addremove", + similarity=95, ) - raise - - def add_try_commit(self, build): - """ - Build and commit the file configuring try - with try_task_config.json and the code-review workflow parameters in JSON - """ - path = os.path.join(self.dir, "try_task_config.json") - config = { - "version": 2, - "parameters": { - "target_tasks_method": "codereview", - "optimize_target_tasks": True, - "phabricator_diff": build.target_phid, - }, - } - diff_phid = build.stack[-1].phid - - if build.revision_url: - message = f"try_task_config for {build.revision_url}" - else: - message = "try_task_config for code-review" - message += f"\nDifferential Diff: {diff_phid}" + ) + except Exception as e: + logger.info( + f"Failed to apply patch: {e}", + phid=patch.phid, + exc_info=True, + ) + raise - # Write content as json and commit it - with open(path, "w") as f: - json.dump(config, f, sort_keys=True, indent=4) + def commit_try_task_config(self, path, message): + """Commit the try_task_config.json file as the bot""" self.repo.add(path.encode("utf-8")) self.repo.commit(message=message, user=DEFAULT_AUTHOR) @@ -457,6 +352,10 @@ def push_to_try(self): ) return tip + def revision_id(self, tip): + """Extract the revision identifier from a push_to_try result""" + return tip.node.decode("utf-8") + def clean(self): """ Steps to clean the mercurial repo @@ -478,11 +377,16 @@ def clean(self): self.repo.pull() -class MercurialWorker: +class MercurialWorker(BaseWorker): """ Mercurial worker maintaining several local clones """ + VCS_ERROR = hglib.error.CommandError + VCS_NAME = "Mercurial" + FAILURE_MODE = "fail:mercurial" + REPOSITORY_CLASS = MercurialRepository + ELIGIBLE_RETRY_ERRORS = [ error.lower() for error in [ @@ -492,67 +396,6 @@ class MercurialWorker: ] ] - def __init__( - self, - skippable_files=[], - ): - self.skippable_files = skippable_files - - def run(self, repository, build): - """ - Apply the stack of patches from the build, handling retries - in case of try server errors - """ - while build.retries <= MAX_PUSH_RETRIES: - start = time.time() - - if build.retries: - logger.warning( - "Trying to apply build's diff after a remote push error " - f"[{build.retries}/{MAX_PUSH_RETRIES}]" - ) - - try: - return self.handle_build(repository, build) - except RetryNeeded: - build.retries += 1 - - if build.retries > MAX_PUSH_RETRIES: - error_log = "Max number of retries has been reached pushing the build to try repository" - logger.warn("Mercurial error on diff", error=error_log, build=build) - return ( - "fail:mercurial", - build, - {"message": error_log, "duration": time.time() - start}, - ) - - # Ensure try is opened - self.wait_try_available() - - # Wait an exponential time before retrying the build - delay = PUSH_RETRY_EXPONENTIAL_DELAY**build.retries - logger.info( - f"An error occurred pushing the build to try, retrying after {delay}s" - ) - time.sleep(delay) - - def is_commit_skippable(self, build): - def get_files_touched_in_diff(rawdiff): - patched = [] - for parsed_diff in rs_parsepatch.get_diffs(rawdiff): - # filename is sometimes of format 'test.txt Tue Feb 05 17:23:40 2019 +0100' - # fix after https://github.com/mozilla/rust-parsepatch/issues/61 - if "filename" in parsed_diff: - filename = parsed_diff["filename"].split(" ")[0] - patched.append(filename) - return patched - - return any( - patched_file in self.skippable_files - for rev in build.stack - for patched_file in get_files_touched_in_diff(rev.patch) - ) - def wait_try_available(self): """ Wait until try status is "open" @@ -582,84 +425,9 @@ def get_status(): ) time.sleep(TRY_STATUS_DELAY) - def is_eligible_for_retry(self, error): - """ - Given a Mercurial error message, if it's an error likely due to a - temporary connection problem, consider it as eligible for retry. - """ - error = error.lower() - return any( - eligible_message in error for eligible_message in self.ELIGIBLE_RETRY_ERRORS - ) - - def handle_build(self, repository, build): - """ - Try to load and apply a diff on local clone - If successful, push to try and send a treeherder link - In case of an unexpected push failure, retry up to MAX_PUSH_RETRIES - times by putting the build task back in the queue - - If the build fail, send a unit result with a warning message - """ - assert isinstance(repository, Repository) - start = time.time() - - try: - # Start by cleaning the repo - repository.clean() - - # First apply patches on local repo - repository.apply_build(build) - - # Check Eligibility: some commits don't need to be pushed to try. - if self.is_commit_skippable(build): - logger.info("This patch series is ineligible for automated try push") - return ( - "fail:ineligible", - build, - { - "message": "Modified files match skippable internal configuration files", - "duration": time.time() - start, - }, - ) - - # Configure the try task - repository.add_try_commit(build) - - # Then push that stack on try - tip = repository.push_to_try() - logger.info("Diff has been pushed !") - - # Publish Treeherder link - uri = TREEHERDER_URL.format(repository.try_name, tip.node.decode("utf-8")) - except hglib.error.CommandError as e: - # Format nicely the error log - error_log = e.err - if isinstance(error_log, bytes): - error_log = error_log.decode("utf-8") - - if self.is_eligible_for_retry(error_log): - raise RetryNeeded - - logger.warn( - "Mercurial error on diff", error=error_log, args=e.args, build=build - ) - return ( - "fail:mercurial", - build, - {"message": error_log, "duration": time.time() - start}, - ) - - except Exception as e: - logger.warn("Failed to process diff", error=e, build=build) - return ( - "fail:general", - build, - {"message": str(e), "duration": time.time() - start}, - ) - - return ( - "success", - build, - {"treeherder_url": uri, "revision": tip.node.decode("utf-8")}, - ) + def format_error(self, error): + """Extract a readable error log from a Mercurial exception""" + error_log = error.err + if isinstance(error_log, bytes): + error_log = error_log.decode("utf-8") + return error_log diff --git a/bot/code_review_bot/vcs.py b/bot/code_review_bot/vcs.py new file mode 100644 index 000000000..3dd5d9d8e --- /dev/null +++ b/bot/code_review_bot/vcs.py @@ -0,0 +1,364 @@ +# 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 json +import os +import time + +import rs_parsepatch +import structlog +from libmozdata.phabricator import PhabricatorPatch + +from code_review_bot.sources.phabricator import PhabricatorBuild + +logger = structlog.get_logger(__name__) + +TREEHERDER_URL = "https://treeherder.mozilla.org/#/jobs?repo={}&revision={}" + +# Number of allowed retries on an unexpected push fail +MAX_PUSH_RETRIES = 4 +# Wait successive exponential delays: 6sec, 36sec, 3.6min, 21.6min +PUSH_RETRY_EXPONENTIAL_DELAY = 6 + + +class RetryNeeded(Exception): + """ + Raised when retrying a build is needed + """ + + +class BaseRepository: + """ + A repository with its try server credentials, independent of the + version control system. + + Subclasses provide the VCS specific operations: cloning, revision + lookups, applying a patch as a commit and pushing to the try server. + """ + + # Revision to apply patches on when the base revision is unknown + DEFAULT_REVISION = None + + def __init__(self, config, cache_root): + assert isinstance(config, dict) + self.name = config["name"] + self.url = config["url"] + self.dir = os.path.join(cache_root, config["name"]) + self.try_url = config["try_url"] + self.try_name = config.get("try_name", "try") + self.default_revision = config.get("default_revision", self.DEFAULT_REVISION) + + # Apply patches to the latest revision when `True`. + self.use_latest_revision = config.get("use_latest_revision", False) + + self._repo = None + + def __str__(self): + return self.name + + def clone(self): + """Clone the repository in the local cache directory, or refresh an + existing clone, so that a stack of patches can be applied on it""" + raise NotImplementedError + + def has_revision(self, revision): + """Check if a revision is directly available in the local repository""" + raise NotImplementedError + + def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: + """Return the revision identifier the stack of patches must be + applied against""" + raise NotImplementedError + + def checkout_base(self, base): + """Update the local checkout to the base revision""" + raise NotImplementedError + + def apply_patch(self, patch: PhabricatorPatch, message, commit): + """Apply a single patch on the local checkout and commit it, + using the author from the Phabricator commit data""" + raise NotImplementedError + + def commit_try_task_config(self, path, message): + """Commit the try_task_config.json file as the bot""" + raise NotImplementedError + + def push_to_try(self): + """Push the locally applied stack to the remote try repository and + return the pushed tip, in the format accepted by revision_id()""" + raise NotImplementedError + + def revision_id(self, tip): + """Extract the revision identifier from a push_to_try result""" + raise NotImplementedError + + def clean(self): + """Restore the local repository to a pristine state, dropping the + commits applied by a previous build and pulling remote updates""" + raise NotImplementedError + + def apply_build(self, build): + """ + Apply a stack of patches to the local repository + and commit them one by one + """ + assert isinstance(build, PhabricatorBuild) + assert len(build.stack) > 0, "No patches to apply" + assert all(map(lambda p: isinstance(p, PhabricatorPatch), build.stack)) + + # Find the first unknown base revision + needed_stack = [] + for patch in reversed(build.stack): + # Skip already merged patches + if patch.merged: + logger.info( + f"Skip applying patch {patch.id} as it's already been merged upstream" + ) + continue + + # Add the patch into the stack only if not already merged ! + needed_stack.insert(0, patch) + + # Stop as soon as a base revision is available + if self.has_revision(patch.base_revision): + logger.info(f"Stopping at revision {patch.base_revision}") + break + + if not needed_stack: + logger.info("All the patches are already applied") + return + + base = self.get_base_identifier(needed_stack) + + # When base revision is missing, update to default revision + build.base_revision = base + build.missing_base_revision = not self.has_revision(base) + if build.missing_base_revision: + logger.warning( + "Missing base revision from Phabricator", + revision=base, + fallback=self.default_revision, + ) + base = self.default_revision + + # Store the actual base revision we used + build.actual_base_revision = base + + self.checkout_base(base) + + for patch in needed_stack: + if patch.commits: + # Use the first commit only + commit = patch.commits[0] + message = "{}\n".format(commit["message"]) + else: + # We should always have some commits here + logger.warning("Missing commit on patch", id=patch.id) + commit = None + message = "" + message += f"Differential Diff: {patch.phid}" + + logger.info("Applying patch", phid=patch.phid, message=message) + self.apply_patch(patch, message, commit) + + def add_try_commit(self, build): + """ + Build and commit the file configuring try + with try_task_config.json and the code-review workflow parameters in JSON + """ + path = os.path.join(self.dir, "try_task_config.json") + config = { + "version": 2, + "parameters": { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + "phabricator_diff": build.target_phid, + }, + } + diff_phid = build.stack[-1].phid + + if build.revision_url: + message = f"try_task_config for {build.revision_url}" + else: + message = "try_task_config for code-review" + message += f"\nDifferential Diff: {diff_phid}" + + # Write content as json and commit it + with open(path, "w") as f: + json.dump(config, f, sort_keys=True, indent=4) + self.commit_try_task_config(path, message) + + +class BaseWorker: + """ + Worker maintaining several local clones and applying builds on them, + independent of the version control system. + """ + + # Exception raised by the VCS layer on a failing command + VCS_ERROR = () + # Human readable VCS name, used in logs + VCS_NAME = None + # Worker output mode on a VCS failure + FAILURE_MODE = None + # Repository class handled by this worker + REPOSITORY_CLASS = BaseRepository + # Lowercase error messages elligible for a push retry + ELIGIBLE_RETRY_ERRORS = [] + + def __init__( + self, + skippable_files=[], + ): + self.skippable_files = skippable_files + + def run(self, repository, build): + """ + Apply the stack of patches from the build, handling retries + in case of try server errors + """ + while build.retries <= MAX_PUSH_RETRIES: + start = time.time() + + if build.retries: + logger.warning( + "Trying to apply build's diff after a remote push error " + f"[{build.retries}/{MAX_PUSH_RETRIES}]" + ) + + try: + return self.handle_build(repository, build) + except RetryNeeded: + build.retries += 1 + + if build.retries > MAX_PUSH_RETRIES: + error_log = "Max number of retries has been reached pushing the build to try repository" + logger.warn( + f"{self.VCS_NAME} error on diff", error=error_log, build=build + ) + return ( + self.FAILURE_MODE, + build, + {"message": error_log, "duration": time.time() - start}, + ) + + # Ensure the remote is available + self.wait_try_available() + + # Wait an exponential time before retrying the build + delay = PUSH_RETRY_EXPONENTIAL_DELAY**build.retries + logger.info( + f"An error occurred pushing the build to try, retrying after {delay}s" + ) + time.sleep(delay) + + def is_commit_skippable(self, build): + def get_files_touched_in_diff(rawdiff): + patched = [] + for parsed_diff in rs_parsepatch.get_diffs(rawdiff): + # filename is sometimes of format 'test.txt Tue Feb 05 17:23:40 2019 +0100' + # fix after https://github.com/mozilla/rust-parsepatch/issues/61 + if "filename" in parsed_diff: + filename = parsed_diff["filename"].split(" ")[0] + patched.append(filename) + return patched + + return any( + patched_file in self.skippable_files + for rev in build.stack + for patched_file in get_files_touched_in_diff(rev.patch) + ) + + def wait_try_available(self): + """ + Hook called before retrying a failed push, doing nothing by default + """ + + def is_eligible_for_retry(self, error): + """ + Given a VCS error message, if it's an error likely due to a + temporary connection problem, consider it as eligible for retry. + """ + error = error.lower() + return any( + eligible_message in error for eligible_message in self.ELIGIBLE_RETRY_ERRORS + ) + + def format_error(self, error): + """Extract a readable error log from a VCS exception""" + raise NotImplementedError + + def handle_build(self, repository, build): + """ + Try to load and apply a diff on local clone + If successful, push to try and send a treeherder link + In case of an unexpected push failure, retry up to MAX_PUSH_RETRIES + times by putting the build task back in the queue + + If the build fail, send a unit result with a warning message + """ + assert isinstance(repository, self.REPOSITORY_CLASS) + start = time.time() + + try: + # Start by cleaning the repo + repository.clean() + + # First apply patches on local repo + repository.apply_build(build) + + # Check Eligibility: some commits don't need to be pushed to try. + if self.is_commit_skippable(build): + logger.info("This patch series is ineligible for automated try push") + return ( + "fail:ineligible", + build, + { + "message": "Modified files match skippable internal configuration files", + "duration": time.time() - start, + }, + ) + + # Configure the try task + repository.add_try_commit(build) + + # Then push that stack on try + tip = repository.push_to_try() + logger.info("Diff has been pushed !") + + # Publish Treeherder link + uri = TREEHERDER_URL.format( + repository.try_name, repository.revision_id(tip) + ) + except self.VCS_ERROR as e: + error_log = self.format_error(e) + + if self.is_eligible_for_retry(error_log): + raise RetryNeeded + + logger.warn( + f"{self.VCS_NAME} error on diff", + error=error_log, + args=e.args, + build=build, + ) + return ( + self.FAILURE_MODE, + build, + {"message": error_log, "duration": time.time() - start}, + ) + + except Exception as e: + logger.warn("Failed to process diff", error=e, build=build) + return ( + "fail:general", + build, + {"message": str(e), "duration": time.time() - start}, + ) + + return ( + "success", + build, + {"treeherder_url": uri, "revision": repository.revision_id(tip)}, + ) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 2655f6fd0..c25a1d958 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -19,7 +19,11 @@ from code_review_bot.backend import BackendAPI from code_review_bot.config import settings from code_review_bot.git import git_clone -from code_review_bot.mercurial import MercurialWorker, Repository, robust_checkout +from code_review_bot.mercurial import ( + MercurialRepository, + MercurialWorker, + robust_checkout, +) from code_review_bot.report.debug import DebugReporter from code_review_bot.revisions import GithubRevision, PhabricatorRevision, Revision from code_review_bot.sources.phabricator import ( @@ -296,7 +300,7 @@ def start_analysis(self, revision): ) # Initialize mercurial repository - repository = Repository( + repository = MercurialRepository( config={ "name": revision.base_repository_conf.name, "try_name": revision.base_repository_conf.try_name, diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 7be58736a..da6d97580 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -25,7 +25,7 @@ from code_review_bot import Level, stats from code_review_bot.backend import BackendAPI from code_review_bot.config import GetAppUserAgent, settings -from code_review_bot.mercurial import Repository +from code_review_bot.mercurial import MercurialRepository from code_review_bot.sources.phabricator import ( PhabricatorActions, PhabricatorBuild, @@ -1162,7 +1162,7 @@ def mock_mc(tmpdir): "try_url": "http://mozilla-central/try", "batch_size": 100, } - repo = Repository(config, tmpdir.realpath()) + repo = MercurialRepository(config, tmpdir.realpath()) repo._repo = build_repository(tmpdir, "mozilla-central") repo.clone = MagicMock(side_effect=lambda: True) return repo @@ -1181,7 +1181,7 @@ def mock_nss(tmpdir): "try_url": "http://nss/try", "batch_size": 100, } - repo = Repository(config, tmpdir.realpath()) + repo = MercurialRepository(config, tmpdir.realpath()) repo._repo = build_repository(tmpdir, "nss") repo.clone = MagicMock(side_effect=lambda: True) return repo diff --git a/bot/tests/test_mercurial.py b/bot/tests/test_mercurial.py index fd637824e..443da766d 100644 --- a/bot/tests/test_mercurial.py +++ b/bot/tests/test_mercurial.py @@ -681,11 +681,11 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): with PhabricatorMock as phab: phab.load_patches_stack(build) - from code_review_bot import mercurial + from code_review_bot import mercurial, vcs - mercurial.MAX_PUSH_RETRIES = 1 + vcs.MAX_PUSH_RETRIES = 1 mercurial.TRY_STATUS_URL = "http://test.status/try" - mercurial.PUSH_RETRY_EXPONENTIAL_DELAY = 0 + vcs.PUSH_RETRY_EXPONENTIAL_DELAY = 0 mercurial.TRY_STATUS_DELAY = 0 mercurial.TRY_STATUS_MAX_WAIT = 0 @@ -693,7 +693,7 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): "http://test.status/try", status=200, json={"result": {"status": "open"}} ) - repository_mock = MagicMock(spec=mercurial.Repository) + repository_mock = MagicMock(spec=mercurial.MercurialRepository) repository_mock.push_to_try.side_effect = [ hglib.error.CommandError( args=("push", "try_url"), @@ -703,6 +703,7 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): ), mock_mc.repo.tip(), ] + repository_mock.revision_id.side_effect = lambda tip: tip.node.decode("utf-8") repository_mock.try_name = "try" repository_mock.retries = 0 @@ -743,11 +744,11 @@ def test_push_failure_max_retries(PhabricatorMock, mock_mc, monkeypatch): with PhabricatorMock as phab: phab.load_patches_stack(build) - from code_review_bot import mercurial + from code_review_bot import mercurial, vcs - mercurial.MAX_PUSH_RETRIES = 2 + vcs.MAX_PUSH_RETRIES = 2 mercurial.TRY_STATUS_URL = "http://test.status/try" - mercurial.PUSH_RETRY_EXPONENTIAL_DELAY = 2 + vcs.PUSH_RETRY_EXPONENTIAL_DELAY = 2 mercurial.TRY_STATUS_DELAY = 0 mercurial.TRY_STATUS_MAX_WAIT = 0 @@ -755,7 +756,7 @@ def test_push_failure_max_retries(PhabricatorMock, mock_mc, monkeypatch): "http://test.status/try", status=200, json={"result": {"status": "open"}} ) - repository_mock = MagicMock(spec=mercurial.Repository) + repository_mock = MagicMock(spec=mercurial.MercurialRepository) repository_mock.push_to_try.side_effect = hglib.error.CommandError( args=("push", "try_url"), ret=1, @@ -799,11 +800,11 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): with PhabricatorMock as phab: phab.load_patches_stack(build) - from code_review_bot import mercurial + from code_review_bot import mercurial, vcs - mercurial.MAX_PUSH_RETRIES = 2 + vcs.MAX_PUSH_RETRIES = 2 mercurial.TRY_STATUS_URL = "http://test.status/try" - mercurial.PUSH_RETRY_EXPONENTIAL_DELAY = 2 + vcs.PUSH_RETRY_EXPONENTIAL_DELAY = 2 mercurial.TRY_STATUS_DELAY = 42 mercurial.TRY_STATUS_MAX_WAIT = 1 @@ -819,7 +820,7 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): "http://test.status/try", status=200, json={"result": {"status": "open"}} ) - repository_mock = MagicMock(spec=mercurial.Repository) + repository_mock = MagicMock(spec=mercurial.MercurialRepository) repository_mock.push_to_try.side_effect = [ hglib.error.CommandError( args=("push", "try_url"), @@ -829,6 +830,7 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): ), mock_mc.repo.tip(), ] + repository_mock.revision_id.side_effect = lambda tip: tip.node.decode("utf-8") repository_mock.try_name = "try" repository_mock.retries = 0