Commit 080dba4a authored by Michael Kozono's avatar Michael Kozono Committed by Rémy Coutable

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`.
parent 1c91c4e3
...@@ -249,7 +249,7 @@ module Gitlab ...@@ -249,7 +249,7 @@ module Gitlab
end end
def drop_temp_table_if_finished 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, UntrackedFile.connection.drop_table(:untracked_files_for_uploads,
if_exists: true) if_exists: true)
end end
......
...@@ -171,8 +171,10 @@ module Gitlab ...@@ -171,8 +171,10 @@ module Gitlab
end end
def drop_temp_table def drop_temp_table
UntrackedFile.connection.drop_table(:untracked_files_for_uploads, unless Rails.env.test? # Dropping a table intermittently breaks test cleanup
if_exists: true) UntrackedFile.connection.drop_table(:untracked_files_for_uploads,
if_exists: true)
end
end end
end end
end end
......
...@@ -14,16 +14,10 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra ...@@ -14,16 +14,10 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra
let!(:uploads) { described_class::Upload } let!(:uploads) { described_class::Upload }
before do before do
DatabaseCleaner.clean
drop_temp_table_if_exists
ensure_temporary_tracking_table_exists ensure_temporary_tracking_table_exists
uploads.delete_all uploads.delete_all
end end
after(:all) do
drop_temp_table_if_exists
end
context 'with untracked files and tracked files in untracked_files_for_uploads' do 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!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) }
let!(:user1) { create(:user, :with_avatar) } let!(:user1) { create(:user, :with_avatar) }
...@@ -122,9 +116,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra ...@@ -122,9 +116,9 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :sidekiq, :migra
end end
it 'drops the temporary tracking table after processing the batch, if there are no untracked rows left' do 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 end
it 'does not block a whole batch because of one bad path' do it 'does not block a whole batch because of one bad path' do
...@@ -260,10 +254,6 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do ...@@ -260,10 +254,6 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UntrackedFile do
ensure_temporary_tracking_table_exists ensure_temporary_tracking_table_exists
end end
after(:all) do
drop_temp_table_if_exists
end
describe '#upload_path' do describe '#upload_path' do
def assert_upload_path(file_path, expected_upload_path) def assert_upload_path(file_path, expected_upload_path)
untracked_file = create_untracked_file(file_path) untracked_file = create_untracked_file(file_path)
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq, :migration, schema: 20180129193323 do
include TrackUntrackedUploadsHelpers include TrackUntrackedUploadsHelpers
include MigrationsHelpers include MigrationsHelpers
let!(:untracked_files_for_uploads) { described_class::UntrackedFile } let!(:untracked_files_for_uploads) { described_class::UntrackedFile }
before do
DatabaseCleaner.clean
end
after do
drop_temp_table_if_exists
end
around do |example| around do |example|
# Especially important so the follow-up migration does not get run # Especially important so the follow-up migration does not get run
Sidekiq::Testing.fake! do Sidekiq::Testing.fake! do
...@@ -76,7 +68,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do ...@@ -76,7 +68,8 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
it 'correctly schedules the follow-up background migration jobs' do it 'correctly schedules the follow-up background migration jobs' do
described_class.new.perform 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) expect(BackgroundMigrationWorker.jobs.size).to eq(1)
end end
...@@ -150,9 +143,11 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do ...@@ -150,9 +143,11 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do
# may not have an upload directory because they have no uploads. # may not have an upload directory because they have no uploads.
context 'when no files were ever uploaded' do context 'when no files were ever uploaded' do
it 'deletes the `untracked_files_for_uploads` table (and does not raise error)' 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 end
end end
...@@ -89,5 +89,5 @@ end ...@@ -89,5 +89,5 @@ end
## Best practices ## Best practices
1. Note that this type of tests do not run within the transaction, we use 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. present.
...@@ -8,10 +8,6 @@ module TrackUntrackedUploadsHelpers ...@@ -8,10 +8,6 @@ module TrackUntrackedUploadsHelpers
Gitlab::BackgroundMigration::PrepareUntrackedUploads.new.send(:ensure_temporary_tracking_table_exists) Gitlab::BackgroundMigration::PrepareUntrackedUploads.new.send(:ensure_temporary_tracking_table_exists)
end 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) def create_or_update_appearance(attrs)
a = Appearance.first_or_initialize(title: 'foo', description: 'bar') a = Appearance.first_or_initialize(title: 'foo', description: 'bar')
a.update!(attrs) a.update!(attrs)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment