From 6f9ec100abb5e5e113fa6070f6eab89ec728fd81 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Fri, 25 Oct 2019 18:05:49 +0100 Subject: [PATCH 1/3] Rubocop Cop to auto-correct all *_any_instance_of spec helpers This cop auto-correct automatically all the references in RSpec as: - expect_any_instance_of -> expect_next_instance_of - allow_any_instance_of -> allow_next_instance_of At the moment it's disabled because I'm going to correct all the offenses in different merge requests. --- .rubocop.yml | 3 + rubocop/cop/rspec/any_instance_of.rb | 80 +++++++++++++++++++ rubocop/rubocop.rb | 1 + .../abuse_reports_controller_spec.rb | 4 +- .../authorizations_controller_spec.rb | 5 +- spec/controllers/metrics_controller_spec.rb | 4 +- spec/controllers/snippets_controller_spec.rb | 12 ++- spec/dependencies/omniauth_saml_spec.rb | 4 +- spec/helpers/application_helper_spec.rb | 4 +- .../rubocop/cop/rspec/any_instance_of_spec.rb | 59 ++++++++++++++ 10 files changed, 167 insertions(+), 9 deletions(-) create mode 100644 rubocop/cop/rspec/any_instance_of.rb create mode 100644 spec/rubocop/cop/rspec/any_instance_of_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index a3f1f57f1401c4..1d5cf7642c2c03 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -297,3 +297,6 @@ Graphql/Descriptions: Include: - 'app/graphql/**/*' - 'ee/app/graphql/**/*' + +RSpec/AnyInstanceOf: + Enabled: false diff --git a/rubocop/cop/rspec/any_instance_of.rb b/rubocop/cop/rspec/any_instance_of.rb new file mode 100644 index 00000000000000..e855fac5093dd0 --- /dev/null +++ b/rubocop/cop/rspec/any_instance_of.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # This cop checks for `allow_any_instance_of` or `expect_any_instance_of` + # usage in specs. + # + # @example + # + # # bad + # allow_any_instance_of(User).to receive(:invalidate_issue_cache_counts) + # + # # bad + # expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts) + # + # # good + # allow_next_instance_of(User) do |instance| + # allow(instance).to receive(:invalidate_issue_cache_counts) + # end + # + # # good + # expect_next_instance_of(User) do |instance| + # expect(instance).to receive(:invalidate_issue_cache_counts) + # end + # + class AnyInstanceOf < RuboCop::Cop::Cop + MESSAGE_EXPECT = 'Do not use `expect_any_instance_of` method, use `expect_next_instance_of` instead.' + MESSAGE_ALLOW = 'Do not use `allow_any_instance_of` method, use `allow_next_instance_of` instead.' + + def_node_search :expect_any_instance_of?, <<~PATTERN + (send + (send nil? :expect_any_instance_of ...) _ ... + ) + PATTERN + def_node_search :allow_any_instance_of?, <<~PATTERN + (send + (send nil? :allow_any_instance_of ...) _ ... + ) + PATTERN + + def on_send(node) + if expect_any_instance_of?(node) + add_offense(node, location: :expression, message: MESSAGE_EXPECT) + elsif allow_any_instance_of?(node) + add_offense(node, location: :expression, message: MESSAGE_ALLOW) + end + end + + def autocorrect(node) + if expect_any_instance_of?(node) + replacement = replacement_expect_any_instance_of(node) + elsif allow_any_instance_of?(node) + replacement = replacement_allow_any_instance_of(node) + end + + lambda do |corrector| + corrector.replace(node.loc.expression, replacement) + end + end + + private + + def replacement_expect_any_instance_of(node) + replacement = node.receiver.source.sub('expect_any_instance_of', 'expect_next_instance_of') + replacement << " do |instance|\n" + replacement << " expect(instance).#{node.method_name} #{node.children.last.source}\n" + replacement << 'end' + end + + def replacement_allow_any_instance_of(node) + replacement = node.receiver.source.sub('allow_any_instance_of', 'allow_next_instance_of') + replacement << " do |instance|\n" + replacement << " allow(instance).#{node.method_name} #{node.children.last.source}\n" + replacement << 'end' + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 70679aa1e780b9..159892ae0c1ea5 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -32,6 +32,7 @@ require_relative 'cop/migration/update_column_in_batches' require_relative 'cop/migration/update_large_table' require_relative 'cop/project_path_helper' +require_relative 'cop/rspec/any_instance_of' require_relative 'cop/rspec/be_success_matcher' require_relative 'cop/rspec/env_assignment' require_relative 'cop/rspec/factories_in_migration_specs' diff --git a/spec/controllers/abuse_reports_controller_spec.rb b/spec/controllers/abuse_reports_controller_spec.rb index e360ab68cf26b9..e573ef4be49b04 100644 --- a/spec/controllers/abuse_reports_controller_spec.rb +++ b/spec/controllers/abuse_reports_controller_spec.rb @@ -49,7 +49,9 @@ end it 'calls notify' do - expect_any_instance_of(AbuseReport).to receive(:notify) + expect_next_instance_of(AbuseReport) do |instance| + expect(instance).to receive(:notify) + end post :create, params: { abuse_report: attrs } end diff --git a/spec/controllers/google_api/authorizations_controller_spec.rb b/spec/controllers/google_api/authorizations_controller_spec.rb index 940bf9c68286a7..4d200140f164b5 100644 --- a/spec/controllers/google_api/authorizations_controller_spec.rb +++ b/spec/controllers/google_api/authorizations_controller_spec.rb @@ -13,8 +13,9 @@ before do sign_in(user) - allow_any_instance_of(GoogleApi::CloudPlatform::Client) - .to receive(:get_token).and_return([token, expires_at]) + allow_next_instance_of(GoogleApi::CloudPlatform::Client) do |instance| + allow(instance).to receive(:get_token).and_return([token, expires_at]) + end end shared_examples_for 'access denied' do diff --git a/spec/controllers/metrics_controller_spec.rb b/spec/controllers/metrics_controller_spec.rb index 7fb3578cd0ad0b..1d378b9b9dc498 100644 --- a/spec/controllers/metrics_controller_spec.rb +++ b/spec/controllers/metrics_controller_spec.rb @@ -23,7 +23,9 @@ allow(Prometheus::Client.configuration).to receive(:multiprocess_files_dir).and_return(metrics_multiproc_dir) allow(Gitlab::Metrics).to receive(:prometheus_metrics_enabled?).and_return(true) allow(Settings.monitoring).to receive(:ip_whitelist).and_return([whitelisted_ip, whitelisted_ip_range]) - allow_any_instance_of(MetricsService).to receive(:metrics_text).and_return("prometheus_counter 1") + allow_next_instance_of(MetricsService) do |instance| + allow(instance).to receive(:metrics_text).and_return("prometheus_counter 1") + end end describe '#index' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index e892c736c69f66..054d448c28deef 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -251,7 +251,9 @@ def create_snippet(snippet_params = {}, additional_params = {}) context 'when the snippet is spam' do before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) + allow_next_instance_of(AkismetService) do |instance| + allow(instance).to receive(:spam?).and_return(true) + end end context 'when the snippet is private' do @@ -323,7 +325,9 @@ def update_snippet(snippet_params = {}, additional_params = {}) context 'when the snippet is spam' do before do - allow_any_instance_of(AkismetService).to receive(:spam?).and_return(true) + allow_next_instance_of(AkismetService) do |instance| + allow(instance).to receive(:spam?).and_return(true) + end end context 'when the snippet is private' do @@ -431,7 +435,9 @@ def update_snippet(snippet_params = {}, additional_params = {}) let(:snippet) { create(:personal_snippet, :public, author: user) } before do - allow_any_instance_of(AkismetService).to receive_messages(submit_spam: true) + allow_next_instance_of(AkismetService) do |instance| + allow(instance).to receive_messages(submit_spam: true) + end stub_application_setting(akismet_enabled: true) end diff --git a/spec/dependencies/omniauth_saml_spec.rb b/spec/dependencies/omniauth_saml_spec.rb index 8a685648c71596..e0ea9c38e697ec 100644 --- a/spec/dependencies/omniauth_saml_spec.rb +++ b/spec/dependencies/omniauth_saml_spec.rb @@ -14,7 +14,9 @@ before do allow(saml_strategy).to receive(:session).and_return(session_mock) - allow_any_instance_of(OneLogin::RubySaml::Response).to receive(:is_valid?).and_return(true) + allow_next_instance_of(OneLogin::RubySaml::Response) do |instance| + allow(instance).to receive(:is_valid?).and_return(true) + end saml_strategy.send(:handle_response, mock_saml_response, {}, settings ) { } end diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index e8c438e459b60d..d3d25d3cb74e0d 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -210,7 +210,9 @@ def element(*arguments) let(:user) { create(:user, static_object_token: 'hunter1') } before do - allow_any_instance_of(ApplicationSetting).to receive(:static_objects_external_storage_url).and_return('https://cdn.gitlab.com') + allow_next_instance_of(ApplicationSetting) do |instance| + allow(instance).to receive(:static_objects_external_storage_url).and_return('https://cdn.gitlab.com') + end allow(helper).to receive(:current_user).and_return(user) end diff --git a/spec/rubocop/cop/rspec/any_instance_of_spec.rb b/spec/rubocop/cop/rspec/any_instance_of_spec.rb new file mode 100644 index 00000000000000..abb617218b7aa6 --- /dev/null +++ b/spec/rubocop/cop/rspec/any_instance_of_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_relative '../../../../rubocop/cop/rspec/any_instance_of' + +describe RuboCop::Cop::RSpec::AnyInstanceOf do + include CopHelper + + subject(:cop) { described_class.new } + + context 'when calling allow_any_instance_of' do + let(:source) do + <<~SRC + allow_any_instance_of(User).to receive(:invalidate_issue_cache_counts) + SRC + end + let(:corrected_source) do + <<~SRC + allow_next_instance_of(User) do |instance| + allow(instance).to receive(:invalidate_issue_cache_counts) + end + SRC + end + + it 'registers an offence' do + inspect_source(source) + expect(cop.offenses.size).to eq(1) + end + + it 'can autocorrect the source' do + expect(autocorrect_source(source)).to eq(corrected_source) + end + end + + context 'when calling expect_any_instance_of' do + let(:source) do + <<~SRC + expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts).with(args).and_return(double) + SRC + end + let(:corrected_source) do + <<~SRC + expect_next_instance_of(User) do |instance| + expect(instance).to receive(:invalidate_issue_cache_counts).with(args).and_return(double) + end + SRC + end + + it 'registers an offence' do + inspect_source(source) + expect(cop.offenses.size).to eq(1) + end + + it 'can autocorrect the source' do + expect(autocorrect_source(source)).to eq(corrected_source) + end + end +end -- GitLab From 5e2b317f2b75fb373118cbb5806230082c16cd8f Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Thu, 31 Oct 2019 09:41:09 +0000 Subject: [PATCH 2/3] Remove duplication between replacement methods --- rubocop/cop/rspec/any_instance_of.rb | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/rubocop/cop/rspec/any_instance_of.rb b/rubocop/cop/rspec/any_instance_of.rb index e855fac5093dd0..070a2ca408fd56 100644 --- a/rubocop/cop/rspec/any_instance_of.rb +++ b/rubocop/cop/rspec/any_instance_of.rb @@ -49,9 +49,9 @@ def on_send(node) def autocorrect(node) if expect_any_instance_of?(node) - replacement = replacement_expect_any_instance_of(node) + replacement = replacement_any_instance_of(node, 'expect') elsif allow_any_instance_of?(node) - replacement = replacement_allow_any_instance_of(node) + replacement = replacement_any_instance_of(node, 'allow') end lambda do |corrector| @@ -61,17 +61,10 @@ def autocorrect(node) private - def replacement_expect_any_instance_of(node) - replacement = node.receiver.source.sub('expect_any_instance_of', 'expect_next_instance_of') + def replacement_any_instance_of(node, rspec_prefix) + replacement = node.receiver.source.sub("#{rspec_prefix}_any_instance_of", "#{rspec_prefix}_next_instance_of") replacement << " do |instance|\n" - replacement << " expect(instance).#{node.method_name} #{node.children.last.source}\n" - replacement << 'end' - end - - def replacement_allow_any_instance_of(node) - replacement = node.receiver.source.sub('allow_any_instance_of', 'allow_next_instance_of') - replacement << " do |instance|\n" - replacement << " allow(instance).#{node.method_name} #{node.children.last.source}\n" + replacement << " #{rspec_prefix}(instance).#{node.method_name} #{node.children.last.source}\n" replacement << 'end' end end -- GitLab From 9dff94db0b9ffe5c0ab5238938e62924301d18b1 Mon Sep 17 00:00:00 2001 From: Arturo Herrero Date: Wed, 6 Nov 2019 10:11:16 +0000 Subject: [PATCH 3/3] Improve code avoiding mutable state --- rubocop/cop/rspec/any_instance_of.rb | 35 +++++++++++-------- .../rubocop/cop/rspec/any_instance_of_spec.rb | 2 ++ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/rubocop/cop/rspec/any_instance_of.rb b/rubocop/cop/rspec/any_instance_of.rb index 070a2ca408fd56..a939af36c13054 100644 --- a/rubocop/cop/rspec/any_instance_of.rb +++ b/rubocop/cop/rspec/any_instance_of.rb @@ -29,14 +29,10 @@ class AnyInstanceOf < RuboCop::Cop::Cop MESSAGE_ALLOW = 'Do not use `allow_any_instance_of` method, use `allow_next_instance_of` instead.' def_node_search :expect_any_instance_of?, <<~PATTERN - (send - (send nil? :expect_any_instance_of ...) _ ... - ) + (send (send nil? :expect_any_instance_of ...) ...) PATTERN def_node_search :allow_any_instance_of?, <<~PATTERN - (send - (send nil? :allow_any_instance_of ...) _ ... - ) + (send (send nil? :allow_any_instance_of ...) ...) PATTERN def on_send(node) @@ -48,11 +44,12 @@ def on_send(node) end def autocorrect(node) - if expect_any_instance_of?(node) - replacement = replacement_any_instance_of(node, 'expect') - elsif allow_any_instance_of?(node) - replacement = replacement_any_instance_of(node, 'allow') - end + replacement = + if expect_any_instance_of?(node) + replacement_any_instance_of(node, 'expect') + elsif allow_any_instance_of?(node) + replacement_any_instance_of(node, 'allow') + end lambda do |corrector| corrector.replace(node.loc.expression, replacement) @@ -62,10 +59,18 @@ def autocorrect(node) private def replacement_any_instance_of(node, rspec_prefix) - replacement = node.receiver.source.sub("#{rspec_prefix}_any_instance_of", "#{rspec_prefix}_next_instance_of") - replacement << " do |instance|\n" - replacement << " #{rspec_prefix}(instance).#{node.method_name} #{node.children.last.source}\n" - replacement << 'end' + method_call = + node.receiver.source.sub( + "#{rspec_prefix}_any_instance_of", + "#{rspec_prefix}_next_instance_of") + + block = <<~RUBY.chomp + do |instance| + #{rspec_prefix}(instance).#{node.method_name} #{node.children.last.source} + end + RUBY + + "#{method_call} #{block}" end end end diff --git a/spec/rubocop/cop/rspec/any_instance_of_spec.rb b/spec/rubocop/cop/rspec/any_instance_of_spec.rb index abb617218b7aa6..b16f8ac189ce44 100644 --- a/spec/rubocop/cop/rspec/any_instance_of_spec.rb +++ b/spec/rubocop/cop/rspec/any_instance_of_spec.rb @@ -25,6 +25,7 @@ it 'registers an offence' do inspect_source(source) + expect(cop.offenses.size).to eq(1) end @@ -49,6 +50,7 @@ it 'registers an offence' do inspect_source(source) + expect(cop.offenses.size).to eq(1) end -- GitLab