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..3b4c393aeb6 --- /dev/null +++ b/spec/linters/migration/too_many_migration_runs.rb @@ -0,0 +1,147 @@ +module RuboCop + module Cop + module Migration + class TooManyMigrationRuns < Base + def on_new_investigation + return unless processed_source.ast + + definitions = extract_migrator_definitions + call_count = count_migrator_calls(definitions) + + return unless call_count > 4 + + add_offense(processed_source.ast, + message: "Too many migration runs (#{call_count}). Combine tests to reduce migrations. See spec/migrations/README.md for further guidance.") + end + + private + + def extract_migrator_definitions + definitions = { + subject_names: [], + method_names: [], + let_names: [], + before_after_blocks: Set.new + } + + # Single pass through the AST to collect all definitions + processed_source.ast.each_descendant(:def, :block) do |node| + case node.type + when :def + extract_migrator_method(node, definitions[:method_names]) + when :block + extract_block_definitions(node, definitions) + end + end + + definitions + end + + def extract_migrator_method(node, method_names) + return unless contains_migrator_run?(node) + + method_names << node.method_name + end + + def extract_block_definitions(node, definitions) + return unless node.send_node + + method_name = node.send_node.method_name + + case method_name + when :subject + extract_named_migrator(node, definitions[:subject_names]) + when :let, :let! + extract_named_migrator(node, definitions[:let_names]) + when :before, :after, :around + definitions[:before_after_blocks].add(node.object_id) if contains_migrator_run?(node) + end + end + + def extract_named_migrator(node, names) + return unless contains_migrator_run?(node) + + first_arg = node.send_node.first_argument + names << first_arg.value if first_arg&.sym_type? + end + + def count_migrator_calls(definitions) + call_count = 0 + + # Single pass through send nodes to count all migration calls + processed_source.ast.each_descendant(:send) do |node| + next unless migration_call?(node, definitions) + + call_count += 1 + end + + call_count + end + + def migration_call?(node, definitions) + in_before_after_block = node.each_ancestor(:block).any? do |ancestor| + definitions[:before_after_blocks].include?(ancestor.object_id) + end + + if in_before_after_block + # Count direct Migrator.run calls inside before/after/around blocks + migrator_run_call?(node) + else + # Count direct calls (not in definitions) or helper invocations + direct_migrator_call?(node) || helper_migration_call?(node, definitions) + end + end + + def direct_migrator_call?(node) + return false unless migrator_run_call?(node) + + !inside_definition?(node) + end + + def migrator_run_call?(node) + return false unless node.method_name == :run + + receiver = node.receiver + return false unless receiver + + # Check for Sequel::Migrator.run or just Migrator.run + if receiver.const_type? + receiver_name = receiver.const_name + return ['Migrator', 'Sequel::Migrator'].include?(receiver_name) + end + + false + end + + def helper_migration_call?(node, definitions) + method = node.method_name + definitions[:subject_names].include?(method) || + definitions[:let_names].include?(method) || + definitions[:method_names].include?(method) + end + + def contains_migrator_run?(node) + node.each_descendant(:send).any? { |send_node| migrator_run_call?(send_node) } + end + + def inside_definition?(node) + node.each_ancestor(:def, :block).any? do |ancestor| + case ancestor.type + when :def + contains_migrator_run?(ancestor) + when :block + next false unless ancestor.send_node + + method = ancestor.send_node.method_name + if %i[subject let let!].include?(method) + contains_migrator_run?(ancestor) + else + %i[before after around].include?(method) + end + end + end + end + end + end + end +end diff --git a/spec/linters/migration/too_many_migration_runs_spec.rb b/spec/linters/migration/too_many_migration_runs_spec.rb new file mode 100644 index 00000000000..662383ae41d --- /dev/null +++ b/spec/linters/migration/too_many_migration_runs_spec.rb @@ -0,0 +1,114 @@ +require 'rubocop' +require 'rubocop/rspec/cop_helper' +require 'rubocop/config' +require 'linters/migration/too_many_migration_runs' + +RSpec.describe RuboCop::Cop::Migration::TooManyMigrationRuns do + include CopHelper + + subject(:cop) { described_class.new(RuboCop::Config.new({})) } + + def migration_call(target) + "Sequel::Migrator.run(db, path, target: #{target})" + end + + def it_blocks(count) + (1..count).map { |n| "it('test #{n}') { #{migration_call(n)} }" }.join("\n") + end + + it 'does not register an offense for 4 or fewer direct migration calls' do + result = inspect_source("RSpec.describe('m') do\n#{it_blocks(4)}\nend") + expect(result).to be_empty + end + + it 'registers an offense for more than 4 direct migration calls' do + result = inspect_source("RSpec.describe('m') do\n#{it_blocks(5)}\nend") + expect(result.size).to eq(1) + expect(result.first.message).to include('(5)') + end + + it 'counts multiple migrations in a single it block' do + source = <<~RUBY + RSpec.describe('m') do + it('test') do + #{(1..5).map { |n| migration_call(n) }.join("\n")} + end + end + RUBY + result = inspect_source(source) + expect(result.size).to eq(1) + expect(result.first.message).to include('(5)') + end + + it 'counts migrations via subject, let, let!, and helper methods' do + source = <<~RUBY + RSpec.describe('m') do + subject(:migrate_subj) { #{migration_call(1)} } + let(:migrate_let) { #{migration_call(2)} } + let!(:migrate_let_bang) { #{migration_call(3)} } + def migrate_method; #{migration_call(4)}; end + + it('t1') { migrate_subj } + it('t2') { migrate_let } + it('t3') { migrate_let_bang } + it('t4') { migrate_method } + it('t5') { migrate_subj } + end + RUBY + result = inspect_source(source) + expect(result.size).to eq(1) + expect(result.first.message).to include('(5)') + end + + it 'does not double-count definitions - only invocations' do + source = <<~RUBY + RSpec.describe('m') do + subject(:migrate) { #{migration_call(1)} } + it('t1') { migrate } + it('t2') { migrate } + it('t3') { migrate } + it('t4') { migrate } + end + RUBY + result = inspect_source(source) + expect(result).to be_empty + end + + it 'counts migrations in before/after blocks' do + source = <<~RUBY + RSpec.describe('m') do + before { #{migration_call(1)}; #{migration_call(2)} } + after { #{migration_call(3)} } + it('t1') { #{migration_call(4)} } + it('t2') { #{migration_call(5)} } + end + RUBY + result = inspect_source(source) + expect(result.size).to eq(1) + expect(result.first.message).to include('(5)') + end + + it 'does not count non-migration let invocations' do + source = <<~RUBY + RSpec.describe('m') do + let(:value) { 'not a migration' } + #{(1..4).map { |n| "it('t#{n}') { value; #{migration_call(n)} }" }.join("\n")} + end + RUBY + result = inspect_source(source) + expect(result).to be_empty + end + + it 'handles empty files and files without migrations' do + expect(inspect_source('')).to be_empty + expect(inspect_source("RSpec.describe('x') { it('y') { expect(1).to eq(1) } }")).to be_empty + end + + it 'detects ::Sequel::Migrator.run and bare Migrator.run' do + %w[::Sequel::Migrator Migrator].each do |const| + source = "RSpec.describe('m') do\n#{(1..5).map { |n| "it('t#{n}') { #{const}.run(db, path, target: #{n}) }" }.join("\n")}\nend" + result = inspect_source(source) + expect(result.size).to eq(1), "Expected offense for #{const}.run" + 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