From 67cdffe4deb5c887c17115d4f974c0e8a267ffd2 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 16 Mar 2021 09:11:17 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- spec/features/import/manifest_import_spec.rb | 2 +- spec/fixtures/ce_sample_schema.json | 0 .../ci/create_job_artifacts_service_spec.rb | 279 --------------------- .../destroy_expired_job_artifacts_service_spec.rb | 252 ------------------- .../ci/job_artifacts/create_service_spec.rb | 279 +++++++++++++++++++++ .../destroy_all_expired_service_spec.rb | 252 +++++++++++++++++++ .../ci/job_artifacts/destroy_batch_service_spec.rb | 81 ++++++ .../ci/job_artifacts_destroy_batch_service_spec.rb | 81 ------ .../destroy_all_expired_service_spec.rb | 81 ++++++ .../destroy_expired_artifacts_service_spec.rb | 81 ------ spec/services/issuable/process_assignees_spec.rb | 32 +-- .../push_options_handler_service_spec.rb | 208 ++++++--------- .../services/merge_request_shared_examples.rb | 90 +++++++ .../expire_artifacts_worker_spec.rb | 2 +- spec/workers/expire_build_artifacts_worker_spec.rb | 2 +- 15 files changed, 877 insertions(+), 845 deletions(-) create mode 100644 spec/fixtures/ce_sample_schema.json delete mode 100644 spec/services/ci/create_job_artifacts_service_spec.rb delete mode 100644 spec/services/ci/destroy_expired_job_artifacts_service_spec.rb create mode 100644 spec/services/ci/job_artifacts/create_service_spec.rb create mode 100644 spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb create mode 100644 spec/services/ci/job_artifacts/destroy_batch_service_spec.rb delete mode 100644 spec/services/ci/job_artifacts_destroy_batch_service_spec.rb create mode 100644 spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb delete mode 100644 spec/services/ci/pipeline_artifacts/destroy_expired_artifacts_service_spec.rb (limited to 'spec') diff --git a/spec/features/import/manifest_import_spec.rb b/spec/features/import/manifest_import_spec.rb index dfd6211a683..ce22171a560 100644 --- a/spec/features/import/manifest_import_spec.rb +++ b/spec/features/import/manifest_import_spec.rb @@ -37,7 +37,7 @@ RSpec.describe 'Import multiple repositories by uploading a manifest file', :js wait_for_requests page.within(second_row) do - expect(page).to have_content 'Done' + expect(page).to have_content 'Complete' expect(page).to have_content("#{group.full_path}/build/blueprint") end end diff --git a/spec/fixtures/ce_sample_schema.json b/spec/fixtures/ce_sample_schema.json new file mode 100644 index 00000000000..e69de29bb2d diff --git a/spec/services/ci/create_job_artifacts_service_spec.rb b/spec/services/ci/create_job_artifacts_service_spec.rb deleted file mode 100644 index 1efd1d390a2..00000000000 --- a/spec/services/ci/create_job_artifacts_service_spec.rb +++ /dev/null @@ -1,279 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::CreateJobArtifactsService do - let_it_be(:project) { create(:project) } - let(:service) { described_class.new(job) } - let(:job) { create(:ci_build, project: project) } - let(:artifacts_sha256) { '0' * 64 } - let(:metadata_file) { nil } - - let(:artifacts_file) do - file_to_upload('spec/fixtures/ci_build_artifacts.zip', sha256: artifacts_sha256) - end - - let(:params) do - { - 'artifact_type' => 'archive', - 'artifact_format' => 'zip' - }.with_indifferent_access - end - - def file_to_upload(path, params = {}) - upload = Tempfile.new('upload') - FileUtils.copy(path, upload.path) - - UploadedFile.new(upload.path, **params) - end - - def unique_metrics_report_uploaders - Gitlab::UsageDataCounters::HLLRedisCounter.unique_events( - event_names: described_class::METRICS_REPORT_UPLOAD_EVENT_NAME, - start_date: 2.weeks.ago, - end_date: 2.weeks.from_now - ) - end - - describe '#execute' do - subject { service.execute(artifacts_file, params, metadata_file: metadata_file) } - - context 'when artifacts file is uploaded' do - it 'saves artifact for the given type' do - expect { subject }.to change { Ci::JobArtifact.count }.by(1) - - new_artifact = job.job_artifacts.last - expect(new_artifact.project).to eq(job.project) - expect(new_artifact.file).to be_present - expect(new_artifact.file_type).to eq(params['artifact_type']) - expect(new_artifact.file_format).to eq(params['artifact_format']) - expect(new_artifact.file_sha256).to eq(artifacts_sha256) - end - - it 'does not track the job user_id' do - subject - - expect(unique_metrics_report_uploaders).to eq(0) - end - - context 'when metadata file is also uploaded' do - let(:metadata_file) do - file_to_upload('spec/fixtures/ci_build_artifacts_metadata.gz', sha256: artifacts_sha256) - end - - before do - stub_application_setting(default_artifacts_expire_in: '1 day') - end - - it 'saves metadata artifact' do - expect { subject }.to change { Ci::JobArtifact.count }.by(2) - - new_artifact = job.job_artifacts.last - expect(new_artifact.project).to eq(job.project) - expect(new_artifact.file).to be_present - expect(new_artifact.file_type).to eq('metadata') - expect(new_artifact.file_format).to eq('gzip') - expect(new_artifact.file_sha256).to eq(artifacts_sha256) - end - - it 'sets expiration date according to application settings' do - expected_expire_at = 1.day.from_now - - expect(subject).to match(a_hash_including(status: :success)) - archive_artifact, metadata_artifact = job.job_artifacts.last(2) - - expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at) - expect(archive_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) - expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) - end - - context 'when expire_in params is set to a specific value' do - before do - params.merge!('expire_in' => '2 hours') - end - - it 'sets expiration date according to the parameter' do - expected_expire_at = 2.hours.from_now - - expect(subject).to match(a_hash_including(status: :success)) - archive_artifact, metadata_artifact = job.job_artifacts.last(2) - - expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at) - expect(archive_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) - expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) - end - end - - context 'when expire_in params is set to `never`' do - before do - params.merge!('expire_in' => 'never') - end - - it 'sets expiration date according to the parameter' do - expected_expire_at = nil - - expect(subject).to be_truthy - archive_artifact, metadata_artifact = job.job_artifacts.last(2) - - expect(job.artifacts_expire_at).to eq(expected_expire_at) - expect(archive_artifact.expire_at).to eq(expected_expire_at) - expect(metadata_artifact.expire_at).to eq(expected_expire_at) - end - end - end - end - - context 'when artifacts file already exists' do - let!(:existing_artifact) do - create(:ci_job_artifact, :archive, file_sha256: existing_sha256, job: job) - end - - context 'when sha256 of uploading artifact is the same of the existing one' do - let(:existing_sha256) { artifacts_sha256 } - - it 'ignores the changes' do - expect { subject }.not_to change { Ci::JobArtifact.count } - expect(subject).to match(a_hash_including(status: :success)) - end - end - - context 'when sha256 of uploading artifact is different than the existing one' do - let(:existing_sha256) { '1' * 64 } - - it 'returns error status' do - expect(Gitlab::ErrorTracking).to receive(:track_exception).and_call_original - - expect { subject }.not_to change { Ci::JobArtifact.count } - expect(subject).to match( - a_hash_including(http_status: :bad_request, - message: 'another artifact of the same type already exists', - status: :error)) - end - end - end - - context 'when artifact type is dotenv' do - let(:artifacts_file) do - file_to_upload('spec/fixtures/build.env.gz', sha256: artifacts_sha256) - end - - let(:params) do - { - 'artifact_type' => 'dotenv', - 'artifact_format' => 'gzip' - }.with_indifferent_access - end - - it 'calls parse service' do - expect_any_instance_of(Ci::ParseDotenvArtifactService) do |service| - expect(service).to receive(:execute).once.and_call_original - end - - expect(subject[:status]).to eq(:success) - expect(job.job_variables.as_json).to contain_exactly( - hash_including('key' => 'KEY1', 'value' => 'VAR1', 'source' => 'dotenv'), - hash_including('key' => 'KEY2', 'value' => 'VAR2', 'source' => 'dotenv')) - end - - context 'when ci_synchronous_artifact_parsing feature flag is disabled' do - before do - stub_feature_flags(ci_synchronous_artifact_parsing: false) - end - - it 'does not call parse service' do - expect(Ci::ParseDotenvArtifactService).not_to receive(:new) - - expect(subject[:status]).to eq(:success) - end - end - end - - context 'when artifact_type is metrics' do - before do - allow(job).to receive(:user_id).and_return(123) - end - - let(:params) { { 'artifact_type' => 'metrics', 'artifact_format' => 'gzip' }.with_indifferent_access } - - it 'tracks the job user_id' do - subject - - expect(unique_metrics_report_uploaders).to eq(1) - end - end - - context 'when artifact type is cluster_applications' do - let(:artifacts_file) do - file_to_upload('spec/fixtures/helm/helm_list_v2_prometheus_missing.json.gz', sha256: artifacts_sha256) - end - - let(:params) do - { - 'artifact_type' => 'cluster_applications', - 'artifact_format' => 'gzip' - }.with_indifferent_access - end - - it 'calls cluster applications parse service' do - expect_next_instance_of(Clusters::ParseClusterApplicationsArtifactService) do |service| - expect(service).to receive(:execute).once.and_call_original - end - - subject - end - - context 'when there is a deployment cluster' do - let(:user) { project.owner } - - before do - job.update!(user: user) - end - - it 'calls cluster applications parse service with job and job user', :aggregate_failures do - expect(Clusters::ParseClusterApplicationsArtifactService).to receive(:new).with(job, user).and_call_original - - subject - end - end - - context 'when ci_synchronous_artifact_parsing feature flag is disabled' do - before do - stub_feature_flags(ci_synchronous_artifact_parsing: false) - end - - it 'does not call parse service' do - expect(Clusters::ParseClusterApplicationsArtifactService).not_to receive(:new) - - expect(subject[:status]).to eq(:success) - end - end - end - - shared_examples 'rescues object storage error' do |klass, message, expected_message| - it "handles #{klass}" do - allow_next_instance_of(JobArtifactUploader) do |uploader| - allow(uploader).to receive(:store!).and_raise(klass, message) - end - - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .and_call_original - - expect(subject).to match( - a_hash_including( - http_status: :service_unavailable, - message: expected_message || message, - status: :error)) - end - end - - it_behaves_like 'rescues object storage error', - Errno::EIO, 'some/path', 'Input/output error - some/path' - - it_behaves_like 'rescues object storage error', - Google::Apis::ServerError, 'Server error' - - it_behaves_like 'rescues object storage error', - Signet::RemoteServerError, 'The service is currently unavailable' - end -end diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb deleted file mode 100644 index d315dd35632..00000000000 --- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb +++ /dev/null @@ -1,252 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state do - include ExclusiveLeaseHelpers - - let(:service) { described_class.new } - - describe '.execute' do - subject { service.execute } - - let_it_be(:artifact, refind: true) do - create(:ci_job_artifact, expire_at: 1.day.ago) - end - - before(:all) do - artifact.job.pipeline.unlocked! - end - - context 'when artifact is expired' do - context 'with preloaded relationships' do - before do - job = create(:ci_build, pipeline: artifact.job.pipeline) - create(:ci_job_artifact, :archive, :expired, job: job) - - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1) - end - - it 'performs the smallest number of queries for job_artifacts' do - log = ActiveRecord::QueryRecorder.new { subject } - - # SELECT expired ci_job_artifacts - 3 queries from each_batch - # PRELOAD projects, routes, project_statistics - # BEGIN - # INSERT into ci_deleted_objects - # DELETE loaded ci_job_artifacts - # DELETE security_findings -- for EE - # COMMIT - # SELECT next expired ci_job_artifacts - - expect(log.count).to be_within(1).of(11) - end - end - - context 'when artifact is not locked' do - it 'deletes job artifact record' do - expect { subject }.to change { Ci::JobArtifact.count }.by(-1) - end - - context 'when the artifact does not a file attached to it' do - it 'does not create deleted objects' do - expect(artifact.exists?).to be_falsy # sanity check - - expect { subject }.not_to change { Ci::DeletedObject.count } - end - end - - context 'when the artifact has a file attached to it' do - before do - artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') - artifact.save! - end - - it 'creates a deleted object' do - expect { subject }.to change { Ci::DeletedObject.count }.by(1) - end - - it 'resets project statistics' do - expect(ProjectStatistics).to receive(:increment_statistic).once - .with(artifact.project, :build_artifacts_size, -artifact.file.size) - .and_call_original - - subject - end - - it 'does not remove the files' do - expect { subject }.not_to change { artifact.file.exists? } - end - end - end - - context 'when artifact is locked' do - before do - artifact.job.pipeline.artifacts_locked! - end - - it 'does not destroy job artifact' do - expect { subject }.not_to change { Ci::JobArtifact.count } - end - end - end - - context 'when artifact is not expired' do - before do - artifact.update_column(:expire_at, 1.day.since) - end - - it 'does not destroy expired job artifacts' do - expect { subject }.not_to change { Ci::JobArtifact.count } - end - end - - context 'when artifact is permanent' do - before do - artifact.update_column(:expire_at, nil) - end - - it 'does not destroy expired job artifacts' do - expect { subject }.not_to change { Ci::JobArtifact.count } - end - end - - context 'when failed to destroy artifact' do - before do - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10) - end - - context 'when the import fails' do - before do - expect(Ci::DeletedObject) - .to receive(:bulk_import) - .once - .and_raise(ActiveRecord::RecordNotDestroyed) - end - - it 'raises an exception and stop destroying' do - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) - .and not_change { Ci::JobArtifact.count }.from(1) - end - end - - context 'when the delete fails' do - before do - expect(Ci::JobArtifact) - .to receive(:id_in) - .once - .and_raise(ActiveRecord::RecordNotDestroyed) - end - - it 'raises an exception rolls back the insert' do - expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) - .and not_change { Ci::DeletedObject.count }.from(0) - end - end - end - - context 'when exclusive lease has already been taken by the other instance' do - before do - stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY, timeout: described_class::LOCK_TIMEOUT) - end - - it 'raises an error and does not start destroying' do - expect { subject }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) - end - end - - context 'when timeout happens' do - let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } - - before do - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 0.seconds) - stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) - - second_artifact.job.pipeline.unlocked! - end - - it 'destroys one artifact' do - expect { subject }.to change { Ci::JobArtifact.count }.by(-1) - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(1) - end - end - - context 'when loop reached loop limit' do - before do - stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1) - stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) - - second_artifact.job.pipeline.unlocked! - end - - let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } - - it 'destroys one artifact' do - expect { subject }.to change { Ci::JobArtifact.count }.by(-1) - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(1) - end - end - - context 'when there are no artifacts' do - before do - artifact.destroy! - end - - it 'does not raise error' do - expect { subject }.not_to raise_error - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(0) - end - end - - context 'when there are artifacts more than batch sizes' do - before do - stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1) - - second_artifact.job.pipeline.unlocked! - end - - let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } - - it 'destroys all expired artifacts' do - expect { subject }.to change { Ci::JobArtifact.count }.by(-2) - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(2) - end - end - - context 'when some artifacts are locked' do - before do - pipeline = create(:ci_pipeline, locked: :artifacts_locked) - job = create(:ci_build, pipeline: pipeline) - create(:ci_job_artifact, expire_at: 1.day.ago, job: job) - end - - it 'destroys only unlocked artifacts' do - expect { subject }.to change { Ci::JobArtifact.count }.by(-1) - end - end - - context 'when all artifacts are locked' do - before do - pipeline = create(:ci_pipeline, locked: :artifacts_locked) - job = create(:ci_build, pipeline: pipeline) - artifact.update!(job: job) - end - - it 'destroys no artifacts' do - expect { subject }.to change { Ci::JobArtifact.count }.by(0) - end - end - end -end diff --git a/spec/services/ci/job_artifacts/create_service_spec.rb b/spec/services/ci/job_artifacts/create_service_spec.rb new file mode 100644 index 00000000000..22aa9e62c6f --- /dev/null +++ b/spec/services/ci/job_artifacts/create_service_spec.rb @@ -0,0 +1,279 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobArtifacts::CreateService do + let_it_be(:project) { create(:project) } + let(:service) { described_class.new(job) } + let(:job) { create(:ci_build, project: project) } + let(:artifacts_sha256) { '0' * 64 } + let(:metadata_file) { nil } + + let(:artifacts_file) do + file_to_upload('spec/fixtures/ci_build_artifacts.zip', sha256: artifacts_sha256) + end + + let(:params) do + { + 'artifact_type' => 'archive', + 'artifact_format' => 'zip' + }.with_indifferent_access + end + + def file_to_upload(path, params = {}) + upload = Tempfile.new('upload') + FileUtils.copy(path, upload.path) + + UploadedFile.new(upload.path, **params) + end + + def unique_metrics_report_uploaders + Gitlab::UsageDataCounters::HLLRedisCounter.unique_events( + event_names: described_class::METRICS_REPORT_UPLOAD_EVENT_NAME, + start_date: 2.weeks.ago, + end_date: 2.weeks.from_now + ) + end + + describe '#execute' do + subject { service.execute(artifacts_file, params, metadata_file: metadata_file) } + + context 'when artifacts file is uploaded' do + it 'saves artifact for the given type' do + expect { subject }.to change { Ci::JobArtifact.count }.by(1) + + new_artifact = job.job_artifacts.last + expect(new_artifact.project).to eq(job.project) + expect(new_artifact.file).to be_present + expect(new_artifact.file_type).to eq(params['artifact_type']) + expect(new_artifact.file_format).to eq(params['artifact_format']) + expect(new_artifact.file_sha256).to eq(artifacts_sha256) + end + + it 'does not track the job user_id' do + subject + + expect(unique_metrics_report_uploaders).to eq(0) + end + + context 'when metadata file is also uploaded' do + let(:metadata_file) do + file_to_upload('spec/fixtures/ci_build_artifacts_metadata.gz', sha256: artifacts_sha256) + end + + before do + stub_application_setting(default_artifacts_expire_in: '1 day') + end + + it 'saves metadata artifact' do + expect { subject }.to change { Ci::JobArtifact.count }.by(2) + + new_artifact = job.job_artifacts.last + expect(new_artifact.project).to eq(job.project) + expect(new_artifact.file).to be_present + expect(new_artifact.file_type).to eq('metadata') + expect(new_artifact.file_format).to eq('gzip') + expect(new_artifact.file_sha256).to eq(artifacts_sha256) + end + + it 'sets expiration date according to application settings' do + expected_expire_at = 1.day.from_now + + expect(subject).to match(a_hash_including(status: :success)) + archive_artifact, metadata_artifact = job.job_artifacts.last(2) + + expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at) + expect(archive_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) + expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) + end + + context 'when expire_in params is set to a specific value' do + before do + params.merge!('expire_in' => '2 hours') + end + + it 'sets expiration date according to the parameter' do + expected_expire_at = 2.hours.from_now + + expect(subject).to match(a_hash_including(status: :success)) + archive_artifact, metadata_artifact = job.job_artifacts.last(2) + + expect(job.artifacts_expire_at).to be_within(1.minute).of(expected_expire_at) + expect(archive_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) + expect(metadata_artifact.expire_at).to be_within(1.minute).of(expected_expire_at) + end + end + + context 'when expire_in params is set to `never`' do + before do + params.merge!('expire_in' => 'never') + end + + it 'sets expiration date according to the parameter' do + expected_expire_at = nil + + expect(subject).to be_truthy + archive_artifact, metadata_artifact = job.job_artifacts.last(2) + + expect(job.artifacts_expire_at).to eq(expected_expire_at) + expect(archive_artifact.expire_at).to eq(expected_expire_at) + expect(metadata_artifact.expire_at).to eq(expected_expire_at) + end + end + end + end + + context 'when artifacts file already exists' do + let!(:existing_artifact) do + create(:ci_job_artifact, :archive, file_sha256: existing_sha256, job: job) + end + + context 'when sha256 of uploading artifact is the same of the existing one' do + let(:existing_sha256) { artifacts_sha256 } + + it 'ignores the changes' do + expect { subject }.not_to change { Ci::JobArtifact.count } + expect(subject).to match(a_hash_including(status: :success)) + end + end + + context 'when sha256 of uploading artifact is different than the existing one' do + let(:existing_sha256) { '1' * 64 } + + it 'returns error status' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).and_call_original + + expect { subject }.not_to change { Ci::JobArtifact.count } + expect(subject).to match( + a_hash_including(http_status: :bad_request, + message: 'another artifact of the same type already exists', + status: :error)) + end + end + end + + context 'when artifact type is dotenv' do + let(:artifacts_file) do + file_to_upload('spec/fixtures/build.env.gz', sha256: artifacts_sha256) + end + + let(:params) do + { + 'artifact_type' => 'dotenv', + 'artifact_format' => 'gzip' + }.with_indifferent_access + end + + it 'calls parse service' do + expect_any_instance_of(Ci::ParseDotenvArtifactService) do |service| + expect(service).to receive(:execute).once.and_call_original + end + + expect(subject[:status]).to eq(:success) + expect(job.job_variables.as_json).to contain_exactly( + hash_including('key' => 'KEY1', 'value' => 'VAR1', 'source' => 'dotenv'), + hash_including('key' => 'KEY2', 'value' => 'VAR2', 'source' => 'dotenv')) + end + + context 'when ci_synchronous_artifact_parsing feature flag is disabled' do + before do + stub_feature_flags(ci_synchronous_artifact_parsing: false) + end + + it 'does not call parse service' do + expect(Ci::ParseDotenvArtifactService).not_to receive(:new) + + expect(subject[:status]).to eq(:success) + end + end + end + + context 'when artifact_type is metrics' do + before do + allow(job).to receive(:user_id).and_return(123) + end + + let(:params) { { 'artifact_type' => 'metrics', 'artifact_format' => 'gzip' }.with_indifferent_access } + + it 'tracks the job user_id' do + subject + + expect(unique_metrics_report_uploaders).to eq(1) + end + end + + context 'when artifact type is cluster_applications' do + let(:artifacts_file) do + file_to_upload('spec/fixtures/helm/helm_list_v2_prometheus_missing.json.gz', sha256: artifacts_sha256) + end + + let(:params) do + { + 'artifact_type' => 'cluster_applications', + 'artifact_format' => 'gzip' + }.with_indifferent_access + end + + it 'calls cluster applications parse service' do + expect_next_instance_of(Clusters::ParseClusterApplicationsArtifactService) do |service| + expect(service).to receive(:execute).once.and_call_original + end + + subject + end + + context 'when there is a deployment cluster' do + let(:user) { project.owner } + + before do + job.update!(user: user) + end + + it 'calls cluster applications parse service with job and job user', :aggregate_failures do + expect(Clusters::ParseClusterApplicationsArtifactService).to receive(:new).with(job, user).and_call_original + + subject + end + end + + context 'when ci_synchronous_artifact_parsing feature flag is disabled' do + before do + stub_feature_flags(ci_synchronous_artifact_parsing: false) + end + + it 'does not call parse service' do + expect(Clusters::ParseClusterApplicationsArtifactService).not_to receive(:new) + + expect(subject[:status]).to eq(:success) + end + end + end + + shared_examples 'rescues object storage error' do |klass, message, expected_message| + it "handles #{klass}" do + allow_next_instance_of(JobArtifactUploader) do |uploader| + allow(uploader).to receive(:store!).and_raise(klass, message) + end + + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .and_call_original + + expect(subject).to match( + a_hash_including( + http_status: :service_unavailable, + message: expected_message || message, + status: :error)) + end + end + + it_behaves_like 'rescues object storage error', + Errno::EIO, 'some/path', 'Input/output error - some/path' + + it_behaves_like 'rescues object storage error', + Google::Apis::ServerError, 'Server error' + + it_behaves_like 'rescues object storage error', + Signet::RemoteServerError, 'The service is currently unavailable' + end +end diff --git a/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb new file mode 100644 index 00000000000..04fa55068f2 --- /dev/null +++ b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb @@ -0,0 +1,252 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_shared_state do + include ExclusiveLeaseHelpers + + let(:service) { described_class.new } + + describe '.execute' do + subject { service.execute } + + let_it_be(:artifact, refind: true) do + create(:ci_job_artifact, expire_at: 1.day.ago) + end + + before(:all) do + artifact.job.pipeline.unlocked! + end + + context 'when artifact is expired' do + context 'with preloaded relationships' do + before do + job = create(:ci_build, pipeline: artifact.job.pipeline) + create(:ci_job_artifact, :archive, :expired, job: job) + + stub_const("#{described_class}::LOOP_LIMIT", 1) + end + + it 'performs the smallest number of queries for job_artifacts' do + log = ActiveRecord::QueryRecorder.new { subject } + + # SELECT expired ci_job_artifacts - 3 queries from each_batch + # PRELOAD projects, routes, project_statistics + # BEGIN + # INSERT into ci_deleted_objects + # DELETE loaded ci_job_artifacts + # DELETE security_findings -- for EE + # COMMIT + # SELECT next expired ci_job_artifacts + + expect(log.count).to be_within(1).of(11) + end + end + + context 'when artifact is not locked' do + it 'deletes job artifact record' do + expect { subject }.to change { Ci::JobArtifact.count }.by(-1) + end + + context 'when the artifact does not a file attached to it' do + it 'does not create deleted objects' do + expect(artifact.exists?).to be_falsy # sanity check + + expect { subject }.not_to change { Ci::DeletedObject.count } + end + end + + context 'when the artifact has a file attached to it' do + before do + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + artifact.save! + end + + it 'creates a deleted object' do + expect { subject }.to change { Ci::DeletedObject.count }.by(1) + end + + it 'resets project statistics' do + expect(ProjectStatistics).to receive(:increment_statistic).once + .with(artifact.project, :build_artifacts_size, -artifact.file.size) + .and_call_original + + subject + end + + it 'does not remove the files' do + expect { subject }.not_to change { artifact.file.exists? } + end + end + end + + context 'when artifact is locked' do + before do + artifact.job.pipeline.artifacts_locked! + end + + it 'does not destroy job artifact' do + expect { subject }.not_to change { Ci::JobArtifact.count } + end + end + end + + context 'when artifact is not expired' do + before do + artifact.update_column(:expire_at, 1.day.since) + end + + it 'does not destroy expired job artifacts' do + expect { subject }.not_to change { Ci::JobArtifact.count } + end + end + + context 'when artifact is permanent' do + before do + artifact.update_column(:expire_at, nil) + end + + it 'does not destroy expired job artifacts' do + expect { subject }.not_to change { Ci::JobArtifact.count } + end + end + + context 'when failed to destroy artifact' do + before do + stub_const("#{described_class}::LOOP_LIMIT", 10) + end + + context 'when the import fails' do + before do + expect(Ci::DeletedObject) + .to receive(:bulk_import) + .once + .and_raise(ActiveRecord::RecordNotDestroyed) + end + + it 'raises an exception and stop destroying' do + expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + .and not_change { Ci::JobArtifact.count }.from(1) + end + end + + context 'when the delete fails' do + before do + expect(Ci::JobArtifact) + .to receive(:id_in) + .once + .and_raise(ActiveRecord::RecordNotDestroyed) + end + + it 'raises an exception rolls back the insert' do + expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed) + .and not_change { Ci::DeletedObject.count }.from(0) + end + end + end + + context 'when exclusive lease has already been taken by the other instance' do + before do + stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY, timeout: described_class::LOCK_TIMEOUT) + end + + it 'raises an error and does not start destroying' do + expect { subject }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + end + end + + context 'when timeout happens' do + let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } + + before do + stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds) + stub_const("#{described_class}::BATCH_SIZE", 1) + + second_artifact.job.pipeline.unlocked! + end + + it 'destroys one artifact' do + expect { subject }.to change { Ci::JobArtifact.count }.by(-1) + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(1) + end + end + + context 'when loop reached loop limit' do + before do + stub_const("#{described_class}::LOOP_LIMIT", 1) + stub_const("#{described_class}::BATCH_SIZE", 1) + + second_artifact.job.pipeline.unlocked! + end + + let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } + + it 'destroys one artifact' do + expect { subject }.to change { Ci::JobArtifact.count }.by(-1) + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(1) + end + end + + context 'when there are no artifacts' do + before do + artifact.destroy! + end + + it 'does not raise error' do + expect { subject }.not_to raise_error + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(0) + end + end + + context 'when there are artifacts more than batch sizes' do + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + + second_artifact.job.pipeline.unlocked! + end + + let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) } + + it 'destroys all expired artifacts' do + expect { subject }.to change { Ci::JobArtifact.count }.by(-2) + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(2) + end + end + + context 'when some artifacts are locked' do + before do + pipeline = create(:ci_pipeline, locked: :artifacts_locked) + job = create(:ci_build, pipeline: pipeline) + create(:ci_job_artifact, expire_at: 1.day.ago, job: job) + end + + it 'destroys only unlocked artifacts' do + expect { subject }.to change { Ci::JobArtifact.count }.by(-1) + end + end + + context 'when all artifacts are locked' do + before do + pipeline = create(:ci_pipeline, locked: :artifacts_locked) + job = create(:ci_build, pipeline: pipeline) + artifact.update!(job: job) + end + + it 'destroys no artifacts' do + expect { subject }.to change { Ci::JobArtifact.count }.by(0) + end + end + end +end diff --git a/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb new file mode 100644 index 00000000000..52aaf73d67e --- /dev/null +++ b/spec/services/ci/job_artifacts/destroy_batch_service_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::JobArtifacts::DestroyBatchService do + include ExclusiveLeaseHelpers + + let(:artifacts) { Ci::JobArtifact.all } + let(:service) { described_class.new(artifacts, pick_up_at: Time.current) } + + describe '.execute' do + subject(:execute) { service.execute } + + let_it_be(:artifact, refind: true) do + create(:ci_job_artifact) + end + + context 'when the artifact has a file attached to it' do + before do + artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + artifact.save! + end + + it 'creates a deleted object' do + expect { subject }.to change { Ci::DeletedObject.count }.by(1) + end + + it 'resets project statistics' do + expect(ProjectStatistics).to receive(:increment_statistic).once + .with(artifact.project, :build_artifacts_size, -artifact.file.size) + .and_call_original + + execute + end + + it 'does not remove the files' do + expect { execute }.not_to change { artifact.file.exists? } + end + + it 'reports metrics for destroyed artifacts' do + expect_next_instance_of(Gitlab::Ci::Artifacts::Metrics) do |metrics| + expect(metrics).to receive(:increment_destroyed_artifacts).with(1).and_call_original + end + + execute + end + end + + context 'when failed to destroy artifact' do + context 'when the import fails' do + before do + expect(Ci::DeletedObject) + .to receive(:bulk_import) + .once + .and_raise(ActiveRecord::RecordNotDestroyed) + end + + it 'raises an exception and stop destroying' do + expect { execute }.to raise_error(ActiveRecord::RecordNotDestroyed) + .and not_change { Ci::JobArtifact.count }.from(1) + end + end + end + + context 'when there are no artifacts' do + let(:artifacts) { Ci::JobArtifact.none } + + before do + artifact.destroy! + end + + it 'does not raise error' do + expect { execute }.not_to raise_error + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(destroyed_artifacts_count: 0, status: :success) + end + end + end +end diff --git a/spec/services/ci/job_artifacts_destroy_batch_service_spec.rb b/spec/services/ci/job_artifacts_destroy_batch_service_spec.rb deleted file mode 100644 index 74fbbf28ef1..00000000000 --- a/spec/services/ci/job_artifacts_destroy_batch_service_spec.rb +++ /dev/null @@ -1,81 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::JobArtifactsDestroyBatchService do - include ExclusiveLeaseHelpers - - let(:artifacts) { Ci::JobArtifact.all } - let(:service) { described_class.new(artifacts, pick_up_at: Time.current) } - - describe '.execute' do - subject(:execute) { service.execute } - - let_it_be(:artifact, refind: true) do - create(:ci_job_artifact) - end - - context 'when the artifact has a file attached to it' do - before do - artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') - artifact.save! - end - - it 'creates a deleted object' do - expect { subject }.to change { Ci::DeletedObject.count }.by(1) - end - - it 'resets project statistics' do - expect(ProjectStatistics).to receive(:increment_statistic).once - .with(artifact.project, :build_artifacts_size, -artifact.file.size) - .and_call_original - - execute - end - - it 'does not remove the files' do - expect { execute }.not_to change { artifact.file.exists? } - end - - it 'reports metrics for destroyed artifacts' do - expect_next_instance_of(Gitlab::Ci::Artifacts::Metrics) do |metrics| - expect(metrics).to receive(:increment_destroyed_artifacts).with(1).and_call_original - end - - execute - end - end - - context 'when failed to destroy artifact' do - context 'when the import fails' do - before do - expect(Ci::DeletedObject) - .to receive(:bulk_import) - .once - .and_raise(ActiveRecord::RecordNotDestroyed) - end - - it 'raises an exception and stop destroying' do - expect { execute }.to raise_error(ActiveRecord::RecordNotDestroyed) - .and not_change { Ci::JobArtifact.count }.from(1) - end - end - end - - context 'when there are no artifacts' do - let(:artifacts) { Ci::JobArtifact.none } - - before do - artifact.destroy! - end - - it 'does not raise error' do - expect { execute }.not_to raise_error - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(destroyed_artifacts_count: 0, status: :success) - end - end - end -end diff --git a/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb new file mode 100644 index 00000000000..3dc4f35df22 --- /dev/null +++ b/spec/services/ci/pipeline_artifacts/destroy_all_expired_service_spec.rb @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::PipelineArtifacts::DestroyAllExpiredService do + let(:service) { described_class.new } + + describe '.execute' do + subject { service.execute } + + context 'when timeout happens' do + before do + stub_const('Ci::PipelineArtifacts::DestroyAllExpiredService::LOOP_TIMEOUT', 0.1.seconds) + allow(service).to receive(:destroy_artifacts_batch) { true } + end + + it 'returns 0 and does not continue destroying' do + is_expected.to eq(0) + end + end + + context 'when there are no artifacts' do + it 'does not raise error' do + expect { subject }.not_to raise_error + end + end + + context 'when the loop limit is reached' do + before do + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::LOOP_LIMIT', 1) + stub_const('::Ci::PipelineArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) + + create_list(:ci_pipeline_artifact, 2, expire_at: 1.week.ago) + end + + it 'destroys one artifact' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-1) + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(1) + end + end + + context 'when there are artifacts more than batch sizes' do + before do + stub_const('Ci::PipelineArtifacts::DestroyAllExpiredService::BATCH_SIZE', 1) + + create_list(:ci_pipeline_artifact, 2, expire_at: 1.week.ago) + end + + it 'destroys all expired artifacts' do + expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2) + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(2) + end + end + + context 'when artifacts are not expired' do + before do + create(:ci_pipeline_artifact, expire_at: 2.days.from_now) + end + + it 'does not destroy pipeline artifacts' do + expect { subject }.not_to change { Ci::PipelineArtifact.count } + end + + it 'reports the number of destroyed artifacts' do + is_expected.to eq(0) + end + end + end + + describe '.destroy_artifacts_batch' do + it 'returns a falsy value without artifacts' do + expect(service.send(:destroy_artifacts_batch)).to be_falsy + end + end +end diff --git a/spec/services/ci/pipeline_artifacts/destroy_expired_artifacts_service_spec.rb b/spec/services/ci/pipeline_artifacts/destroy_expired_artifacts_service_spec.rb deleted file mode 100644 index ac1a590face..00000000000 --- a/spec/services/ci/pipeline_artifacts/destroy_expired_artifacts_service_spec.rb +++ /dev/null @@ -1,81 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::PipelineArtifacts::DestroyExpiredArtifactsService do - let(:service) { described_class.new } - - describe '.execute' do - subject { service.execute } - - context 'when timeout happens' do - before do - stub_const('Ci::PipelineArtifacts::DestroyExpiredArtifactsService::LOOP_TIMEOUT', 0.1.seconds) - allow(service).to receive(:destroy_artifacts_batch) { true } - end - - it 'returns 0 and does not continue destroying' do - is_expected.to eq(0) - end - end - - context 'when there are no artifacts' do - it 'does not raise error' do - expect { subject }.not_to raise_error - end - end - - context 'when the loop limit is reached' do - before do - stub_const('::Ci::PipelineArtifacts::DestroyExpiredArtifactsService::LOOP_LIMIT', 1) - stub_const('::Ci::PipelineArtifacts::DestroyExpiredArtifactsService::BATCH_SIZE', 1) - - create_list(:ci_pipeline_artifact, 2, expire_at: 1.week.ago) - end - - it 'destroys one artifact' do - expect { subject }.to change { Ci::PipelineArtifact.count }.by(-1) - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(1) - end - end - - context 'when there are artifacts more than batch sizes' do - before do - stub_const('Ci::PipelineArtifacts::DestroyExpiredArtifactsService::BATCH_SIZE', 1) - - create_list(:ci_pipeline_artifact, 2, expire_at: 1.week.ago) - end - - it 'destroys all expired artifacts' do - expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2) - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(2) - end - end - - context 'when artifacts are not expired' do - before do - create(:ci_pipeline_artifact, expire_at: 2.days.from_now) - end - - it 'does not destroy pipeline artifacts' do - expect { subject }.not_to change { Ci::PipelineArtifact.count } - end - - it 'reports the number of destroyed artifacts' do - is_expected.to eq(0) - end - end - end - - describe '.destroy_artifacts_batch' do - it 'returns a falsy value without artifacts' do - expect(service.send(:destroy_artifacts_batch)).to be_falsy - end - end -end diff --git a/spec/services/issuable/process_assignees_spec.rb b/spec/services/issuable/process_assignees_spec.rb index 876c84957cc..45d57a1772a 100644 --- a/spec/services/issuable/process_assignees_spec.rb +++ b/spec/services/issuable/process_assignees_spec.rb @@ -4,10 +4,10 @@ require 'spec_helper' RSpec.describe Issuable::ProcessAssignees do describe '#execute' do - it 'returns assignee_ids when assignee_ids are specified' do + it 'returns assignee_ids when add_assignee_ids and remove_assignee_ids are not specified' do process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), - add_assignee_ids: %w(2 4 6), - remove_assignee_ids: %w(4 7 11), + add_assignee_ids: nil, + remove_assignee_ids: nil, existing_assignee_ids: %w(1 3 9), extra_assignee_ids: %w(2 5 12)) result = process.execute @@ -15,19 +15,19 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(5 7 9).sort) end - it 'combines other ids when assignee_ids is empty' do - process = Issuable::ProcessAssignees.new(assignee_ids: [], - add_assignee_ids: %w(2 4 6), - remove_assignee_ids: %w(4 7 11), + it 'combines other ids when assignee_ids is nil' do + process = Issuable::ProcessAssignees.new(assignee_ids: nil, + add_assignee_ids: nil, + remove_assignee_ids: nil, existing_assignee_ids: %w(1 3 11), extra_assignee_ids: %w(2 5 12)) result = process.execute - expect(result.sort).to eq(%w(1 2 3 5 6 12).sort) + expect(result.sort).to eq(%w(1 2 3 5 11 12).sort) end - it 'combines other ids when assignee_ids is nil' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines other ids when both add_assignee_ids and remove_assignee_ids are not empty' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: %w(2 4 6), remove_assignee_ids: %w(4 7 11), existing_assignee_ids: %w(1 3 11), @@ -37,8 +37,8 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(1 2 3 5 6 12).sort) end - it 'combines other ids when assignee_ids and add_assignee_ids are nil' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines other ids when remove_assignee_ids is not empty' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: nil, remove_assignee_ids: %w(4 7 11), existing_assignee_ids: %w(1 3 11), @@ -48,8 +48,8 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(1 2 3 5 12).sort) end - it 'combines other ids when assignee_ids and remove_assignee_ids are nil' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines other ids when add_assignee_ids is not empty' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: %w(2 4 6), remove_assignee_ids: nil, existing_assignee_ids: %w(1 3 11), @@ -59,8 +59,8 @@ RSpec.describe Issuable::ProcessAssignees do expect(result.sort).to eq(%w(1 2 4 3 5 6 11 12).sort) end - it 'combines ids when only add_assignee_ids and remove_assignee_ids are passed' do - process = Issuable::ProcessAssignees.new(assignee_ids: nil, + it 'combines ids when existing_assignee_ids and extra_assignee_ids are omitted' do + process = Issuable::ProcessAssignees.new(assignee_ids: %w(5 7 9), add_assignee_ids: %w(2 4 6), remove_assignee_ids: %w(4 7 11)) result = process.execute diff --git a/spec/services/merge_requests/push_options_handler_service_spec.rb b/spec/services/merge_requests/push_options_handler_service_spec.rb index c2769d4fa88..b5086ea3a82 100644 --- a/spec/services/merge_requests/push_options_handler_service_spec.rb +++ b/spec/services/merge_requests/push_options_handler_service_spec.rb @@ -6,10 +6,12 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do include ProjectForksHelper let_it_be(:project) { create(:project, :public, :repository) } - let_it_be(:user) { create(:user, developer_projects: [project]) } - let_it_be(:forked_project) { fork_project(project, user, repository: true) } + let_it_be(:user1) { create(:user, developer_projects: [project]) } + let_it_be(:user2) { create(:user, developer_projects: [project]) } + let_it_be(:user3) { create(:user, developer_projects: [project]) } + let_it_be(:forked_project) { fork_project(project, user1, repository: true) } - let(:service) { described_class.new(project, user, changes, push_options) } + let(:service) { described_class.new(project, user1, changes, push_options) } let(:source_branch) { 'fix' } let(:target_branch) { 'feature' } let(:title) { 'my title' } @@ -23,32 +25,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:default_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{project.default_branch}" } let(:error_mr_required) { "A merge_request.create push option is required to create a merge request for branch #{source_branch}" } - shared_examples_for 'a service that can create a merge request' do - subject(:last_mr) { MergeRequest.last } - - it 'creates a merge request with the correct target branch and assigned user' do - branch = push_options[:target] || project.default_branch - - expect { service.execute }.to change { MergeRequest.count }.by(1) - expect(last_mr.target_branch).to eq(branch) - expect(last_mr.assignees).to contain_exactly(user) - end - - context 'when project has been forked', :sidekiq_might_not_need_inline do - let(:forked_project) { fork_project(project, user, repository: true) } - let(:service) { described_class.new(forked_project, user, changes, push_options) } - - before do - allow(forked_project).to receive(:empty_repo?).and_return(false) - end - - it 'sets the correct source and target project' do - service.execute - - expect(last_mr.source_project).to eq(forked_project) - expect(last_mr.target_project).to eq(project) - end - end + before do + stub_licensed_features(multiple_merge_request_assignees: false) end shared_examples_for 'a service that can set the target of a merge request' do @@ -91,7 +69,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do expect(last_mr.auto_merge_enabled).to eq(true) expect(last_mr.auto_merge_strategy).to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) - expect(last_mr.merge_user).to eq(user) + expect(last_mr.merge_user).to eq(user1) expect(last_mr.merge_params['sha']).to eq(change[:newrev]) end end @@ -116,12 +94,6 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end end - shared_examples_for 'a service that does not create a merge request' do - it do - expect { service.execute }.not_to change { MergeRequest.count } - end - end - shared_examples_for 'a service that does not update a merge request' do it do expect { service.execute }.not_to change { MergeRequest.maximum(:updated_at) } @@ -133,6 +105,18 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do include_examples 'a service that does not update a merge request' end + shared_examples 'with a deleted branch' do + let(:changes) { deleted_branch_changes } + + it_behaves_like 'a service that does nothing' + end + + shared_examples 'with the project default branch' do + let(:changes) { default_branch_changes } + + it_behaves_like 'a service that does nothing' + end + describe '`create` push option' do let(:push_options) { { create: true } } @@ -155,17 +139,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`merge_when_pipeline_succeeds` push option' do @@ -217,17 +192,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the merge request to merge when pipeline succeeds' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`remove_source_branch` push option' do @@ -239,11 +205,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -281,17 +245,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can remove the source branch when it is merged' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`target` push option' do @@ -343,17 +298,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the target of a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`title` push option' do @@ -405,17 +351,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the title of a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`description` push option' do @@ -467,17 +404,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can set the description of a merge request' end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`label` push option' do @@ -529,17 +457,8 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can change labels of a merge request', 2 end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } - - it_behaves_like 'a service that does nothing' - end - - context 'with the project default branch' do - let(:changes) { default_branch_changes } - - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe '`unlabel` push option' do @@ -551,11 +470,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -572,11 +489,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that does not create a merge request' it 'adds an error to the service' do - error = "A merge_request.create push option is required to create a merge request for branch #{source_branch}" - service.execute - expect(service.errors).to include(error) + expect(service.errors).to include(error_mr_required) end context 'when coupled with the `create` push option' do @@ -595,17 +510,42 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do it_behaves_like 'a service that can change labels of a merge request', 1 end - context 'with a deleted branch' do - let(:changes) { deleted_branch_changes } + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + shared_examples 'with an existing branch that has a merge request open in foss' do + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can change assignees of a merge request', 1 + end - context 'with the project default branch' do - let(:changes) { default_branch_changes } + describe '`assign` push option' do + let(:assigned) { { user2.id => 1, user3.id => 1 } } + let(:unassigned) { nil } + let(:push_options) { { assign: assigned, unassign: unassigned } } - it_behaves_like 'a service that does nothing' - end + it_behaves_like 'with a new branch', 1 + it_behaves_like 'with an existing branch but no open MR', 1 + it_behaves_like 'with an existing branch that has a merge request open in foss' + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' + end + + describe '`unassign` push option' do + let(:assigned) { { user2.id => 1, user3.id => 1 } } + let(:unassigned) { { user1.id => 1, user3.id => 1 } } + let(:push_options) { { assign: assigned, unassign: unassigned } } + + it_behaves_like 'with a new branch', 1 + it_behaves_like 'with an existing branch but no open MR', 1 + it_behaves_like 'with an existing branch that has a merge request open in foss' + + it_behaves_like 'with a deleted branch' + it_behaves_like 'with the project default branch' end describe 'multiple pushed branches' do @@ -645,7 +585,9 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end describe 'no user' do - let(:user) { nil } + let(:user1) { nil } + let(:user2) { nil } + let(:user3) { nil } let(:push_options) { { create: true } } let(:changes) { new_branch_changes } @@ -661,7 +603,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do let(:changes) { new_branch_changes } it 'records an error' do - Members::DestroyService.new(user).execute(ProjectMember.find_by!(user_id: user.id)) + Members::DestroyService.new(user1).execute(ProjectMember.find_by!(user_id: user1.id)) service.execute @@ -707,7 +649,7 @@ RSpec.describe MergeRequests::PushOptionsHandlerService do end describe 'when MRs are not enabled' do - let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user) } } + let(:project) { create(:project, :public, :repository).tap { |pr| pr.add_developer(user1) } } let(:push_options) { { create: true } } let(:changes) { new_branch_changes } diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb index 56179b6cd00..178b6bc47e1 100644 --- a/spec/support/shared_examples/services/merge_request_shared_examples.rb +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -73,3 +73,93 @@ RSpec.shared_examples 'merge request reviewers cache counters invalidator' do described_class.new(project, user, {}).execute(merge_request) end end + +RSpec.shared_examples_for 'a service that can create a merge request' do + subject(:last_mr) { MergeRequest.last } + + it 'creates a merge request with the correct target branch' do + branch = push_options[:target] || project.default_branch + + expect { service.execute }.to change { MergeRequest.count }.by(1) + expect(last_mr.target_branch).to eq(branch) + end + + context 'when project has been forked', :sidekiq_might_not_need_inline do + let(:forked_project) { fork_project(project, user1, repository: true) } + let(:service) { described_class.new(forked_project, user1, changes, push_options) } + + before do + allow(forked_project).to receive(:empty_repo?).and_return(false) + end + + it 'sets the correct source and target project' do + service.execute + + expect(last_mr.source_project).to eq(forked_project) + expect(last_mr.target_project).to eq(project) + end + end +end + +RSpec.shared_examples_for 'a service that does not create a merge request' do + it do + expect { service.execute }.not_to change { MergeRequest.count } + end +end + +# In the non-foss version of GitLab, there can be many assignees, so +# there 'count' can be something other than 0 or 1. In the foss +# version of GitLab, there can be only one assignee though, so 'count' +# can only be 0 or 1. +RSpec.shared_examples_for 'a service that can change assignees of a merge request' do |count| + subject(:last_mr) { MergeRequest.last } + + it 'changes assignee count' do + service.execute + + expect(last_mr.assignees.count).to eq(count) + end +end + +RSpec.shared_examples 'with an existing branch that has a merge request open' do |count| + let(:changes) { existing_branch_changes } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch)} + + it_behaves_like 'a service that does not create a merge request' + it_behaves_like 'a service that can change assignees of a merge request', count +end + +RSpec.shared_examples 'when coupled with the `create` push option' do |count| + let(:push_options) { { create: true, assign: assigned, unassign: unassigned } } + + it_behaves_like 'a service that can create a merge request' + it_behaves_like 'a service that can change assignees of a merge request', count +end + +RSpec.shared_examples 'with a new branch' do |count| + let(:changes) { new_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + it_behaves_like 'when coupled with the `create` push option', count +end + +RSpec.shared_examples 'with an existing branch but no open MR' do |count| + let(:changes) { existing_branch_changes } + + it_behaves_like 'a service that does not create a merge request' + + it 'adds an error to the service' do + service.execute + + expect(service.errors).to include(error_mr_required) + end + + it_behaves_like 'when coupled with the `create` push option', count +end diff --git a/spec/workers/ci/pipeline_artifacts/expire_artifacts_worker_spec.rb b/spec/workers/ci/pipeline_artifacts/expire_artifacts_worker_spec.rb index 2bdd8345374..ad9c08d02cb 100644 --- a/spec/workers/ci/pipeline_artifacts/expire_artifacts_worker_spec.rb +++ b/spec/workers/ci/pipeline_artifacts/expire_artifacts_worker_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Ci::PipelineArtifacts::ExpireArtifactsWorker do end it 'executes a service' do - expect_next_instance_of(::Ci::PipelineArtifacts::DestroyExpiredArtifactsService) do |instance| + expect_next_instance_of(::Ci::PipelineArtifacts::DestroyAllExpiredService) do |instance| expect(instance).to receive(:execute) end diff --git a/spec/workers/expire_build_artifacts_worker_spec.rb b/spec/workers/expire_build_artifacts_worker_spec.rb index 6d73d715d21..3f8da3fb71c 100644 --- a/spec/workers/expire_build_artifacts_worker_spec.rb +++ b/spec/workers/expire_build_artifacts_worker_spec.rb @@ -7,7 +7,7 @@ RSpec.describe ExpireBuildArtifactsWorker do describe '#perform' do it 'executes a service' do - expect_next_instance_of(Ci::DestroyExpiredJobArtifactsService) do |instance| + expect_next_instance_of(Ci::JobArtifacts::DestroyAllExpiredService) do |instance| expect(instance).to receive(:execute).and_call_original end -- cgit v1.2.1