diff --git a/.rubocop_cc.yml b/.rubocop_cc.yml index 9f6497665e4..7e9f7b8ad1d 100644 --- a/.rubocop_cc.yml +++ b/.rubocop_cc.yml @@ -12,6 +12,7 @@ require: - ./spec/linters/migration/add_constraint_name.rb - ./spec/linters/migration/include_string_size.rb - ./spec/linters/migration/require_primary_key.rb + - ./spec/linters/migration/too_many_migration_runs.rb - ./spec/linters/match_requires_with_includes.rb - ./spec/linters/prefer_oj_over_other_json_libraries.rb @@ -114,7 +115,9 @@ Style/HashSyntax: EnforcedShorthandSyntax: consistent Style/RaiseArgs: EnforcedStyle: compact - +Migration/TooManyMigrationRuns: + Exclude: + - spec/linters/migration/too_many_migration_runs.rb #### ENABLED SECTION Capybara/ClickLinkOrButtonStyle: diff --git a/spec/linters/migration/too_many_migration_runs.rb b/spec/linters/migration/too_many_migration_runs.rb new file mode 100644 index 00000000000..a97eadfd00d --- /dev/null +++ b/spec/linters/migration/too_many_migration_runs.rb @@ -0,0 +1,135 @@ +module RuboCop + module Cop + module Migration + class TooManyMigrationRuns < Base + MSG = 'Too many migration runs (%d). Combine tests to reduce migrations. See spec/migrations/README.md for further guidance.'.freeze + MAX_CALLS = 4 + + def on_new_investigation + calls = 0 + migrator_subject_names = [] + migrator_method_names = [] + migrator_let_names = [] + migrator_before_after_blocks = Set.new + + extract_migrator_definitions(migrator_subject_names, migrator_method_names, + migrator_let_names, migrator_before_after_blocks) + + count_migrator_calls(calls, migrator_subject_names, migrator_method_names, + migrator_let_names, migrator_before_after_blocks) + end + + def extract_migrator_definitions(subject_names, method_names, let_names, before_after_blocks) + processed_source.ast.each_descendant(:def) do |node| + method_name = extract_migrator_method_name(node) + method_names << method_name if method_name + end + + processed_source.ast.each_descendant(:block) do |node| + subject_name = extract_migrator_subject_name(node) + subject_names << subject_name if subject_name + + let_name = extract_migrator_let_name(node) + let_names << let_name if let_name + + before_after_blocks.add(node.object_id) if is_before_after_around_with_migrator?(node) + end + end + + def count_migrator_calls(_calls, subjects, methods, lets, before_after_blocks) + call_count = count_before_after_migrations(before_after_blocks) + call_count += count_send_node_migrations(subjects, methods, lets, before_after_blocks) + + add_offense(processed_source.ast, message: sprintf(MSG, call_count)) if call_count > MAX_CALLS + end + + def count_before_after_migrations(before_after_blocks) + call_count = 0 + processed_source.ast.each_descendant(:block) do |node| + call_count += count_direct_migrations_in_node(node) if before_after_blocks.include?(node.object_id) + end + call_count + end + + def count_send_node_migrations(subjects, methods, lets, before_after_blocks) + call_count = 0 + processed_source.ast.each_descendant(:send) do |node| + next if node.each_ancestor(:block).any? { |a| before_after_blocks.include?(a.object_id) } + + call_count += count_migration_call(node, subjects, methods, lets) + end + call_count + end + + def count_migration_call(node, subjects, methods, lets) + return 1 if direct_migrator_call?(node) + return 1 if helper_migration_call?(node, subjects, methods, lets) + + 0 + end + + def direct_migrator_call?(node) + return false unless node.method_name == :run && node.receiver&.source&.include?('Migrator') + + !inside_definition?(node) + end + + def helper_migration_call?(node, subjects, methods, lets) + subjects.include?(node.method_name) || + lets.include?(node.method_name) || + methods.include?(node.method_name) + end + + private + + def extract_migrator_method_name(node) + return nil unless node.type == :def + return nil unless node.source.include?('Sequel::Migrator.run') + + node.method_name + end + + def extract_migrator_subject_name(node) + return nil unless node.send_node.method_name == :subject + return nil unless node.source.include?('Sequel::Migrator.run') + + first_arg = node.send_node.first_argument + first_arg&.sym_type? ? first_arg.value : nil + end + + def extract_migrator_let_name(node) + return nil unless %i[let let!].include?(node.send_node.method_name) + return nil unless node.source.include?('Sequel::Migrator.run') + + first_arg = node.send_node.first_argument + first_arg&.sym_type? ? first_arg.value : nil + end + + def is_before_after_around_with_migrator?(node) + return false unless node.send_node + return false unless %i[before after around].include?(node.send_node.method_name) + + node.source.include?('Sequel::Migrator.run') + end + + def count_direct_migrations_in_node(node) + count = 0 + node.each_descendant(:send) do |descendant| + count += 1 if descendant.method_name == :run && descendant.receiver&.source&.include?('Migrator') + end + count + end + + def inside_definition?(node) + node.each_ancestor(:def).any? { |a| a.source.include?('Sequel::Migrator.run') } || + node.each_ancestor(:block).any? do |a| + %i[subject let let!].include?(a.send_node&.method_name) && a.source.include?('Sequel::Migrator.run') + end || + node.each_ancestor(:block).any? do |a| + %i[before after around].include?(a.send_node&.method_name) + end + end + end + 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 62bb7ff1d02..c3af1f6c70b 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 @@ -1,3 +1,4 @@ +# rubocop:disable Migration/TooManyMigrationRuns require 'spec_helper' require 'migrations/helpers/migration_shared_context' @@ -53,3 +54,4 @@ end end end +# rubocop:enable Migration/TooManyMigrationRuns diff --git a/spec/migrations/20251117123719_add_state_to_stacks_spec.rb b/spec/migrations/20251117123719_add_state_to_stacks_spec.rb index ce3a56afcae..504ad77ba9c 100644 --- a/spec/migrations/20251117123719_add_state_to_stacks_spec.rb +++ b/spec/migrations/20251117123719_add_state_to_stacks_spec.rb @@ -2,90 +2,45 @@ require 'migrations/helpers/migration_shared_context' RSpec.describe 'migration to add state column to stacks table', isolation: :truncation, type: :migration do + subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } + include_context 'migration' do let(:migration_filename) { '20251117123719_add_state_to_stacks.rb' } end - describe 'stacks table' do - subject(:run_migration) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) } - - describe 'up' do - it 'adds a column `state`' do - expect(db[:stacks].columns).not_to include(:state) - run_migration - expect(db[:stacks].columns).to include(:state) - end - - it 'sets the default value of existing stacks to ACTIVE' do - db[:stacks].insert(guid: SecureRandom.uuid, name: 'existing-stack', description: 'An existing stack') - run_migration - expect(db[:stacks].first(name: 'existing-stack')[:state]).to eq('ACTIVE') - end + it 'adds state column with defaults/constraints (up) and removes it (down), idempotently' do + # Setup: insert existing stack before migration + db[:stacks].insert(guid: SecureRandom.uuid, name: 'existing-stack', description: 'An existing stack') + expect(db[:stacks].columns).not_to include(:state) - it 'sets the default value of new stacks to ACTIVE' do - run_migration - db[:stacks].insert(guid: SecureRandom.uuid, name: 'new-stack', description: 'A new stack') - expect(db[:stacks].first(name: 'new-stack')[:state]).to eq('ACTIVE') - end + # Run migration UP + run_migration - it 'forbids null values' do - run_migration - expect do - db[:stacks].insert(guid: SecureRandom.uuid, name: 'null-state-stack', description: 'A stack with null state', state: nil) - end.to raise_error(Sequel::NotNullConstraintViolation) - end + # Verify column was added with correct behavior + expect(db[:stacks].columns).to include(:state) + expect(db[:stacks].first(name: 'existing-stack')[:state]).to eq('ACTIVE') - it 'allows valid state values' do - run_migration - %w[ACTIVE DEPRECATED RESTRICTED DISABLED].each do |state| - expect do - db[:stacks].insert(guid: SecureRandom.uuid, name: "stack-#{state.downcase}", description: "A #{state} stack", state: state) - end.not_to raise_error - expect(db[:stacks].first(name: "stack-#{state.downcase}")[:state]).to eq(state) - end - end + db[:stacks].insert(guid: SecureRandom.uuid, name: 'new-stack', description: 'A new stack') + expect(db[:stacks].first(name: 'new-stack')[:state]).to eq('ACTIVE') - context 'when the column already exists' do - before do - db.alter_table :stacks do - add_column :state, String, null: false, default: 'ACTIVE', size: 255 unless @db.schema(:stacks).map(&:first).include?(:state) - end - end + expect do + db[:stacks].insert(guid: SecureRandom.uuid, name: 'null-state-stack', description: 'A stack with null state', state: nil) + end.to raise_error(Sequel::NotNullConstraintViolation) - it 'does not fail' do - expect(db[:stacks].columns).to include(:state) - expect { run_migration }.not_to raise_error - expect(db[:stacks].columns).to include(:state) - end - end + %w[DEPRECATED RESTRICTED DISABLED].each do |state| + db[:stacks].insert(guid: SecureRandom.uuid, name: "stack-#{state.downcase}", description: "A #{state} stack", state: state) + expect(db[:stacks].first(name: "stack-#{state.downcase}")[:state]).to eq(state) end - describe 'down' do - subject(:run_rollback) { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) } + # Verify UP is idempotent + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index, allow_missing_migration_files: true) }.not_to raise_error - before do - run_migration - end + # Run migration DOWN + Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) + expect(db[:stacks].columns).not_to include(:state) - it 'removes the `state` column' do - expect(db[:stacks].columns).to include(:state) - run_rollback - expect(db[:stacks].columns).not_to include(:state) - end - - context 'when the column does not exist' do - before do - db.alter_table :stacks do - drop_column :state if @db.schema(:stacks).map(&:first).include?(:state) - end - end - - it 'does not fail' do - expect(db[:stacks].columns).not_to include(:state) - expect { run_rollback }.not_to raise_error - expect(db[:stacks].columns).not_to include(:state) - end - end - end + # Verify DOWN is idempotent + expect { Sequel::Migrator.run(db, migrations_path, target: current_migration_index - 1, allow_missing_migration_files: true) }.not_to raise_error + expect(db[:stacks].columns).not_to include(:state) 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 e288c34f12d..cc85de3c8f7 100644 --- a/spec/migrations/helpers/bigint_migration_step1_shared_context.rb +++ b/spec/migrations/helpers/bigint_migration_step1_shared_context.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/TooManyMigrationRuns require 'migrations/helpers/migration_shared_context' require 'database/bigint_migration' @@ -183,3 +184,4 @@ end end end +# rubocop:enable Migration/TooManyMigrationRuns diff --git a/spec/migrations/helpers/bigint_migration_step3_shared_context.rb b/spec/migrations/helpers/bigint_migration_step3_shared_context.rb index f17dd1d0860..3efb43f5c74 100644 --- a/spec/migrations/helpers/bigint_migration_step3_shared_context.rb +++ b/spec/migrations/helpers/bigint_migration_step3_shared_context.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/TooManyMigrationRuns require 'migrations/helpers/migration_shared_context' require 'database/bigint_migration' @@ -218,3 +219,4 @@ end end end +# rubocop:enable Migration/TooManyMigrationRuns