Add spec for migrations

db/post_migrate/20180427161409_migrate_legacy_artifacts_to_job_artifacts.rb

@@ -14,18 +14,21 @@ class MigrateLegacyArtifactsToJobArtifacts < ActiveRecord::Migration
 
     ##
     # Jobs which have a value on `artifacts_file` column are targetted.
-    # In addition, jobs which have had job_artifacts already are untargetted.
-    # This usually doesn't happen, however, to avoid unique constraint violation, it's safe to have the condition.
+    # In addition, jobs which have already had job_artifacts are untargetted.
+    # This usually doesn't happen, however, if it's the case, background migrations will be aborted
     scope :legacy_artifacts, -> do
-      where('artifacts_file IS NOT NULL AND artifacts_file <> ?', '').
-      where('NOT EXISTS (SELECT 1 FROM ci_job_artifacts WHERE ci_job_artifacts.id == ci_builds.id AND (file_type == 1 OR file_type == 2))')
+      where('artifacts_file IS NOT NULL AND artifacts_file <> ?', '')
+    end
+
+    scope :without_new_artifacts, -> do
+      where('NOT EXISTS (SELECT 1 FROM ci_job_artifacts WHERE ci_job_artifacts.id = ci_builds.id AND (file_type = 1 OR file_type = 2))')
     end
   end
 
   def up
     disable_statement_timeout
 
