diff options
Diffstat (limited to 'spec/services/merge_requests')
11 files changed, 122 insertions, 68 deletions
diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 69bab3b1ea4..9ae310d8cee 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe MergeRequests::AfterCreateService do + include AfterNextHelpers + let_it_be(:merge_request) { create(:merge_request) } subject(:after_create_service) do @@ -27,6 +29,14 @@ RSpec.describe MergeRequests::AfterCreateService do execute_service end + it 'calls the merge request activity counter' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_create_mr_action) + .with(user: merge_request.author) + + execute_service + end + it 'creates a new merge request notification' do expect(notification_service) .to receive(:new_merge_request).with(merge_request, merge_request.author) @@ -56,11 +66,15 @@ RSpec.describe MergeRequests::AfterCreateService do execute_service end - it 'records a namespace onboarding progress action' do - expect(NamespaceOnboardingAction).to receive(:create_action) - .with(merge_request.target_project.namespace, :merge_request_created).and_call_original + it 'registers an onboarding progress action' do + OnboardingProgress.onboard(merge_request.target_project.namespace) + + expect_next(OnboardingProgressService, merge_request.target_project.namespace) + .to receive(:execute).with(action: :merge_request_created).and_call_original + + execute_service - expect { execute_service }.to change(NamespaceOnboardingAction, :count).by(1) + expect(OnboardingProgress.completed?(merge_request.target_project.namespace, :merge_request_created)).to be(true) end end end diff --git a/spec/services/merge_requests/cleanup_refs_service_spec.rb b/spec/services/merge_requests/cleanup_refs_service_spec.rb index 38c0e204e54..a1822a4d5ba 100644 --- a/spec/services/merge_requests/cleanup_refs_service_spec.rb +++ b/spec/services/merge_requests/cleanup_refs_service_spec.rb @@ -91,6 +91,26 @@ RSpec.describe MergeRequests::CleanupRefsService do it_behaves_like 'service that does not clean up merge request refs' end + context 'when a git error is raised' do + context 'Gitlab::Git::Repository::GitError' do + before do + allow(merge_request.project.repository).to receive(:delete_refs).and_raise(Gitlab::Git::Repository::GitError) + end + + it_behaves_like 'service that does not clean up merge request refs' + end + + context 'Gitlab::Git::CommandError' do + before do + allow_next_instance_of(Gitlab::Git::KeepAround) do |keep_around| + expect(keep_around).to receive(:kept_around?).and_raise(Gitlab::Git::CommandError) + end + end + + it_behaves_like 'service that does not clean up merge request refs' + end + end + context 'when cleanup schedule fails to update' do before do allow(merge_request.cleanup_schedule).to receive(:update).and_return(false) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 67fb4eaade5..48f56b3ec68 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -18,6 +18,7 @@ RSpec.describe MergeRequests::CloseService do describe '#execute' do it_behaves_like 'cache counters invalidator' + it_behaves_like 'merge request reviewers cache counters invalidator' context 'valid params' do let(:service) { described_class.new(project, user, {}) } @@ -75,6 +76,14 @@ RSpec.describe MergeRequests::CloseService do described_class.new(project, user, {}).execute(merge_request) end + it 'calls the merge request activity counter' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_close_mr_action) + .with(user: user) + + described_class.new(project, user, {}).execute(merge_request) + end + it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do service = described_class.new(project, user, {}) diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index 86e49fe601c..be8c41bc4a1 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -66,10 +66,11 @@ RSpec.describe MergeRequests::CreateFromIssueService do expect { service.execute }.to change(target_project.merge_requests, :count).by(1) end - it 'sets the merge request author to current user', :sidekiq_might_not_need_inline do + it 'sets the merge request author to current user and assigns them', :sidekiq_might_not_need_inline do result = service.execute expect(result[:merge_request].author).to eq(user) + expect(result[:merge_request].assignees).to eq([user]) end it 'sets the merge request source branch to the new issue branch', :sidekiq_might_not_need_inline do diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index d042b318d02..4f47a22b07c 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -155,6 +155,12 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do expect(Todo.where(attributes).count).to eq 1 end + + it 'invalidates counter cache for reviewers', :use_clean_rails_memory_store_caching do + expect { merge_request } + .to change { user2.review_requested_open_merge_requests_count } + .by(1) + end end context 'when head pipelines already exist for merge request source branch', :sidekiq_inline do diff --git a/spec/services/merge_requests/export_csv_service_spec.rb b/spec/services/merge_requests/export_csv_service_spec.rb index ecb17b3fe77..4ce032c396e 100644 --- a/spec/services/merge_requests/export_csv_service_spec.rb +++ b/spec/services/merge_requests/export_csv_service_spec.rb @@ -27,11 +27,11 @@ RSpec.describe MergeRequests::ExportCsvService 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(', ')) + expect(csv['Assignees'].split(', ')).to match_array(merge_request.assignees.map(&:name)) end it 'contains the usernames of assignees' do - expect(csv['Assignee Usernames']).to eq(merge_request.assignees.map(&:username).join(', ')) + expect(csv['Assignee Usernames'].split(', ')).to match_array(merge_request.assignees.map(&:username)) end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index d0e3102f157..dd37d87e3f5 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -37,6 +37,10 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request.in_progress_merge_commit_sha).to be_nil end + it 'does not update squash_commit_sha if it is not a squash' do + expect(merge_request.squash_commit_sha).to be_nil + end + it 'sends email to user2 about merge of new merge_request' do email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) @@ -76,6 +80,12 @@ RSpec.describe MergeRequests::MergeService do expect(merge_commit.message).to eq('Merge commit message') expect(squash_commit.message).to eq("Squash commit message\n") end + + it 'persists squash_commit_sha' do + squash_commit = merge_request.merge_commit.parents.last + + expect(merge_request.squash_commit_sha).to eq(squash_commit.id) + end end end @@ -361,6 +371,7 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end @@ -381,6 +392,7 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end @@ -395,10 +407,27 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil expect(merge_request.merge_error).to include(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 + error_message = 'error message' + + allow(service).to receive(:repository).and_raise(Gitlab::Git::PreReceiveError, "GitLab: #{error_message}") + allow(service).to receive(:execute_hooks) + merge_request.update!(squash: true) + + service.execute(merge_request) + + expect(merge_request).to be_open + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil + expect(merge_request.merge_error).to include('Something went wrong during merge pre-receive hook') + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + context 'when fast-forward merge is not allowed' do before do allow_any_instance_of(Repository).to receive(:ancestor?).and_return(nil) @@ -415,6 +444,7 @@ RSpec.describe MergeRequests::MergeService do expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.squash_commit_sha).to be_nil expect(merge_request.merge_error).to include(error_message) expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) end diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index 725fc16fa7c..17bfa9d7368 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -124,14 +124,6 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar it_behaves_like 'mergeable merge request' - context 'when lock is disabled' do - before do - stub_feature_flags(merge_ref_auto_sync_lock: false) - end - - it_behaves_like 'mergeable merge request' - end - context 'when concurrent calls' do it 'waits first lock and returns "cached" result in subsequent calls' do threads = execute_within_threads(amount: 3) @@ -167,25 +159,6 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar end end - context 'disabled merge ref sync feature flag' do - before do - stub_feature_flags(merge_ref_auto_sync: false) - end - - it 'returns error and no payload' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge ref is outdated due to disabled feature') - expect(result.payload).to be_empty - end - - it 'ignores merge-ref and updates merge status' do - expect { subject }.to change(merge_request, :merge_status).from('unchecked').to('can_be_merged') - end - end - context 'when broken' do before do expect(merge_request).to receive(:broken?) { true } @@ -305,28 +278,6 @@ RSpec.describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shar context 'recheck enforced' do subject { described_class.new(merge_request).execute(recheck: true) } - context 'when MR is mergeable and merge-ref auto-sync is disabled' do - before do - stub_feature_flags(merge_ref_auto_sync: false) - merge_request.mark_as_mergeable! - end - - it 'returns ServiceResponse.error' do - result = subject - - expect(result).to be_a(ServiceResponse) - expect(result.error?).to be(true) - expect(result.message).to eq('Merge ref is outdated due to disabled feature') - expect(result.payload).to be_empty - end - - it 'merge status is not changed' do - subject - - expect(merge_request.merge_status).to eq('can_be_merged') - end - end - context 'when MR is marked as mergeable, but repo is not mergeable and MR is not opened' do before do # Making sure that we don't touch the merge-status after diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb index 402f753c0af..6523b5a158c 100644 --- a/spec/services/merge_requests/post_merge_service_spec.rb +++ b/spec/services/merge_requests/post_merge_service_spec.rb @@ -15,6 +15,7 @@ RSpec.describe MergeRequests::PostMergeService do describe '#execute' do it_behaves_like 'cache counters invalidator' + it_behaves_like 'merge request reviewers cache counters invalidator' it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do # Cache the counter before the MR changed state. @@ -37,6 +38,14 @@ RSpec.describe MergeRequests::PostMergeService do subject end + it 'calls the merge request activity counter' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_merge_mr_action) + .with(user: user) + + subject + end + it 'deletes non-latest diffs' do diff_removal_service = instance_double(MergeRequests::DeleteNonLatestDiffsService, execute: nil) diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb index 2bd83dc36a8..8541d597581 100644 --- a/spec/services/merge_requests/reopen_service_spec.rb +++ b/spec/services/merge_requests/reopen_service_spec.rb @@ -17,6 +17,7 @@ RSpec.describe MergeRequests::ReopenService do describe '#execute' do it_behaves_like 'cache counters invalidator' + it_behaves_like 'merge request reviewers cache counters invalidator' context 'valid params' do let(:service) { described_class.new(project, user, {}) } @@ -80,6 +81,14 @@ RSpec.describe MergeRequests::ReopenService do described_class.new(project, user, {}).execute(merge_request) end + it 'calls the merge request activity counter' do + expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) + .to receive(:track_reopen_mr_action) + .with(user: user) + + described_class.new(project, user, {}).execute(merge_request) + end + it 'refreshes the number of open merge requests for a valid MR' do service = described_class.new(project, user, {}) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index ed8872b71f7..9eb82dcd0ad 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -431,14 +431,6 @@ RSpec.describe MergeRequests::UpdateService, :mailer do describe 'merge' do it_behaves_like 'correct merge behavior' - - context 'when merge_orchestration_service feature flag is disabled' do - before do - stub_feature_flags(merge_orchestration_service: false) - end - - it_behaves_like 'correct merge behavior' - end end context 'todos' do @@ -529,6 +521,19 @@ RSpec.describe MergeRequests::UpdateService, :mailer do should_email(user2) should_email(user3) end + + it 'updates open merge request counter for reviewers', :use_clean_rails_memory_store_caching do + merge_request.reviewers = [user3] + + # Cache them to ensure the cache gets invalidated on update + expect(user2.review_requested_open_merge_requests_count).to eq(0) + expect(user3.review_requested_open_merge_requests_count).to eq(1) + + update_merge_request(reviewer_ids: [user2.id]) + + expect(user2.review_requested_open_merge_requests_count).to eq(1) + expect(user3.review_requested_open_merge_requests_count).to eq(0) + end end context 'when the milestone is removed' do @@ -837,20 +842,20 @@ RSpec.describe MergeRequests::UpdateService, :mailer do it 'does not allow a maintainer of the target project to set `allow_collaboration`' do target_project.add_developer(user) - update_merge_request(allow_collaboration: true, title: 'Updated title') + update_merge_request(allow_collaboration: false, title: 'Updated title') expect(merge_request.title).to eq('Updated title') - expect(merge_request.allow_collaboration).to be_falsy + expect(merge_request.allow_collaboration).to be_truthy end it 'is allowed by a user that can push to the source and can update the merge request' do merge_request.update!(assignees: [user]) source_project.add_developer(user) - update_merge_request(allow_collaboration: true, title: 'Updated title') + update_merge_request(allow_collaboration: false, title: 'Updated title') expect(merge_request.title).to eq('Updated title') - expect(merge_request.allow_collaboration).to be_truthy + expect(merge_request.allow_collaboration).to be_falsy end end |