diff options
Diffstat (limited to 'spec/services/merge_requests')
7 files changed, 246 insertions, 161 deletions
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index b037b73752e..0e51de48fb1 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -19,45 +19,54 @@ describe MergeRequests::CloseService do describe '#execute' do it_behaves_like 'cache counters invalidator' - context 'valid params' do - let(:service) { described_class.new(project, user, {}) } + [true, false].each do |state_tracking_enabled| + context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do + let(:service) { described_class.new(project, user, {}) } - before do - allow(service).to receive(:execute_hooks) + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - perform_enqueued_jobs do - @merge_request = service.execute(merge_request) + allow(service).to receive(:execute_hooks) + + perform_enqueued_jobs do + @merge_request = service.execute(merge_request) + end end - end - it { expect(@merge_request).to be_valid } - it { expect(@merge_request).to be_closed } + it { expect(@merge_request).to be_valid } + it { expect(@merge_request).to be_closed } - it 'executes hooks with close action' do - expect(service).to have_received(:execute_hooks) + it 'executes hooks with close action' do + expect(service).to have_received(:execute_hooks) .with(@merge_request, 'close') - end + end - it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'sends email to user2 about assign of new merge_request', :sidekiq_might_not_need_inline do + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end - it 'creates system note about merge_request reassign' do - note = @merge_request.notes.last - expect(note.note).to include 'closed' - end + it 'creates system note about merge_request reassign' do + if state_tracking_enabled + event = @merge_request.resource_state_events.last + expect(event.state).to eq('closed') + else + note = @merge_request.notes.last + expect(note.note).to include 'closed' + end + end - it 'marks todos as done' do - expect(todo.reload).to be_done - end + it 'marks todos as done' do + expect(todo.reload).to be_done + end - context 'when auto merge is enabled' do - let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + context 'when auto merge is enabled' do + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } - it 'cancels the auto merge' do - expect(@merge_request).not_to be_auto_merge_enabled + it 'cancels the auto merge' do + expect(@merge_request).not_to be_auto_merge_enabled + end end end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 9155db16d17..bb40c399b6e 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -59,7 +59,7 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do it 'creates exactly 1 create MR event', :sidekiq_might_not_need_inline do attributes = { - action: Event::CREATED, + action: :created, target_id: merge_request.id, target_type: merge_request.class.name } @@ -136,11 +136,11 @@ describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do let!(:pipeline_3) { create(:ci_pipeline, project: project, ref: "other_branch", project_id: project.id) } before do - # rubocop: disable DestroyAll + # rubocop: disable Cop/DestroyAll project.merge_requests .where(source_branch: opts[:source_branch], target_branch: opts[:target_branch]) .destroy_all - # rubocop: enable DestroyAll + # rubocop: enable Cop/DestroyAll end it 'sets head pipeline' do diff --git a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb index a1c86467f34..2adf808619d 100644 --- a/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb +++ b/spec/services/merge_requests/delete_non_latest_diffs_service_spec.rb @@ -48,12 +48,12 @@ describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_shared_ end it 'schedules no removal if there is no non-latest diffs' do - # rubocop: disable DestroyAll + # rubocop: disable Cop/DestroyAll merge_request .merge_request_diffs .where.not(id: merge_request.latest_merge_request_diff_id) .destroy_all - # rubocop: enable DestroyAll + # rubocop: enable Cop/DestroyAll expect(DeleteDiffFilesWorker).not_to receive(:bulk_perform_in) diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index 87fcd70a298..415b351e13a 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -21,65 +21,74 @@ describe MergeRequests::FfMergeService do end describe '#execute' do - context 'valid params' do - let(:service) { described_class.new(project, user, valid_merge_params) } - - def execute_ff_merge - perform_enqueued_jobs do - service.execute(merge_request) + [true, false].each do |state_tracking_enabled| + context "valid params with state_tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}" do + let(:service) { described_class.new(project, user, valid_merge_params) } + + def execute_ff_merge + perform_enqueued_jobs do + service.execute(merge_request) + end end - end - before do - allow(service).to receive(:execute_hooks) - end + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - it "does not create merge commit" do - execute_ff_merge + allow(service).to receive(:execute_hooks) + end - source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha - target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha + it "does not create merge commit" do + execute_ff_merge - expect(source_branch_sha).to eq(target_branch_sha) - end + source_branch_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha + target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha - it 'keeps the merge request valid' do - expect { execute_ff_merge } - .not_to change { merge_request.valid? } - end + expect(source_branch_sha).to eq(target_branch_sha) + end - it 'updates the merge request to merged' do - expect { execute_ff_merge } - .to change { merge_request.merged? } - .from(false) - .to(true) - end + it 'keeps the merge request valid' do + expect { execute_ff_merge } + .not_to change { merge_request.valid? } + end - it 'sends email to user2 about merge of new merge_request' do - execute_ff_merge + it 'updates the merge request to merged' do + expect { execute_ff_merge } + .to change { merge_request.merged? } + .from(false) + .to(true) + end - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'sends email to user2 about merge of new merge_request' do + execute_ff_merge - it 'creates system note about merge_request merge' do - execute_ff_merge + email = ActionMailer::Base.deliveries.last + expect(email.to.first).to eq(user2.email) + expect(email.subject).to include(merge_request.title) + end - note = merge_request.notes.last - expect(note.note).to include 'merged' - end + it 'creates system note about merge_request merge' do + execute_ff_merge - it 'does not update squash_commit_sha if it is not a squash' do - expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } - end + if state_tracking_enabled + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + else + note = merge_request.notes.last + expect(note.note).to include 'merged' + end + end - it 'updates squash_commit_sha if it is a squash' do - merge_request.update!(squash: true) + it 'does not update squash_commit_sha if it is not a squash' do + expect { execute_ff_merge }.not_to change { merge_request.squash_commit_sha } + end - expect { execute_ff_merge } - .to change { merge_request.squash_commit_sha } - .from(nil) + it 'updates squash_commit_sha if it is a squash' do + merge_request.update!(squash: true) + + expect { execute_ff_merge } + .to change { merge_request.squash_commit_sha } + .from(nil) + end end end @@ -87,7 +96,7 @@ describe MergeRequests::FfMergeService do let(:service) { described_class.new(project, user, valid_merge_params.merge(commit_message: 'Awesome message')) } before do - allow(Rails.logger).to receive(:error) + allow(Gitlab::AppLogger).to receive(:error) end it 'logs and saves error if there is an exception' do @@ -99,7 +108,7 @@ describe MergeRequests::FfMergeService do service.execute(merge_request) expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if there is an PreReceiveError exception' do @@ -111,7 +120,7 @@ describe MergeRequests::FfMergeService do service.execute(merge_request) expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'does not update squash_commit_sha if squash merge is not successful' do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index bcad822b1dc..2274d917527 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -20,7 +20,11 @@ describe MergeRequests::MergeService do end context 'valid params' do + let(:state_tracking) { true } + before do + stub_feature_flags(track_resource_state_change_events: state_tracking) + allow(service).to receive(:execute_hooks) perform_enqueued_jobs do @@ -42,9 +46,22 @@ describe MergeRequests::MergeService do expect(email.subject).to include(merge_request.title) end - it 'creates system note about merge_request merge' do - note = merge_request.notes.last - expect(note.note).to include 'merged' + context 'note creation' do + context 'when resource state event tracking is disabled' do + let(:state_tracking) { false } + + it 'creates system note about merge_request merge' do + note = merge_request.notes.last + expect(note.note).to include 'merged' + end + end + + context 'when resource state event tracking is enabled' do + it 'creates resource state event about merge_request merge' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + end + end end context 'when squashing' do @@ -55,7 +72,7 @@ describe MergeRequests::MergeService do end let(:merge_request) do - # A merge reqeust with 5 commits + # A merge request with 5 commits create(:merge_request, :simple, author: user2, assignees: [user2], @@ -277,7 +294,7 @@ describe MergeRequests::MergeService do context 'error handling' do before do - allow(Rails.logger).to receive(:error) + allow(Gitlab::AppLogger).to receive(:error) end context 'when source is missing' do @@ -289,7 +306,7 @@ describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to eq(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end end @@ -302,7 +319,7 @@ describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to include('Something went wrong during merge') - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if user is not authorized' do @@ -326,7 +343,7 @@ describe MergeRequests::MergeService do service.execute(merge_request) expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if there is a merge conflict' do @@ -340,7 +357,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end context 'when squashing' do @@ -359,7 +376,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end it 'logs and saves error if there is a squash in progress' do @@ -373,7 +390,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end context 'when fast-forward merge is not allowed' do @@ -393,7 +410,7 @@ describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) - expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end end end diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index 94e65d895ac..e60ff6eb98a 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -362,76 +362,101 @@ describe MergeRequests::RefreshService do end end - context 'push to origin repo target branch', :sidekiq_might_not_need_inline do - context 'when all MRs to the target branch had diffs' do + [true, false].each do |state_tracking_enabled| + context "push to origin repo target branch with state tracking #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do before do - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) end - it 'updates the merge state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done + context 'when all MRs to the target branch had diffs' do + before do + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end + + it 'updates the merge state' do + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + expect(@fork_merge_request.notes.last.note).to include('merged') + end + end end - end - context 'when an MR to be closed was empty already' do - let!(:empty_fork_merge_request) do - create(:merge_request, - source_project: @fork_project, - source_branch: 'master', - target_branch: 'master', - target_project: @project) + context 'when an MR to be closed was empty already' do + let!(:empty_fork_merge_request) do + create(:merge_request, + source_project: @fork_project, + source_branch: 'master', + target_branch: 'master', + target_project: @project) + end + + before do + # This spec already has a fake push, so pretend that we were targeting + # feature all along. + empty_fork_merge_request.update_columns(target_branch: 'feature') + + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + empty_fork_merge_request.reload + end + + it 'only updates the non-empty MRs' do + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_merged + + expect(empty_fork_merge_request).to be_open + expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') + expect(empty_fork_merge_request.notes).to be_empty + + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + expect(@fork_merge_request.notes.last.note).to include('merged') + end + end end + end + context "manual merge of source branch #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do before do - # This spec already has a fake push, so pretend that we were targeting - # feature all along. - empty_fork_merge_request.update_columns(target_branch: 'feature') + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + # Merge master -> feature branch + @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') + commit = @project.repository.commit('feature') + service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') reload_mrs - empty_fork_merge_request.reload end - it 'only updates the non-empty MRs' do - expect(@merge_request).to be_merged - expect(@merge_request.notes.last.note).to include('merged') + it 'updates the merge state' do + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + expect(@fork_merge_request.notes.last.note).to include('merged') + end + expect(@merge_request).to be_merged + expect(@merge_request.diffs.size).to be > 0 expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - - expect(empty_fork_merge_request).to be_open - expect(empty_fork_merge_request.merge_request_diff.state).to eq('empty') - expect(empty_fork_merge_request.notes).to be_empty + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done end end end - context 'manual merge of source branch', :sidekiq_might_not_need_inline do - before do - # Merge master -> feature branch - @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') - commit = @project.repository.commit('feature') - service.new(@project, @user).execute(@oldrev, commit.id, 'refs/heads/feature') - reload_mrs - end - - it 'updates the merge state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@merge_request.diffs.size).to be > 0 - expect(@fork_merge_request).to be_merged - expect(@fork_merge_request.notes.last.note).to include('merged') - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done - end - end - context 'push to fork repo source branch', :sidekiq_might_not_need_inline do let(:refresh_service) { service.new(@fork_project, @user) } @@ -583,20 +608,29 @@ describe MergeRequests::RefreshService do end end - context 'push to origin repo target branch after fork project was removed' do - before do - @fork_project.destroy - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - end + [true, false].each do |state_tracking_enabled| + context "push to origin repo target branch after fork project was removed #{state_tracking_enabled ? 'enabled' : 'disabled'}" do + before do + stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - it 'updates the merge request state' do - expect(@merge_request.notes.last.note).to include('merged') - expect(@merge_request).to be_merged - expect(@fork_merge_request).to be_open - expect(@fork_merge_request.notes).to be_empty - expect(@build_failed_todo).to be_done - expect(@fork_build_failed_todo).to be_done + @fork_project.destroy + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs + end + + it 'updates the merge request state' do + if state_tracking_enabled + expect(@merge_request.resource_state_events.last.state).to eq('merged') + else + expect(@merge_request.notes.last.note).to include('merged') + end + + expect(@merge_request).to be_merged + expect(@fork_merge_request).to be_open + expect(@fork_merge_request.notes).to be_empty + expect(@build_failed_todo).to be_done + expect(@fork_build_failed_todo).to be_done + end end end diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 25ab79d70c3..3807c44b01f 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -20,8 +20,11 @@ describe MergeRequests::ReopenService do context 'valid params' do let(:service) { described_class.new(project, user, {}) } + let(:state_tracking) { true } before do + stub_feature_flags(track_resource_state_change_events: state_tracking) + allow(service).to receive(:execute_hooks) perform_enqueued_jobs do @@ -43,9 +46,22 @@ describe MergeRequests::ReopenService do expect(email.subject).to include(merge_request.title) end - it 'creates system note about merge_request reopen' do - note = merge_request.notes.last - expect(note.note).to include 'reopened' + context 'note creation' do + context 'when state event tracking is disabled' do + let(:state_tracking) { false } + + it 'creates system note about merge_request reopen' do + note = merge_request.notes.last + expect(note.note).to include 'reopened' + end + end + + context 'when state event tracking is enabled' do + it 'creates resource state event about merge_request reopen' do + event = merge_request.resource_state_events.last + expect(event.state).to eq('reopened') + end + end end end |