From 080dba4a7e036e4bcb9cae69c5de6bfa33ff8b2e Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 13 Feb 2018 12:31:30 -0800 Subject: [PATCH] Avoid dropping tables in test 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`. --- .../populate_untracked_uploads.rb | 2 +- .../prepare_untracked_uploads.rb | 6 ++++-- .../populate_untracked_uploads_spec.rb | 14 ++------------ .../prepare_untracked_uploads_spec.rb | 19 +++++++------------ spec/migrations/README.md | 2 +- .../track_untracked_uploads_helpers.rb | 4 ---- 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(untracked_files_for_uploads.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey + expect(background_migration).to receive(:drop_temp_table) + + 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)