From 9051e3c2e44fed24ff955d27a4f5cdf56c114a3c Mon Sep 17 00:00:00 2001 From: Bala Kumar Date: Fri, 11 Jul 2025 22:01:33 +0530 Subject: [PATCH] Add cop to avoid stubbing of migration_has_finished? for elastic data migration service --- .rubocop.yml | 4 + .../avoid_stubbing_data_migration_service.rb | 95 ++++++++++++++ ...id_stubbing_data_migration_service_spec.rb | 123 ++++++++++++++++++ 3 files changed, 222 insertions(+) create mode 100644 rubocop/cop/search/avoid_stubbing_data_migration_service.rb create mode 100644 spec/rubocop/cop/search/avoid_stubbing_data_migration_service_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index a3b7c182fe1841..947d317954d56b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1286,6 +1286,10 @@ Search/AvoidCheckingFinishedOnDeprecatedMigrations: - 'ee/lib/gitlab/elastic/**/*.rb' - 'ee/spec/support/helpers/elasticsearch_helpers.rb' +Search/AvoidStubbingDataMigrationService: + Include: + - 'ee/spec/**/*.rb' + # See https://gitlab.com/gitlab-org/gitlab/-/issues/407233 Cop/ExperimentsTestCoverage: Enabled: true diff --git a/rubocop/cop/search/avoid_stubbing_data_migration_service.rb b/rubocop/cop/search/avoid_stubbing_data_migration_service.rb new file mode 100644 index 00000000000000..e1d9b710271100 --- /dev/null +++ b/rubocop/cop/search/avoid_stubbing_data_migration_service.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Search + # Cop to prevent stubbing `migration_has_finished?` on `Elastic::DataMigrationService` + # + # @example + # # bad + # allow(Elastic::DataMigrationService).to receive(:migration_has_finished?).with( + # :migration_name).and_return(true) + # + # # bad + # expect(Elastic::DataMigrationService).to receive(:migration_has_finished?).with( + # :migration_name).and_return(true) + # + # # good + # set_elastic_migration_to(:migration_name, including: true) + # + class AvoidStubbingDataMigrationService < RuboCop::Cop::Base + extend AutoCorrector + + MSG = 'Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. ' \ + 'Use the `set_elastic_migration_to` helper instead.' + + def on_send(node) + check_node(node) + end + + def on_csend(node) + check_node(node) + end + + private + + def check_node(node) + # Only check allow and expect calls + return unless %i[allow expect].include?(node.method_name) + return unless node.arguments.any? + + receiver = node.first_argument + + # Check if receiver is Elastic::DataMigrationService + return unless elastic_data_migration_service?(receiver) + + # Check if this is part of a stubbing chain that includes migration_has_finished? + # Look at the parent node (which should be the method chain) + return unless part_of_migration_has_finished_stub?(node) + + add_offense(node) do |corrector| + corrector.replace(receiver, 'self') + end + end + + def elastic_data_migration_service?(node) + return false unless node&.const_type? + + node.const_name == 'Elastic::DataMigrationService' + end + + def part_of_migration_has_finished_stub?(node) + # Check if this allow/expect call is part of a larger expression + # that includes .to receive(:migration_has_finished?) + + # Walk up the parent chain to find the root of the expression + current = node + current = current.parent while current.parent&.send_type? + + # Now search the entire expression tree for receive(:migration_has_finished?) + contains_migration_has_finished_receive?(current) + end + + def contains_migration_has_finished_receive?(node) + return false unless node + + # Check if this node itself is the receive call we're looking for + if node.send_type? && + node.method?(:receive) && + node.arguments.any? && + node.first_argument.sym_type? && + node.first_argument.value == :migration_has_finished? + return true + end + + # Check all children recursively, but avoid infinite recursion + node.children.any? do |child| + next false unless child.is_a?(Parser::AST::Node) + + contains_migration_has_finished_receive?(child) + end + end + end + end + end +end diff --git a/spec/rubocop/cop/search/avoid_stubbing_data_migration_service_spec.rb b/spec/rubocop/cop/search/avoid_stubbing_data_migration_service_spec.rb new file mode 100644 index 00000000000000..9ff6e89cb89da6 --- /dev/null +++ b/spec/rubocop/cop/search/avoid_stubbing_data_migration_service_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' +require_relative '../../../../rubocop/cop/search/avoid_stubbing_data_migration_service' + +RSpec.describe RuboCop::Cop::Search::AvoidStubbingDataMigrationService, feature_category: :global_search do + let(:cop) { described_class.new } + + context 'when stubbing migration_has_finished? on Elastic::DataMigrationService' do + it 'registers an offense for allow with method chaining' do + expect_offense(<<~RUBY) + allow(Elastic::DataMigrationService).to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use the `set_elastic_migration_to` helper instead. + .with(:some_migration).and_return(true) + RUBY + + expect_correction(<<~RUBY) + allow(self).to receive(:migration_has_finished?) + .with(:some_migration).and_return(true) + RUBY + end + + it 'registers an offense for expect with method chaining' do + expect_offense(<<~RUBY) + expect(Elastic::DataMigrationService).to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use the `set_elastic_migration_to` helper instead. + .with(:some_migration).and_return(true) + RUBY + + expect_correction(<<~RUBY) + expect(self).to receive(:migration_has_finished?) + .with(:some_migration).and_return(true) + RUBY + end + + it 'registers an offense for allow without method chaining' do + expect_offense(<<~RUBY) + allow(Elastic::DataMigrationService).to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use the `set_elastic_migration_to` helper instead. + RUBY + + expect_correction(<<~RUBY) + allow(self).to receive(:migration_has_finished?) + RUBY + end + + it 'registers an offense for expect without method chaining' do + expect_offense(<<~RUBY) + expect(Elastic::DataMigrationService).to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use the `set_elastic_migration_to` helper instead. + RUBY + + expect_correction(<<~RUBY) + expect(self).to receive(:migration_has_finished?) + RUBY + end + + it 'registers an offense for fully qualified constant' do + expect_offense(<<~RUBY) + allow(::Elastic::DataMigrationService).to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use the `set_elastic_migration_to` helper instead. + RUBY + + expect_correction(<<~RUBY) + allow(self).to receive(:migration_has_finished?) + RUBY + end + + it 'registers an offense with safe navigation operator' do + expect_offense(<<~RUBY) + allow(Elastic::DataMigrationService)&.to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use the `set_elastic_migration_to` helper instead. + RUBY + + expect_correction(<<~RUBY) + allow(self)&.to receive(:migration_has_finished?) + RUBY + end + end + + context 'when not stubbing migration_has_finished? on Elastic::DataMigrationService' do + it 'does not register an offense when stubbing on self' do + expect_no_offenses(<<~RUBY) + allow(self).to receive(:migration_has_finished?) + .with(:some_migration).and_return(true) + RUBY + end + + it 'does not register an offense when stubbing other methods on Elastic::DataMigrationService' do + expect_no_offenses(<<~RUBY) + allow(Elastic::DataMigrationService).to receive(:other_method) + .with(:some_migration).and_return(true) + RUBY + end + + it 'does not register an offense when stubbing migration_has_finished? on other classes' do + expect_no_offenses(<<~RUBY) + allow(SomeOtherClass).to receive(:migration_has_finished?) + .with(:some_migration).and_return(true) + RUBY + end + + it 'does not register an offense for regular method calls' do + expect_no_offenses(<<~RUBY) + Elastic::DataMigrationService.migration_has_finished?(:some_migration) + RUBY + end + end + + context 'when in RSpec context' do + it 'registers offense in describe block' do + expect_offense(<<~RUBY) + RSpec.describe 'Test' do + it 'bad example' do + allow(Elastic::DataMigrationService).to receive(:migration_has_finished?) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid stubbing `migration_has_finished?` on `Elastic::DataMigrationService`. Use `allow(self).to receive(:migration_has_finished?)` instead. + .with(:some_migration).and_return(true) + end + end + RUBY + end + end +end -- GitLab