From d77de08f48b0d43465918c959c41f6a951bd8d21 Mon Sep 17 00:00:00 2001 From: johha Date: Tue, 20 Jan 2026 17:10:41 +0100 Subject: [PATCH 1/4] Optimize migration specs by consolidating test cases to reduce execution time --- ...40_backfill_status_for_deployments_spec.rb | 264 +++++++----------- ...0_streamline_annotation_key_prefix_spec.rb | 39 +-- ...00_add_annotation_label_uniqueness_spec.rb | 84 ++---- ...add_delete_cascade_to_foreign_keys_spec.rb | 41 ++- ...131908_add_user_guid_to_jobs_table_spec.rb | 10 +- ...ask_guid_index_to_app_usage_events_spec.rb | 58 ++-- ...742_add_jobs_user_guid_state_index_spec.rb | 11 +- ..._add_foreign_key_apps_droplet_guid_spec.rb | 33 +-- ...straint_quota_definitions_name_key_spec.rb | 75 ++--- ...ed_service_bindings_enabled_column_spec.rb | 25 +- ...ove_unnecessary_fk_in_job_warnings_spec.rb | 18 +- ..._add_user_id_index_to_roles_tables_spec.rb | 114 ++++---- ...250610212414_add_user_to_processes_spec.rb | 46 +-- .../20250630170610_add_user_to_tasks_spec.rb | 46 +-- ...27_allow_multiple_service_bindings_spec.rb | 38 +-- ...ecurity_groups_spaces_unique_index_spec.rb | 23 +- ...135214_route_bindings_unique_index_spec.rb | 17 +- ...ided_metadata_to_service_instances_spec.rb | 57 ++-- 18 files changed, 363 insertions(+), 636 deletions(-) diff --git a/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb b/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb index 39101ca3703..99224af9022 100644 --- a/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb +++ b/spec/migrations/20190712210940_backfill_status_for_deployments_spec.rb @@ -12,169 +12,117 @@ let(:app) { VCAP::CloudController::AppModel.make } - context 'when a deployment has state DEPLOYED' do - let!(:deployment_with_state_deployed) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deployed', - state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to FINALIZED without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_deployed.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) - expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) - expect(deployment.status_reason).to be_nil - end - end + it 'backfills status_value based on deployment state' do + # Create all deployment variations + deployment_deployed = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-deployed', + state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state CANCELED' do - let!(:deployment_with_state_canceled) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-canceled', - state: VCAP::CloudController::DeploymentModel::CANCELED_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to FINALIZED without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_canceled.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELED_STATE) - expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) - expect(deployment.status_reason).to be_nil - end - end + deployment_canceled = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-canceled', + state: VCAP::CloudController::DeploymentModel::CANCELED_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state FAILED' do - let!(:deployment_with_state_failed) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-failed', - state: 'FAILED', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to FINALIZED without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_failed.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) - expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) - expect(deployment.status_reason).to be_nil - end - end + deployment_failed = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-failed', + state: 'FAILED', + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state DEPLOYING' do - let!(:deployment_with_state_deploying) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deploying', - state: VCAP::CloudController::DeploymentModel::DEPLOYING_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to DEPLOYING without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_deploying.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to be_nil - end - end + deployment_deploying = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-deploying', + state: VCAP::CloudController::DeploymentModel::DEPLOYING_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state CANCELING' do - let!(:deployment_with_state_canceling) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-canceling', - state: VCAP::CloudController::DeploymentModel::CANCELING_STATE, - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to DEPLOYING without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_canceling.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to be_nil - end - end + deployment_canceling = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-canceling', + state: VCAP::CloudController::DeploymentModel::CANCELING_STATE, + app: app, + original_web_process_instance_count: 1 + ) - context 'when a deployment has state FAILING' do - let!(:deployment_with_state_failing) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-failing', - state: 'FAILING', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'sets status_value to DEPLOYING without changing the state' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_failing.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to be_nil - end - end + deployment_failing = VCAP::CloudController::DeploymentModel.create( + guid: 'with-state-failing', + state: 'FAILING', + app: app, + original_web_process_instance_count: 1 + ) + + deployment_with_existing_status = VCAP::CloudController::DeploymentModel.create( + guid: 'with-existing-status', + state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, + status_value: 'foo', + status_reason: 'bar', + app: app, + original_web_process_instance_count: 1 + ) + + deployment_failing_with_reason = VCAP::CloudController::DeploymentModel.create( + guid: 'failing-with-reason', + state: 'FAILING', + status_value: 'foo', + status_reason: 'bar', + app: app, + original_web_process_instance_count: 1 + ) - context 'when the deployment already has a status' do - context 'when the deployment has state DEPLOYED' do - let!(:deployment_with_state_deployed) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deployed', - state: VCAP::CloudController::DeploymentModel::DEPLOYED_STATE, - status_value: 'foo', - status_reason: 'bar', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'does not reset the status value' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_deployed.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) - expect(deployment.status_value).to eq('foo') - expect(deployment.status_reason).to eq('bar') - end - end - - context 'when the deployment has state FAILING' do - let!(:deployment_with_state_failing) do - VCAP::CloudController::DeploymentModel.create( - guid: 'with-state-deployed', - state: 'FAILING', - status_value: 'foo', - status_reason: 'bar', - app: app, - original_web_process_instance_count: 1 - ) - end - - it 'does not reset the status reason' do - Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) - deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_state_failing.guid).first - - expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) - expect(deployment.status_value).to eq('DEPLOYING') - expect(deployment.status_reason).to eq('bar') - end - end + # Run migration once + Sequel::Migrator.run(VCAP::CloudController::DeploymentModel.db, tmp_migrations_dir, table: :my_fake_table) + + # Test: DEPLOYED state -> FINALIZED status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_deployed.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) + expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) + expect(deployment.status_reason).to be_nil + + # Test: CANCELED state -> FINALIZED status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_canceled.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELED_STATE) + expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) + expect(deployment.status_reason).to be_nil + + # Test: FAILED state -> DEPLOYED state + FINALIZED status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_failed.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) + expect(deployment.status_value).to eq(VCAP::CloudController::DeploymentModel::FINALIZED_STATUS_VALUE) + expect(deployment.status_reason).to be_nil + + # Test: DEPLOYING state -> DEPLOYING status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_deploying.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to be_nil + + # Test: CANCELING state -> DEPLOYING status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_canceling.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::CANCELING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to be_nil + + # Test: FAILING state -> DEPLOYING state + DEPLOYING status + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_failing.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to be_nil + + # Test: existing status_value is not reset + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_with_existing_status.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYED_STATE) + expect(deployment.status_value).to eq('foo') + expect(deployment.status_reason).to eq('bar') + + # Test: existing status_reason is preserved + deployment = VCAP::CloudController::DeploymentModel.where(guid: deployment_failing_with_reason.guid).first + expect(deployment.state).to eq(VCAP::CloudController::DeploymentModel::DEPLOYING_STATE) + expect(deployment.status_value).to eq('DEPLOYING') + expect(deployment.status_reason).to eq('bar') end end diff --git a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb index b3e70aaeec6..7de9196d9c7 100644 --- a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb +++ b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb @@ -7,7 +7,7 @@ end describe 'annotation tables' do - it 'converts all legacy key_prefixes to annotations with prefixes in the key_prefix column' do + it 'converts legacy key_prefixes to prefixes in key_prefix column and leaves non-legacy values unchanged' do db[:isolation_segments].insert(name: 'bommel', guid: '123') db[:isolation_segment_annotations].insert( guid: 'bommel', @@ -17,28 +17,29 @@ key: 'mylegacyprefix/mykey', value: 'some_value' ) - a1 = db[:isolation_segment_annotations].first(resource_guid: '123') - 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] - expect(b1[:updated_at]).not_to eq a1[:updated_at] - expect(b1[:resource_guid]).to eq a1[:resource_guid] - expect(b1[:key_prefix]).not_to eq a1[:key_prefix] - expect(b1[:key]).not_to eq a1[:key] - expect(b1[:key_prefix]).to eq 'mylegacyprefix' - expect(b1[:key]).to eq 'mykey' - end + db[:isolation_segment_annotations].insert(guid: 'bommel2', resource_guid: '123', key_prefix: 'myprefix', key: 'mykey', value: 'some_value') + db[:isolation_segment_annotations].insert(guid: 'bommel3', resource_guid: '123', key: 'mykey2', value: 'some_value2') - it 'doesnt touch any values that have no legacy key_prefix in its key field' do - db[:isolation_segments].insert(name: 'bommel', guid: '123') - db[:isolation_segment_annotations].insert(guid: 'bommel', resource_guid: '123', key_prefix: 'myprefix', key: 'mykey', value: 'some_value') - db[:isolation_segment_annotations].insert(guid: 'bommel2', resource_guid: '123', key: 'mykey2', value: 'some_value2') + a1 = db[:isolation_segment_annotations].first(key: 'mylegacyprefix/mykey') b1 = db[:isolation_segment_annotations].first(key: 'mykey') b2 = db[:isolation_segment_annotations].first(key: 'mykey2') + 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') + + # Check legacy prefix was converted + a1_after = db[:isolation_segment_annotations].first(guid: 'bommel') + expect(a1_after[:guid]).to eq a1[:guid] + expect(a1_after[:created_at]).to eq a1[:created_at] + expect(a1_after[:updated_at]).not_to eq a1[:updated_at] + expect(a1_after[:resource_guid]).to eq a1[:resource_guid] + expect(a1_after[:key_prefix]).not_to eq a1[:key_prefix] + expect(a1_after[:key]).not_to eq a1[:key] + expect(a1_after[:key_prefix]).to eq 'mylegacyprefix' + expect(a1_after[:key]).to eq 'mykey' + + # Check non-legacy values unchanged + c1 = db[:isolation_segment_annotations].first(guid: 'bommel2') + c2 = db[:isolation_segment_annotations].first(guid: 'bommel3') expect(b1.values).to eq(c1.values) expect(b2.values).to eq(c2.values) end diff --git a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb index fea59a4f8a9..99996e3df9e 100644 --- a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb +++ b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb @@ -11,25 +11,19 @@ let(:label) { VCAP::CloudController::IsolationSegmentLabelModel } describe 'annotation tables' do - it 'truncates keys to 63 characters' do + it 'truncates keys to 63 characters and leaves shorter keys unchanged' do i1 = isolation_segment.create(name: 'bommel') - key_name = 'a' * 64 + key_name_long = 'a' * 64 truncated_key_name = 'a' * 63 + key_name_short = 'b' * 63 - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value') + a1 = annotation.create(resource_guid: i1.guid, key_name: key_name_long, value: 'some_value') + a2 = annotation.create(resource_guid: i1.guid, key_name: key_name_short, value: 'some_value2') 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 - it 'leaves keys that are shorter than 64 characters unchanged' do - i1 = isolation_segment.create(name: 'bommel') - key_name = 'a' * 63 - - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'some_value') - - 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) + expect(a1.reload.key_name).to eq(truncated_key_name) + expect(a2.reload.key_name).to eq(key_name_short) end it 'removes duplicate annotations but keeps one with smallest id' do @@ -88,10 +82,11 @@ expect(b4.reload).to be_a(annotation) end - it 'does not allow adding a duplicate' do + it 'does not allow adding a duplicate but allows different annotations' do i1 = isolation_segment.create(name: 'bommel') i2 = isolation_segment.create(name: 'sword') key = 'a' * 63 + key_b = 'b' * 63 # In case key_prefix is not set annotation.create(resource_guid: i1.guid, key_name: key, value: 'v1') @@ -100,36 +95,21 @@ expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # Test: does not allow adding a duplicate 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) - end - - it 'does allow adding a different annotation' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 - key_b = 'b' * 63 - - 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') - a2 = annotation.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = annotation.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - 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') + # Test: does allow adding different annotations + a1 = annotation.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') + a2 = annotation.create(resource_guid: i2.guid, key_name: key_b, value: 'v2') + b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key, value: 'v4') + b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'sword', key_name: key, value: 'v5') - expect(annotation.all.count).to eq(7) + expect(annotation.where(key_name: key_b).count).to eq(2) expect(a1.reload).to be_a(annotation) expect(a2.reload).to be_a(annotation) - expect(a3.reload).to be_a(annotation) expect(b1.reload).to be_a(annotation) expect(b2.reload).to be_a(annotation) - expect(b3.reload).to be_a(annotation) - expect(b4.reload).to be_a(annotation) end end @@ -190,10 +170,11 @@ expect(b4.reload).to be_a(label) end - it 'does not allow adding a duplicate' do + it 'does not allow adding a duplicate but allows different labels' do i1 = isolation_segment.create(name: 'bommel') i2 = isolation_segment.create(name: 'sword') key = 'a' * 63 + key_b = 'b' * 63 # In case key_prefix is not set label.create(resource_guid: i1.guid, key_name: key, value: 'v1') @@ -202,36 +183,21 @@ expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # Test: does not allow adding a duplicate 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) - end - - it 'does allow adding a different label' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 - key_b = 'b' * 63 - - 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') - a2 = label.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - 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') + # Test: does allow adding different labels + a1 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') + a2 = label.create(resource_guid: i2.guid, key_name: key_b, value: 'v2') + b1 = label.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key, value: 'v4') + b2 = label.create(resource_guid: i2.guid, key_prefix: 'sword', key_name: key, value: 'v5') - expect(label.all.count).to eq(7) + expect(label.where(key_name: key_b).count).to eq(2) expect(a1.reload).to be_a(label) expect(a2.reload).to be_a(label) - expect(a3.reload).to be_a(label) expect(b1.reload).to be_a(label) expect(b2.reload).to be_a(label) - expect(b3.reload).to be_a(label) - expect(b4.reload).to be_a(label) end end end 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 index 0a0ac990d42..9e0d290847a 100644 --- a/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb +++ b/spec/migrations/20240115163000_add_delete_cascade_to_foreign_keys_spec.rb @@ -12,33 +12,28 @@ db[:builds].delete end - context 'before adding the foreign key' do - it 'allows inserts with a build_guid that does not exist' do - expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.not_to raise_error - end - end - - context 'after adding the foreign key' do - it 'prevents inserts with a build_guid that does not exist' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + it 'adds foreign key constraint and deletes orphaned entries' do + # Before migration: allows inserts with non-existent build_guid + expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid_test', build_guid: 'not_exists') }.not_to raise_error + db[:buildpack_lifecycle_data].delete - expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - end + # Setup test data + db[:builds].insert(guid: 'build_guid') + db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'build_guid') + db[:buildpack_lifecycle_buildpacks].insert(guid: 'blb_guid', buildpack_lifecycle_data_guid: 'bld_guid') + db[:buildpack_lifecycle_data].insert(guid: 'another_bld_guid', build_guid: 'not_exists') + db[:buildpack_lifecycle_buildpacks].insert(guid: 'another_blb_guid', buildpack_lifecycle_data_guid: 'another_bld_guid') - it 'deleted orphaned buildpack_lifecycle_data entries but kept valid ones' do - db[:builds].insert(guid: 'build_guid') - db[:buildpack_lifecycle_data].insert(guid: 'bld_guid', build_guid: 'build_guid') - db[:buildpack_lifecycle_buildpacks].insert(guid: 'blb_guid', buildpack_lifecycle_data_guid: 'bld_guid') - db[:buildpack_lifecycle_data].insert(guid: 'another_bld_guid', build_guid: 'not_exists') - db[:buildpack_lifecycle_buildpacks].insert(guid: 'another_blb_guid', buildpack_lifecycle_data_guid: 'another_bld_guid') + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + # After migration: prevents inserts with non-existent build_guid + expect { db[:buildpack_lifecycle_data].insert(guid: 'bld_guid_new', build_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - expect(db[:buildpack_lifecycle_data].where(guid: 'bld_guid').count).to eq(1) - expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'blb_guid').count).to eq(1) - expect(db[:buildpack_lifecycle_data].where(guid: 'another_bld_guid').count).to eq(0) - expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'another_blb_guid').count).to eq(0) - end + # After migration: orphaned entries deleted, valid ones kept + expect(db[:buildpack_lifecycle_data].where(guid: 'bld_guid').count).to eq(1) + expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'blb_guid').count).to eq(1) + expect(db[:buildpack_lifecycle_data].where(guid: 'another_bld_guid').count).to eq(0) + expect(db[:buildpack_lifecycle_buildpacks].where(guid: 'another_blb_guid').count).to eq(0) end end end diff --git a/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb b/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb index 52800f06c55..8a57d64a66c 100644 --- a/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb +++ b/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb @@ -7,15 +7,13 @@ end describe 'jobs table' do - it 'adds a column `user_guid`' do + it 'adds a column `user_guid` and an index on it' do expect(db[:jobs].columns).not_to include(:user_guid) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:jobs].columns).to include(:user_guid) - end - - it 'adds an index on the user_guid column' do expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + expect(db[:jobs].columns).to include(:user_guid) expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) end diff --git a/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb b/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb index 29f484a1532..b200cae6482 100644 --- a/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb +++ b/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb @@ -8,51 +8,27 @@ describe 'app_usage_events table' do describe 'up migration' do - context 'index does not exist' do - it 'adds an index on the task_guid column' do - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - end - end - - context 'index already exists' do - before do - db.add_index :app_usage_events, :task_guid, name: :app_usage_events_task_guid_index - end - - it 'does not fail' do - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - end + it 'adds an index on the task_guid column and handles idempotency' do + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) + + # Test idempotency: running again when index exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) end end describe 'down migration' do - context 'index does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :app_usage_events, :task_guid, name: :app_usage_events_task_guid_index - end - - it 'does not fail' do - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - end - end - - context 'index does exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - it 'removes the index' do - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - end + it 'removes the index and handles idempotency' do + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) + + # Test idempotency: running rollback again when index doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) end end end diff --git a/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb b/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb index 13b4a929d2a..cb885c748a1 100644 --- a/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb +++ b/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb @@ -19,17 +19,14 @@ def partial_index_present end describe 'jobs table' do - it 'removes index `jobs_user_guid_index`' do + it 'removes index `jobs_user_guid_index` and adds `jobs_user_guid_state_index`' do skip if db.database_type != :postgres expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - end - - it 'adds an index `jobs_user_guid_state_index`' do - skip if db.database_type != :postgres expect(partial_index_present).to be_falsey + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) expect(partial_index_present).to be_truthy end diff --git a/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb b/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb index dcaa4483b22..1d2940b8673 100644 --- a/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb +++ b/spec/migrations/20240904132000_add_foreign_key_apps_droplet_guid_spec.rb @@ -11,29 +11,24 @@ db[:apps].delete end - context 'before adding the foreign key' do - it 'allows inserts with a droplet_guid that does not exist' do - expect { db[:apps].insert(guid: 'app_guid', droplet_guid: 'not_exists') }.not_to raise_error - end - end - - context 'after adding the foreign key' do - it 'prevents inserts with a droplet_guid that does not exist' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + it 'adds foreign key constraint and removes invalid references' do + # Before migration: allows inserts with non-existent droplet_guid + expect { db[:apps].insert(guid: 'app_guid_test', droplet_guid: 'not_exists') }.not_to raise_error + db[:apps].delete - expect { db[:apps].insert(guid: 'app_guid', droplet_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - end + # Setup test data + db[:droplets].insert(guid: 'droplet_guid', state: 'some_state') + db[:apps].insert(guid: 'app_guid', droplet_guid: 'droplet_guid') + db[:apps].insert(guid: 'another_app_guid', droplet_guid: 'not_exists') - it 'removed references to not existing droplets' do - db[:droplets].insert(guid: 'droplet_guid', state: 'some_state') - db[:apps].insert(guid: 'app_guid', droplet_guid: 'droplet_guid') - db[:apps].insert(guid: 'another_app_guid', droplet_guid: 'not_exists') + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + # After migration: prevents inserts with non-existent droplet_guid + expect { db[:apps].insert(guid: 'app_guid_new', droplet_guid: 'not_exists') }.to raise_error(Sequel::ForeignKeyConstraintViolation) - expect(db[:apps].where(guid: 'app_guid').get(:droplet_guid)).to eq('droplet_guid') - expect(db[:apps].where(guid: 'another_app_guid').get(:droplet_guid)).to be_nil - end + # After migration: invalid references removed, valid ones kept + expect(db[:apps].where(guid: 'app_guid').get(:droplet_guid)).to eq('droplet_guid') + expect(db[:apps].where(guid: 'another_app_guid').get(:droplet_guid)).to be_nil end end end diff --git a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb index be08fa3a352..6839e3da3b9 100644 --- a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb +++ b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb @@ -11,22 +11,15 @@ skip if db.database_type != :mysql end - it 'removes the unique constraint' do + it 'removes the unique constraint and handles idempotency' do expect(db.indexes(:quota_definitions)).to include(:name) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:name) - end - - context 'unique constraint on name column does not exist' do - before do - db.drop_index :quota_definitions, :name, name: :name - end - it 'does not throw an error' do - expect(db.indexes(:quota_definitions)).not_to include(:name) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).not_to include(:name) - end + # Test idempotency: if constraint already removed, doesn't error + db.drop_index :quota_definitions, :name, name: :name, if_exists: true + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).not_to include(:name) end end @@ -35,24 +28,14 @@ skip if db.database_type != :postgres end - it 'removes the unique constraint' do + it 'removes the unique constraint and handles idempotency' do expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - end - context 'unique constraint on name column does not exist' do - before do - db.alter_table :quota_definitions do - drop_constraint :quota_definitions_name_key - end - end - - it 'does not throw an error' do - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - end + # Test idempotency: if constraint already removed, doesn't error + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) end end end @@ -67,26 +50,14 @@ skip if db.database_type != :mysql end - it 'adds the unique constraint' do + it 'adds the unique constraint and handles idempotency' do expect(db.indexes(:quota_definitions)).not_to include(:name) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:name) - end - context 'unique constraint on name column already exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - - db.alter_table :quota_definitions do - add_index :name, name: :name - end - end - - it 'does not fail' do - expect(db.indexes(:quota_definitions)).to include(:name) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).to include(:name) - end + # Test idempotency: if constraint already exists, doesn't fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).to include(:name) end end @@ -95,26 +66,14 @@ skip if db.database_type != :postgres end - it 'adds the unique constraint' do + it 'adds the unique constraint and handles idempotency' do expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - end - context 'unique constraint on name column already exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - - db.alter_table :quota_definitions do - add_unique_constraint :name, name: :quota_definitions_name_key - end - end - - it 'does not fail' do - expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - end + # Test idempotency: if constraint already exists, doesn't fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) end end end diff --git a/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb b/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb index 184bb16bb90..8d48828fe0f 100644 --- a/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb +++ b/spec/migrations/20241203085500_add_apps_file_based_service_bindings_enabled_column_spec.rb @@ -7,28 +7,27 @@ end describe 'apps table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } + it 'adds column with correct properties and default values' do + # Insert an app before migration to test default value on existing entries + db[:apps].insert(guid: 'existing_app_guid') - it 'adds a column `file_based_service_bindings_enabled`' do + # Verify column doesn't exist yet expect(db[:apps].columns).not_to include(:file_based_service_bindings_enabled) - run_migration + + # Run migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + + # Verify column was added expect(db[:apps].columns).to include(:file_based_service_bindings_enabled) - end - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration + # Verify default value on existing entries is false expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_service_bindings_enabled]).to be(false) - end - it 'sets the default value of new entries to false' do - run_migration + # Verify default value on new entries is false db[:apps].insert(guid: 'new_app_guid') expect(db[:apps].first(guid: 'new_app_guid')[:file_based_service_bindings_enabled]).to be(false) - end - it 'forbids null values' do - run_migration + # Verify null values are forbidden expect { db[:apps].insert(guid: 'app_guid__nil', file_based_service_bindings_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) end end diff --git a/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb b/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb index d3dd97ef9d6..c954f0cb703 100644 --- a/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb +++ b/spec/migrations/20250116144231_remove_unnecessary_fk_in_job_warnings_spec.rb @@ -7,25 +7,15 @@ end describe 'job_warnings table' do - it 'removes the fk constraint as well as the column' do + it 'removes the fk constraint and column, handles idempotency' do + # Run migration Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) expect(db.foreign_key_list(:job_warnings)).to be_empty expect(db[:job_warnings].columns).not_to include(:fk_jobs_id) - end - - context 'foreign key constraint does not exist' do - before do - unless db.foreign_key_list(:job_warnings).empty? - db.alter_table(:job_warnings) do - drop_foreign_key :fk_jobs_id - end - end - end - it 'does not fail' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end + # Test idempotency: running again when constraint doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end end diff --git a/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb b/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb index 5e66ac7a0aa..b9e8d335aec 100644 --- a/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb +++ b/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb @@ -1,76 +1,76 @@ require 'spec_helper' require 'migrations/helpers/migration_shared_context' -RSpec.shared_examples 'adding an index for table' do |table| - describe "#{table} table" do - let(:table_sym) { table.to_sym } - let(:index_sym) { :"#{table}_user_id_index" } +RSpec.describe 'migration to add an index for user_id on all roles tables', isolation: :truncation, type: :migration do + include_context 'migration' do + let(:migration_filename) { '20250318112800_add_user_id_index_to_roles_tables.rb' } + end - before do - skip unless db.database_type == :postgres - end + let(:tables) do + %w[ + organizations_auditors + organizations_billing_managers + organizations_managers + organizations_users + spaces_auditors + spaces_developers + spaces_managers + spaces_supporters + ] + end + + before do + skip unless db.database_type == :postgres + end - describe 'up migration' do - context 'index does not exist' do - it 'adds the index' do - expect(db.indexes(table_sym)).not_to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).to include(index_sym) - end + describe 'up migration' do + it 'adds indexes for all tables and handles idempotency' do + # Verify initial state: no indexes exist + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).not_to include(index_sym) end - context 'index already exists' do - before do - db.add_index table_sym, :user_id, name: index_sym - end + # Run migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - it 'does not fail' do - expect(db.indexes(table_sym)).to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).to include(index_sym) - end + # Verify all indexes were created + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).to include(index_sym) end + + # Test idempotency: running migration again should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end + end - describe 'down migration' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end + describe 'down migration' do + it 'removes indexes from all tables and handles idempotency' do + # Run up migration first + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - context 'index exists' do - it 'removes the index' do - expect(db.indexes(table_sym)).to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).not_to include(index_sym) - end + # Verify indexes exist + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).to include(index_sym) end - context 'index does not exist' do - before do - db.drop_index table_sym, :user_id, name: index_sym - end + # Run down migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - it 'does not fail' do - expect(db.indexes(table_sym)).not_to include(index_sym) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(table_sym)).not_to include(index_sym) - end + # Verify all indexes were removed + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).not_to include(index_sym) end - end - end -end -RSpec.describe 'migration to add an index for user_id on all roles tables', isolation: :truncation, type: :migration do - include_context 'migration' do - let(:migration_filename) { '20250318112800_add_user_id_index_to_roles_tables.rb' } + # Test idempotency: running rollback again should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + end end - - include_examples 'adding an index for table', 'organizations_auditors' - include_examples 'adding an index for table', 'organizations_billing_managers' - include_examples 'adding an index for table', 'organizations_managers' - include_examples 'adding an index for table', 'organizations_users' - include_examples 'adding an index for table', 'spaces_auditors' - include_examples 'adding an index for table', 'spaces_developers' - include_examples 'adding an index for table', 'spaces_managers' - include_examples 'adding an index for table', 'spaces_supporters' end diff --git a/spec/migrations/20250610212414_add_user_to_processes_spec.rb b/spec/migrations/20250610212414_add_user_to_processes_spec.rb index 37deae8fcc6..3b43aaf621e 100644 --- a/spec/migrations/20250610212414_add_user_to_processes_spec.rb +++ b/spec/migrations/20250610212414_add_user_to_processes_spec.rb @@ -7,48 +7,24 @@ end describe 'processes table' do - it 'adds a column `user`' do + it 'adds a column `user` and handles idempotency' do expect(db[:processes].columns).not_to include(:user) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:processes].columns).to include(:user) - end - - describe 'idempotency of up' do - context '`user` column already exists' do - before do - db.add_column :processes, :user, String, size: 255, if_not_exists: true - end - it 'does not fail' do - expect(db[:processes].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test idempotency: running again when column exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end describe 'idempotency of down' do - context 'user column exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - it 'continues to remove the `user_guid` column' do - expect(db[:processes].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:processes].columns).not_to include(:user) - end - end - - context 'column does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_column :processes, :user - end - - it 'does not fail' do - expect(db[:processes].columns).not_to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + it 'removes column and handles idempotency' do + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + expect(db[:processes].columns).to include(:user) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:processes].columns).not_to include(:user) + + # Test idempotency: running rollback again when column doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error end end end diff --git a/spec/migrations/20250630170610_add_user_to_tasks_spec.rb b/spec/migrations/20250630170610_add_user_to_tasks_spec.rb index 6d52d8bfca2..2bbd52a6366 100644 --- a/spec/migrations/20250630170610_add_user_to_tasks_spec.rb +++ b/spec/migrations/20250630170610_add_user_to_tasks_spec.rb @@ -7,48 +7,24 @@ end describe 'tasks table' do - it 'adds a column `user`' do + it 'adds a column `user` and handles idempotency' do expect(db[:tasks].columns).not_to include(:user) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:tasks].columns).to include(:user) - end - - describe 'idempotency of up' do - context '`user` column already exists' do - before do - db.add_column :tasks, :user, String, size: 255, if_not_exists: true - end - it 'does not fail' do - expect(db[:tasks].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test idempotency: running again when column exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end describe 'idempotency of down' do - context 'user column exists' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - it 'continues to remove the `user_guid` column' do - expect(db[:tasks].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:tasks].columns).not_to include(:user) - end - end - - context 'column does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_column :tasks, :user - end - - it 'does not fail' do - expect(db[:tasks].columns).not_to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + it 'removes column and handles idempotency' do + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + expect(db[:tasks].columns).to include(:user) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:tasks].columns).not_to include(:user) + + # Test idempotency: running rollback again when column doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error end end end diff --git a/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb b/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb index 707a727609a..a9709316ff4 100644 --- a/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb +++ b/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb @@ -8,60 +8,34 @@ describe 'service_bindings table' do context 'up migration' do - it 'is in the correct state before migration' do + it 'replaces unique constraints with indexes and handles idempotency' do + # Verify initial state expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - end - it 'migrates successfully' do expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name) expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid) expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) - end - it 'does not fail if indexes/constraints are already in desired state' do - db.alter_table :service_bindings do - drop_constraint :unique_service_binding_service_instance_guid_app_guid - drop_constraint :unique_service_binding_app_guid_name - end - if db.database_type == :postgres - db.add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true, concurrently: true - db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true, concurrently: true - else - db.add_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_not_exists: true - db.add_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_not_exists: true - end + # Test idempotency: running again should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end context 'down migration' do - it 'rolls back successfully' do + it 'rolls back successfully and handles idempotency' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - end - - it 'does not fail if indexes/constraints are already in desired state' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - db.alter_table :service_bindings do - add_unique_constraint %i[service_instance_guid app_guid], name: :unique_service_binding_service_instance_guid_app_guid - add_unique_constraint %i[app_guid name], name: :unique_service_binding_app_guid_name - end - if db.database_type == :postgres - db.drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true, concurrently: true - db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true, concurrently: true - else - db.drop_index :service_bindings, %i[app_guid service_instance_guid], name: :service_bindings_app_guid_service_instance_guid_index, if_exists: true - db.drop_index :service_bindings, %i[app_guid name], name: :service_bindings_app_guid_name_index, if_exists: true - end + # Test idempotency: running rollback again should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error end end diff --git a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb index 94affbcf364..5ba2ee65516 100644 --- a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb +++ b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb @@ -13,12 +13,12 @@ describe 'security_groups_spaces table' do context 'up migration' do - it 'is in the correct state before migration' do + it 'removes duplicates, updates indexes, and handles idempotency' do + # Verify initial state expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) - end - it 'removes duplicates and migrates successfully by adding unique index' do + # Insert test data with duplicates db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id) @@ -44,31 +44,20 @@ # Verify indexes are updated expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids) expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) - end - it 'does not fail if indexes/constraints are already in desired state' do - db.alter_table :security_groups_spaces do - add_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids - drop_index %i[security_group_id space_id], name: :sgs_spaces_ids - end + # Test idempotency: running again should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end context 'down migration' do - it 'rolls back successfully' do + it 'rolls back successfully and handles idempotency' do expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) - end - it 'does not fail if indexes/constraints are already in desired state' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - db.alter_table :security_groups_spaces do - add_index %i[security_group_id space_id], name: :sgs_spaces_ids - drop_index %i[security_group_id space_id], unique: true, name: :security_groups_spaces_ids - end + # Test idempotency: running rollback again should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error end end diff --git a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb index 6b385e908e2..88928f99cb9 100644 --- a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb +++ b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb @@ -14,11 +14,11 @@ describe 'route_bindings table' do context 'up migration' do - it 'is in the correct state before migration' do + it 'removes duplicates, adds unique index, and handles idempotency' do + # Verify initial state expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) - end - it 'removes duplicates and migrates successfully by adding unique index' do + # Insert test data with duplicates db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) @@ -43,25 +43,20 @@ # Verify index is added expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) - end - it 'does not fail if indexes/constraints are already in desired state' do - db.alter_table(:route_bindings) { add_index %i[route_id service_instance_id], unique: true, name: :route_bindings_route_id_service_instance_id_index } + # Test idempotency: running again should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end context 'down migration' do - it 'rolls back successfully' do + it 'rolls back successfully and handles idempotency' do expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) expect(db.indexes(:route_bindings)).to include(:route_id) if db.database_type == :mysql - end - it 'does not fail if indexes/constraints are already in desired state' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - db.alter_table(:route_bindings) { drop_index %i[route_id service_instance_id], unique: true, name: :route_bindings_route_id_service_instance_id_index } + # Test idempotency: running rollback again should not fail expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error end end diff --git a/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb b/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb index eb0797a4559..0be03ed1fbd 100644 --- a/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb +++ b/spec/migrations/20251121174647_add_broker_provided_metadata_to_service_instances_spec.rb @@ -12,33 +12,41 @@ end describe 'service_instances table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - let(:space) { VCAP::CloudController::Space.make } - it 'adds a column `broker_provided_metadata`' do + it 'adds broker_provided_metadata column with correct properties' do + # Insert a service instance before migration to test preservation + db[:service_instances].insert( + guid: 'existing-service-instance-guid', + name: 'existing-instance', + space_id: space.id + ) + + # Verify column doesn't exist yet expect(db[:service_instances].columns).not_to include(:broker_provided_metadata) - run_migration + + # Run migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) + + # Verify column was added expect(db[:service_instances].columns).to include(:broker_provided_metadata) - end - it 'allows null values for broker_provided_metadata' do - run_migration - # Insert a service instance without broker_provided_metadata + # Verify existing instance was preserved with null metadata + existing_instance = db[:service_instances].first(guid: 'existing-service-instance-guid') + expect(existing_instance).not_to be_nil + expect(existing_instance[:broker_provided_metadata]).to be_nil + + # Verify null values are allowed db[:service_instances].insert( guid: 'test-service-instance-guid', name: 'test-instance', space_id: space.id, broker_provided_metadata: nil ) - # Verify the insert succeeded and the column is null - instance = db[:service_instances].first(guid: 'test-service-instance-guid') - expect(instance[:broker_provided_metadata]).to be_nil - end + instance_with_null = db[:service_instances].first(guid: 'test-service-instance-guid') + expect(instance_with_null[:broker_provided_metadata]).to be_nil - it 'accepts text values for broker_provided_metadata' do - run_migration - # Insert a service instance with broker_provided_metadata + # Verify text values are accepted metadata_json = '{"labels": {"version": "1.0"}, "attributes": {"engine": "postgresql"}}' db[:service_instances].insert( guid: 'test-service-instance-with-metadata', @@ -46,23 +54,8 @@ space_id: space.id, broker_provided_metadata: metadata_json ) - # Verify the metadata was stored correctly - instance = db[:service_instances].first(guid: 'test-service-instance-with-metadata') - expect(instance[:broker_provided_metadata]).to eq(metadata_json) - end - - it 'preserves existing service instances after migration' do - # Insert a service instance before migration - db[:service_instances].insert( - guid: 'existing-service-instance-guid', - name: 'existing-instance', - space_id: space.id - ) - run_migration - # Verify the existing instance still exists and has null metadata - instance = db[:service_instances].first(guid: 'existing-service-instance-guid') - expect(instance).not_to be_nil - expect(instance[:broker_provided_metadata]).to be_nil + instance_with_metadata = db[:service_instances].first(guid: 'test-service-instance-with-metadata') + expect(instance_with_metadata[:broker_provided_metadata]).to eq(metadata_json) end end end From f56e055fabd2b32f78d87ac3fc3d4f9f0678d4d0 Mon Sep 17 00:00:00 2001 From: johha Date: Wed, 21 Jan 2026 09:08:28 +0100 Subject: [PATCH 2/4] fix migration file name and test --- ...0_drop_unique_constraint_quota_definitions_name_key_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb index 6839e3da3b9..ffcc7567616 100644 --- a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb +++ b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb @@ -3,7 +3,7 @@ RSpec.describe 'migration to add or remove unique constraint on name column in quota_definitions table', isolation: :truncation, type: :migration do include_context 'migration' do - let(:migration_filename) { '20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb' } + let(:migration_filename) { '20241016118000_drop_unique_constraint_quota_definitions_name_key.rb' } end describe 'up migration' do context 'mysql' do @@ -17,7 +17,6 @@ expect(db.indexes(:quota_definitions)).not_to include(:name) # Test idempotency: if constraint already removed, doesn't error - db.drop_index :quota_definitions, :name, name: :name, if_exists: true expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:name) end From c9c236dc5ef88e57c3b3c5867dae2abc011050fc Mon Sep 17 00:00:00 2001 From: johha Date: Wed, 21 Jan 2026 12:01:50 +0100 Subject: [PATCH 3/4] Improve test resourecs names and adjust docs --- ...0_streamline_annotation_key_prefix_spec.rb | 48 ++++++++----------- spec/migrations/Readme.md | 11 +++-- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb index 7de9196d9c7..8732775be59 100644 --- a/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb +++ b/spec/migrations/20230822153000_streamline_annotation_key_prefix_spec.rb @@ -8,40 +8,34 @@ describe 'annotation tables' do it 'converts legacy key_prefixes to prefixes in key_prefix column and leaves non-legacy values unchanged' do - db[:isolation_segments].insert(name: 'bommel', guid: '123') - db[:isolation_segment_annotations].insert( - guid: 'bommel', - created_at: Time.now - 60, - updated_at: Time.now - 60, - resource_guid: '123', - key: 'mylegacyprefix/mykey', - value: 'some_value' - ) - db[:isolation_segment_annotations].insert(guid: 'bommel2', resource_guid: '123', key_prefix: 'myprefix', key: 'mykey', value: 'some_value') - db[:isolation_segment_annotations].insert(guid: 'bommel3', resource_guid: '123', key: 'mykey2', value: 'some_value2') + resource_guid = 'iso-seg-guid' + db[:isolation_segments].insert(name: 'iso_seg', guid: resource_guid) + db[:isolation_segment_annotations].insert(guid: 'anno-1-guid', resource_guid: resource_guid, key: 'mylegacyprefix/mykey', value: 'some_value') + db[:isolation_segment_annotations].insert(guid: 'anno-2-guid', resource_guid: resource_guid, key_prefix: 'myprefix', key: 'mykey', value: 'some_value') + db[:isolation_segment_annotations].insert(guid: 'anno-3-guid', resource_guid: resource_guid, key: 'yourkey', value: 'some_other_value') - a1 = db[:isolation_segment_annotations].first(key: 'mylegacyprefix/mykey') - b1 = db[:isolation_segment_annotations].first(key: 'mykey') - b2 = db[:isolation_segment_annotations].first(key: 'mykey2') + anno1 = db[:isolation_segment_annotations].first(guid: 'anno-1-guid') + anno2 = db[:isolation_segment_annotations].first(key: 'mykey') + anno3 = db[:isolation_segment_annotations].first(key: 'yourkey') expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error # Check legacy prefix was converted - a1_after = db[:isolation_segment_annotations].first(guid: 'bommel') - expect(a1_after[:guid]).to eq a1[:guid] - expect(a1_after[:created_at]).to eq a1[:created_at] - expect(a1_after[:updated_at]).not_to eq a1[:updated_at] - expect(a1_after[:resource_guid]).to eq a1[:resource_guid] - expect(a1_after[:key_prefix]).not_to eq a1[:key_prefix] - expect(a1_after[:key]).not_to eq a1[:key] - expect(a1_after[:key_prefix]).to eq 'mylegacyprefix' - expect(a1_after[:key]).to eq 'mykey' + anno1_after_mig = db[:isolation_segment_annotations].first(guid: 'anno-1-guid') + expect(anno1_after_mig[:guid]).to eq anno1[:guid] + expect(anno1_after_mig[:created_at]).to eq anno1[:created_at] + expect(anno1_after_mig[:updated_at]).not_to eq anno1[:updated_at] + expect(anno1_after_mig[:resource_guid]).to eq anno1[:resource_guid] + expect(anno1_after_mig[:key_prefix]).not_to eq anno1[:key_prefix] + expect(anno1_after_mig[:key]).not_to eq anno1[:key] + expect(anno1_after_mig[:key_prefix]).to eq 'mylegacyprefix' + expect(anno1_after_mig[:key]).to eq 'mykey' # Check non-legacy values unchanged - c1 = db[:isolation_segment_annotations].first(guid: 'bommel2') - c2 = db[:isolation_segment_annotations].first(guid: 'bommel3') - expect(b1.values).to eq(c1.values) - expect(b2.values).to eq(c2.values) + anno2_after_mig = db[:isolation_segment_annotations].first(guid: 'anno-2-guid') + anno3_after_mig = db[:isolation_segment_annotations].first(guid: 'anno-3-guid') + expect(anno2.values).to eq(anno2_after_mig.values) + expect(anno3.values).to eq(anno3_after_mig.values) end end end diff --git a/spec/migrations/Readme.md b/spec/migrations/Readme.md index eba2e86e178..22693090e8a 100644 --- a/spec/migrations/Readme.md +++ b/spec/migrations/Readme.md @@ -77,8 +77,10 @@ To create resilient and reliable migrations, follow these guidelines: Sequel migration tests have a distinct operation compared to conventional RSpec tests. Primarily, they execute the Down migration and restore the database state to its previous form before the test-specific migration was carried out. The process includes running a test, creating assets, executing a specific migration file for testing, and asserting certain behaviors. However, be aware that Sequel migration tests impose specific limitations and requirements on test writing. -1. The migration spec should not be influenced by any Cloud Controller code. This requirement is the same as for the migration itself. Any model changes can modify the old migrations and alter the behaviors and results of the tests. Therefore, don't select data via the CC models, instead make the selects in raw Sequel and assert the behavior you'd like to test. -1. It's recommended to use the `migration` shared context, as it ensures that the database schema first reverts to the version before the migration you aim to test. This shared context also provides a directory containing a single migration for running a particular migration within a test. When the test is done, this shared context makes sure to restore the correct schema by running the migrations that post-date the one being tested. Thus, avoiding cases of a half-migrated database that could result in random test failures. It also makes sure to test not every migration that comes after the test migration, but just a single migration is executed and then the expected behavior is evaluated. +### Limitations and Requirements +1. **Independence**: The migration spec should not be influenced by any Cloud Controller code. This requirement is the same as for the migration itself. Any model changes can modify the old migrations and alter the behaviors and results of the tests. Therefore, don't select data via the CC models, instead make the selects in raw Sequel and assert the behavior you'd like to test. +1. **Shared Context**: It's recommended to use the `migration` shared context, as it ensures that the database schema first reverts to the version before the migration you aim to test. This shared context also provides a directory containing a single migration for running a particular migration within a test. When the test is done, this shared context makes sure to restore the correct schema by running the migrations that post-date the one being tested. Thus, avoiding cases of a half-migrated database that could result in random test failures. It also makes sure to test not every migration that comes after the test migration, but just a single migration is executed and then the expected behavior is evaluated. +1. **Performance**: Running migrations (`Sequel::Migrator.run(...)`) can be time-consuming and take serval seconds with each call. Therefore, it's advisable to limit the number of migration calls within a single spec file as much as possible. Group related tests into a single it block even if it does not follow the usual RSpec best practices. This approach minimizes the number of migration calls and enhances test performance. ### Usage @@ -95,12 +97,15 @@ RSpec.describe 'migration to modify isolation_segments', isolation: :truncation describe 'isolation_segments table' do it 'retains the initial name and guid' do - db[:isolation_segments].insert(name: 'bommel', guid: '123') + db[:isolation_segments].insert(name: 'name', guid: '123') a1 = db[:isolation_segment_annotations].first(resource_guid: '123') 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] + + # Test idempotency of the migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error end end end From e3215e9780366de054d1b11e35e9b23c2513815a Mon Sep 17 00:00:00 2001 From: johha Date: Wed, 21 Jan 2026 15:36:09 +0100 Subject: [PATCH 4/4] optimize migration tests further --- ...00_add_annotation_label_uniqueness_spec.rb | 294 +++++++++--------- ...131908_add_user_guid_to_jobs_table_spec.rb | 82 +---- ...ask_guid_index_to_app_usage_events_spec.rb | 34 +- ...742_add_jobs_user_guid_state_index_spec.rb | 76 +---- ...straint_quota_definitions_name_key_spec.rb | 58 +--- ...ed_service_binding_feature_columns_spec.rb | 281 ++++------------- ..._add_user_id_index_to_roles_tables_spec.rb | 26 +- ...250610212414_add_user_to_processes_spec.rb | 24 +- .../20250630170610_add_user_to_tasks_spec.rb | 24 +- ...27_allow_multiple_service_bindings_spec.rb | 53 ++-- ...ecurity_groups_spaces_unique_index_spec.rb | 94 +++--- ...135214_route_bindings_unique_index_spec.rb | 89 +++--- .../bigint_migration_step1_shared_context.rb | 70 ++--- .../bigint_migration_step3_shared_context.rb | 129 +++----- 14 files changed, 467 insertions(+), 867 deletions(-) diff --git a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb index 99996e3df9e..6a9d27aad46 100644 --- a/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb +++ b/spec/migrations/20240102150000_add_annotation_label_uniqueness_spec.rb @@ -1,203 +1,195 @@ require 'spec_helper' require 'migrations/helpers/migration_shared_context' - RSpec.describe 'migration to add unique constraint to annotation and labels', isolation: :truncation, type: :migration do include_context 'migration' do let(:migration_filename) { '20240102150000_add_annotation_label_uniqueness.rb' } end - let(:isolation_segment) { VCAP::CloudController::IsolationSegmentModel } let(:annotation) { VCAP::CloudController::IsolationSegmentAnnotationModel } let(:label) { VCAP::CloudController::IsolationSegmentLabelModel } describe 'annotation tables' do - it 'truncates keys to 63 characters and leaves shorter keys unchanged' do + it 'handles key truncation, duplicate removal, and uniqueness constraints' do + # Setup data for truncation test i1 = isolation_segment.create(name: 'bommel') key_name_long = 'a' * 64 truncated_key_name = 'a' * 63 key_name_short = 'b' * 63 + trunc_a1 = annotation.create(resource_guid: i1.guid, key_name: key_name_long, value: 'some_value') + trunc_a2 = annotation.create(resource_guid: i1.guid, key_name: key_name_short, value: 'some_value2') - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name_long, value: 'some_value') - a2 = annotation.create(resource_guid: i1.guid, key_name: key_name_short, value: 'some_value2') - - 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) - expect(a2.reload.key_name).to eq(key_name_short) - end - - it 'removes duplicate annotations but keeps one with smallest id' do - i1 = isolation_segment.create(name: 'bommel') - key_name = 'a' * 63 + # Setup data for duplicate removal test + i2 = isolation_segment.create(name: 'duplicate_test') + key_c = 'c' * 63 # In case key_prefix is not set - a1 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'v1') - a2 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'v2') - a3 = annotation.create(resource_guid: i1.guid, key_name: key_name, value: 'v3') + dup_a1 = annotation.create(resource_guid: i2.guid, key_name: key_c, value: 'v1') + dup_a2 = annotation.create(resource_guid: i2.guid, key_name: key_c, value: 'v2') + dup_a3 = annotation.create(resource_guid: i2.guid, key_name: key_c, value: 'v3') + # In case key_prefix is set - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_name, value: 'v1') - b2 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_name, value: 'v2') - b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_name, value: 'v3') + dup_b1 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_c, value: 'v1') + dup_b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_c, value: 'v2') + dup_b3 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_c, value: 'v3') - expect(a1.id).to be < a2.id - expect(a1.id).to be < a3.id - expect(b1.id).to be < b2.id - expect(b1.id).to be < b3.id + expect(dup_a1.id).to be < dup_a2.id + expect(dup_a1.id).to be < dup_a3.id + expect(dup_b1.id).to be < dup_b2.id + expect(dup_b1.id).to be < dup_b3.id - 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) - expect { a3.reload }.to raise_error(Sequel::NoExistingObject) - expect(b1.reload).to be_a(annotation) - expect { b2.reload }.to raise_error(Sequel::NoExistingObject) - expect { b3.reload }.to raise_error(Sequel::NoExistingObject) - end - - it 'does not remove records if any column of key, key_prefix or resource_guid is different' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 - key_b = 'b' * 63 + # Setup data for preservation test (different columns) + i3 = isolation_segment.create(name: 'sword') + key_d = 'd' * 63 + key_e = 'e' * 63 # In case key_prefix is not set - a1 = annotation.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') - a2 = annotation.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = annotation.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - 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') + pres_a1 = annotation.create(resource_guid: i1.guid, key_name: key_d, value: 'v1') + pres_a2 = annotation.create(resource_guid: i3.guid, key_name: key_d, value: 'v2') + pres_a3 = annotation.create(resource_guid: i1.guid, key_name: key_e, value: 'v3') - 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 set + pres_b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_d, value: 'v1') + pres_b2 = annotation.create(resource_guid: i3.guid, key_prefix: 'prefix1', key_name: key_d, value: 'v2') + pres_b3 = annotation.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_e, value: 'v3') + pres_b4 = annotation.create(resource_guid: i1.guid, key_prefix: 'prefix2', key_name: key_d, value: 'v4') - expect(annotation.all.count).to eq(7) - expect(a1.reload).to be_a(annotation) - expect(a2.reload).to be_a(annotation) - expect(a3.reload).to be_a(annotation) - expect(b1.reload).to be_a(annotation) - expect(b2.reload).to be_a(annotation) - expect(b3.reload).to be_a(annotation) - expect(b4.reload).to be_a(annotation) - end - - it 'does not allow adding a duplicate but allows different annotations' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key = 'a' * 63 - key_b = 'b' * 63 + # Setup data for uniqueness constraint test + i4 = isolation_segment.create(name: 'unique_test') + i5 = isolation_segment.create(name: 'unique_test2') + key_f = 'f' * 63 + key_g = 'g' * 63 # In case key_prefix is not set - annotation.create(resource_guid: i1.guid, key_name: key, value: 'v1') + annotation.create(resource_guid: i4.guid, key_name: key_f, value: 'v1') + # In case key_prefix is set - annotation.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1') + annotation.create(resource_guid: i5.guid, key_prefix: 'unique_prefix', key_name: key_f, value: 'v1') + # Run migration once expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - # Test: does not allow adding a duplicate - 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) - - # Test: does allow adding different annotations - a1 = annotation.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - a2 = annotation.create(resource_guid: i2.guid, key_name: key_b, value: 'v2') - b1 = annotation.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key, value: 'v4') - b2 = annotation.create(resource_guid: i2.guid, key_prefix: 'sword', key_name: key, value: 'v5') - - expect(annotation.where(key_name: key_b).count).to eq(2) - expect(a1.reload).to be_a(annotation) - expect(a2.reload).to be_a(annotation) - expect(b1.reload).to be_a(annotation) - expect(b2.reload).to be_a(annotation) + # Verify truncation behavior + expect(trunc_a1.reload.key_name).to eq(truncated_key_name) + expect(trunc_a2.reload.key_name).to eq(key_name_short) + + # Verify duplicate removal (keeps smallest id) + expect(annotation.where(resource_guid: i2.guid, key_name: key_c).count).to eq(2) + expect(dup_a1.reload).to be_a(annotation) + expect { dup_a2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_a3.reload }.to raise_error(Sequel::NoExistingObject) + expect(dup_b1.reload).to be_a(annotation) + expect { dup_b2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_b3.reload }.to raise_error(Sequel::NoExistingObject) + + # Verify preservation of records with different columns + expect(pres_a1.reload).to be_a(annotation) + expect(pres_a2.reload).to be_a(annotation) + expect(pres_a3.reload).to be_a(annotation) + expect(pres_b1.reload).to be_a(annotation) + expect(pres_b2.reload).to be_a(annotation) + expect(pres_b3.reload).to be_a(annotation) + expect(pres_b4.reload).to be_a(annotation) + + # Verify uniqueness constraints: does not allow adding a duplicate + expect { annotation.create(resource_guid: i4.guid, key_name: key_f, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + expect { annotation.create(resource_guid: i5.guid, key_prefix: 'unique_prefix', key_name: key_f, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + + # Verify uniqueness constraints: does allow adding different annotations + uniq_a1 = annotation.create(resource_guid: i4.guid, key_name: key_g, value: 'v3') + uniq_a2 = annotation.create(resource_guid: i5.guid, key_name: key_g, value: 'v2') + uniq_b1 = annotation.create(resource_guid: i4.guid, key_prefix: 'other_prefix', key_name: key_f, value: 'v4') + uniq_b2 = annotation.create(resource_guid: i5.guid, key_prefix: 'other_prefix', key_name: key_f, value: 'v5') + expect(annotation.where(key_name: key_g).count).to eq(2) + expect(uniq_a1.reload).to be_a(annotation) + expect(uniq_a2.reload).to be_a(annotation) + expect(uniq_b1.reload).to be_a(annotation) + expect(uniq_b2.reload).to be_a(annotation) end end describe 'labels tables' do - it 'removes duplicate annotations but keeps one with smallest id' do - i1 = isolation_segment.create(name: 'bommel') - key = 'a' * 63 + it 'handles duplicate removal and uniqueness constraints' do + # Setup data for duplicate removal test + i1 = isolation_segment.create(name: 'label_dup_test') + key_a = 'a' * 63 # In case key_prefix is not set - a1 = label.create(resource_guid: i1.guid, key_name: key, value: 'v1') - a2 = label.create(resource_guid: i1.guid, key_name: key, value: 'v2') - a3 = label.create(resource_guid: i1.guid, key_name: key, value: 'v3') - # In case key_prefix is set - b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key, value: 'v1') - b2 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key, value: 'v2') - b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key, value: 'v3') + dup_a1 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') + dup_a2 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v2') + dup_a3 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v3') - expect(a1.id).to be < a2.id - expect(a1.id).to be < a3.id - expect(b1.id).to be < b2.id - expect(b1.id).to be < b3.id - - 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) - expect { a3.reload }.to raise_error(Sequel::NoExistingObject) - expect(b1.reload).to be_a(label) - expect { b2.reload }.to raise_error(Sequel::NoExistingObject) - expect { b3.reload }.to raise_error(Sequel::NoExistingObject) - end - - it 'does not remove records if any column of key_name, key_prefix or resource_guid is different' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key_a = 'a' * 63 + # In case key_prefix is set + dup_b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') + dup_b2 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') + dup_b3 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v3') + expect(dup_a1.id).to be < dup_a2.id + expect(dup_a1.id).to be < dup_a3.id + expect(dup_b1.id).to be < dup_b2.id + expect(dup_b1.id).to be < dup_b3.id + + # Setup data for preservation test (different columns) + i2 = isolation_segment.create(name: 'label_preserve_test') key_b = 'b' * 63 + key_c = 'c' * 63 # In case key_prefix is not set - a1 = label.create(resource_guid: i1.guid, key_name: key_a, value: 'v1') - a2 = label.create(resource_guid: i2.guid, key_name: key_a, value: 'v2') - a3 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - # In case key_prefix is set - b1 = label.create(resource_guid: i1.guid, key_prefix: 'bommel', key_name: key_a, value: 'v1') - b2 = label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key_a, value: 'v2') - 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, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + pres_a1 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v1') + pres_a2 = label.create(resource_guid: i2.guid, key_name: key_b, value: 'v2') + pres_a3 = label.create(resource_guid: i1.guid, key_name: key_c, value: 'v3') - expect(label.all.count).to eq(7) - expect(a1.reload).to be_a(label) - expect(a2.reload).to be_a(label) - expect(a3.reload).to be_a(label) - expect(b1.reload).to be_a(label) - expect(b2.reload).to be_a(label) - expect(b3.reload).to be_a(label) - expect(b4.reload).to be_a(label) - end + # In case key_prefix is set + pres_b1 = label.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_b, value: 'v1') + pres_b2 = label.create(resource_guid: i2.guid, key_prefix: 'prefix1', key_name: key_b, value: 'v2') + pres_b3 = label.create(resource_guid: i1.guid, key_prefix: 'prefix1', key_name: key_c, value: 'v3') + pres_b4 = label.create(resource_guid: i1.guid, key_prefix: 'prefix2', key_name: key_b, value: 'v4') - it 'does not allow adding a duplicate but allows different labels' do - i1 = isolation_segment.create(name: 'bommel') - i2 = isolation_segment.create(name: 'sword') - key = 'a' * 63 - key_b = 'b' * 63 + # Setup data for uniqueness constraint test + i3 = isolation_segment.create(name: 'label_unique_test') + i4 = isolation_segment.create(name: 'label_unique_test2') + key_d = 'd' * 63 + key_e = 'e' * 63 # In case key_prefix is not set - label.create(resource_guid: i1.guid, key_name: key, value: 'v1') + label.create(resource_guid: i3.guid, key_name: key_d, value: 'v1') + # In case key_prefix is set - label.create(resource_guid: i2.guid, key_prefix: 'bommel', key_name: key, value: 'v1') + label.create(resource_guid: i4.guid, key_prefix: 'unique_prefix', key_name: key_d, value: 'v1') + # Run migration once expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - # Test: does not allow adding a duplicate - 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) - - # Test: does allow adding different labels - a1 = label.create(resource_guid: i1.guid, key_name: key_b, value: 'v3') - a2 = label.create(resource_guid: i2.guid, key_name: key_b, value: 'v2') - b1 = label.create(resource_guid: i1.guid, key_prefix: 'sword', key_name: key, value: 'v4') - b2 = label.create(resource_guid: i2.guid, key_prefix: 'sword', key_name: key, value: 'v5') - - expect(label.where(key_name: key_b).count).to eq(2) - expect(a1.reload).to be_a(label) - expect(a2.reload).to be_a(label) - expect(b1.reload).to be_a(label) - expect(b2.reload).to be_a(label) + # Verify duplicate removal (keeps smallest id) + expect(label.where(resource_guid: i1.guid, key_name: key_a).count).to eq(2) + expect(dup_a1.reload).to be_a(label) + expect { dup_a2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_a3.reload }.to raise_error(Sequel::NoExistingObject) + expect(dup_b1.reload).to be_a(label) + expect { dup_b2.reload }.to raise_error(Sequel::NoExistingObject) + expect { dup_b3.reload }.to raise_error(Sequel::NoExistingObject) + + # Verify preservation of records with different columns + expect(pres_a1.reload).to be_a(label) + expect(pres_a2.reload).to be_a(label) + expect(pres_a3.reload).to be_a(label) + expect(pres_b1.reload).to be_a(label) + expect(pres_b2.reload).to be_a(label) + expect(pres_b3.reload).to be_a(label) + expect(pres_b4.reload).to be_a(label) + + # Verify uniqueness constraints: does not allow adding a duplicate + expect { label.create(resource_guid: i3.guid, key_name: key_d, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + expect { label.create(resource_guid: i4.guid, key_prefix: 'unique_prefix', key_name: key_d, value: 'v2') }.to raise_error(Sequel::UniqueConstraintViolation) + + # Verify uniqueness constraints: does allow adding different labels + uniq_a1 = label.create(resource_guid: i3.guid, key_name: key_e, value: 'v3') + uniq_a2 = label.create(resource_guid: i4.guid, key_name: key_e, value: 'v2') + uniq_b1 = label.create(resource_guid: i3.guid, key_prefix: 'other_prefix', key_name: key_d, value: 'v4') + uniq_b2 = label.create(resource_guid: i4.guid, key_prefix: 'other_prefix', key_name: key_d, value: 'v5') + expect(label.where(key_name: key_e).count).to eq(2) + expect(uniq_a1.reload).to be_a(label) + expect(uniq_a2.reload).to be_a(label) + expect(uniq_b1.reload).to be_a(label) + expect(uniq_b2.reload).to be_a(label) end end end diff --git a/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb b/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb index 8a57d64a66c..7fc29acbacd 100644 --- a/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb +++ b/spec/migrations/20240314131908_add_user_guid_to_jobs_table_spec.rb @@ -7,82 +7,30 @@ end describe 'jobs table' do - it 'adds a column `user_guid` and an index on it' do + it 'adds column and index, and handles idempotency gracefully' do + # Test basic up migration expect(db[:jobs].columns).not_to include(:user_guid) expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:jobs].columns).to include(:user_guid) expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - - describe 'idempotency of up' do - context '`user_guid` column already exists' do - before do - db.add_column :jobs, :user_guid, String, size: 255 - end - - it 'does not fail' do - expect(db[:jobs].columns).to include(:user_guid) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - - it 'continues to create the index' do - expect(db[:jobs].columns).to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - end - - context 'index already exists' do - before do - db.add_column :jobs, :user_guid, String, size: 255 - db.add_index :jobs, :user_guid, name: :jobs_user_guid_index - end - - it 'does not fail' do - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - end - - describe 'idempotency of down' do - context 'index does not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index - end - it 'does not fail' do - expect(db[:jobs].columns).to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end - - it 'continues to remove the `user_guid` column' do - expect(db[:jobs].columns).to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:jobs].columns).not_to include(:user_guid) - end - end + # Test up migration again (both column and index already exist - idempotency) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:jobs].columns).to include(:user_guid) + expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - context 'index and column do not exist' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index - db.drop_column :jobs, :user_guid - end + # Test down migration with pre-dropped index + db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:jobs].columns).not_to include(:user_guid) + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - it 'does not fail' do - expect(db[:jobs].columns).not_to include(:user_guid) - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test down migration when both already removed (idempotency) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:jobs].columns).not_to include(:user_guid) + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) end end end diff --git a/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb b/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb index b200cae6482..48b1513cd1f 100644 --- a/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb +++ b/spec/migrations/20240529195136_add_task_guid_index_to_app_usage_events_spec.rb @@ -7,29 +7,23 @@ end describe 'app_usage_events table' do - describe 'up migration' do - it 'adds an index on the task_guid column and handles idempotency' do - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) + it 'adds and removes index with idempotency' do + # Test up migration + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - # Test idempotency: running again when index exists should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - end - end + # Test up migration idempotency: running again when index exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - describe 'down migration' do - it 'removes the index and handles idempotency' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - expect(db.indexes(:app_usage_events)).to include(:app_usage_events_task_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) + # Test down migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - # Test idempotency: running rollback again when index doesn't exist should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) - end + # Test down migration idempotency: running rollback again when index doesn't exist should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:app_usage_events)).not_to include(:app_usage_events_task_guid_index) end end end diff --git a/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb b/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb index cb885c748a1..f8601a9ef64 100644 --- a/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb +++ b/spec/migrations/20240820070742_add_jobs_user_guid_state_index_spec.rb @@ -19,8 +19,10 @@ def partial_index_present end describe 'jobs table' do - it 'removes index `jobs_user_guid_index` and adds `jobs_user_guid_state_index`' do + it 'replaces indexes and handles idempotency gracefully' do skip if db.database_type != :postgres + + # Test basic up migration expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) expect(partial_index_present).to be_falsey @@ -28,69 +30,21 @@ def partial_index_present expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) expect(partial_index_present).to be_truthy - end - - describe 'idempotency of up' do - context '`jobs_user_guid_index` does not exist' do - before do - skip if db.database_type != :postgres - db.drop_index :jobs, :user_guid, name: :jobs_user_guid_index - end - - it 'continues to create the index' do - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect(partial_index_present).to be_falsey - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(partial_index_present).to be_truthy - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - end - end - - context '`jobs_user_guid_state_index` already exists' do - before do - skip if db.database_type != :postgres - db.add_index :jobs, %i[user_guid state], name: :jobs_user_guid_state_index, where: "state IN ('PROCESSING', 'POLLING')" - end - - it 'does not fail' do - expect(partial_index_present).to be_truthy - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - end - - describe 'idempotency of down' do - context '`jobs_user_guid_state_index` does not exist' do - before do - skip if db.database_type != :postgres - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.drop_index :jobs, %i[user_guid state], name: :jobs_user_guid_state_index - end - it 'restores `jobs_user_guid_index`' do - expect(partial_index_present).to be_falsey - expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(partial_index_present).to be_falsey - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - end + # Test up migration idempotency: running again when new index exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:jobs)).not_to include(:jobs_user_guid_index) + expect(partial_index_present).to be_truthy - context '`jobs_user_guid_index` already exists' do - before do - skip if db.database_type != :postgres - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - db.add_index :jobs, :user_guid, name: :jobs_user_guid_index - end + # Test down migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(partial_index_present).to be_falsey + expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - it 'does not fail' do - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - expect(partial_index_present).to be_truthy - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(partial_index_present).to be_falsey - expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) - end - end + # Test down migration idempotency: running again when old index exists should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(partial_index_present).to be_falsey + expect(db.indexes(:jobs)).to include(:jobs_user_guid_index) end end end diff --git a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb index ffcc7567616..62bb7ff1d02 100644 --- a/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb +++ b/spec/migrations/20241016118000_drop_unique_constraint_quota_definitions_name_key_spec.rb @@ -5,72 +5,48 @@ include_context 'migration' do let(:migration_filename) { '20241016118000_drop_unique_constraint_quota_definitions_name_key.rb' } end - describe 'up migration' do + describe 'quota_definitions table' do context 'mysql' do - before do + it 'removes and restores unique constraint with idempotency' do skip if db.database_type != :mysql - end - it 'removes the unique constraint and handles idempotency' do + # Test up migration - removes unique constraint expect(db.indexes(:quota_definitions)).to include(:name) expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:name) - # Test idempotency: if constraint already removed, doesn't error + # Test up migration idempotency - constraint already removed expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:name) - end - end - - context 'postgres' do - before do - skip if db.database_type != :postgres - end - - it 'removes the unique constraint and handles idempotency' do - expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - # Test idempotency: if constraint already removed, doesn't error - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) - end - end - end - - describe 'down migration' do - before do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - end - - context 'mysql' do - before do - skip if db.database_type != :mysql - end - - it 'adds the unique constraint and handles idempotency' do - expect(db.indexes(:quota_definitions)).not_to include(:name) + # Test down migration - restores unique constraint expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:name) - # Test idempotency: if constraint already exists, doesn't fail + # Test down migration idempotency - constraint already exists expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:name) end end context 'postgres' do - before do + it 'removes and restores unique constraint with idempotency' do skip if db.database_type != :postgres - end - it 'adds the unique constraint and handles idempotency' do + # Test up migration - removes unique constraint + expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) + + # Test up migration idempotency - constraint already removed + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:quota_definitions)).not_to include(:quota_definitions_name_key) + + # Test down migration - restores unique constraint expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) - # Test idempotency: if constraint already exists, doesn't fail + # Test down migration idempotency - constraint already exists expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db.indexes(:quota_definitions)).to include(:quota_definitions_name_key) end diff --git a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb index 3a897e77723..436a459aa72 100644 --- a/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb +++ b/spec/migrations/20250225132929_add_apps_file_based_service_binding_feature_columns_spec.rb @@ -7,235 +7,62 @@ end describe 'apps table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - describe 'up' do - describe 'column service_binding_k8s_enabled' do - it 'adds a column `service_binding_k8s_enabled`' do - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - run_migration - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - end - - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration - expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) - end - - it 'sets the default value of new entries to false' do - run_migration - db[:apps].insert(guid: 'new_app_guid') - expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) - end - - it 'forbids null values' do - run_migration - expect { db[:apps].insert(guid: 'app_guid__nil', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) - end - - context 'when it already exists' do - before do - db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true - end - - it 'does not fail' do - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect { run_migration }.not_to raise_error - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) - end - end - end - - describe 'column file_based_vcap_services_enabled' do - it 'adds a column `file_based_vcap_services_enabled`' do - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - run_migration - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - end - - it 'sets the default value of existing entries to false' do - db[:apps].insert(guid: 'existing_app_guid') - run_migration - expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_vcap_services_enabled]).to be(false) - end - - it 'sets the default value of new entries to false' do - run_migration - db[:apps].insert(guid: 'new_app_guid') - expect(db[:apps].first(guid: 'new_app_guid')[:file_based_vcap_services_enabled]).to be(false) - end - - it 'forbids null values' do - run_migration - expect { db[:apps].insert(guid: 'app_guid__nil', file_based_vcap_services_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) - end - - context 'when it already exists' do - before do - db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true - end - - it 'does not fail' do - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect { run_migration }.not_to raise_error - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) - end - end + it 'adds and removes columns with defaults, constraints, and handles idempotency' do + # Setup: Insert existing app to test default values + db[:apps].insert(guid: 'existing_app_guid') + + # Verify initial state + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) if check_constraint_supported?(db) + + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # Verify both columns were added + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + + # Verify default values for existing entries + expect(db[:apps].first(guid: 'existing_app_guid')[:service_binding_k8s_enabled]).to be(false) + expect(db[:apps].first(guid: 'existing_app_guid')[:file_based_vcap_services_enabled]).to be(false) + + # Verify default values for new entries + db[:apps].insert(guid: 'new_app_guid') + expect(db[:apps].first(guid: 'new_app_guid')[:service_binding_k8s_enabled]).to be(false) + expect(db[:apps].first(guid: 'new_app_guid')[:file_based_vcap_services_enabled]).to be(false) + + # Verify null constraints + expect { db[:apps].insert(guid: 'app_guid_nil_k8s', service_binding_k8s_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + expect { db[:apps].insert(guid: 'app_guid_nil_vcap', file_based_vcap_services_enabled: nil) }.to raise_error(Sequel::NotNullConstraintViolation) + + # Verify check constraint (if supported) + if check_constraint_supported?(db) + expect(check_constraint_exists?(db)).to be(true) + expect { db[:apps].insert(guid: 'app_both_true', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| + expect(error.inspect).to include('only_one_sb_feature_enabled') + end) end - describe 'check constraint' do - context 'when supported' do - before do - skip 'check constraint not supported by db' unless check_constraint_supported?(db) - end - - it 'adds the check constraint' do - expect(check_constraint_exists?(db)).to be(false) - run_migration - expect(check_constraint_exists?(db)).to be(true) - end - - it 'forbids setting both features to true' do - run_migration - expect { db[:apps].insert(guid: 'some_app', file_based_vcap_services_enabled: true, service_binding_k8s_enabled: true) }.to(raise_error do |error| - expect(error.inspect).to include('only_one_sb_feature_enabled') - end) - end - - context 'when it already exists' do - before do - db.add_column :apps, :service_binding_k8s_enabled, :boolean, default: false, null: false, if_not_exists: true - db.add_column :apps, :file_based_vcap_services_enabled, :boolean, default: false, null: false, if_not_exists: true - db.alter_table :apps do - add_constraint(name: :only_one_sb_feature_enabled) do - Sequel.lit('NOT (service_binding_k8s_enabled AND file_based_vcap_services_enabled)') - end - end - end - - it 'does not fail' do - expect { run_migration }.not_to raise_error - end - end - end - - context 'when not supported' do - before do - skip 'check constraint supported by db' if check_constraint_supported?(db) - end - - it 'does not fail' do - expect { run_migration }.not_to raise_error - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - end - end - end - end - - describe 'down' do - subject(:run_rollback) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) } - - before do - run_migration - end - - describe 'column service_binding_k8s_enabled' do - it 'removes column `service_binding_k8s_enabled`' do - expect(db[:apps].columns).to include(:service_binding_k8s_enabled) - run_rollback - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - end - - context 'when it does not exist' do - before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - drop_column :service_binding_k8s_enabled - end - end - - it 'does not fail' do - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) - end - end - end - - describe 'column file_based_vcap_services_enabled' do - it 'removes column `file_based_vcap_services_enabled`' do - expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) - run_rollback - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - end - - context 'when it does not exist' do - before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - drop_column :file_based_vcap_services_enabled - end - end - - it 'does not fail' do - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) - end - end - end - - describe 'check constraint' do - context 'when supported' do - before do - skip 'check constraint not supported by db' unless check_constraint_supported?(db) - end - - it 'removes the check constraint' do - expect(check_constraint_exists?(db)).to be(true) - run_rollback - expect(check_constraint_exists?(db)).to be(false) - end - - context 'when it does not exist' do - before do - db.alter_table :apps do - drop_constraint :only_one_sb_feature_enabled - end - end - - it 'does not fail' do - expect(check_constraint_exists?(db)).to be(false) - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - expect(check_constraint_exists?(db)).to be(false) - end - end - end - - context 'when not supported' do - before do - skip 'check constraint supported by db' if check_constraint_supported?(db) - end - - it 'does not fail' do - expect { run_rollback }.not_to raise_error - expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) - expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) - end - end - end + # Test up migration idempotency: running migration again should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:apps].columns).to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(true) if check_constraint_supported?(db) + + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + + # Verify columns were removed + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) + + # Test down migration idempotency: running rollback again should not fail + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:apps].columns).not_to include(:service_binding_k8s_enabled) + expect(db[:apps].columns).not_to include(:file_based_vcap_services_enabled) + expect(check_constraint_exists?(db)).to be(false) end end end diff --git a/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb b/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb index b9e8d335aec..87e7c9c1da5 100644 --- a/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb +++ b/spec/migrations/20250318112800_add_user_id_index_to_roles_tables_spec.rb @@ -23,8 +23,8 @@ skip unless db.database_type == :postgres end - describe 'up migration' do - it 'adds indexes for all tables and handles idempotency' do + describe 'roles tables' do + it 'adds and removes user_id indexes for all tables with idempotency' do # Verify initial state: no indexes exist tables.each do |table| table_sym = table.to_sym @@ -32,7 +32,7 @@ expect(db.indexes(table_sym)).not_to include(index_sym) end - # Run migration + # === UP MIGRATION === expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error # Verify all indexes were created @@ -42,24 +42,15 @@ expect(db.indexes(table_sym)).to include(index_sym) end - # Test idempotency: running migration again should not fail + # Test up migration idempotency expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - - describe 'down migration' do - it 'removes indexes from all tables and handles idempotency' do - # Run up migration first - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - - # Verify indexes exist tables.each do |table| table_sym = table.to_sym index_sym = :"#{table}_user_id_index" expect(db.indexes(table_sym)).to include(index_sym) end - # Run down migration + # === DOWN MIGRATION === expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error # Verify all indexes were removed @@ -69,8 +60,13 @@ expect(db.indexes(table_sym)).not_to include(index_sym) end - # Test idempotency: running rollback again should not fail + # Test down migration idempotency expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + tables.each do |table| + table_sym = table.to_sym + index_sym = :"#{table}_user_id_index" + expect(db.indexes(table_sym)).not_to include(index_sym) + end end end end diff --git a/spec/migrations/20250610212414_add_user_to_processes_spec.rb b/spec/migrations/20250610212414_add_user_to_processes_spec.rb index 3b43aaf621e..6b859a26139 100644 --- a/spec/migrations/20250610212414_add_user_to_processes_spec.rb +++ b/spec/migrations/20250610212414_add_user_to_processes_spec.rb @@ -7,25 +7,25 @@ end describe 'processes table' do - it 'adds a column `user` and handles idempotency' do + it 'adds and removes user column with idempotency' do + # Verify initial state expect(db[:processes].columns).not_to include(:user) + + # === UP MIGRATION === expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:processes].columns).to include(:user) - # Test idempotency: running again when column exists should not fail + # Test up migration idempotency expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end + expect(db[:processes].columns).to include(:user) - describe 'idempotency of down' do - it 'removes column and handles idempotency' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - expect(db[:processes].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:processes].columns).not_to include(:user) + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:processes].columns).not_to include(:user) - # Test idempotency: running rollback again when column doesn't exist should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:processes].columns).not_to include(:user) end end end diff --git a/spec/migrations/20250630170610_add_user_to_tasks_spec.rb b/spec/migrations/20250630170610_add_user_to_tasks_spec.rb index 2bbd52a6366..015487f6a7b 100644 --- a/spec/migrations/20250630170610_add_user_to_tasks_spec.rb +++ b/spec/migrations/20250630170610_add_user_to_tasks_spec.rb @@ -7,25 +7,25 @@ end describe 'tasks table' do - it 'adds a column `user` and handles idempotency' do + it 'adds and removes user column with idempotency' do + # Verify initial state expect(db[:tasks].columns).not_to include(:user) + + # === UP MIGRATION === expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db[:tasks].columns).to include(:user) - # Test idempotency: running again when column exists should not fail + # Test up migration idempotency expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end + expect(db[:tasks].columns).to include(:user) - describe 'idempotency of down' do - it 'removes column and handles idempotency' do - Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) - expect(db[:tasks].columns).to include(:user) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db[:tasks].columns).not_to include(:user) + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:tasks].columns).not_to include(:user) - # Test idempotency: running rollback again when column doesn't exist should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:tasks].columns).not_to include(:user) end end end diff --git a/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb b/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb index a9709316ff4..d1a75a4db87 100644 --- a/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb +++ b/spec/migrations/20251015071027_allow_multiple_service_bindings_spec.rb @@ -7,37 +7,36 @@ end describe 'service_bindings table' do - context 'up migration' do - it 'replaces unique constraints with indexes and handles idempotency' do - # Verify initial state - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) + it 'replaces unique constraints with indexes and handles idempotency' do + # Verify initial state + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name) - expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid) - expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) - expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_app_guid_name) + expect(db.indexes(:service_bindings)).not_to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) - # Test idempotency: running again should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).to include(:service_bindings_app_guid_name_index) - context 'down migration' do - it 'rolls back successfully and handles idempotency' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) - expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) - expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_service_instance_guid_index) + expect(db.indexes(:service_bindings)).not_to include(:service_bindings_app_guid_name_index) - # Test idempotency: running rollback again should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_service_instance_guid_app_guid) + expect(db.indexes(:service_bindings)).to include(:unique_service_binding_app_guid_name) end end end diff --git a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb index 5ba2ee65516..2a0d4a378f4 100644 --- a/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb +++ b/spec/migrations/20251016120006_security_groups_spaces_unique_index_spec.rb @@ -12,54 +12,52 @@ let(:sec_group_2) { VCAP::CloudController::SecurityGroup.make } describe 'security_groups_spaces table' do - context 'up migration' do - it 'removes duplicates, updates indexes, and handles idempotency' do - # Verify initial state - expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) - expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) - - # Insert test data with duplicates - db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_1.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) - db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) - - # Count duplicates before migration - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(2) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(3) - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - - # Verify duplicates are removed after migration - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) - expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(1) - - # Verify indexes are updated - expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids) - expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) - - # Test idempotency: running again should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - - context 'down migration' do - it 'rolls back successfully and handles idempotency' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) - expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) - - # Test idempotency: running rollback again should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + it 'removes duplicates, updates indexes, and handles idempotency' do + # Verify initial state + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) + + # Insert test data with duplicates + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_1.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_1.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + db[:security_groups_spaces].insert(security_group_id: sec_group_2.id, space_id: space_2.id) + + # Count duplicates before migration + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(2) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(3) + + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # Verify duplicates are removed after migration + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_1.id, space_id: space_2.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_1.id).count).to eq(1) + expect(db[:security_groups_spaces].where(security_group_id: sec_group_2.id, space_id: space_2.id).count).to eq(1) + + # Verify indexes are updated + expect(db.indexes(:security_groups_spaces)).not_to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) + + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:security_groups_spaces_ids) + + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) + + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:security_groups_spaces)).to include(:sgs_spaces_ids) + expect(db.indexes(:security_groups_spaces)).not_to include(:security_groups_spaces_ids) end end end diff --git a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb index 88928f99cb9..9cbc119a0f0 100644 --- a/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb +++ b/spec/migrations/20251028135214_route_bindings_unique_index_spec.rb @@ -13,52 +13,49 @@ let(:route_2) { VCAP::CloudController::Route.make(space:) } describe 'route_bindings table' do - context 'up migration' do - it 'removes duplicates, adds unique index, and handles idempotency' do - # Verify initial state - expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) - - # Insert test data with duplicates - db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) - - # Count duplicates before migration - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(2) - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(3) - - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - - # Verify duplicates are removed after migration - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) - expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(1) - - # Verify index is added - expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) - - # Test idempotency: running again should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - end - end - - context 'down migration' do - it 'rolls back successfully and handles idempotency' do - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) - expect(db.indexes(:route_bindings)).to include(:route_id) if db.database_type == :mysql - - # Test idempotency: running rollback again should not fail - expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error - end + it 'removes duplicates, manages unique index, and handles idempotency' do + # Verify initial state + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) + + # Insert test data with duplicates + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_1.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_1.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + db[:route_bindings].insert(route_id: route_2.id, service_instance_id: service_instance_2.id, guid: SecureRandom.uuid) + + # Count duplicates before migration + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(2) + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(3) + + # === UP MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + + # Verify duplicates are removed after migration + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_1.id, route_id: route_2.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_1.id).count).to eq(1) + expect(db[:route_bindings].where(service_instance_id: service_instance_2.id, route_id: route_2.id).count).to eq(1) + + # Verify index is added + expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) + + # Test up migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).to include(:route_bindings_route_id_service_instance_id_index) + + # === DOWN MIGRATION === + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) + expect(db.indexes(:route_bindings)).to include(:route_id) if db.database_type == :mysql + + # Test down migration idempotency + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db.indexes(:route_bindings)).not_to include(:route_bindings_route_id_service_instance_id_index) end end end diff --git a/spec/migrations/helpers/bigint_migration_step1_shared_context.rb b/spec/migrations/helpers/bigint_migration_step1_shared_context.rb index 6bae6daf9ed..e288c34f12d 100644 --- a/spec/migrations/helpers/bigint_migration_step1_shared_context.rb +++ b/spec/migrations/helpers/bigint_migration_step1_shared_context.rb @@ -2,8 +2,6 @@ require 'database/bigint_migration' RSpec.shared_context 'bigint migration step1' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - include_context 'migration' let(:skip_bigint_id_migration) { nil } @@ -29,25 +27,19 @@ db[table].delete end - it "changes the id column's type to bigint" do + it "changes the id column's type to bigint and does not add id_bigint column" do expect(db).to have_table_with_column_and_type(table, :id, 'integer') - - run_migration - - expect(db).to have_table_with_column_and_type(table, :id, 'bigint') - end - - it 'does not add the id_bigint column' do expect(db).not_to have_table_with_column(table, :id_bigint) - run_migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db).to have_table_with_column_and_type(table, :id, 'bigint') expect(db).not_to have_table_with_column(table, :id_bigint) end describe 'backfill' do before do - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it 'fails with a proper error message' do @@ -65,39 +57,26 @@ db[table].delete # Necessary to successfully run subsequent migrations in the after block of the migration shared context... end - it "does not change the id column's type" do + it 'keeps id as integer, adds id_bigint column and creates trigger function' do expect(db).to have_table_with_column_and_type(table, :id, 'integer') - - run_migration - - expect(db).to have_table_with_column_and_type(table, :id, 'integer') - end - - it 'adds the id_bigint column' do expect(db).not_to have_table_with_column(table, :id_bigint) + expect(db).not_to have_trigger_function_for_table(table) - run_migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).to have_table_with_column_and_type(table, :id_bigint, 'bigint') - end - - it 'creates the trigger function' do - expect(db).not_to have_trigger_function_for_table(table) - - run_migration expect(db).to have_trigger_function_for_table(table) end - it 'does not populate the id_bigint column for an existing entry' do - run_migration + it 'does not populate id_bigint for existing entries but does for new entries' do + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error + # Existing entry should not have id_bigint populated expect(db[table].where(id: old_id).get(:id_bigint)).to be_nil - end - - it 'automatically populates the id_bigint column for a new entry' do - run_migration + # New entry should have id_bigint automatically populated new_id = insert.call(db) expect(db[table].where(id: new_id).get(:id_bigint)).to eq(new_id) end @@ -105,8 +84,7 @@ describe 'backfill' do before do 100.times { insert.call(db) } - - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end context 'default batch size' do @@ -159,7 +137,7 @@ expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).not_to have_table_with_column(table, :id_bigint) - run_migration + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).not_to have_table_with_column(table, :id_bigint) @@ -168,18 +146,16 @@ end describe 'down' do - subject(:run_rollback) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) } - context 'when the table is empty' do before do db[table].delete - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it "reverts the id column's type to integer" do expect(db).to have_table_with_column_and_type(table, :id, 'bigint') - run_rollback + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_column_and_type(table, :id, 'integer') end @@ -188,26 +164,20 @@ context 'when the table is not empty' do before do insert.call(db) - run_migration + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end after do db[table].delete # Necessary to successfully run subsequent migrations in the after block of the migration shared context... end - it 'drops the id_bigint column' do + it 'drops the id_bigint column and trigger function' do expect(db).to have_table_with_column(table, :id_bigint) - - run_rollback - - expect(db).not_to have_table_with_column(table, :id_bigint) - end - - it 'drops the trigger function' do expect(db).to have_trigger_function_for_table(table) - run_rollback + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db).not_to have_table_with_column(table, :id_bigint) expect(db).not_to have_trigger_function_for_table(table) end end diff --git a/spec/migrations/helpers/bigint_migration_step3_shared_context.rb b/spec/migrations/helpers/bigint_migration_step3_shared_context.rb index 064e3a5292a..f17dd1d0860 100644 --- a/spec/migrations/helpers/bigint_migration_step3_shared_context.rb +++ b/spec/migrations/helpers/bigint_migration_step3_shared_context.rb @@ -2,10 +2,6 @@ require 'database/bigint_migration' RSpec.shared_context 'bigint migration step3a' do - subject(:run_migration_step1) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - subject(:run_migration_step3a) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) } - let(:migration_filename) { migration_filename_step1 } let(:current_migration_index_step3a) { migration_filename_step3a.match(/\A\d+/)[0].to_i } @@ -26,7 +22,7 @@ context 'when the id_bigint column was added' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end context 'when backfilling was completed' do @@ -37,7 +33,7 @@ it 'adds a check constraint' do expect(db).not_to have_table_with_check_constraint(table) - run_migration_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_check_constraint(table) end @@ -50,7 +46,7 @@ it 'fails ...' do expect do - run_migration_step3a + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) end.to raise_error(/Failed to add check constraint on '#{table}' table!/) end end @@ -59,13 +55,13 @@ context "when the migration was concluded (id column's type switched)" do before do db[table].delete - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it 'does not add a check constraint' do expect(db).not_to have_table_with_check_constraint(table) - run_migration_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) }.not_to raise_error expect(db).not_to have_table_with_check_constraint(table) end @@ -76,13 +72,13 @@ let(:skip_bigint_id_migration) { true } before do - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) end it 'does not add a check constraint' do expect(db).not_to have_table_with_check_constraint(table) - run_migration_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) }.not_to raise_error expect(db).not_to have_table_with_check_constraint(table) end @@ -90,20 +86,18 @@ end describe 'down' do - subject(:run_rollback_step3a) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a - 1, allow_missing_migration_files: true) } - context 'when migration step 3a was executed' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) VCAP::BigintMigration.backfill(logger, db, table) - run_migration_step3a + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) end it 'drops the check constraint' do expect(db).to have_table_with_check_constraint(table) - run_rollback_step3a + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a - 1, allow_missing_migration_files: true) }.not_to raise_error expect(db).not_to have_table_with_check_constraint(table) end @@ -112,12 +106,6 @@ end RSpec.shared_context 'bigint migration step3b' do - subject(:run_migration_step1) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - subject(:run_migration_step3a) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) } - - subject(:run_migration_step3b) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) } - let(:migration_filename) { migration_filename_step1 } let(:current_migration_index_step3a) { migration_filename_step3a.match(/\A\d+/)[0].to_i } let(:current_migration_index_step3b) { migration_filename_step3b.match(/\A\d+/)[0].to_i } @@ -138,41 +126,26 @@ context 'when migration step 3a was executed' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) VCAP::BigintMigration.backfill(logger, db, table) - run_migration_step3a + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) end - it 'drops the check constraint' do + it 'completes the bigint migration: drops constraints, renames columns, and maintains primary key' do + # Verify pre-migration state expect(db).to have_table_with_check_constraint(table) - - run_migration_step3b - - expect(db).not_to have_table_with_check_constraint(table) - end - - it 'drops the trigger function' do expect(db).to have_trigger_function_for_table(table) - - run_migration_step3b - - expect(db).not_to have_trigger_function_for_table(table) - end - - it 'drops the id column and renames the id_bigint column to id' do expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).to have_table_with_column_and_type(table, :id_bigint, 'bigint') + expect(db).to have_table_with_primary_key(table, :id) - run_migration_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) }.not_to raise_error + # Verify post-migration state + expect(db).not_to have_table_with_check_constraint(table) + expect(db).not_to have_trigger_function_for_table(table) expect(db).to have_table_with_column_and_type(table, :id, 'bigint') expect(db).not_to have_table_with_column(table, :id_bigint) - end - - it 'uses the (bigint) id column as primary key' do - expect(db).to have_table_with_primary_key(table, :id) - - run_migration_step3b expect(db).to have_table_with_primary_key(table, :id) end @@ -182,7 +155,7 @@ expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) - run_migration_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) }.not_to raise_error expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) end @@ -191,7 +164,7 @@ it 'uses an identity with correct start value for the (bigint) id column' do last_id_before_migration = insert.call(db) - run_migration_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) }.not_to raise_error first_id_after_migration = insert.call(db) expect(first_id_after_migration).to eq(last_id_before_migration + 1) @@ -200,71 +173,47 @@ end describe 'down' do - subject(:run_rollback_step3b) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b - 1, allow_missing_migration_files: true) } - context 'when migration step 3b was executed' do before do insert.call(db) - run_migration_step1 + Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) VCAP::BigintMigration.backfill(logger, db, table) - run_migration_step3a - run_migration_step3b + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3a, allow_missing_migration_files: true) + Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b, allow_missing_migration_files: true) end it 'uses an identity with correct start value for the (integer) id column' do last_id_before_migration = insert.call(db) - run_rollback_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b - 1, allow_missing_migration_files: true) }.not_to raise_error first_id_after_migration = insert.call(db) expect(first_id_after_migration).to eq(last_id_before_migration + 1) end - it 'has an index on timestamp + (integer) id column' do - if db.schema(table).any? { |col| col[0] == :timestamp } - - expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) - - run_rollback_step3b - - expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) - expect(db).not_to have_table_with_index_on_columns(table, %i[timestamp id_bigint]) - end - end - - it 'uses the (integer) id column as primary key' do - expect(db).to have_table_with_primary_key(table, :id) - - run_rollback_step3b - - expect(db).to have_table_with_primary_key(table, :id) - end - - it 'renames the id column to id_bigint and re-adds the (integer) id column' do + it 'reverts the bigint migration: restores columns, constraints, and indexes' do + # Verify pre-rollback state expect(db).to have_table_with_column_and_type(table, :id, 'bigint') expect(db).not_to have_table_with_column(table, :id_bigint) + expect(db).to have_table_with_primary_key(table, :id) + expect(db).not_to have_trigger_function_for_table(table) + expect(db).not_to have_table_with_check_constraint(table) - run_rollback_step3b + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index_step3b - 1, allow_missing_migration_files: true) }.not_to raise_error + # Verify post-rollback state expect(db).to have_table_with_column_and_type(table, :id, 'integer') expect(db).to have_table_with_column_and_type(table, :id_bigint, 'bigint') expect(db).to have_table_with_column_and_attribute(table, :id_bigint, :allow_null, true) - end - - it 're-creates the trigger function' do - expect(db).not_to have_trigger_function_for_table(table) - - run_rollback_step3b - + expect(db).to have_table_with_primary_key(table, :id) expect(db).to have_trigger_function_for_table(table) - end - - it 're-adds the check constraint (this also ensures that id was correctly backfilled)' do - expect(db).not_to have_table_with_check_constraint(table) - - run_rollback_step3b - expect(db).to have_table_with_check_constraint(table) + + # Verify timestamp index if applicable + if db.schema(table).any? { |col| col[0] == :timestamp } + expect(db).to have_table_with_index_on_columns(table, %i[timestamp id]) + expect(db).not_to have_table_with_index_on_columns(table, %i[timestamp id_bigint]) + end end end end