Simplify migration-shared-context (#3749)

svkrieger · web-flow · commit 66ce8055d8e4 · 2024-04-23T15:56:56.000+02:00
When errors are encountered during the setup of migration tests, especially when DB migrations are run up to the one which is currently being tested, it was not possible to set breakpoints in the migration file. This was because the migration-shared-context copied the currently tested migration into its own folder and also the currently tested migration + all newer migrations into another folder. So breakpoints do not work.
Copying the migration files for only applying the correct migrations is not necessary. For the path we can use the normal path to the migrations folder. Additionally, we can use `target` to specify the migration to which we want to migrate. The correct target is calculated by using the current migrations timestamp and the latest migration's timestamp. This works for both, the up and the down path.
diff --git a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb
@@ -18,7 +18,7 @@
         value: 'some_value'
       )
       a1 = db[:isolation_segment_annotations].first(resource_guid: '123')
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       b1 = db[:isolation_segment_annotations].first(resource_guid: '123')
       expect(b1[:guid]).to eq a1[:guid]
       expect(b1[:created_at]).to eq a1[:created_at]
@@ -36,7 +36,7 @@
       db[:isolation_segment_annotations].insert(guid: 'bommel2', resource_guid: '123', key: 'mykey2', value: 'some_value2')
       b1 = db[:isolation_segment_annotations].first(key: 'mykey')
       b2 = db[:isolation_segment_annotations].first(key: 'mykey2')
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       c1 = db[:isolation_segment_annotations].first(key: 'mykey')
       c2 = db[:isolation_segment_annotations].first(key: 'mykey2')
       expect(b1.values).to eq(c1.values)
diff --git a/spec/migrations/20231016094900_microsecond_timestamp_msql_asg_update_spec.rb b/spec/migrations/20231016094900_microsecond_timestamp_msql_asg_update_spec.rb
@@ -26,7 +26,7 @@
       end
 
       # Change TIMESTAMP to TIMESTAMP(6)
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       # the migration shouldn't add accuracy to previously inserted values
       t1_post_migration = ds.first(id: 1)
diff --git a/spec/migrations/20231113105256_add_service_plan_id_index_spec.rb b/spec/migrations/20231113105256_add_service_plan_id_index_spec.rb
@@ -7,6 +7,6 @@
   end
 
   it 'succeeds' do
-    Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
   end
 end
diff --git a/spec/migrations/20231205143526_remove_deployments_with_degenerate_spec.rb b/spec/migrations/20231205143526_remove_deployments_with_degenerate_spec.rb
@@ -20,7 +20,7 @@
 
       expect { db[:deployments].where(guid: 'degenerate_guid').delete }.to raise_error(Sequel::ForeignKeyConstraintViolation)
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       expect(db[:deployments].where(status_reason: 'DEGENERATE').count).to eq(0)
 
diff --git a/spec/migrations/20231221123000_rename_annotations_key_column_spec.rb b/spec/migrations/20231221123000_rename_annotations_key_column_spec.rb
@@ -43,7 +43,7 @@
         expect(db[table.to_sym].columns).to include(:key)
         expect(db[table.to_sym].columns).not_to include(:key_name)
       end
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       annotation_tables.each do |table|
         expect(db[table.to_sym].columns).not_to include(:key)
         expect(db[table.to_sym].columns).to include(:key_name)
diff --git a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb
@@ -18,7 +18,7 @@
 
       a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value')
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       expect(a1.reload.key_name).to eq(truncated_key_name)
     end
 
@@ -28,7 +28,7 @@
 
       a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value')
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       expect(a1.reload.key_name).to eq(key_name)
     end
 
@@ -50,7 +50,7 @@
       expect(b1.id).to be < b2.id
       expect(b1.id).to be < b3.id
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       expect(annotation.where(key_name:).count).to eq(2)
       expect(a1.reload).to be_a(annotation)
       expect { a2.reload }.to raise_error(Sequel::NoExistingObject)
@@ -76,7 +76,7 @@
       b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_b, value: 'v3')
       b4 = annotation.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key_a, value: 'v4')
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       expect(annotation.all.count).to eq(7)
       expect(a1.reload).to be_a(annotation)
@@ -98,7 +98,7 @@
       # In case key_prefix is set
       annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1')
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       expect { annotation.create(resource_guid: i1.guid, key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation)
       expect { annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation)
@@ -110,7 +110,7 @@
       key_a = 'a' * 63
       key_b = 'b' * 63
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       # In case key_prefix is not set
       a1 = annotation.create(resource_guid: i1.guid, key_name: key_a, value: 'v1')
@@ -152,7 +152,7 @@
       expect(b1.id).to be < b2.id
       expect(b1.id).to be < b3.id
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       expect(label.where(key_name: key).count).to eq(2)
       expect(a1.reload).to be_a(label)
       expect { a2.reload }.to raise_error(Sequel::NoExistingObject)
@@ -178,7 +178,7 @@
       b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_b, value: 'v3')
       b4 = label.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key_a, value: 'v4')
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       expect(label.all.count).to eq(7)
       expect(a1.reload).to be_a(label)
@@ -200,7 +200,7 @@
       # In case key_prefix is set
       label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1')
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       expect { label.create(resource_guid: i1.guid, key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation)
       expect { label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation)
@@ -212,7 +212,7 @@
       key_a = 'a' * 63
       key_b = 'b' * 63
 
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
 
       # In case key_prefix is not set
       a1 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v1')
diff --git a/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb b/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb
@@ -20,7 +20,7 @@
 
     context 'after adding the foreign key' do
       it 'prevents inserts with a build_guid that does not exist' do
-        Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+        Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
 
         expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation)
       end
