diff options
Diffstat (limited to 'spec/services/merge_requests')
8 files changed, 250 insertions, 116 deletions
diff --git a/spec/services/merge_requests/approval_service_spec.rb b/spec/services/merge_requests/approval_service_spec.rb index 81fc5661032..e7fe5c19fa3 100644 --- a/spec/services/merge_requests/approval_service_spec.rb +++ b/spec/services/merge_requests/approval_service_spec.rb @@ -82,39 +82,12 @@ RSpec.describe MergeRequests::ApprovalService, feature_category: :code_review_wo it 'records a value' do service.execute(merge_request) - expect(merge_request.approvals.last.patch_id_sha).not_to be_nil + expect(merge_request.approvals.last.patch_id_sha).to eq(merge_request.current_patch_id_sha) end - context 'when base_sha is nil' do + context 'when MergeRequest#current_patch_id_sha is nil' do it 'records patch_id_sha as nil' do - expect_next_instance_of(Gitlab::Diff::DiffRefs) do |diff_ref| - expect(diff_ref).to receive(:base_sha).at_least(:once).and_return(nil) - end - - service.execute(merge_request) - - expect(merge_request.approvals.last.patch_id_sha).to be_nil - end - end - - context 'when head_sha is nil' do - it 'records patch_id_sha as nil' do - expect_next_instance_of(Gitlab::Diff::DiffRefs) do |diff_ref| - expect(diff_ref).to receive(:head_sha).at_least(:once).and_return(nil) - end - - service.execute(merge_request) - - expect(merge_request.approvals.last.patch_id_sha).to be_nil - end - end - - context 'when base_sha and head_sha match' do - it 'records patch_id_sha as nil' do - expect_next_instance_of(Gitlab::Diff::DiffRefs) do |diff_ref| - expect(diff_ref).to receive(:base_sha).at_least(:once).and_return("abc123") - expect(diff_ref).to receive(:head_sha).at_least(:once).and_return("abc123") - end + expect(merge_request).to receive(:current_patch_id_sha).and_return(nil) service.execute(merge_request) diff --git a/spec/services/merge_requests/create_ref_service_spec.rb b/spec/services/merge_requests/create_ref_service_spec.rb index 5f7b9430416..b99187f9a56 100644 --- a/spec/services/merge_requests/create_ref_service_spec.rb +++ b/spec/services/merge_requests/create_ref_service_spec.rb @@ -246,13 +246,13 @@ RSpec.describe MergeRequests::CreateRefService, feature_category: :merge_trains expect_next_instance_of(described_class) do |instance| original = instance.method(:maybe_merge!) - expect(instance).to receive(:maybe_merge!) do |*args| + expect(instance).to receive(:maybe_merge!) do |*args, **kwargs| # Corrupt target_ref before the merge, simulating a race with # another instance of the service for the same MR. source_sha is # just an arbitrary valid commit that differs from what was just # written. project.repository.write_ref(target_ref, source_sha) - original.call(*args) + original.call(*args, **kwargs) end end 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 d9e60911ada..7ce2317918d 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 @@ -17,7 +17,8 @@ RSpec.describe MergeRequests::DeleteNonLatestDiffsService, :clean_gitlab_redis_s merge_request.reset end - it 'schedules non-latest merge request diffs removal' do + it 'schedules non-latest merge request diffs removal', + quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/426807' do diffs = merge_request.merge_request_diffs expect(diffs.count).to eq(4) diff --git a/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb new file mode 100644 index 00000000000..14173c19bfb --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_conflict_status_service_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckConflictStatusService, feature_category: :code_review_workflow do + subject(:check_conflict_status) { described_class.new(merge_request: merge_request, params: {}) } + + let(:merge_request) { build(:merge_request) } + + describe '#execute' do + let(:result) { check_conflict_status.execute } + + before do + allow(merge_request).to receive(:can_be_merged?).and_return(can_be_merged) + end + + context 'when MergeRequest#can_be_merged is true' do + let(:can_be_merged) { true } + + it 'returns a check result with status success' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + + context 'when MergeRequest#can_be_merged is false' do + let(:can_be_merged) { false } + + it 'returns a check result with status failed' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + expect(result.payload[:reason]).to eq(:conflict) + end + end + end + + describe '#skip?' do + it 'returns false' do + expect(check_conflict_status.skip?).to eq false + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_conflict_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 index cb624705a02..3837022232d 100644 --- a/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb +++ b/spec/services/merge_requests/mergeability/check_draft_status_service_spec.rb @@ -3,9 +3,12 @@ require 'spec_helper' RSpec.describe MergeRequests::Mergeability::CheckDraftStatusService, feature_category: :code_review_workflow do - subject(:check_draft_status) { described_class.new(merge_request: merge_request, params: {}) } + subject(:check_draft_status) { described_class.new(merge_request: merge_request, params: params) } - let(:merge_request) { build(:merge_request) } + let_it_be(:merge_request) { build(:merge_request) } + + let(:params) { { skip_draft_check: skip_check } } + let(:skip_check) { false } describe '#execute' do let(:result) { check_draft_status.execute } @@ -33,8 +36,20 @@ RSpec.describe MergeRequests::Mergeability::CheckDraftStatusService, feature_cat end describe '#skip?' do - it 'returns false' do - expect(check_draft_status.skip?).to eq false + context 'when skip check param is true' do + let(:skip_check) { true } + + it 'returns true' do + expect(check_draft_status.skip?).to eq true + end + end + + context 'when skip check param is false' do + let(:skip_check) { false } + + it 'returns false' do + expect(check_draft_status.skip?).to eq false + end end end diff --git a/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb b/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb new file mode 100644 index 00000000000..31ec44856b1 --- /dev/null +++ b/spec/services/merge_requests/mergeability/check_rebase_status_service_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::Mergeability::CheckRebaseStatusService, feature_category: :code_review_workflow do + subject(:check_rebase_status) { described_class.new(merge_request: merge_request, params: params) } + + let(:merge_request) { build(:merge_request) } + let(:params) { { skip_rebase_check: skip_check } } + let(:skip_check) { false } + + describe '#execute' do + let(:result) { check_rebase_status.execute } + + before do + allow(merge_request).to receive(:should_be_rebased?).and_return(should_be_rebased) + end + + context 'when the merge request should be rebased' do + let(:should_be_rebased) { true } + + it 'returns a check result with status failed' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS + expect(result.payload[:reason]).to eq :need_rebase + end + end + + context 'when the merge request should not be rebased' do + let(:should_be_rebased) { false } + + it 'returns a check result with status success' do + expect(result.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS + end + end + end + + describe '#skip?' do + context 'when skip check is true' do + let(:skip_check) { true } + + it 'returns true' do + expect(check_rebase_status.skip?).to eq true + end + end + + context 'when skip check is false' do + let(:skip_check) { false } + + it 'returns false' do + expect(check_rebase_status.skip?).to eq false + end + end + end + + describe '#cacheable?' do + it 'returns false' do + expect(check_rebase_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 bfff582994b..546d583a2fb 100644 --- a/spec/services/merge_requests/mergeability/run_checks_service_spec.rb +++ b/spec/services/merge_requests/mergeability/run_checks_service_spec.rb @@ -3,16 +3,32 @@ require 'spec_helper' RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redis_cache, feature_category: :code_review_workflow do + let(:checks) { MergeRequest.all_mergeability_checks } + let(:execute_all) { false } + subject(:run_checks) { described_class.new(merge_request: merge_request, params: {}) } describe '#execute' do - subject(:execute) { run_checks.execute } + subject(:execute) { run_checks.execute(checks, execute_all: execute_all) } let_it_be(:merge_request) { create(:merge_request) } let(:params) { {} } let(:success_result) { Gitlab::MergeRequests::Mergeability::CheckResult.success } + shared_examples 'checks are all executed' do + context 'when all checks are set to be executed' do + let(:execute_all) { true } + + specify do + result = execute + + expect(result.success?).to eq(success?) + expect(result.payload[:results].count).to eq(expected_count) + end + end + end + context 'when every check is skipped', :eager_load do before do MergeRequests::Mergeability::CheckBaseService.subclasses.each do |subclass| @@ -25,17 +41,28 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi it 'is still a success' do expect(execute.success?).to eq(true) end + + it_behaves_like 'checks are all executed' do + let(:success?) { true } + let(:expected_count) { 0 } + end end context 'when a check is skipped' do - it 'does not execute the check' do - merge_request.mergeability_checks.each do |check| + before do + 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 + allow_next_instance_of(MergeRequests::Mergeability::CheckCiStatusService) do |service| + allow(service).to receive(:skip?).and_return(true) + end + end + + it 'does not execute the check' do expect_next_instance_of(MergeRequests::Mergeability::CheckCiStatusService) do |service| expect(service).to receive(:skip?).and_return(true) expect(service).not_to receive(:execute) @@ -43,6 +70,34 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi expect(execute.success?).to eq(true) end + + it_behaves_like 'checks are all executed' do + let(:success?) { true } + let(:expected_count) { checks.count - 1 } + end + + context 'when one check fails' do + let(:failed_result) { Gitlab::MergeRequests::Mergeability::CheckResult.failed(payload: { reason: 'failed' }) } + + before do + allow_next_instance_of(MergeRequests::Mergeability::CheckOpenStatusService) do |service| + allow(service).to receive(:skip?).and_return(false) + allow(service).to receive(:execute).and_return(failed_result) + end + end + + it 'returns the failure reason' do + result = execute + + expect(result.success?).to eq(false) + expect(execute.payload[:failure_reason]).to eq(:failed) + end + + it_behaves_like 'checks are all executed' do + let(:success?) { false } + let(:expected_count) { checks.count - 1 } + end + end end context 'when a check is not skipped' do @@ -50,7 +105,7 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi let(:merge_check) { instance_double(MergeRequests::Mergeability::CheckCiStatusService) } before do - merge_request.mergeability_checks.each do |check| + checks.each do |check| allow_next_instance_of(check) do |service| allow(service).to receive(:skip?).and_return(true) end @@ -64,11 +119,13 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi context 'when the check is cacheable' do context 'when the check is cached' do - it 'returns the cached result' do + before do expect_next_instance_of(Gitlab::MergeRequests::Mergeability::ResultsStore) do |service| expect(service).to receive(:read).with(merge_check: merge_check).and_return(success_result) end + end + it 'returns the cached result' do expect_next_instance_of(MergeRequests::Mergeability::Logger, merge_request: merge_request) do |logger| expect(logger).to receive(:instrument).with(mergeability_name: 'check_ci_status_service').and_call_original expect(logger).to receive(:commit) @@ -76,15 +133,22 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi expect(execute.success?).to eq(true) end + + it_behaves_like 'checks are all executed' do + let(:success?) { true } + let(:expected_count) { 1 } + end end context 'when the check is not cached' do - it 'writes and returns the result' do + before do expect_next_instance_of(Gitlab::MergeRequests::Mergeability::ResultsStore) do |service| expect(service).to receive(:read).with(merge_check: merge_check).and_return(nil) expect(service).to receive(:write).with(merge_check: merge_check, result_hash: success_result.to_hash).and_return(true) end + end + it 'writes and returns the result' do expect_next_instance_of(MergeRequests::Mergeability::Logger, merge_request: merge_request) do |logger| expect(logger).to receive(:instrument).with(mergeability_name: 'check_ci_status_service').and_call_original expect(logger).to receive(:commit) @@ -92,6 +156,11 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi expect(execute.success?).to eq(true) end + + it_behaves_like 'checks are all executed' do + let(:success?) { true } + let(:expected_count) { 1 } + end end end @@ -106,76 +175,4 @@ RSpec.describe MergeRequests::Mergeability::RunChecksService, :clean_gitlab_redi end end end - - describe '#success?' do - subject(:success) { run_checks.success? } - - let_it_be(:merge_request) { create(:merge_request) } - - context 'when the execute method has been executed' do - before do - run_checks.execute - end - - context 'when all the checks succeed' do - it 'returns true' do - expect(success).to eq(true) - end - end - - context 'when one check fails' do - before do - allow(merge_request).to receive(:open?).and_return(false) - run_checks.execute - end - - it 'returns false' do - expect(success).to eq(false) - end - end - end - - context 'when execute has not been exectued' do - it 'raises an error' do - expect { subject } - .to raise_error(/Execute needs to be called before/) - end - end - end - - describe '#failure_reason' do - subject(:failure_reason) { run_checks.failure_reason } - - let_it_be(:merge_request) { create(:merge_request) } - - context 'when the execute method has been executed' do - context 'when all the checks succeed' do - before do - run_checks.execute - end - - it 'returns nil' do - expect(failure_reason).to eq(nil) - end - end - - context 'when one check fails' do - before do - allow(merge_request).to receive(:open?).and_return(false) - run_checks.execute - end - - it 'returns the open reason' do - expect(failure_reason).to eq(:not_open) - end - end - end - - context 'when execute has not been exectued' do - it 'raises an error' do - expect { subject } - .to raise_error(/Execute needs to be called before/) - end - end - end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 72e41f7b814..f5494f429c3 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -788,7 +788,7 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re update_merge_request({ label_ids: [label.id] }) end - expect(merge_request.reload.updated_at).to be > Time.current + expect(merge_request.reload.updated_at).to be_future end end @@ -897,6 +897,27 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re update_merge_request(title: 'New title') end + context 'when additional_merge_when_checks_ready is enabled' do + it 'publishes a DraftStateChangeEvent' do + expected_data = { + current_user_id: user.id, + merge_request_id: merge_request.id + } + + expect { update_merge_request(title: 'New title') }.to publish_event(MergeRequests::DraftStateChangeEvent).with(expected_data) + end + end + + context 'when additional_merge_when_checks_ready is disabled' do + before do + stub_feature_flags(additional_merge_when_checks_ready: false) + end + + it 'does not publish a DraftStateChangeEvent' do + expect { update_merge_request(title: 'New title') }.not_to publish_event(MergeRequests::DraftStateChangeEvent) + end + end + context 'when removing through wip_event param' do it 'removes Draft from the title' do expect { update_merge_request({ wip_event: "ready" }) } @@ -923,6 +944,27 @@ RSpec.describe MergeRequests::UpdateService, :mailer, feature_category: :code_re should_not_email(non_subscriber) end + context 'when additional_merge_when_checks_ready is enabled' do + it 'publishes a DraftStateChangeEvent' do + expected_data = { + current_user_id: user.id, + merge_request_id: merge_request.id + } + + expect { update_merge_request(title: 'Draft: New title') }.to publish_event(MergeRequests::DraftStateChangeEvent).with(expected_data) + end + end + + context 'when additional_merge_when_checks_ready is disabled' do + before do + stub_feature_flags(additional_merge_when_checks_ready: false) + end + + it 'does not publish a DraftStateChangeEvent' do + expect { update_merge_request(title: 'Draft: New title') }.not_to publish_event(MergeRequests::DraftStateChangeEvent) + end + end + it 'triggers GraphQL subscription mergeRequestMergeStatusUpdated' do expect(GraphqlTriggers).to receive(:merge_request_merge_status_updated).with(merge_request) |