diff options
Diffstat (limited to 'spec/services/merge_requests')
| -rw-r--r-- | spec/services/merge_requests/close_service_spec.rb | 67 | ||||
| -rw-r--r-- | spec/services/merge_requests/export_csv_service_spec.rb | 115 | ||||
| -rw-r--r-- | spec/services/merge_requests/ff_merge_service_spec.rb | 104 | ||||
| -rw-r--r-- | spec/services/merge_requests/merge_service_spec.rb | 22 | ||||
| -rw-r--r-- | spec/services/merge_requests/refresh_service_spec.rb | 139 | ||||
| -rw-r--r-- | spec/services/merge_requests/reopen_service_spec.rb | 20 | ||||
| -rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 53 |
7 files changed, 303 insertions, 217 deletions
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index e7ac286f48b..67fb4eaade5 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -19,54 +19,45 @@ RSpec.describe MergeRequests::CloseService do describe '#execute' do it_behaves_like 'cache counters invalidator' - [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, {}) } + context 'valid params' do + let(:service) { described_class.new(project, user, {}) } - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - - allow(service).to receive(:execute_hooks) + before do + allow(service).to receive(:execute_hooks) - perform_enqueued_jobs do - @merge_request = service.execute(merge_request) - end + perform_enqueued_jobs do + @merge_request = service.execute(merge_request) 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) - .with(@merge_request, 'close') - end + it 'executes hooks with close action' do + expect(service).to have_received(:execute_hooks) + .with(@merge_request, 'close') + 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 - 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 'creates a resource event' do + event = @merge_request.resource_state_events.last + expect(event.state).to eq('closed') + 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 - end + it 'cancels the auto merge' do + expect(@merge_request).not_to be_auto_merge_enabled end end end diff --git a/spec/services/merge_requests/export_csv_service_spec.rb b/spec/services/merge_requests/export_csv_service_spec.rb new file mode 100644 index 00000000000..8161a444231 --- /dev/null +++ b/spec/services/merge_requests/export_csv_service_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::ExportCsvService do + let_it_be(:merge_request) { create(:merge_request) } + let(:csv) { CSV.parse(subject.csv_data, headers: true).first } + + subject { described_class.new(MergeRequest.where(id: merge_request.id)) } + + describe 'csv_data' do + it 'contains the correct information', :aggregate_failures do + expect(csv['MR IID']).to eq(merge_request.iid.to_s) + expect(csv['Title']).to eq(merge_request.title) + expect(csv['State']).to eq(merge_request.state) + expect(csv['Description']).to eq(merge_request.description) + expect(csv['Source Branch']).to eq(merge_request.source_branch) + expect(csv['Target Branch']).to eq(merge_request.target_branch) + expect(csv['Source Project ID']).to eq(merge_request.source_project_id.to_s) + expect(csv['Target Project ID']).to eq(merge_request.target_project_id.to_s) + expect(csv['Author']).to eq(merge_request.author.name) + expect(csv['Author Username']).to eq(merge_request.author.username) + end + + describe 'assignees' do + context 'when assigned' do + let_it_be(:merge_request) { create(:merge_request, assignees: create_list(:user, 2)) } + + it 'contains the names of assignees' do + expect(csv['Assignees']).to eq(merge_request.assignees.map(&:name).join(', ')) + end + + it 'contains the usernames of assignees' do + expect(csv['Assignee Usernames']).to eq(merge_request.assignees.map(&:username).join(', ')) + end + end + + context 'when not assigned' do + it 'returns empty strings' do + expect(csv['Assignees']).to eq('') + expect(csv['Assignee Usernames']).to eq('') + end + end + end + + describe 'approvers' do + context 'when approved' do + let_it_be(:merge_request) { create(:merge_request) } + let(:approvers) { create_list(:user, 2) } + + before do + merge_request.approved_by_users = approvers + end + + it 'contains the names of approvers separated by a comma' do + expect(csv['Approvers'].split(', ')).to contain_exactly(approvers[0].name, approvers[1].name) + end + + it 'contains the usernames of approvers separated by a comma' do + expect(csv['Approver Usernames'].split(', ')).to contain_exactly(approvers[0].username, approvers[1].username) + end + end + + context 'when not approved' do + it 'returns empty strings' do + expect(csv['Approvers']).to eq('') + expect(csv['Approver Usernames']).to eq('') + end + end + end + + describe 'merged user' do + context 'MR is merged' do + let_it_be(:merge_request) { create(:merge_request, :merged, :with_merged_metrics) } + + it 'is merged' do + expect(csv['State']).to eq('merged') + end + + it 'has a merged user' do + expect(csv['Merged User']).to eq(merge_request.metrics.merged_by.name) + expect(csv['Merged Username']).to eq(merge_request.metrics.merged_by.username) + end + end + + context 'MR is not merged' do + it 'returns empty strings' do + expect(csv['Merged User']).to eq('') + expect(csv['Merged Username']).to eq('') + end + end + end + + describe 'milestone' do + context 'milestone is assigned' do + let_it_be(:merge_request) { create(:merge_request) } + let_it_be(:milestone) { create(:milestone, :active, project: merge_request.project) } + + before do + merge_request.update!(milestone_id: milestone.id) + end + + it 'contains the milestone ID' do + expect(csv['Milestone ID']).to eq(merge_request.milestone.id.to_s) + end + end + + context 'no milestone is assigned' do + it 'returns an empty string' do + expect(csv['Milestone ID']).to eq('') + end + end + end + end +end diff --git a/spec/services/merge_requests/ff_merge_service_spec.rb b/spec/services/merge_requests/ff_merge_service_spec.rb index 55856deeaca..64c473d947f 100644 --- a/spec/services/merge_requests/ff_merge_service_spec.rb +++ b/spec/services/merge_requests/ff_merge_service_spec.rb @@ -22,74 +22,72 @@ RSpec.describe MergeRequests::FfMergeService do end describe '#execute' do - [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 + 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) end + end - before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) + before do + allow(service).to receive(:execute_hooks) + end - allow(service).to receive(:execute_hooks) - end + it "does not create merge commit" do + execute_ff_merge - it "does not create merge commit" do - execute_ff_merge + 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 - 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 + expect(source_branch_sha).to eq(target_branch_sha) + end - expect(source_branch_sha).to eq(target_branch_sha) - end + it 'keeps the merge request valid' do + expect { execute_ff_merge } + .not_to change { merge_request.valid? } + end - it 'keeps the merge request valid' do - expect { execute_ff_merge } - .not_to change { merge_request.valid? } - end + it 'updates the merge request to merged' do + expect { execute_ff_merge } + .to change { merge_request.merged? } + .from(false) + .to(true) + end - it 'updates the merge request to merged' do - expect { execute_ff_merge } - .to change { merge_request.merged? } - .from(false) - .to(true) - end + it 'sends email to user2 about merge of new merge_request' do + execute_ff_merge - it 'sends email to user2 about merge of new merge_request' 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 - email = ActionMailer::Base.deliveries.last - expect(email.to.first).to eq(user2.email) - expect(email.subject).to include(merge_request.title) - end + it 'creates resource event about merge_request merge' do + execute_ff_merge - it 'creates system note about merge_request merge' do - execute_ff_merge + event = merge_request.resource_state_events.last + expect(event.state).to eq('merged') + 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 'does not update squash_commit_sha if it is not a squash' do + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - 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 }.not_to change { merge_request.squash_commit_sha } + expect(merge_request.in_progress_merge_commit_sha).to be_nil + end - it 'updates squash_commit_sha if it is a squash' do - merge_request.update!(squash: true) + it 'updates squash_commit_sha if it is a squash' do + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original - expect { execute_ff_merge } - .to change { merge_request.squash_commit_sha } - .from(nil) - end + merge_request.update!(squash: true) + + expect { execute_ff_merge } + .to change { merge_request.squash_commit_sha } + .from(nil) + + expect(merge_request.in_progress_merge_commit_sha).to be_nil end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 8328f461029..d0e3102f157 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -20,12 +20,9 @@ RSpec.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) + expect(merge_request).to receive(:update_and_mark_in_progress_merge_commit_sha).twice.and_call_original perform_enqueued_jobs do service.execute(merge_request) @@ -47,20 +44,9 @@ RSpec.describe MergeRequests::MergeService do end 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 + 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 diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index cace1e0bf09..ca0c4b29ebe 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -367,76 +367,58 @@ RSpec.describe MergeRequests::RefreshService do end end - [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 + context 'push to origin repo target branch', :sidekiq_might_not_need_inline do + context 'when all MRs to the target branch had diffs' do before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) + service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') + reload_mrs end - 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 - 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 + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') 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) - 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) + 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') + 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 + 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 + 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 + + expect(@merge_request.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') end end - context "manual merge of source branch #{state_tracking_enabled ? 'enabled' : 'disabled'}", :sidekiq_might_not_need_inline do + context 'manual merge of source branch', :sidekiq_might_not_need_inline do before do - stub_feature_flags(track_resource_state_change_events: state_tracking_enabled) - # Merge master -> feature branch @project.repository.merge(@user, @merge_request.diff_head_sha, @merge_request, 'Test message') commit = @project.repository.commit('feature') @@ -445,13 +427,8 @@ RSpec.describe MergeRequests::RefreshService do end 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.resource_state_events.last.state).to eq('merged') + expect(@fork_merge_request.resource_state_events.last.state).to eq('merged') expect(@merge_request).to be_merged expect(@merge_request.diffs.size).to be > 0 @@ -616,29 +593,21 @@ RSpec.describe MergeRequests::RefreshService do end 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) + 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 - @fork_project.destroy! - service.new(@project, @user).execute(@oldrev, @newrev, 'refs/heads/feature') - reload_mrs - end + it 'updates the merge request state' do + expect(@merge_request.resource_state_events.last.state).to eq('merged') - 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 + 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 diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 0066834180e..ffc2ebb344c 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -20,11 +20,8 @@ RSpec.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 @@ -47,20 +44,9 @@ RSpec.describe MergeRequests::ReopenService do end 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 + 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 diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 3c3e10495d3..ed8872b71f7 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -53,7 +53,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do title: 'New title', description: 'Also please fix', assignee_ids: [user.id], - reviewer_ids: [user.id], + reviewer_ids: [], state_event: 'close', label_ids: [label.id], target_branch: 'target', @@ -78,7 +78,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request).to be_valid expect(@merge_request.title).to eq('New title') expect(@merge_request.assignees).to match_array([user]) - expect(@merge_request.reviewers).to match_array([user]) + expect(@merge_request.reviewers).to match_array([]) expect(@merge_request).to be_closed expect(@merge_request.labels.count).to eq(1) expect(@merge_request.labels.first.title).to eq(label.name) @@ -116,6 +116,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do labels: [], mentioned_users: [user2], assignees: [user3], + reviewers: [], milestone: nil, total_time_spent: 0, description: "FYI #{user2.to_reference}" @@ -138,6 +139,35 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(note.note).to include "assigned to #{user.to_reference} and unassigned #{user3.to_reference}" end + context 'with reviewers' do + let(:opts) { { reviewer_ids: [user2.id] } } + + context 'when merge_request_reviewers feature is disabled' do + before(:context) do + stub_feature_flags(merge_request_reviewers: false) + end + + it 'does not create a system note about merge_request review request' do + note = find_note('review requested from') + + expect(note).to be_nil + end + end + + context 'when merge_request_reviewers feature is enabled' do + before(:context) do + stub_feature_flags(merge_request_reviewers: true) + end + + it 'creates system note about merge_request review request' do + note = find_note('requested review from') + + expect(note).not_to be_nil + expect(note.note).to include "requested review from #{user2.to_reference}" + end + end + end + it 'creates a resource label event' do event = merge_request.resource_label_events.last @@ -467,15 +497,15 @@ RSpec.describe MergeRequests::UpdateService, :mailer do end context 'when reviewers gets changed' do - before do + it 'marks pending todo as done' do update_merge_request({ reviewer_ids: [user2.id] }) - end - it 'marks pending todo as done' do expect(pending_todo.reload).to be_done end it 'creates a pending todo for new review request' do + update_merge_request({ reviewer_ids: [user2.id] }) + attributes = { project: project, author: user, @@ -488,6 +518,17 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(Todo.where(attributes).count).to eq 1 end + + it 'sends email reviewer change notifications to old and new reviewers', :sidekiq_might_not_need_inline do + merge_request.reviewers = [user2] + + perform_enqueued_jobs do + update_merge_request({ reviewer_ids: [user3.id] }) + end + + should_email(user2) + should_email(user3) + end end context 'when the milestone is removed' do @@ -542,7 +583,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer do context 'when the labels change' do before do - Timecop.freeze(1.minute.from_now) do + travel_to(1.minute.from_now) do update_merge_request({ label_ids: [label.id] }) end end |