-    MigrateLegacyArtifactsToJobArtifacts::Build.legacy_artifacts.tap do |relation|
+    MigrateLegacyArtifactsToJobArtifacts::Build.legacy_artifacts.without_new_artifacts.tap do |relation|
       queue_background_migration_jobs_by_range_at_intervals(relation,
                                                             MIGRATION,
                                                             5.minutes,

lib/gitlab/background_migration/migrate_legacy_artifacts.rb

@@ -9,7 +9,13 @@ module Gitlab
         self.table_name = 'ci_builds'
         self.inheritance_column = :_type_disabled
 
-        scope :legacy_artifacts, -> { where('artifacts_file IS NOT NULL OR artifacts_file <> ?', '') }
+        scope :legacy_artifacts, -> do
+          where('artifacts_file IS NOT NULL AND artifacts_file <> ?', '')
+        end
+
+        scope :without_new_artifacts, -> do
+          where('NOT EXISTS (SELECT 1 FROM ci_job_artifacts WHERE ci_job_artifacts.id = ci_builds.id AND (file_type = 1 OR file_type = 2))')
+        end
       end
 
       class JobArtifact < ActiveRecord::Base
@@ -38,7 +44,7 @@ module Gitlab
 
         # Build rows
         MigrateLegacyArtifacts::Build
-          .legacy_artifacts
+          .legacy_artifacts.without_new_artifacts
           .where(id: (start_id..stop_id)).find_each do |build|
           rows << build_archive_row(build)
           rows << build_metadata_row(build) if build.artifacts_metadata

spec/lib/gitlab/background_migration/migrate_legacy_artifacts_spec.rb

 require 'spec_helper'
 
 describe Gitlab::BackgroundMigration::MigrateLegacyArtifacts, :migration, schema: 20180427161409 do
+  let(:namespaces) { table(:namespaces) }
   let(:projects) { table(:projects) }
   let(:pipelines) { table(:ci_pipelines) }
   let(:jobs) { table(:ci_builds) }
   let(:job_artifacts) { table(:ci_job_artifacts) }
 
-  context 'when legacy artifacts exist' do
+  context 'when a pipeline exists' do
+    let(:pipeline_id) { 1 }
+    let(:project_id) { 123 }
+
     before do
-      projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce')
-      pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a')
-  
-      jobs.create!(id: 1, commit_id: 1, project_id: 123, status: :success)
-      jobs.create!(id: 2, commit_id: 1, project_id: 123, status: :success)
-      jobs.create!(id: 3, commit_id: 1, project_id: 123, status: :failed)
-      jobs.create!(id: 4, commit_id: 1, project_id: 123, status: :success)
-      jobs.create!(id: 5, commit_id: 1, project_id: 123, status: :pending)
-      jobs.create!(id: 6, commit_id: 1, project_id: 123, status: :pending)
+      namespaces.create!(id: 1, name: 'gitlab', path: 'gitlab-org')
+      projects.create!(id: project_id, name: 'gitlab', path: 'gitlab-ce', namespace_id: 1)
+      pipelines.create!(id: pipeline_id, project_id: project_id, ref: 'master', sha: 'adf43c3a')
     end
 
-    it 'migrates' do
+    context 'when a legacy artifacts exists' do
+      let(:artifacts_expire_at) { 1.day.since.to_s }
+      let(:file_store) { 2 }
+      let(:job_id) { 1 }
+      let(:file_type_archive) { 1 }
+      let(:file_type_metadata) { 2 }
+      let(:file_location_legacy_path) { 1 }
 
-      # And file access
-    end
+      let(:artifacts_archive_attributes) do
+        {
+          artifacts_file: 'archive.zip',
+          artifacts_file_store: file_store,
+          artifacts_size: 123,
+          artifacts_expire_at: artifacts_expire_at
+        }
+      end
 
-    context 'when job_artifacts has been already existed' do
-      it 'migrates' do
+      let(:artifacts_metadata_attributes) do
+        {
+          artifacts_metadata: 'metadata.gz',
+          artifacts_metadata_store: file_store
+        }
+      end
+
+      before do
+        jobs.create!(id: job_id, commit_id: pipeline_id, project_id: project_id, status: :success, **artifacts_archive_attributes, **artifacts_metadata_attributes)
+      end
+
+      it 'has legacy artifacts' do
+        expect(jobs.pluck('artifacts_file, artifacts_file_store, artifacts_size, artifacts_expire_at')).to eq([artifacts_archive_attributes.values])
+        expect(jobs.pluck('artifacts_metadata, artifacts_metadata_store')).to eq([artifacts_metadata_attributes.values])
+      end
 
-        # And file access
+      it 'does not have new artifacts yet' do
+        expect(job_artifacts.count).to be_zero
+      end
+
+      context 'when the record exists inside of the range of a background migration' do
+        let(:range) { [1, 1] }
+
+        it 'migrates' do
+          described_class.new.perform(*range)
+
+          expect(job_artifacts.order(:id).pluck('project_id, job_id, file_type, file_store, size, expire_at, file, file_sha256, file_location'))
+            .to eq([[project_id, job_id, file_type_archive,  file_store, artifacts_archive_attributes[:artifacts_size], artifacts_expire_at, 'archive.zip', nil, file_location_legacy_path],
+                    [project_id, job_id, file_type_metadata, file_store,                                           nil, artifacts_expire_at, 'metadata.gz', nil, file_location_legacy_path]])
+
+          expect(jobs.pluck('artifacts_file, artifacts_file_store, artifacts_size, artifacts_expire_at')).to eq([[nil, nil, nil, artifacts_expire_at]])
+          expect(jobs.pluck('artifacts_metadata, artifacts_metadata_store')).to eq([[nil, nil]])
+        end
+
+        context 'when file_store is nil' do
+          let(:file_store) { nil }
+
+          it 'fills file_store by 1 (ObjectStorage::Store::LOCAL)' do
+            expect(jobs.pluck('artifacts_file_store, artifacts_metadata_store')).to eq([[nil, nil]])
+
+            described_class.new.perform(*range)
+  
+            expect(job_artifacts.pluck('file_store')).to eq([1, 1])
+          end
+        end
+
+        context 'when new artifacts has already existed' do
+          before do
+            job_artifacts.create!(id: 1, project_id: project_id, job_id: job_id, file_type: 1, size: 123, file: 'archive.zip')
+          end
+
+          it 'does not migrate' do
+            described_class.new.perform(*range)
+  
+            expect(job_artifacts.pluck('id')).to eq([1])
+          end
+        end
+      end
+
+      context 'when the record exists outside of the range of a background migration' do
+        let(:range) { [2, 2] }
+
+        it 'does not migrate' do
+          described_class.new.perform(*range)
+
+          expect(job_artifacts.count).to be_zero
+        end
       end
     end
-  end
 
-  context 'when legacy artifacts do not exist' do
+    context 'when legacy artifacts do not exist' do
+      before do
+        jobs.create!(id: 1, commit_id: pipeline_id, project_id: project_id, status: :success)
+      end
 
-  end
+      it 'is not found from database' do
+        expect(jobs.pluck('artifacts_file, artifacts_file_store, artifacts_size, artifacts_expire_at')).to eq([[nil, nil, nil, nil]])
+        expect(jobs.pluck('artifacts_metadata, artifacts_metadata_store')).to eq([[nil, nil]])
+      end
 
-  def create_file(job, object_storage = false)
-    Tmpfile.create
-    JobArtifactUploader.
-    job.update_column(artifacts_file: nil)
-    job.update_column(artifacts_file_store: nil)
-    job.update_column(artifacts_size: nil)
-    job.update_column(artifacts_metadata: nil)
-    job.update_column(artifacts_metadata_store: nil)
+      context 'when the record exists inside of the range of a background migration' do
+        let(:range) { [1, 1] }
+
+        it 'does not migrate' do
+          described_class.new.perform(*range)
+
+          expect(job_artifacts.count).to be_zero
+        end
+      end
+    end
   end
 end

spec/migrations/migrate_legacy_artifacts_to_job_artifacts_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20180427161409_migrate_legacy_artifacts_to_job_artifacts.rb')
+
+describe MigrateLegacyArtifactsToJobArtifacts, :migration, :sidekiq do
+  let(:migration_class) { Gitlab::BackgroundMigration::MigrateLegacyArtifacts }
+  let(:migration_name)  { migration_class.to_s.demodulize }
+
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:pipelines) { table(:ci_pipelines) }
+  let(:jobs) { table(:ci_builds) }
+  let(:job_artifacts) { table(:ci_job_artifacts) }
+
+  before do
+    namespaces.create!(id: 1, name: 'gitlab', path: 'gitlab-org')
+    projects.create!(id: 1, name: 'gitlab', path: 'gitlab-ce', namespace_id: 1)
+    pipelines.create!(id: 1, project_id: 1, ref: 'master', sha: 'adf43c3a')
+  end
+
+  context 'when a legacy artifacts exists' do
+    before do
+      jobs.create!(id: 1, commit_id: 1, project_id: 1, status: :success, artifacts_file: 'archive.zip', artifacts_metadata: 'metadata.gz')
+    end
+
+    it 'schedules a background migration' do
+      Sidekiq::Testing.fake! do
+        Timecop.freeze do
+          migrate!
+  
+          expect(migration_name).to be_scheduled_delayed_migration(5.minutes, 1, 1)
+          expect(BackgroundMigrationWorker.jobs.size).to eq 1
+        end
+      end
+    end
+
+    context 'when new artifacts has already existed' do
+      before do
+        job_artifacts.create!(id: 1, project_id: 1, job_id: 1, file_type: 1, size: 123, file: 'archive.zip')
+      end
+
+      it 'does not schedule background migrations' do
+        Sidekiq::Testing.fake! do
+          migrate!
+  
+          expect(BackgroundMigrationWorker.jobs.size).to eq 0
+        end
+      end
+    end
+  end
+
+  context 'when legacy artifacts do not exist' do
+    before do
+      jobs.create!(id: 1, commit_id: 1, project_id: 1, status: :success)
+    end
+
+    it 'does not schedule background migrations' do
+      Sidekiq::Testing.fake! do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq 0
+      end
+    end
+  end
+end