nf-core · Aratz · Oct 12, 2022 · Oct 11, 2022 · Oct 11, 2022 · Oct 11, 2022
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@
 - Don't remove local copy of modules repo, only update it with fetch ([#1881](https://github.com/nf-core/tools/pull/1881))
 - Add subworkflow commands create-test-yml, create and install ([#1897](https://github.com/nf-core/tools/pull/1897))
 - Update subworkflows install so it installs also imported modules and subworkflows ([#1904](https://github.com/nf-core/tools/pull/1904))
+- Improve test coverage of sync.py
 - `check_up_to_date()` function from `modules_json` also checks for subworkflows.
 
 ### Modules

diff --git a/nf_core/sync.py b/nf_core/sync.py
@@ -5,6 +5,7 @@
 import json
 import logging
 import os
+import re
 import shutil
 
 import git
@@ -69,7 +70,8 @@ def __init__(
         self.pipeline_dir = os.path.abspath(pipeline_dir)
         self.from_branch = from_branch
         self.original_branch = None
-        self.merge_branch = f"nf-core-template-merge-{nf_core.__version__}"
+        self.original_merge_branch = f"nf-core-template-merge-{nf_core.__version__}"
+        self.merge_branch = self.original_merge_branch
         self.made_changes = False
         self.make_pr = make_pr
         self.gh_pr_returned_data = {}
@@ -291,17 +293,18 @@ def create_merge_base_branch(self):
         # Check if branch exists already
         branch_list = [b.name for b in self.repo.branches]
         if self.merge_branch in branch_list:
-            original_merge_branch = self.merge_branch
-            # Try to create new branch with number at the end
-            # If <branch_name>-2 already exists, increase the number until branch is new
-            branch_no = 2
-            self.merge_branch = f"{original_merge_branch}-{branch_no}"
-            while self.merge_branch in branch_list:
-                branch_no += 1
-                self.merge_branch = f"{original_merge_branch}-{branch_no}"
-            log.info(
-                f"Branch already existed: '{original_merge_branch}', creating branch '{self.merge_branch}' instead."
+            merge_branch_format = re.compile(rf"{self.original_merge_branch}-(\d+)")
+            max_branch = max(
+                [1]
+                + [
+                    int(merge_branch_format.match(branch).groups()[0])
+                    for branch in branch_list
+                    if merge_branch_format.match(branch)
+                ]
             )
+            new_branch = f"{self.original_merge_branch}-{max_branch+1}"
+            log.info(f"Branch already existed: '{self.merge_branch}', creating branch '{new_branch}' instead.")
+            self.merge_branch = new_branch
 
         # Create new branch and checkout
         log.info(f"Checking out merge base branch '{self.merge_branch}'")
@@ -444,4 +447,4 @@ def reset_target_dir(self):
         try:
             self.repo.git.checkout(self.original_branch)
         except GitCommandError as e:
-            raise SyncException(f"Could not reset to original branch `{self.from_branch}`:\n{e}")
+            raise SyncException(f"Could not reset to original branch `{self.original_branch}`:\n{e}")
diff --git a/tests/test_sync.py b/tests/test_sync.py
@@ -9,6 +9,9 @@
 import unittest
 from unittest import mock
 
+import git
+import pytest
+
 import nf_core.create
 import nf_core.sync
 
@@ -26,6 +29,8 @@ def setUp(self):
             "testing", "test pipeline", "tester", outdir=self.pipeline_dir, plain=True
         )
         self.create_obj.init_pipeline()
+        self.remote_path = os.path.join(self.tmp_dir, "remote_repo")
+        self.remote_repo = git.Repo.init(self.remote_path, bare=True)
 
     def tearDown(self):
         if os.path.exists(self.tmp_dir):
@@ -35,11 +40,9 @@ def tearDown(self):
     def test_inspect_sync_dir_notgit(self, tmp_dir):
         """Try syncing an empty directory"""
         psync = nf_core.sync.PipelineSync(tmp_dir)
-        try:
+        with pytest.raises(nf_core.sync.SyncException) as exc_info:
             psync.inspect_sync_dir()
-            raise UserWarning("Should have hit an exception")
-        except nf_core.sync.SyncException as e:
-            assert "does not appear to be a git repository" in e.args[0]
+        assert "does not appear to be a git repository" in exc_info.value.args[0]
 
     def test_inspect_sync_dir_dirty(self):
         """Try syncing a pipeline with uncommitted changes"""
@@ -49,40 +52,33 @@ def test_inspect_sync_dir_dirty(self):
         # Try to sync, check we halt with the right error
         psync = nf_core.sync.PipelineSync(self.pipeline_dir)
         try:
-            psync.inspect_sync_dir()
-            raise UserWarning("Should have hit an exception")
-        except nf_core.sync.SyncException as e:
+            with pytest.raises(nf_core.sync.SyncException) as exc_info:
+                psync.inspect_sync_dir()
+            assert exc_info.value.args[0].startswith("Uncommitted changes found in pipeline directory!")
+        finally:
             os.remove(test_fn)
-            assert e.args[0].startswith("Uncommitted changes found in pipeline directory!")
-        except Exception as e:
-            os.remove(test_fn)
-            raise e
 
     def test_get_wf_config_no_branch(self):
         """Try getting a workflow config when the branch doesn't exist"""
         # Try to sync, check we halt with the right error
         psync = nf_core.sync.PipelineSync(self.pipeline_dir, from_branch="foo")
-        try:
+        with pytest.raises(nf_core.sync.SyncException) as exc_info:
             psync.inspect_sync_dir()
             psync.get_wf_config()
-            raise UserWarning("Should have hit an exception")
-        except nf_core.sync.SyncException as e:
-            assert e.args[0] == "Branch `foo` not found!"
+        assert exc_info.value.args[0] == "Branch `foo` not found!"
 
     def test_get_wf_config_missing_required_config(self):
         """Try getting a workflow config, then make it miss a required config option"""
         # Try to sync, check we halt with the right error
         psync = nf_core.sync.PipelineSync(self.pipeline_dir)
         psync.required_config_vars = ["fakethisdoesnotexist"]
-        try:
+        with pytest.raises(nf_core.sync.SyncException) as exc_info:
             psync.inspect_sync_dir()
             psync.get_wf_config()
-            raise UserWarning("Should have hit an exception")
-        except nf_core.sync.SyncException as e:
-            # Check that we did actually get some config back
-            assert psync.wf_config["params.validate_params"] == "true"
-            # Check that we raised because of the missing fake config var
-            assert e.args[0] == "Workflow config variable `fakethisdoesnotexist` not found!"
+        # Check that we did actually get some config back
+        assert psync.wf_config["params.validate_params"] == "true"
+        # Check that we raised because of the missing fake config var
+        assert exc_info.value.args[0] == "Workflow config variable `fakethisdoesnotexist` not found!"
 
     def test_checkout_template_branch(self):
         """Try checking out the TEMPLATE branch of the pipeline"""
@@ -91,6 +87,18 @@ def test_checkout_template_branch(self):
         psync.get_wf_config()
         psync.checkout_template_branch()
 
+    def test_checkout_template_branch_no_template(self):
+        """Try checking out the TEMPLATE branch of the pipeline when it does not exist"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+
+        psync.repo.delete_head("TEMPLATE")
+
+        with pytest.raises(nf_core.sync.SyncException) as exc_info:
+            psync.checkout_template_branch()
+        assert exc_info.value.args[0] == "Could not check out branch 'origin/TEMPLATE' or 'TEMPLATE'"
+
     def test_delete_template_branch_files(self):
         """Confirm that we can delete all files in the TEMPLATE branch"""
         psync = nf_core.sync.PipelineSync(self.pipeline_dir)
@@ -153,11 +161,64 @@ def test_push_template_branch_error(self):
         open(test_fn, "a").close()
         psync.commit_template_changes()
         # Try to push changes
-        try:
+        with pytest.raises(nf_core.sync.PullRequestException) as exc_info:
             psync.push_template_branch()
-            raise UserWarning("Should have hit an exception")
-        except nf_core.sync.PullRequestException as e:
-            assert e.args[0].startswith("Could not push TEMPLATE branch")
+        assert exc_info.value.args[0].startswith("Could not push TEMPLATE branch")
+
+    def test_create_merge_base_branch(self):
+        """Try creating a merge base branch"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+
+        psync.create_merge_base_branch()
+
+        assert psync.merge_branch in psync.repo.branches
+
+    def test_create_merge_base_branch_thrice(self):
+        """Try creating a merge base branch thrice
+
+        This is needed because the first time this function is called, the
+        merge branch does not exist yet (it is only created at the end of the
+        create_merge_base_branch function) and the if-statement is ignored.
+        Also, the second time this function is called, the existing merge
+        branch only has the base format, i.e. without the -{branch_no} at the
+        end, so it is needed to call it a third time to make sure this is
+        picked up.
+        """
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+
+        for _ in range(3):
+            psync.create_merge_base_branch()
+
+        assert psync.merge_branch in psync.repo.branches
+        for branch_no in [2, 3]:
+            assert f"{psync.original_merge_branch}-{branch_no}" in psync.repo.branches
+
+    def test_push_merge_branch(self):
+        """Try pushing merge branch"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+        psync.repo.create_remote("origin", self.remote_path)
+
+        psync.create_merge_base_branch()
+        psync.push_merge_branch()
+
+        assert psync.merge_branch in [b.name for b in self.remote_repo.branches]
+
+    def test_push_merge_branch_without_create_branch(self):
+        """Try pushing merge branch without creating first"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+        psync.repo.create_remote("origin", self.remote_path)
+
+        with pytest.raises(nf_core.sync.PullRequestException) as exc_info:
+            psync.push_merge_branch()
+        assert exc_info.value.args[0].startswith(f"Could not push branch '{psync.merge_branch}'")
 
     def mocked_requests_get(url, **kwargs):
         """Helper function to emulate POST requests responses from the web"""
@@ -175,10 +236,28 @@ def __init__(self, data, status_code):
             def json(self):
                 return self.data
 
-        url_template = "https://api.github.com/repos/{}/response/pulls?head=TEMPLATE&base=None"
-        if url == url_template.format("no_existing_pr"):
+        url_template = "https://api.github.com/repos/{}/response/"
+        if url == os.path.join(url_template.format("no_existing_pr"), "pulls?head=TEMPLATE&base=None"):
             response_data = []
             return MockResponse(response_data, 200)
+        elif url == os.path.join(url_template.format("list_prs"), "pulls"):
+            response_data = [
+                {
+                    "state": "closed",
+                    "head": {"ref": "nf-core-template-merge-2"},
+                    "base": {"ref": "master"},
+                    "html_url": "pr_url",
+                }
+            ] + [
+                {
+                    "state": "open",
+                    "head": {"ref": f"nf-core-template-merge-{branch_no}"},
+                    "base": {"ref": "master"},
+                    "html_url": "pr_url",
+                }
+                for branch_no in range(3, 7)
+            ]
+            return MockResponse(response_data, 200)
 
         return MockResponse({"html_url": url}, 404)
 
@@ -246,8 +325,99 @@ def test_make_pull_request_bad_response(self, mock_post, mock_get):
         psync.gh_username = "bad_url"
         psync.gh_repo = "bad_url/response"
         os.environ["GITHUB_AUTH_TOKEN"] = "test"
-        try:
+        with pytest.raises(nf_core.sync.PullRequestException) as exc_info:
             psync.make_pull_request()
-            raise UserWarning("Should have hit an exception")
-        except nf_core.sync.PullRequestException as e:
-            assert e.args[0].startswith("Something went badly wrong - GitHub API PR failed - got return code 404")
+        assert exc_info.value.args[0].startswith(
+            "Something went badly wrong - GitHub API PR failed - got return code 404"
+        )
+
+    @mock.patch("nf_core.utils.gh_api.get", side_effect=mocked_requests_get)
+    def test_close_open_template_merge_prs(self, mock_get):
+        """Try closing all open prs"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+        psync.gh_api.get = mock_get
+        psync.gh_username = "list_prs"
+        psync.gh_repo = "list_prs/response"
+        os.environ["GITHUB_AUTH_TOKEN"] = "test"
+
+        with mock.patch("nf_core.sync.PipelineSync.close_open_pr") as mock_close_open_pr:
+            psync.close_open_template_merge_prs()
+
+            prs = mock_get(f"https://api.github.com/repos/{psync.gh_repo}/pulls").data
+            for pr in prs:
+                if pr["state"] != "open":
+                    continue
+                else:
+                    mock_close_open_pr.assert_any_call(pr)
+
+    @mock.patch("nf_core.utils.gh_api.post", side_effect=mocked_requests_post)
+    @mock.patch("nf_core.utils.gh_api.patch", side_effect=mocked_requests_patch)
+    def test_close_open_pr(self, mock_patch, mock_post):
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+        psync.gh_api.post = mock_post
+        psync.gh_api.patch = mock_patch
+        psync.gh_username = "bad_url"
+        psync.gh_repo = "bad_url/response"
+        os.environ["GITHUB_AUTH_TOKEN"] = "test"
+        pr = {
+            "state": "open",
+            "head": {"ref": "nf-core-template-merge-3"},
+            "base": {"ref": "master"},
+            "html_url": "pr_html_url",
+            "url": "url_to_update_pr",
+            "comments_url": "pr_comments_url",
+        }
+
+        assert psync.close_open_pr(pr)
+        assert mock_patch.called_once_with("url_to_update_pr")
+
+    @mock.patch("nf_core.utils.gh_api.post", side_effect=mocked_requests_post)
+    @mock.patch("nf_core.utils.gh_api.patch", side_effect=mocked_requests_patch)
+    def test_close_open_pr_fail(self, mock_patch, mock_post):
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+        psync.gh_api.post = mock_post
+        psync.gh_api.patch = mock_patch
+        psync.gh_username = "bad_url"
+        psync.gh_repo = "bad_url/response"
+        os.environ["GITHUB_AUTH_TOKEN"] = "test"
+        pr = {
+            "state": "open",
+            "head": {"ref": "nf-core-template-merge-3"},
+            "base": {"ref": "master"},
+            "html_url": "pr_html_url",
+            "url": "bad_url_to_update_pr",
+            "comments_url": "pr_comments_url",
+        }
+
+        assert not psync.close_open_pr(pr)
+        assert mock_patch.called_once_with("bad_url_to_update_pr")
+
+    def test_reset_target_dir(self):
+        """Try resetting target pipeline directory"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+
+        psync.repo.git.checkout("dev")
+
+        psync.reset_target_dir()
+
+        assert psync.repo.heads[0].name == "TEMPLATE"
+
+    def test_reset_target_dir_fake_branch(self):
+        """Try resetting target pipeline directory but original branch does not exist"""
+        psync = nf_core.sync.PipelineSync(self.pipeline_dir)
+        psync.inspect_sync_dir()
+        psync.get_wf_config()
+
+        psync.original_branch = "fake_branch"
+
+        with pytest.raises(nf_core.sync.SyncException) as exc_info:
+            psync.reset_target_dir()
+        assert exc_info.value.args[0].startswith("Could not reset to original branch `fake_branch`")