@@ -30,7 +30,7 @@
         db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'build_guid')
         db[:buildpack_lifecycle_data].insert(guid: 'another_bld_guid', build_guid: 'not_exists')
 
-        Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+        Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
 
         expect(db[:buildpack_lifecycle_data].where(guid: 'bld_guid').count).to eq(1)
         expect(db[:buildpack_lifecycle_data].where(guid: 'another_bld_guid').count).to eq(0)
diff --git a/spec/migrations/20240219113000_add_routes_space_id_index_spec.rb b/spec/migrations/20240219113000_add_routes_space_id_index_spec.rb
@@ -7,6 +7,6 @@
   end
 
   it 'succeeds' do
-    Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
   end
 end
diff --git a/spec/migrations/20240222131500_change_delayed_jobs_reserve_index_spec.rb b/spec/migrations/20240222131500_change_delayed_jobs_reserve_index_spec.rb
@@ -7,6 +7,6 @@
   end
 
   it 'succeeds' do
-    Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true)
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true)
   end
 end
diff --git a/spec/migrations/Readme.md b/spec/migrations/Readme.md
@@ -79,8 +79,8 @@ Sequel migration tests have a distinct operation compared to conventional RSpec
 
 ### Usage
 
-Here’s a guide on how to write a migration spec. The `migration` shared context uses the `migration_file` variable, containing the filename of the migration under review. This shared context also provides a `migration_to_test` variable to perform the specific migration. All other processes, including migrating one version before the test migration and fully migrating the table after the test has finished, are handled automatically.
-
+Here’s a guide on how to write a migration spec. The `migration` shared context uses the `migration_file` variable, containing the filename of the migration under review. This shared context also provides a path to all migrations in `migrations_path` as well as the index of the current migration in `current_migration_index` variable to perform the specific migration. All other processes, including migrating one version before the test migration and fully migrating the table after the test has finished, are handled automatically.
+For testing the down part of a migration one can use `Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)`
 ```ruby
 require 'spec_helper'
 require 'migrations/helpers/migration_shared_context'
@@ -94,7 +94,7 @@ RSpec.describe 'migration to modify isolation_segments', isolation: :truncation
     it 'retains the initial name and guid' do
       db[:isolation_segments].insert(name: 'bommel', guid: '123')
       a1 = db[:isolation_segment_annotations].first(resource_guid: '123')
-      expect { Sequel::Migrator.run(db, migration_to_test, allow_missing_migration_files: true) }.not_to raise_error
+      expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error
       b1 = db[:isolation_segment_annotations].first(resource_guid: '123')
       expect(b1[:guid]).to eq a1[:guid]
       expect(b1[:name]).to eq a1[:name]
diff --git a/spec/migrations/helpers/migration_shared_context.rb b/spec/migrations/helpers/migration_shared_context.rb
@@ -1,44 +1,26 @@
 require 'migrations/helpers/matchers'
 
-def mktmpsubdir(tmpdir, name)
-  dir = File.join(tmpdir, name)
-  Dir.mkdir(dir)
-  dir
-end
-
 RSpec.shared_context 'migration' do
-  let(:tmpdir) { Dir.mktmpdir }
-  let(:down_migrations) { mktmpsubdir(tmpdir, 'down_migrations') }
-  let(:migration_to_test) { mktmpsubdir(tmpdir, 'migration_to_test') }
+  let(:migrations_path) { DBMigrator::SEQUEL_MIGRATIONS }
   let(:db) { Sequel::Model.db }
+  let(:current_migration_index) { migration_filename.match(/\A\d+/)[0].to_i }
 
   before do
     allow(db).to receive(:add_index).with(anything, anything, add_index_options).and_call_original
     allow(db).to receive(:drop_index).with(anything, anything, drop_index_options).and_call_original
 
     Sequel.extension :migration
 
-    # Find all migrations
-    all_migration_files = Dir.glob(sprintf('%s/*.rb', DBMigrator::SEQUEL_MIGRATIONS))
-    # Calculate the index of our migration file we`d like to test
-    migration_index = all_migration_files.find_index { |file| file.end_with?(migration_filename) }
-    # Make a file list of the migration file we like to test plus all migrations after the one we want to test
-    migration_files_after_test = all_migration_files[migration_index...]
-
-    # Copy them to a temp directory
-    FileUtils.cp(migration_files_after_test, down_migrations)
-    FileUtils.cp(File.join(DBMigrator::SEQUEL_MIGRATIONS, migration_filename), migration_to_test)
-
-    # Copy migration helper files to temp directory
-    FileUtils.cp_r(File.join(DBMigrator::MIGRATIONS_DIR, 'helpers'), tmpdir)
-
     # Revert the given migration and everything newer so we are at the database version exactly before our migration we want to test.
-    Sequel::Migrator.run(db, down_migrations, target: 0, allow_missing_migration_files: true)
+    Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true)
   end
 
   after do
+    all_migration_filepaths = Dir.glob(sprintf('%s/*.rb', migrations_path))
+    last_migration_file_name = all_migration_filepaths[-1].split('/')[-1]
+    last_migration_index = last_migration_file_name.match(/\A\d+/)[0].to_i
+
     # Complete the migration to not leave the test database half migrated and following tests fail due to this
-    Sequel::Migrator.run(db, down_migrations, allow_missing_migration_files: true)
-    FileUtils.rm_rf(tmpdir)
+    Sequel::Migrator.run(db, migrations_path, target: last_migration_index, allow_missing_migration_files: true)
   end
 end
