Add TooManyMigrationRuns cop to test performance (#4850)

Author: johha

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

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

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