Avoid dropping tables in testmk-test-branchmk-fix-pg-undefined-table-ci-errors

And use :migration tag to use deletion strategy, and to avoid caching tables, and to lock into a particular schema.

Attempting to fix intermittent spec errors `PG::UndefinedTable: ERROR:  relation "public.untracked_files_for_uploads" does not exist`.

6 files changed, 15 insertions, 32 deletions

diff --git a/lib/gitlab/background_migration/populate_untracked_uploads.rb b/lib/gitlab/background_migration/populate_untracked_uploads.rb
index 8a8e770940e..ee55fabd6f0 100644
--- a/lib/gitlab/background_migration/populate_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/populate_untracked_uploads.rb
@@ -249,7 +249,7 @@ module Gitlab
       end
 
       def drop_temp_table_if_finished
-        if UntrackedFile.all.empty?
+        if UntrackedFile.all.empty? && !Rails.env.test? # Dropping a table intermittently breaks test cleanup
           UntrackedFile.connection.drop_table(:untracked_files_for_uploads,
                                               if_exists: true)
         end
diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
index 298de005b9b..914a9e48a2f 100644
--- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb
+++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb
@@ -171,8 +171,10 @@ module Gitlab
       end
 
       def drop_temp_table
-        UntrackedFile.connection.drop_table(:untracked_files_for_uploads,
-                                            if_exists: true)
+        unless Rails.env.test? # Dropping a table intermittently breaks test cleanup
+          UntrackedFile.connection.drop_table(:untracked_files_for_uploads,
+                                              if_exists: true)
+        end
       end
     end
   end
diff --git a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
index fb3f29ff4c9..c8fa252439a 100644
--- a/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb
@@ -14,16 +14,10 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra
   let!(:uploads) { described_class::Upload }
 
   before do
-    DatabaseCleaner.clean
-    drop_temp_table_if_exists
     ensure_temporary_tracking_table_exists
     uploads.delete_all
   end
 
-  after(:all) do
-    drop_temp_table_if_exists
-  end
-
   context 'with untracked files and tracked files in untracked_files_for_uploads' do
     let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
     let!(:user1) { create(:user, :with_avatar) }
@@ -122,9 +116,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra
     end
 
     it 'drops the temporary tracking table after processing the batch, if there are no untracked rows left' do
-      subject.perform(1, untracked_files_for_uploads.last.id)
+      expect(subject).to receive(:drop_temp_table_if_finished)
 
-      expect(ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey
+      subject.perform(1, untracked_files_for_uploads.last.id)
     end
 
     it 'does not block a whole batch because of one bad path' do
@@ -260,10 +254,6 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do
     ensure_temporary_tracking_table_exists
   end
 
-  after(:all) do
-    drop_temp_table_if_exists
-  end
-
   describe '#upload_path' do
     def assert_upload_path(file_path, expected_upload_path)
       untracked_file = create_untracked_file(file_path)
diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
index 43f3548eadc..ca77e64ae40 100644
--- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
+++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb
@@ -1,19 +1,11 @@
 require 'spec_helper'
 
-describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
+describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migration, schema: 20180129193323 do
   include TrackUntrackedUploadsHelpers
   include MigrationsHelpers
 
   let!(:untracked_files_for_uploads) { described_class::UntrackedFile }
 
-  before do
-    DatabaseCleaner.clean
-  end
-
-  after do
-    drop_temp_table_if_exists
-  end
-
   around do |example|
     # Especially important so the follow-up migration does not get run
     Sidekiq::Testing.fake! do
@@ -76,7 +68,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
       it 'correctly schedules the follow-up background migration jobs' do
         described_class.new.perform
 
-        expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5)
+        ids = described_class::UntrackedFile.all.order(:id).pluck(:id)
+        expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(ids.first, ids.last)
         expect(BackgroundMigrationWorker.jobs.size).to eq(1)
       end
 
@@ -150,9 +143,11 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
   # may not have an upload directory because they have no uploads.
   context 'when no files were ever uploaded' do
     it 'deletes the `untracked_files_for_uploads` table (and does not raise error)' do
-      described_class.new.perform
+      background_migration = described_class.new
+
+      expect(background_migration).to receive(:drop_temp_table)
 
-      expect(untracked_files_for_uploads.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey
+      background_migration.perform
     end
   end
 end
diff --git a/spec/migrations/README.md b/spec/migrations/README.md
index 45cf25b96de..49760fa62b8 100644
--- a/spec/migrations/README.md
+++ b/spec/migrations/README.md
@@ -89,5 +89,5 @@ end
 ## Best practices
 
 1. Note that this type of tests do not run within the transaction, we use
-a truncation database cleanup strategy. Do not depend on transaction being
+a deletion database cleanup strategy. Do not depend on transaction being
 present.
diff --git a/spec/support/track_untracked_uploads_helpers.rb b/spec/support/track_untracked_uploads_helpers.rb
index 5752078d2a0..a8b3ed1f41c 100644
--- a/spec/support/track_untracked_uploads_helpers.rb
+++ b/spec/support/track_untracked_uploads_helpers.rb
@@ -8,10 +8,6 @@ module TrackUntrackedUploadsHelpers
     Gitlab::BackgroundMigration::PrepareUntrackedUploads.new.send(:ensure_temporary_tracking_table_exists)
   end
 
-  def drop_temp_table_if_exists
-    ActiveRecord::Base.connection.drop_table(:untracked_files_for_uploads) if ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads)
-  end
-
   def create_or_update_appearance(attrs)
     a = Appearance.first_or_initialize(title: 'foo', description: 'bar')
     a.update!(attrs)