A few fixes + increased robustness of tests

marge/branch.py

@@ -0,0 +1,25 @@
+from . import gitlab
+
+
+GET = gitlab.GET
+
+
+class Branch(gitlab.Resource):
+
+    @classmethod
+    def fetch_by_name(cls, project_id, branch, api):
+        info = api.call(GET(
+            '/projects/{project_id}/repository/branches/{branch}'.format(
+                project_id=project_id,
+                branch=branch,
+            ),
+        ))
+        return cls(api, info)
+
+    @property
+    def name(self):
+        return self.info['name']
+
+    @property
+    def protected(self):
+        return self.info['protected']

marge/job.py

@@ -5,6 +5,7 @@
 from datetime import datetime, timedelta
 
 from . import git
+from .branch import Branch
 from .interval import IntervalUnion
 from .project import Project
 from .user import User
@@ -145,6 +146,10 @@ def wait_for_ci_to_pass(self, merge_request, commit_sha=None):
                 log.info('CI for MR !%s passed', merge_request.iid)
                 return
 
+            if ci_status == 'skipped':
+                log.info('CI for MR !%s skipped', merge_request.iid)
+                return
+
             if ci_status == 'failed':
                 raise CannotMerge('CI failed!')
 
@@ -196,7 +201,7 @@ def fetch_source_project(self, merge_request):
             remote = 'source'
             remote_url = source_project.ssh_url_to_repo
             self._repo.fetch(
-                remote=remote,
+                remote_name=remote,
                 remote_url=remote_url,
             )
         return source_project, remote_url, remote
