From 9fbce7a5093b0d57268a0abf3493d48f4b23f392 Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sat, 1 Sep 2018 18:49:59 +0100 Subject: [PATCH 1/5] Add tests for forked merge requests By default the tests run on a mock merge request with the same source and target project. This leaves us without much coverage for cases where we want to merge as a fork. This change also restructures the tests to make better use of fixtures, as this make's writing the code to test both types of merge request easier. Finally, we make sure that all mocks used have `autospec` set so that we don't accidentally miss bugs where wrong methods are called, or wrong arguments are passed. --- tests/gitlab_api_mock.py | 6 +- tests/test_batch_job.py | 96 ++++++++++++---------- tests/test_job.py | 44 ++++++---- tests/test_single_job.py | 173 ++++++++++++++++++++++++--------------- 4 files changed, 194 insertions(+), 125 deletions(-) diff --git a/tests/gitlab_api_mock.py b/tests/gitlab_api_mock.py index 88ab6fb8..be1aaf9f 100644 --- a/tests/gitlab_api_mock.py +++ b/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): 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,8 @@ def __init__(self, gitlab_url=None): 'work_in_progress': False, 'web_url': 'http://git.example.com/group/project/merge_request/666', } + 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' diff --git a/tests/test_batch_job.py b/tests/test_batch_job.py index 21270bf2..1971e8a4 100644 --- a/tests/test_batch_job.py +++ b/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,15 +13,23 @@ 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'] @@ -29,29 +37,29 @@ def get_batch_merge_job(self, **batch_merge_kwargs): 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')}), ) diff --git a/tests/test_job.py b/tests/test_job.py index d12e89ca..737f7ef6 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -1,20 +1,28 @@ # pylint: disable=protected-access from datetime import timedelta -from unittest.mock import ANY, Mock, patch +from unittest.mock import ANY, Mock, patch, create_autospec import pytest -import marge.interval from marge.job import CannotMerge, MergeJob, MergeJobOptions, SkipMerge +import marge.interval +import marge.git +import marge.gitlab +import marge.merge_request +import marge.project +import marge.user class TestJob(object): + def _mock_merge_request(self, **options): + return create_autospec(marge.merge_request.MergeRequest, spec_set=True, **options) + def get_merge_job(self, **merge_kwargs): params = { - 'api': Mock(), - 'user': Mock(), - 'project': Mock(), - 'repo': Mock(), + 'api': create_autospec(marge.gitlab.Api, spec_set=True), + 'user': create_autospec(marge.user.User, spec_set=True), + 'project': create_autospec(marge.project.Project, spec_set=True), + 'repo': create_autospec(marge.git.Repo, spec_set=True), 'options': MergeJobOptions.default(), } params.update(merge_kwargs) @@ -22,7 +30,7 @@ def get_merge_job(self, **merge_kwargs): def test_get_source_project_when_is_target_project(self): merge_job = self.get_merge_job() - merge_request = Mock() + merge_request = self._mock_merge_request() merge_request.source_project_id = merge_job._project.id r_source_project = merge_job.get_source_project(merge_request) assert r_source_project is merge_job._project @@ -30,7 +38,7 @@ def test_get_source_project_when_is_target_project(self): def test_get_source_project_when_is_fork(self): with patch('marge.job.Project') as project_class: merge_job = self.get_merge_job() - merge_request = Mock() + merge_request = self._mock_merge_request() r_source_project = merge_job.get_source_project(merge_request) project_class.fetch_by_id.assert_called_once_with( @@ -41,10 +49,12 @@ def test_get_source_project_when_is_fork(self): assert r_source_project is project_class.fetch_by_id.return_value def test_get_mr_ci_status(self): - with patch('marge.job.Pipeline') as pipeline_class: - pipeline_class.pipelines_by_branch.return_value = [Mock(sha='abc', status='success')] + with patch('marge.job.Pipeline', autospec=True) as pipeline_class: + pipeline_class.pipelines_by_branch.return_value = [ + Mock(spec=pipeline_class, sha='abc', status='success'), + ] merge_job = self.get_merge_job() - merge_request = Mock(sha='abc') + merge_request = self._mock_merge_request(sha='abc') r_ci_status = merge_job.get_mr_ci_status(merge_request) @@ -57,7 +67,7 @@ def test_get_mr_ci_status(self): def test_ensure_mergeable_mr_not_assigned(self): merge_job = self.get_merge_job() - merge_request = Mock( + merge_request = self._mock_merge_request( state='opened', work_in_progress=False, squash=False, @@ -68,7 +78,7 @@ def test_ensure_mergeable_mr_not_assigned(self): def test_ensure_mergeable_mr_state_not_ok(self): merge_job = self.get_merge_job() - merge_request = Mock( + merge_request = self._mock_merge_request( assignee_id=merge_job._user.id, state='merged', work_in_progress=False, @@ -80,7 +90,7 @@ def test_ensure_mergeable_mr_state_not_ok(self): def test_ensure_mergeable_mr_not_approved(self): merge_job = self.get_merge_job() - merge_request = Mock( + merge_request = self._mock_merge_request( assignee_id=merge_job._user.id, state='opened', work_in_progress=False, @@ -95,7 +105,7 @@ def test_ensure_mergeable_mr_not_approved(self): def test_ensure_mergeable_mr_wip(self): merge_job = self.get_merge_job() - merge_request = Mock( + merge_request = self._mock_merge_request( assignee_id=merge_job._user.id, state='opened', work_in_progress=True, @@ -108,7 +118,7 @@ def test_ensure_mergeable_mr_wip(self): def test_ensure_mergeable_mr_squash_and_trailers(self): merge_job = self.get_merge_job(options=MergeJobOptions.default(add_reviewers=True)) - merge_request = Mock( + merge_request = self._mock_merge_request( assignee_id=merge_job._user.id, state='opened', work_in_progress=False, @@ -125,7 +135,7 @@ def test_ensure_mergeable_mr_squash_and_trailers(self): def test_unassign_from_mr(self): merge_job = self.get_merge_job() - merge_request = Mock() + merge_request = self._mock_merge_request() # when we are not the author merge_job.unassign_from_mr(merge_request) diff --git a/tests/test_single_job.py b/tests/test_single_job.py index a00c66f0..283874a0 100644 --- a/tests/test_single_job.py +++ b/tests/test_single_job.py @@ -1,6 +1,9 @@ import contextlib +from collections import namedtuple from datetime import timedelta -from unittest.mock import ANY, Mock, patch +from unittest.mock import ANY, patch, create_autospec + +import pytest import marge.commit import marge.interval @@ -36,33 +39,34 @@ def _pipeline(sha1, status): class SingleJobMockLab(MockLab): - def __init__(self, gitlab_url=None): - super().__init__(gitlab_url) + def __init__(self, gitlab_url=None, fork=False): + super().__init__(gitlab_url, fork=fork) api = self.api self.rewritten_sha = rewritten_sha = 'af7a' api.add_pipelines( - self.project_info['id'], + self.merge_request_info['source_project_id'], _pipeline(sha1=rewritten_sha, status='running'), from_state='pushed', to_state='passed', ) api.add_pipelines( - self.project_info['id'], + self.merge_request_info['source_project_id'], _pipeline(sha1=rewritten_sha, status='success'), from_state=['passed', 'merged'], ) + source_project_id = self.merge_request_info['source_project_id'] api.add_transition( - GET('/projects/1234/repository/branches/useless_new_feature'), + GET('/projects/{}/repository/branches/useless_new_feature'.format(source_project_id)), Ok({'commit': _commit(commit_id=rewritten_sha, status='running')}), from_state='pushed', ) api.add_transition( - GET('/projects/1234/repository/branches/useless_new_feature'), + GET('/projects/{}/repository/branches/useless_new_feature'.format(source_project_id)), Ok({'commit': _commit(commit_id=rewritten_sha, status='success')}), from_state='passed' ) api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=self.merge_request_info['iid']), dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Ok({}), @@ -98,7 +102,10 @@ def assign_to_author(): author_assigned = True self.api.add_transition( - PUT('/projects/1234/merge_requests/54', args={'assignee_id': self.author_id}), + PUT( + '/projects/1234/merge_requests/{iid}'.format(iid=self.merge_request_info['iid']), + args={'assignee_id': self.author_id}, + ), assign_to_author, ) error_note = "I couldn't merge this branch: %s" % message @@ -117,56 +124,85 @@ def branch_update(self, side_effect=None): marge.single_merge_job.SingleMergeJob, 'update_from_target_branch_and_push', side_effect=side_effect, + autospec=True, ): yield -# pylint: disable=attribute-defined-outside-init -@patch('time.sleep') class TestUpdateAndAccept(object): + TestParams = namedtuple('TestParams', ['fork', 'source_project_id']) + + @pytest.fixture( + params=[ + TestParams(fork=True, source_project_id=4321), + TestParams(fork=False, source_project_id=1234), + ] + ) + def test_params(self, request): + return request.param + + @pytest.fixture(autouse=True) + def patch_sleep(self): + with patch('time.sleep'): + yield - def setup_method(self, _method): - self.mocklab = SingleJobMockLab() - self.api = self.mocklab.api + @pytest.fixture() + def mocklab(self, test_params): + print('fork: %s' % test_params.fork) + return SingleJobMockLab(fork=test_params.fork) - def make_job(self, options=None): - api, mocklab = self.api, self.mocklab + @pytest.fixture() + def api(self, mocklab): + return mocklab.api + def make_job(self, api, mocklab, options=None): project_id = mocklab.project_info['id'] merge_request_iid = mocklab.merge_request_info['iid'] project = marge.project.Project.fetch_by_id(project_id, api) merge_request = MergeRequest.fetch_by_iid(project_id, merge_request_iid, api) - repo = Mock(marge.git.Repo) + repo = create_autospec(marge.git.Repo, spec_set=True) options = options or marge.job.MergeJobOptions.default() - user = marge.user.User.myself(self.api) + user = marge.user.User.myself(api) return marge.single_merge_job.SingleMergeJob( api=api, user=user, project=project, merge_request=merge_request, repo=repo, options=options, ) - def test_succeeds_first_time(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_succeeds_first_time(self, api, mocklab): with mocklab.branch_update(): - job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False)) + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) job.execute() assert api.state == 'merged' assert api.notes == [] - def test_fails_on_not_acceptable_if_master_did_not_move(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_fails_on_not_acceptable_if_master_did_not_move( + self, api, mocklab, test_params + ): new_branch_head_sha = '99ba110035' api.add_transition( - GET('/projects/1234/repository/branches/useless_new_feature'), + GET( + '/projects/{source_project_id}/repository/branches/useless_new_feature'.format( + source_project_id=test_params.source_project_id, + ), + ), Ok({'commit': _commit(commit_id=new_branch_head_sha, status='success')}), from_state='pushed', to_state='pushed_but_head_changed' ) with mocklab.branch_update(): with mocklab.expected_failure("Someone pushed to branch while we were trying to merge"): - job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False)) + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) job.execute() assert api.state == 'pushed_but_head_changed' @@ -174,18 +210,17 @@ def test_fails_on_not_acceptable_if_master_did_not_move(self, unused_time_sleep) "I couldn't merge this branch: Someone pushed to branch while we were trying to merge", ] - def test_succeeds_second_time_if_master_moved(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_succeeds_second_time_if_master_moved(self, api, mocklab, test_params): moved_master_sha = 'fafafa' first_rewritten_sha = '1o1' api.add_pipelines( - mocklab.project_info['id'], + mocklab.merge_request_info['source_project_id'], _pipeline(sha1=first_rewritten_sha, status='success'), from_state=['pushed_but_master_moved', 'merged_rejected'], ) api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=mocklab.merge_request_info['iid']), dict( sha=first_rewritten_sha, should_remove_source_branch=True, @@ -196,7 +231,11 @@ def test_succeeds_second_time_if_master_moved(self, unused_time_sleep): from_state='pushed_but_master_moved', to_state='merge_rejected', ) api.add_transition( - GET('/projects/1234/repository/branches/useless_new_feature'), + GET( + '/projects/{source_project_id}/repository/branches/useless_new_feature'.format( + source_project_id=test_params.source_project_id, + ), + ), Ok({'commit': _commit(commit_id=first_rewritten_sha, status='success')}), from_state='pushed_but_master_moved' ) @@ -216,7 +255,11 @@ def push_effects(): yield moved_master_sha, 'deadbeef', mocklab.rewritten_sha with mocklab.branch_update(side_effect=push_effects()): - job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False)) + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) job.execute() assert api.state == 'merged' @@ -224,12 +267,11 @@ def push_effects(): "My job would be easier if people didn't jump the queue and push directly... *sigh*", ] - def test_handles_races_for_merging(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_handles_races_for_merging(self, api, mocklab): rewritten_sha = mocklab.rewritten_sha api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=mocklab.merge_request_info['iid']), dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Error(marge.gitlab.NotFound(404, {'message': '404 Branch Not Found'})), @@ -240,17 +282,16 @@ def test_handles_races_for_merging(self, unused_time_sleep): from_state='someone_else_merged', ) with mocklab.branch_update(): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'someone_else_merged' assert api.notes == [] - def test_handles_request_becoming_wip_after_push(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_handles_request_becoming_wip_after_push(self, api, mocklab): rewritten_sha = mocklab.rewritten_sha api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=mocklab.merge_request_info['iid']), dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})), @@ -262,17 +303,16 @@ def test_handles_request_becoming_wip_after_push(self, unused_time_sleep): ) message = 'The request was marked as WIP as I was processing it (maybe a WIP commit?)' with mocklab.branch_update(), mocklab.expected_failure(message): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'now_is_wip' assert api.notes == ["I couldn't merge this branch: %s" % message] - def test_guesses_git_hook_error_on_merge_refusal(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_guesses_git_hook_error_on_merge_refusal(self, api, mocklab): rewritten_sha = mocklab.rewritten_sha api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=mocklab.merge_request_info['iid']), dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})), @@ -287,17 +327,16 @@ def test_guesses_git_hook_error_on_merge_refusal(self, unused_time_sleep): 'is rejecting my commits; maybe my email needs to be white-listed?' ) with mocklab.branch_update(), mocklab.expected_failure(message): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'rejected_by_git_hook' assert api.notes == ["I couldn't merge this branch: %s" % message] - def test_discovers_if_someone_closed_the_merge_request(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_discovers_if_someone_closed_the_merge_request(self, api, mocklab): rewritten_sha = mocklab.rewritten_sha api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=mocklab.merge_request_info['iid']), dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})), @@ -309,17 +348,16 @@ def test_discovers_if_someone_closed_the_merge_request(self, unused_time_sleep): ) message = 'Someone closed the merge request while I was attempting to merge it.' with mocklab.branch_update(), mocklab.expected_failure(message): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'oops_someone_closed_it' assert api.notes == ["I couldn't merge this branch: %s" % message] - def test_tells_explicitly_that_gitlab_refused_to_merge(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_tells_explicitly_that_gitlab_refused_to_merge(self, api, mocklab): rewritten_sha = mocklab.rewritten_sha api.add_transition( PUT( - '/projects/1234/merge_requests/54/merge', + '/projects/1234/merge_requests/{iid}/merge'.format(iid=mocklab.merge_request_info['iid']), dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Error(marge.gitlab.MethodNotAllowed(405, {'message': '405 Method Not Allowed'})), @@ -327,18 +365,17 @@ def test_tells_explicitly_that_gitlab_refused_to_merge(self, unused_time_sleep): ) message = "Gitlab refused to merge this request and I don't know why!" with mocklab.branch_update(), mocklab.expected_failure(message): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'rejected_for_mysterious_reasons' assert api.notes == ["I couldn't merge this branch: %s" % message] - def test_wont_merge_wip_stuff(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_wont_merge_wip_stuff(self, api, mocklab): wip_merge_request = dict(mocklab.merge_request_info, work_in_progress=True) api.add_merge_request(wip_merge_request, from_state='initial') with mocklab.expected_failure("Sorry, I can't merge requests marked as Work-In-Progress!"): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'initial' @@ -346,8 +383,7 @@ def test_wont_merge_wip_stuff(self, unused_time_sleep): "I couldn't merge this branch: Sorry, I can't merge requests marked as Work-In-Progress!", ] - def test_wont_merge_branches_with_autosquash_if_rewriting(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_wont_merge_branches_with_autosquash_if_rewriting(self, api, mocklab): autosquash_merge_request = dict(mocklab.merge_request_info, squash=True) api.add_merge_request(autosquash_merge_request, from_state='initial') admin_user = dict(mocklab.user_info, is_admin=True) @@ -357,35 +393,42 @@ def test_wont_merge_branches_with_autosquash_if_rewriting(self, unused_time_slee for rewriting_opt in ('add_tested', 'add_reviewers'): with mocklab.expected_failure(message): - job = self.make_job(marge.job.MergeJobOptions.default(**{rewriting_opt: True})) + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(**{rewriting_opt: True}), + ) job.execute() assert api.state == 'initial' with mocklab.branch_update(): - job = self.make_job() + job = self.make_job(api, mocklab) job.execute() assert api.state == 'merged' - @patch('marge.job.log') - def test_waits_for_approvals(self, mock_log, unused_time_sleep): - api, mocklab = self.api, self.mocklab + @patch('marge.job.log', autospec=True) + def test_waits_for_approvals(self, mock_log, api, mocklab): with mocklab.branch_update(): job = self.make_job( - marge.job.MergeJobOptions.default(approval_timeout=timedelta(seconds=5), reapprove=True)) + api, + mocklab, + options=marge.job.MergeJobOptions.default( + approval_timeout=timedelta(seconds=5), reapprove=True, + ), + ) job.execute() mock_log.info.assert_any_call('Checking if approvals have reset') mock_log.debug.assert_any_call('Approvals haven\'t reset yet, sleeping for %s secs', ANY) assert api.state == 'merged' - def test_fails_if_changes_already_exist(self, unused_time_sleep): - api, mocklab = self.api, self.mocklab + def test_fails_if_changes_already_exist(self, api, mocklab): expected_message = 'these changes already exist in branch `{}`'.format( mocklab.merge_request_info['target_branch'], ) with mocklab.expected_failure(expected_message): - job = self.make_job() + job = self.make_job(api, mocklab) job.repo.rebase.return_value = mocklab.initial_master_sha job.repo.get_commit_hash.return_value = mocklab.initial_master_sha job.execute() From 9c9588e7bf9a24317548ace7a03644ab17af7610 Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sat, 1 Sep 2018 18:57:44 +0100 Subject: [PATCH 2/5] Fix TypeError when fetching source project This is now caught by the tests. Fixes: #122 --- marge/job.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/marge/job.py b/marge/job.py index 3b3e46dc..623648c0 100644 --- a/marge/job.py +++ b/marge/job.py @@ -196,7 +196,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 From 52f871588323adbb8c8d6ddf4763ce8b0c2786f2 Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sat, 1 Sep 2018 21:28:24 +0100 Subject: [PATCH 3/5] Handle CI status 'skipped' Handle the case where GitLab skips CI (due to a `[ci skip]` present in the commit message. Also add some tests for CI being canceled/failed. Fixes: #95 --- marge/job.py | 4 +++ tests/test_single_job.py | 71 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/marge/job.py b/marge/job.py index 623648c0..09f99c6d 100644 --- a/marge/job.py +++ b/marge/job.py @@ -145,6 +145,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!') diff --git a/tests/test_single_job.py b/tests/test_single_job.py index 283874a0..9ee4c3a1 100644 --- a/tests/test_single_job.py +++ b/tests/test_single_job.py @@ -70,7 +70,7 @@ def __init__(self, gitlab_url=None, fork=False): dict(sha=rewritten_sha, should_remove_source_branch=True, merge_when_pipeline_succeeds=True), ), Ok({}), - from_state='passed', to_state='merged', + from_state=['passed', 'skipped'], to_state='merged', ) api.add_merge_request(dict(self.merge_request_info, state='merged'), from_state='merged') api.add_transition( @@ -183,6 +183,75 @@ def test_succeeds_first_time(self, api, mocklab): assert api.state == 'merged' assert api.notes == [] + def test_succeeds_if_skipped(self, api, mocklab): + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline(sha1=mocklab.rewritten_sha, status='running'), + from_state='pushed', to_state='skipped', + ) + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline(sha1=mocklab.rewritten_sha, status='skipped'), + from_state=['skipped', 'merged'], + ) + + with mocklab.branch_update(): + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) + job.execute() + + assert api.state == 'merged' + assert api.notes == [] + + def test_fails_if_ci_fails(self, api, mocklab): + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline(sha1=mocklab.rewritten_sha, status='running'), + from_state='pushed', to_state='failed', + ) + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline(sha1=mocklab.rewritten_sha, status='failed'), + from_state=['failed'], + ) + + with mocklab.branch_update(): + with mocklab.expected_failure("CI failed!"): + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(), + ) + job.execute() + + assert api.state == 'failed' + + def test_fails_if_ci_canceled(self, api, mocklab): + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline(sha1=mocklab.rewritten_sha, status='running'), + from_state='pushed', to_state='canceled', + ) + api.add_pipelines( + mocklab.merge_request_info['source_project_id'], + _pipeline(sha1=mocklab.rewritten_sha, status='canceled'), + from_state=['canceled'], + ) + + with mocklab.branch_update(): + with mocklab.expected_failure("Someone canceled the CI."): + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(), + ) + job.execute() + + assert api.state == 'canceled' + def test_fails_on_not_acceptable_if_master_did_not_move( self, api, mocklab, test_params ): From 5819da5b2cc44c504e4b3a2dc23542edeb21e92b Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sun, 2 Sep 2018 12:57:06 +0100 Subject: [PATCH 4/5] Handle error on pushing to protected branches When rewriting commits, we need to push the updates to the original branch. If the branch is protected, then Marge will be unable to do so, and should return an appropriate error message. Fixes: #128 --- marge/branch.py | 25 +++++++++++++++++++++++++ marge/job.py | 9 ++++++++- tests/test_single_job.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 marge/branch.py diff --git a/marge/branch.py b/marge/branch.py new file mode 100644 index 00000000..f0ef33ea --- /dev/null +++ b/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'] diff --git a/marge/job.py b/marge/job.py index 09f99c6d..ec3bda55 100644 --- a/marge/job.py +++ b/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 @@ -258,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: @@ -267,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: diff --git a/tests/test_single_job.py b/tests/test_single_job.py index 9ee4c3a1..48933739 100644 --- a/tests/test_single_job.py +++ b/tests/test_single_job.py @@ -29,6 +29,13 @@ def _commit(commit_id, status): } +def _branch(name, protected=False): + return { + 'name': name, + 'protected': protected, + } + + def _pipeline(sha1, status): return { 'id': 47, @@ -279,6 +286,29 @@ def test_fails_on_not_acceptable_if_master_did_not_move( "I couldn't merge this branch: Someone pushed to branch while we were trying to merge", ] + def test_fails_if_branch_is_protected( + self, api, mocklab, test_params + ): + api.add_transition( + GET( + '/projects/{source_project_id}/repository/branches/useless_new_feature'.format( + source_project_id=test_params.source_project_id, + ), + ), + Ok(_branch('useless_new_feature', protected=True)), + from_state='initial', to_state='protected' + ) + with mocklab.expected_failure("Sorry, I can't push rewritten changes to protected branches!"): + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) + job.repo.push.side_effect = marge.git.GitError() + job.execute() + + assert api.state == 'protected' + def test_succeeds_second_time_if_master_moved(self, api, mocklab, test_params): moved_master_sha = 'fafafa' first_rewritten_sha = '1o1' From 556841ce68518dd659aa894cbe1eb672dcc71228 Mon Sep 17 00:00:00 2001 From: Jaime Lennox Date: Sun, 2 Sep 2018 14:22:40 +0100 Subject: [PATCH 5/5] Handle merging when source branch is master Fixes an issue uncovered by #124 where Marge throws an assertion error if the source/target branches are from the same project and the source branch is master. We assume that merges will always be into master, which is obviously not always the case; you may have a workflow requiring you to merge from master into another branch (e.g. a staging/production branch). --- marge/job.py | 2 - tests/gitlab_api_mock.py | 10 ++++- tests/test_single_job.py | 88 +++++++++++++++++++++++++++++++++++----- 3 files changed, 86 insertions(+), 14 deletions(-) diff --git a/marge/job.py b/marge/job.py index ec3bda55..937151be 100644 --- a/marge/job.py +++ b/marge/job.py @@ -289,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): diff --git a/tests/gitlab_api_mock.py b/tests/gitlab_api_mock.py index be1aaf9f..4d410645 100644 --- a/tests/gitlab_api_mock.py +++ b/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, fork=False): + 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') @@ -60,6 +60,8 @@ def __init__(self, gitlab_url=None, fork=False): '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) @@ -74,7 +76,11 @@ def __init__(self, gitlab_url=None, fork=False): ) 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}}), ) diff --git a/tests/test_single_job.py b/tests/test_single_job.py index 48933739..8caad0cf 100644 --- a/tests/test_single_job.py +++ b/tests/test_single_job.py @@ -1,6 +1,7 @@ import contextlib from collections import namedtuple from datetime import timedelta +from functools import partial from unittest.mock import ANY, patch, create_autospec import pytest @@ -36,38 +37,46 @@ def _branch(name, protected=False): } -def _pipeline(sha1, status): +def _pipeline(sha1, status, ref='useless_new_feature'): return { 'id': 47, 'status': status, - 'ref': 'useless_new_feature', + 'ref': ref, 'sha': sha1, } class SingleJobMockLab(MockLab): - def __init__(self, gitlab_url=None, fork=False): - super().__init__(gitlab_url, fork=fork) + def __init__(self, gitlab_url=None, fork=False, merge_request_options=None): + super().__init__(gitlab_url, fork=fork, merge_request_options=merge_request_options) api = self.api self.rewritten_sha = rewritten_sha = 'af7a' api.add_pipelines( self.merge_request_info['source_project_id'], - _pipeline(sha1=rewritten_sha, status='running'), + _pipeline(sha1=rewritten_sha, status='running', ref=self.merge_request_info['source_branch']), from_state='pushed', to_state='passed', ) api.add_pipelines( self.merge_request_info['source_project_id'], - _pipeline(sha1=rewritten_sha, status='success'), + _pipeline(sha1=rewritten_sha, status='success', ref=self.merge_request_info['source_branch']), from_state=['passed', 'merged'], ) source_project_id = self.merge_request_info['source_project_id'] api.add_transition( - GET('/projects/{}/repository/branches/useless_new_feature'.format(source_project_id)), + GET( + '/projects/{}/repository/branches/{}'.format( + source_project_id, self.merge_request_info['source_branch'], + ), + ), Ok({'commit': _commit(commit_id=rewritten_sha, status='running')}), from_state='pushed', ) api.add_transition( - GET('/projects/{}/repository/branches/useless_new_feature'.format(source_project_id)), + GET( + '/projects/{}/repository/branches/{}'.format( + source_project_id, self.merge_request_info['source_branch'], + ), + ), Ok({'commit': _commit(commit_id=rewritten_sha, status='success')}), from_state='passed' ) @@ -81,7 +90,7 @@ def __init__(self, gitlab_url=None, fork=False): ) api.add_merge_request(dict(self.merge_request_info, state='merged'), from_state='merged') api.add_transition( - GET('/projects/1234/repository/branches/master'), + GET('/projects/1234/repository/branches/{}'.format(self.merge_request_info['target_branch'])), Ok({'commit': {'id': self.rewritten_sha}}), from_state='merged' ) @@ -155,9 +164,12 @@ def patch_sleep(self): @pytest.fixture() def mocklab(self, test_params): - print('fork: %s' % test_params.fork) return SingleJobMockLab(fork=test_params.fork) + @pytest.fixture() + def mocklab_factory(self, test_params): + return partial(SingleJobMockLab, fork=test_params.fork) + @pytest.fixture() def api(self, mocklab): return mocklab.api @@ -190,6 +202,32 @@ def test_succeeds_first_time(self, api, mocklab): assert api.state == 'merged' assert api.notes == [] + def test_succeeds_with_updated_branch(self, api, mocklab): + api.add_transition( + GET( + '/projects/1234/repository/branches/{source}'.format( + source=mocklab.merge_request_info['source_branch'], + ), + ), + Ok({'commit': {'id': mocklab.rewritten_sha}}), + from_state='initial', to_state='pushed', + ) + with patch.object( + marge.single_merge_job.SingleMergeJob, + 'add_trailers', + side_effect=lambda *_: mocklab.push_updated()[2], + autospec=True, + ): + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) + job.execute() + + assert api.state == 'merged' + assert api.notes == [] + def test_succeeds_if_skipped(self, api, mocklab): api.add_pipelines( mocklab.merge_request_info['source_project_id'], @@ -213,6 +251,36 @@ def test_succeeds_if_skipped(self, api, mocklab): assert api.state == 'merged' assert api.notes == [] + def test_succeeds_if_source_is_master(self, mocklab_factory): + mocklab = mocklab_factory( + merge_request_options={'source_branch': 'master', 'target_branch': 'production'}, + ) + api = mocklab.api + api.add_transition( + GET( + '/projects/1234/repository/branches/{source}'.format( + source=mocklab.merge_request_info['source_branch'], + ), + ), + Ok({'commit': {'id': mocklab.rewritten_sha}}), + from_state='initial', to_state='pushed', + ) + with patch.object( + marge.single_merge_job.SingleMergeJob, + 'add_trailers', + side_effect=lambda *_: mocklab.push_updated()[2], + autospec=True, + ): + job = self.make_job( + api, + mocklab, + options=marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False), + ) + job.execute() + + assert api.state == 'merged' + assert api.notes == [] + def test_fails_if_ci_fails(self, api, mocklab): api.add_pipelines( mocklab.merge_request_info['source_project_id'],