1 files changed, 54 insertions, 80 deletions
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
index 04fa55068f2..7a91ad9dcc1 100644
--- a/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb
+++ b/spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb
@@ -10,20 +10,16 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
   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
+    let_it_be(:locked_pipeline) { create(:ci_pipeline, :artifacts_locked) }
+    let_it_be(:pipeline) { create(:ci_pipeline, :unlocked) }
+    let_it_be(:locked_job) { create(:ci_build, :success, pipeline: locked_pipeline) }
+    let_it_be(:job) { create(:ci_build, :success, pipeline: pipeline) }
 
     context 'when artifact is expired' do
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+
       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
 
@@ -39,7 +35,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
           # COMMIT
           # SELECT next expired ci_job_artifacts
 
-          expect(log.count).to be_within(1).of(11)
+          expect(log.count).to be_within(1).of(10)
         end
       end
 
@@ -48,7 +44,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
           expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
         end
 
-        context 'when the artifact does not a file attached to it' do
+        context 'when the artifact does not have a file attached to it' do
           it 'does not create deleted objects' do
             expect(artifact.exists?).to be_falsy # sanity check
 
@@ -57,10 +53,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
         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
+          let!(:artifact) { create(:ci_job_artifact, :expired, :zip, job: job) }
 
           it 'creates a deleted object' do
             expect { subject }.to change { Ci::DeletedObject.count }.by(1)
@@ -81,9 +74,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
       end
 
       context 'when artifact is locked' do
-        before do
-          artifact.job.pipeline.artifacts_locked!
-        end
+        let!(:artifact) { create(:ci_job_artifact, :expired, job: locked_job) }
 
         it 'does not destroy job artifact' do
           expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -92,9 +83,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when artifact is not expired' do
-      before do
-        artifact.update_column(:expire_at, 1.day.since)
-      end
+      let!(:artifact) { create(:ci_job_artifact, job: job) }
 
       it 'does not destroy expired job artifacts' do
         expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -102,9 +91,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when artifact is permanent' do
-      before do
-        artifact.update_column(:expire_at, nil)
-      end
+      let!(:artifact) { create(:ci_job_artifact, expire_at: nil, job: job) }
 
       it 'does not destroy expired job artifacts' do
         expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -112,6 +99,8 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when failed to destroy artifact' do
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+
       before do
         stub_const("#{described_class}::LOOP_LIMIT", 10)
       end
@@ -146,58 +135,67 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when exclusive lease has already been taken by the other instance' do
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+
       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)
+                          .and not_change { Ci::JobArtifact.count }.from(1)
       end
     end
 
-    context 'when timeout happens' do
-      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+    context 'with a second artifact and batch size of 1' do
+      let(:second_job) { create(:ci_build, :success, pipeline: pipeline) }
+      let!(:second_artifact) { create(:ci_job_artifact, :archive, expire_at: 1.day.ago, job: second_job) }
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
 
       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 timeout happens' do
+        before do
+          stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds)
+        end
 
-    context 'when loop reached loop limit' do
-      before do
-        stub_const("#{described_class}::LOOP_LIMIT", 1)
-        stub_const("#{described_class}::BATCH_SIZE", 1)
+        it 'destroys one artifact' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        end
 
-        second_artifact.job.pipeline.unlocked!
+        it 'reports the number of destroyed artifacts' do
+          is_expected.to eq(1)
+        end
       end
 
-      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+      context 'when loop reached loop limit' do
+        before do
+          stub_const("#{described_class}::LOOP_LIMIT", 1)
+        end
 
-      it 'destroys one artifact' do
-        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        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
 
-      it 'reports the number of destroyed artifacts' do
-        is_expected.to eq(1)
+      context 'when the number of artifacts is greater than than batch size' do
+        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
     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
@@ -207,42 +205,18 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
       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
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+      let!(:locked_artifact) { create(:ci_job_artifact, :expired, job: locked_job) }
 
       it 'destroys only unlocked artifacts' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        expect(locked_artifact).to be_persisted
       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
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: locked_job) }
 
       it 'destroys no artifacts' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(0)