Add TooManyMigrationRuns cop to test performance

.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:

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

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

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

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

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