diff options
Diffstat (limited to 'spec/services/merge_requests')
14 files changed, 271 insertions, 31 deletions
diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 4d20d62b864..9b064da44b8 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -61,7 +61,7 @@ RSpec.describe MergeRequests::ApprovalService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request, user: user) + .with(project: project, current_user: user, merge_request: merge_request) .and_call_original service.execute(merge_request) diff --git a/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb b/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb index ae8846974ce..b2326a28e63 100644 --- a/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/bulk_remove_attention_requested_service_spec.rb @@ -40,6 +40,10 @@ RSpec.describe MergeRequests::BulkRemoveAttentionRequestedService do expect(reviewer.state).to eq 'reviewed' expect(assignee.state).to eq 'reviewed' end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [assignee_user, user] } + 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 a196c944eda..49f691e97e2 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -454,7 +454,7 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end - context 'when source and target projects are different' do + shared_examples 'when source and target projects are different' do let(:target_project) { fork_project(project, nil, repository: true) } let(:opts) do @@ -497,9 +497,14 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end it 'creates the merge request', :sidekiq_might_not_need_inline do + expect_next_instance_of(MergeRequest) do |instance| + expect(instance).to receive(:eager_fetch_ref!).and_call_original + end + merge_request = described_class.new(project: project, current_user: user, params: opts).execute expect(merge_request).to be_persisted + expect(merge_request.iid).to be > 0 end it 'does not create the merge request when the target project is archived' do @@ -511,6 +516,8 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end + it_behaves_like 'when source and target projects are different' + context 'when user sets source project id' do let(:another_project) { create(:project) } diff --git a/spec/services/merge_requests/handle_assignees_change_service_spec.rb b/spec/services/merge_requests/handle_assignees_change_service_spec.rb index fa3b1614e21..26f53f55b0f 100644 --- a/spec/services/merge_requests/handle_assignees_change_service_spec.rb +++ b/spec/services/merge_requests/handle_assignees_change_service_spec.rb @@ -89,12 +89,18 @@ RSpec.describe MergeRequests::HandleAssigneesChangeService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: user, merge_request: merge_request, user: user) + .with(project: project, current_user: user, merge_request: merge_request) .and_call_original execute end + it 'updates attention requested by of assignee' do + execute + + expect(merge_request.find_assignee(assignee).updated_state_by).to eq(user) + end + it 'tracks users assigned event' do expect(Gitlab::UsageDataCounters::MergeRequestActivityUniqueCounter) .to receive(:track_users_assigned_to_mr).once.with(users: [assignee]) diff --git a/spec/services/merge_requests/merge_orchestration_service_spec.rb b/spec/services/merge_requests/merge_orchestration_service_spec.rb index da37cc97857..ebcd2f0e277 100644 --- a/spec/services/merge_requests/merge_orchestration_service_spec.rb +++ b/spec/services/merge_requests/merge_orchestration_service_spec.rb @@ -64,7 +64,7 @@ RSpec.describe MergeRequests::MergeOrchestrationService do context 'when merge request is not mergeable' do before do - allow(merge_request).to receive(:mergeable_state?) { false } + allow(merge_request).to receive(:mergeable?) { false } end it 'does nothing' do @@ -87,7 +87,7 @@ RSpec.describe MergeRequests::MergeOrchestrationService do context 'when merge request is not mergeable' do before do - allow(merge_request).to receive(:mergeable_state?) { false } + allow(merge_request).to receive(:mergeable?) { false } end it { is_expected.to eq(false) } diff --git a/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb new file mode 100644 index 00000000000..9e178c121ef --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_broken_status_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckBrokenStatusService do + subject(:check_broken_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + before do + expect(merge_request).to receive(:broken?).and_return(broken) + end + + context 'when the merge request is broken' do + let(:broken) { true } + + it 'returns a check result with status failed' do + expect(check_broken_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + + context 'when the merge request is not broken' do + let(:broken) { false } + + it 'returns a check result with status success' do + expect(check_broken_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_broken_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_broken_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb new file mode 100644 index 00000000000..c24d40967c4 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_discussions_status_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckDiscussionsStatusService do + subject(:check_discussions_status) { described_class.new(merge_request: merge_request, params: params) } + + let(:merge_request) { build(:merge_request) } + let(:params) { { skip_discussions_check: skip_check } } + let(:skip_check) { false } + + describe '#execute' do + before do + expect(merge_request).to receive(:mergeable_discussions_state?).and_return(mergeable) + end + + context 'when the merge request is in a mergable state' do + let(:mergeable) { true } + + it 'returns a check result with status success' do + expect(check_discussions_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when the merge request is not in a mergeable state' do + let(:mergeable) { false } + + it 'returns a check result with status failed' do + expect(check_discussions_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + end + + describe '#skip?' do + context 'when skip check is true' do + let(:skip_check) { true } + + it 'returns true' do + expect(check_discussions_status.skip?).to eq true + end + end + + context 'when skip check is false' do + let(:skip_check) { false } + + it 'returns false' do + expect(check_discussions_status.skip?).to eq false + end + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_discussions_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb new file mode 100644 index 00000000000..923cff220ef --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckDraftStatusService do + subject(:check_draft_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + before do + expect(merge_request).to receive(:draft?).and_return(draft) + end + + context 'when the merge request is a draft' do + let(:draft) { true } + + it 'returns a check result with status failed' do + expect(check_draft_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + + context 'when the merge request is not a draft' do + let(:draft) { false } + + it 'returns a check result with status success' do + expect(check_draft_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_draft_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_draft_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb new file mode 100644 index 00000000000..b1c9a930317 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_open_status_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckOpenStatusService do + subject(:check_open_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + before do + expect(merge_request).to receive(:open?).and_return(open) + end + + context 'when the merge request is open' do + let(:open) { true } + + it 'returns a check result with status success' do + expect(check_open_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when the merge request is not open' do + let(:open) { false } + + it 'returns a check result with status failed' do + expect(check_open_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_open_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_open_status.cacheable?).to eq false + end + end +end diff --git a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb index 71ad23bc68c..d4ee4afd71d 100644 --- a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb +++ b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb @@ -35,12 +35,19 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do context 'when a check is skipped' do it 'does not execute the check' do + described_class::CHECKS.each do |check| + allow_next_instance_of(check) do |service| + allow(service).to receive(:skip?).and_return(false) + allow(service).to receive(:execute).and_return(success_result) + end + end + expect_next_instance_of(MergeRequests::Mergeability::CheckCiStatusService) do |service| expect(service).to receive(:skip?).and_return(true) expect(service).not_to receive(:execute) end - expect(execute).to match_array([]) + expect(execute).to match_array([success_result, success_result, success_result, success_result]) end end @@ -49,6 +56,12 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService do let(:merge_check) { instance_double(MergeRequests::Mergeability::CheckCiStatusService) } before do + described_class::CHECKS.each do |check| + allow_next_instance_of(check) do |service| + allow(service).to receive(:skip?).and_return(true) + end + end + expect(MergeRequests::Mergeability::CheckCiStatusService).to receive(:new).and_return(merge_check) expect(merge_check).to receive(:skip?).and_return(false) allow(merge_check).to receive(:cacheable?).and_return(cacheable) diff --git a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb index b333d4af6cf..20b5cf5e3a1 100644 --- a/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb +++ b/spec/services/merge_requests/reload_merge_head_diff_service_spec.rb @@ -47,15 +47,5 @@ RSpec.describe MergeRequests::ReloadMergeHeadDiffService do expect(merge_request.reload.merge_head_diff).not_to eq(existing_merge_head_diff) end end - - context 'when default_merge_ref_for_diffs feature flag is disabled' do - before do - stub_feature_flags(default_merge_ref_for_diffs: false) - end - - it 'returns error' do - expect(subject[:status]).to eq(:error) - end - end end end diff --git a/spec/services/merge_requests/remove_attention_requested_service_spec.rb b/spec/services/merge_requests/remove_attention_requested_service_spec.rb index 875afc2dc7e..450204ebfdd 100644 --- a/spec/services/merge_requests/remove_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/remove_attention_requested_service_spec.rb @@ -4,23 +4,20 @@ require 'spec_helper' RSpec.describe MergeRequests::RemoveAttentionRequestedService do let(:current_user) { create(:user) } - let(:user) { create(:user) } - let(:assignee_user) { create(:user) } - let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [assignee_user]) } - let(:reviewer) { merge_request.find_reviewer(user) } - let(:assignee) { merge_request.find_assignee(assignee_user) } + let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } + let(:reviewer) { merge_request.find_reviewer(current_user) } + let(:assignee) { merge_request.find_assignee(current_user) } let(:project) { merge_request.project } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: user) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } let(:result) { service.execute } before do project.add_developer(current_user) - project.add_developer(user) end describe '#execute' do context 'invalid permissions' do - let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request, user: user) } + let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } it 'returns an error' do expect(result[:status]).to eq :error @@ -28,7 +25,7 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do end context 'reviewer does not exist' do - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: create(:user)) } + let(:service) { described_class.new(project: project, current_user: create(:user), merge_request: merge_request) } it 'returns an error' do expect(result[:status]).to eq :error @@ -46,10 +43,14 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do expect(reviewer.state).to eq 'reviewed' end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [current_user] } + end end context 'assignee exists' do - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: assignee_user) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } before do assignee.update!(state: :reviewed) @@ -65,12 +66,16 @@ RSpec.describe MergeRequests::RemoveAttentionRequestedService do expect(assignee.state).to eq 'reviewed' end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [current_user] } + end end context 'assignee is the same as reviewer' do - let(:merge_request) { create(:merge_request, reviewers: [user], assignees: [user]) } - let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request, user: user) } - let(:assignee) { merge_request.find_assignee(user) } + let(:merge_request) { create(:merge_request, reviewers: [current_user], assignees: [current_user]) } + let(:service) { described_class.new(project: project, current_user: current_user, merge_request: merge_request) } + let(:assignee) { merge_request.find_assignee(current_user) } it 'updates reviewers and assignees state' do service.execute diff --git a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb index 63fa61b8097..dcaac5d2699 100644 --- a/spec/services/merge_requests/toggle_attention_requested_service_spec.rb +++ b/spec/services/merge_requests/toggle_attention_requested_service_spec.rb @@ -59,6 +59,13 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do expect(reviewer.state).to eq 'attention_requested' end + it 'adds who toggled attention' do + service.execute + reviewer.reload + + expect(reviewer.updated_state_by).to eq current_user + end + it 'creates a new todo for the reviewer' do expect(todo_service).to receive(:create_attention_requested_todo).with(merge_request, current_user, user) @@ -73,11 +80,21 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .with(project: project, current_user: current_user, merge_request: merge_request) .and_call_original service.execute end + + it 'invalidates cache' do + cache_mock = double + + expect(cache_mock).to receive(:delete).with(['users', user.id, 'attention_requested_open_merge_requests_count']) + + allow(Rails).to receive(:cache).and_return(cache_mock) + + service.execute + end end context 'assignee exists' do @@ -112,11 +129,15 @@ RSpec.describe MergeRequests::ToggleAttentionRequestedService do it 'removes attention requested state' do expect(MergeRequests::RemoveAttentionRequestedService).to receive(:new) - .with(project: project, current_user: current_user, merge_request: merge_request, user: current_user) + .with(project: project, current_user: current_user, merge_request: merge_request) .and_call_original service.execute end + + it_behaves_like 'invalidates attention request cache' do + let(:users) { [assignee_user] } + end end context 'assignee is the same as reviewer' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 48d9f019274..eb587797201 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -215,6 +215,14 @@ RSpec.describe MergeRequests::UpdateService, :mailer do MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) end + + it 'updates attention requested by of reviewer' do + opts[:reviewers] = [user2] + + MergeRequests::UpdateService.new(project: project, current_user: user, params: opts).execute(merge_request) + + expect(merge_request.find_reviewer(user2).updated_state_by).to eq(user) + end end context 'when reviewers did not change' do |