@@ -254,7 +259,7 @@ def update_from_target_branch_and_push(
             if updated_sha == target_sha:
                 raise CannotMerge('these changes already exist in branch `{}`'.format(target_branch))
             rewritten_sha = self.add_trailers(merge_request) or updated_sha
-            branch_rewritten = True
+            branch_rewritten = rewritten_sha != updated_sha
             repo.push(source_branch, source_repo_url=source_repo_url, force=True)
             changes_pushed = True
         except git.GitError:
@@ -263,6 +268,12 @@ def update_from_target_branch_and_push(
             if not branch_rewritten:
                 raise CannotMerge('failed on filter-branch; check my logs!')
             if not changes_pushed:
+                if (
+                    branch_rewritten and Branch.fetch_by_name(
+                        merge_request.source_project_id, merge_request.source_branch, self._api,
+                    ).protected
+                ):
+                    raise CannotMerge('Sorry, I can\'t push rewritten changes to protected branches!')
                 if self.opts.use_merge_strategy:
                     raise CannotMerge('failed to push merged changes, check my logs!')
                 else:
@@ -278,8 +289,6 @@ def update_from_target_branch_and_push(
             if source_branch != 'master':
                 repo.checkout_branch('master')
                 repo.remove_branch(source_branch)
-            else:
-                assert source_repo_url is not None
 
 
 def _get_reviewer_names_and_emails(approvals, api):

tests/gitlab_api_mock.py

@@ -24,7 +24,7 @@ def commit(commit_id, status):
 
 
 class MockLab(object):  # pylint: disable=too-few-public-methods
-    def __init__(self, gitlab_url=None):
+    def __init__(self, gitlab_url=None, fork=False, merge_request_options=None):
         self.gitlab_url = gitlab_url = gitlab_url or 'http://git.example.com'
         self.api = api = Api(gitlab_url=gitlab_url, auth_token='no-token', initial_state='initial')
 
@@ -35,7 +35,9 @@ def __init__(self, gitlab_url=None):
         api.add_user(self.user_info, is_current=True)
 
         self.project_info = dict(test_project.INFO)
+        self.forked_project_info = {**self.project_info, **{'id': 4321}}
         api.add_project(self.project_info)
+        api.add_project(self.forked_project_info)
 
         self.commit_info = dict(test_commit.INFO)
         api.add_commit(self.project_info['id'], self.commit_info)
@@ -58,6 +60,10 @@ def __init__(self, gitlab_url=None):
             'work_in_progress': False,
             'web_url': 'http://git.example.com/group/project/merge_request/666',
         }
+        if merge_request_options is not None:
+            self.merge_request_info.update(merge_request_options)
+        if fork:
+            self.merge_request_info.update({'iid': 55, 'source_project_id': '4321'})
         api.add_merge_request(self.merge_request_info)
 
         self.initial_master_sha = '505e'
@@ -70,7 +76,11 @@ def __init__(self, gitlab_url=None):
         )
         api.add_approvals(self.approvals_info)
         api.add_transition(
-            GET('/projects/1234/repository/branches/master'),
+            GET(
+                '/projects/1234/repository/branches/{target}'.format(
+                    target=self.merge_request_info['target_branch'],
+                ),
+            ),
             Ok({'commit': {'id': self.initial_master_sha}}),
         )
 

tests/test_batch_job.py

@@ -1,5 +1,5 @@
 # pylint: disable=protected-access
-from unittest.mock import ANY, Mock, patch
+from unittest.mock import ANY, patch, create_autospec
 
 import pytest
 
@@ -13,45 +13,53 @@
 from tests.gitlab_api_mock import MockLab, Ok, commit
 
 
-# pylint: disable=attribute-defined-outside-init
 class TestBatchJob(object):
-    def setup_method(self, _method):
-        self.mocklab = MockLab()
-        self.api = self.mocklab.api
+    @pytest.fixture(params=[True, False])
+    def fork(self, request):
+        return request.param
 
-    def get_batch_merge_job(self, **batch_merge_kwargs):
-        api, mocklab = self.api, self.mocklab
+    @pytest.fixture()
+    def mocklab(self, fork):
+        return MockLab(fork=fork)
 
+    @pytest.fixture()
+    def api(self, mocklab):
+        return mocklab.api
+
+    def _mock_merge_request(self, **options):
+        return create_autospec(marge.merge_request.MergeRequest, spec_set=True, **options)
+
+    def get_batch_merge_job(self, api, mocklab, **batch_merge_kwargs):
         project_id = mocklab.project_info['id']
         merge_request_iid = mocklab.merge_request_info['iid']
 
         merge_request = MergeRequest.fetch_by_iid(project_id, merge_request_iid, api)
 
         params = {
             'api': api,
-            'user': marge.user.User.myself(self.api),
+            'user': marge.user.User.myself(api),
             'project': marge.project.Project.fetch_by_id(project_id, api),
-            'repo': Mock(marge.git.Repo),
+            'repo': create_autospec(marge.git.Repo, spec_set=True),
             'options': MergeJobOptions.default(),
             'merge_requests': [merge_request]
         }
         params.update(batch_merge_kwargs)
         return BatchMergeJob(**params)
 
-    def test_remove_batch_branch(self):
-        repo = Mock()
-        batch_merge_job = self.get_batch_merge_job(repo=repo)
+    def test_remove_batch_branch(self, api, mocklab):
+        repo = create_autospec(marge.git.Repo, spec_set=True)
+        batch_merge_job = self.get_batch_merge_job(api, mocklab, repo=repo)
         batch_merge_job.remove_batch_branch()
         repo.remove_branch.assert_called_once_with(
             BatchMergeJob.BATCH_BRANCH_NAME,
         )
 
-    def test_close_batch_mr(self):
+    def test_close_batch_mr(self, api, mocklab):
         with patch('marge.batch_job.MergeRequest') as mr_class:
-            batch_mr = Mock()
+            batch_mr = self._mock_merge_request()
             mr_class.search.return_value = [batch_mr]
 
-            batch_merge_job = self.get_batch_merge_job()
+            batch_merge_job = self.get_batch_merge_job(api, mocklab)
             batch_merge_job.close_batch_mr()
 
             params = {
@@ -68,12 +76,12 @@ def test_close_batch_mr(self):
             )
             batch_mr.close.assert_called_once()
 
-    def test_create_batch_mr(self):
+    def test_create_batch_mr(self, api, mocklab):
         with patch('marge.batch_job.MergeRequest') as mr_class:
-            batch_mr = Mock()
+            batch_mr = self._mock_merge_request()
             mr_class.create.return_value = batch_mr
 
-            batch_merge_job = self.get_batch_merge_job()
+            batch_merge_job = self.get_batch_merge_job(api, mocklab)
             target_branch = 'master'
             r_batch_mr = batch_merge_job.create_batch_mr(target_branch)
 
@@ -90,26 +98,27 @@ def test_create_batch_mr(self):
             )
             assert r_batch_mr is batch_mr
 
