1 files changed, 103 insertions, 12 deletions

diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
index 3d5329811ad..c8d426ee657 100644
--- a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
+++ b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
@@ -5,26 +5,85 @@ 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(:service) { described_class.new }
-
-    let_it_be(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+    let_it_be(:artifact, reload: 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 'when artifact is not locked' do
+      context 'with preloaded relationships' do
         before do
-          artifact.job.pipeline.unlocked!
+          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 'destroys job artifact' do
+        it 'performs the smallest number of queries for job_artifacts' do
+          log = ActiveRecord::QueryRecorder.new { subject }
+
+          # SELECT expired ci_job_artifacts
+          # PRELOAD projects, routes, project_statistics
+          # BEGIN
+          # INSERT into ci_deleted_objects
+          # DELETE loaded ci_job_artifacts
+          # DELETE security_findings  -- for EE
+          # COMMIT
+          expect(log.count).to be_within(1).of(8)
+        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
+
+          it 'reports metrics for destroyed artifacts' do
+            counter = service.send(:destroyed_artifacts_counter)
+
+            expect(counter).to receive(:increment).with({}, 1).and_call_original
+
+            subject
+          end
+        end
       end
 
       context 'when artifact is locked' do
@@ -61,14 +120,34 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     context 'when failed to destroy artifact' do
       before do
         stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10)
+      end
 
-        allow_any_instance_of(Ci::JobArtifact)
-          .to receive(:destroy!)
-          .and_raise(ActiveRecord::RecordNotDestroyed)
+      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
 
-      it 'raises an exception and stop destroying' do
-        expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed)
+      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
 
@@ -85,7 +164,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     context 'when timeout happens' do
       before do
         stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 1.second)
-        allow_any_instance_of(described_class).to receive(:destroy_batch) { true }
+        allow_any_instance_of(described_class).to receive(:destroy_artifacts_batch) { true }
       end
 
       it 'returns false and does not continue destroying' do
@@ -176,4 +255,16 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
       end
     end
   end
+
+  describe '.destroy_job_artifacts_batch' do
+    it 'returns a falsy value without artifacts' do
+      expect(service.send(:destroy_job_artifacts_batch)).to be_falsy
+    end
+  end
+
+  describe '.destroy_pipeline_artifacts_batch' do
+    it 'returns a falsy value without artifacts' do
+      expect(service.send(:destroy_pipeline_artifacts_batch)).to be_falsy
+    end
+  end
 end