-    def test_get_mrs_with_common_target_branch(self):
+    def test_get_mrs_with_common_target_branch(self, api, mocklab):
         master_mrs = [
-            Mock(target_branch='master'),
-            Mock(target_branch='master'),
+            self._mock_merge_request(target_branch='master'),
+            self._mock_merge_request(target_branch='master'),
         ]
         non_master_mrs = [
-            Mock(target_branch='non_master'),
-            Mock(target_branch='non_master'),
+            self._mock_merge_request(target_branch='non_master'),
+            self._mock_merge_request(target_branch='non_master'),
         ]
         batch_merge_job = self.get_batch_merge_job(
+            api, mocklab,
             merge_requests=non_master_mrs + master_mrs,
         )
         r_maser_mrs = batch_merge_job.get_mrs_with_common_target_branch('master')
         assert r_maser_mrs == master_mrs
 
     @patch.object(BatchMergeJob, 'get_mr_ci_status')
-    def test_ensure_mergeable_mr_ci_not_ok(self, bmj_get_mr_ci_status):
-        batch_merge_job = self.get_batch_merge_job()
+    def test_ensure_mergeable_mr_ci_not_ok(self, bmj_get_mr_ci_status, api, mocklab):
+        batch_merge_job = self.get_batch_merge_job(api, mocklab)
         bmj_get_mr_ci_status.return_value = 'failed'
-        merge_request = Mock(
+        merge_request = self._mock_merge_request(
             assignee_id=batch_merge_job._user.id,
             state='opened',
             work_in_progress=False,
@@ -121,19 +130,19 @@ def test_ensure_mergeable_mr_ci_not_ok(self, bmj_get_mr_ci_status):
 
         assert str(exc_info.value) == 'This MR has not passed CI.'
 
-    def test_push_batch(self):
-        batch_merge_job = self.get_batch_merge_job()
+    def test_push_batch(self, api, mocklab):
+        batch_merge_job = self.get_batch_merge_job(api, mocklab)
         batch_merge_job.push_batch()
         batch_merge_job._repo.push.assert_called_once_with(
             BatchMergeJob.BATCH_BRANCH_NAME,
             force=True,
         )
 
-    def test_ensure_mr_not_changed(self):
+    def test_ensure_mr_not_changed(self, api, mocklab):
         with patch('marge.batch_job.MergeRequest') as mr_class:
-            batch_merge_job = self.get_batch_merge_job()
-            merge_request = Mock()
-            changed_merge_request = Mock()
+            batch_merge_job = self.get_batch_merge_job(api, mocklab)
+            merge_request = self._mock_merge_request()
+            changed_merge_request = self._mock_merge_request()
             mr_class.fetch_by_iid.return_value = changed_merge_request
 
             with pytest.raises(CannotMerge):
@@ -145,24 +154,27 @@ def test_ensure_mr_not_changed(self):
                 batch_merge_job._api,
             )
 
-    def test_fuse_mr_when_target_branch_was_moved(self):
-        batch_merge_job = self.get_batch_merge_job()
-        merge_request = Mock(target_branch='master')
+    def test_fuse_mr_when_target_branch_was_moved(self, api, mocklab):
+        batch_merge_job = self.get_batch_merge_job(api, mocklab)
+        merge_request = self._mock_merge_request(target_branch='master')
         with pytest.raises(CannotBatch) as exc_info:
             batch_merge_job.accept_mr(merge_request, 'abc')
         assert str(exc_info.value) == 'Someone was naughty and by-passed marge'
 
-    def test_fuse_mr_when_source_branch_was_moved(self):
-        api, mocklab = self.api, self.mocklab
-        batch_merge_job = self.get_batch_merge_job()
-        merge_request = Mock(
-            source_project_id=batch_merge_job._project.id,
+    def test_fuse_mr_when_source_branch_was_moved(self, api, mocklab):
+        batch_merge_job = self.get_batch_merge_job(api, mocklab)
+        merge_request = self._mock_merge_request(
+            source_project_id=mocklab.merge_request_info['source_project_id'],
             target_branch='master',
-            source_branch=self.mocklab.merge_request_info['source_branch'],
+            source_branch=mocklab.merge_request_info['source_branch'],
         )
 
         api.add_transition(
-            GET('/projects/1234/repository/branches/useless_new_feature'),
+            GET(
+                '/projects/{project_iid}/repository/branches/useless_new_feature'.format(
+                    project_iid=mocklab.merge_request_info['source_project_id'],
+                ),
+            ),
             Ok({'commit': commit(commit_id='abc', status='running')}),
         )