From d94671b5690f44520d7ee5d6a95f0f047c5b8d7a Mon Sep 17 00:00:00 2001 From: Viacheslav Mefodin Date: Sat, 30 May 2020 15:35:29 +0300 Subject: [PATCH 1/4] Introduce ConfigShortcuts mixin Since some of patterns like `hook?` should be available not only in cops, but also in "concepts", e.g. `ExampleGroup` - the solution would be to put config accessor shortcuts into mixin. --- lib/rubocop-rspec.rb | 1 + lib/rubocop/cop/rspec/cop.rb | 1 + lib/rubocop/rspec/config_shortcuts.rb | 18 ++++++++++++++++++ 3 files changed, 20 insertions(+) create mode 100644 lib/rubocop/rspec/config_shortcuts.rb diff --git a/lib/rubocop-rspec.rb b/lib/rubocop-rspec.rb index cf9231dd8..b46f08143 100644 --- a/lib/rubocop-rspec.rb +++ b/lib/rubocop-rspec.rb @@ -13,6 +13,7 @@ require_relative 'rubocop/rspec/wording' require_relative 'rubocop/rspec/language' require_relative 'rubocop/rspec/language/node_pattern' +require_relative 'rubocop/rspec/config_shortcuts' require_relative 'rubocop/rspec/concept' require_relative 'rubocop/rspec/example_group' require_relative 'rubocop/rspec/example' diff --git a/lib/rubocop/cop/rspec/cop.rb b/lib/rubocop/cop/rspec/cop.rb index fa8e30427..da8d0e070 100644 --- a/lib/rubocop/cop/rspec/cop.rb +++ b/lib/rubocop/cop/rspec/cop.rb @@ -40,6 +40,7 @@ module RSpec class Cop < WorkaroundCop include RuboCop::RSpec::Language include RuboCop::RSpec::Language::NodePattern + include RuboCop::RSpec::ConfigShortcuts DEFAULT_CONFIGURATION = RuboCop::RSpec::CONFIG.fetch('AllCops').fetch('RSpec') diff --git a/lib/rubocop/rspec/config_shortcuts.rb b/lib/rubocop/rspec/config_shortcuts.rb new file mode 100644 index 000000000..fad325056 --- /dev/null +++ b/lib/rubocop/rspec/config_shortcuts.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module RuboCop + module RSpec + # Config shortcuts for RuboCop::Cop::Rspec::Cop + # and Rubocop::RSpec::ExampleGroup + module ConfigShortcuts + def rspec_aliases(setting) + config + .for_all_cops + .fetch('RSpec', {}) + .fetch('Aliases', {}) + .fetch(setting, []) + .map(&:to_sym) + end + end + end +end From a6f41c6f7a0a43bf3fc78cd255453580a2ddaa6a Mon Sep 17 00:00:00 2001 From: Viacheslav Mefodin Date: Sat, 30 May 2020 15:45:55 +0300 Subject: [PATCH 2/4] Make `hook?` node pattern to consider hook aliases --- lib/rubocop/rspec/language.rb | 32 ++++++++++++------- lib/rubocop/rspec/language/node_pattern.rb | 9 +++++- spec/rubocop/cop/rspec/expect_in_hook_spec.rb | 26 +++++++++++++++ 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/lib/rubocop/rspec/language.rb b/lib/rubocop/rspec/language.rb index 1e5ef49db..d89e6d8c2 100644 --- a/lib/rubocop/rspec/language.rb +++ b/lib/rubocop/rspec/language.rb @@ -6,6 +6,16 @@ module RSpec module Language RSPEC = '{(const {nil? cbase} :RSpec) nil?}' + module Patterns + BLOCK_PATTERN = <<-PATTERN + (block (send {(const {nil? cbase} :RSpec) nil?} $_ ...) ...) + PATTERN + + SEND_PATTERN = <<-PATTERN + (send {(const {nil? cbase} :RSpec) nil?} $_ ...) + PATTERN + end + # Set of method selectors class SelectorSet def initialize(selectors) @@ -91,17 +101,17 @@ module Examples end module Hooks - ALL = SelectorSet.new( - %i[ - prepend_before - before - append_before - around - prepend_after - after - append_after - ] - ) + NAMES = %i[ + prepend_before + before + append_before + around + prepend_after + after + append_after + ].freeze + + ALL = SelectorSet.new(NAMES) module Scopes ALL = SelectorSet.new( diff --git a/lib/rubocop/rspec/language/node_pattern.rb b/lib/rubocop/rspec/language/node_pattern.rb index 665b7ad70..8caa699ff 100644 --- a/lib/rubocop/rspec/language/node_pattern.rb +++ b/lib/rubocop/rspec/language/node_pattern.rb @@ -15,11 +15,18 @@ module NodePattern def_node_matcher :example?, Examples::ALL.block_pattern - def_node_matcher :hook?, Hooks::ALL.block_pattern + def hook?(node) + block_pattern(node) do |hook| + hooks = Hooks::NAMES + rspec_aliases('Hooks') + hooks.include?(hook) + end + end def_node_matcher :let?, Helpers::ALL.block_or_block_pass_pattern def_node_matcher :subject?, Subject::ALL.block_pattern + + def_node_matcher :block_pattern, Patterns::BLOCK_PATTERN end end end diff --git a/spec/rubocop/cop/rspec/expect_in_hook_spec.rb b/spec/rubocop/cop/rspec/expect_in_hook_spec.rb index 47593c54a..f7207fdeb 100644 --- a/spec/rubocop/cop/rspec/expect_in_hook_spec.rb +++ b/spec/rubocop/cop/rspec/expect_in_hook_spec.rb @@ -76,4 +76,30 @@ end RUBY end + + # TODO: These tests just demonstrate that hooks aliases work in rewritten cops + it 'does NOT add an offense for `expect` with block in `custom_hook` hook' do + expect_no_offenses(<<-RUBY) + setup do + expect { something }.to eq('foo') + end + RUBY + end + + context 'when `custom_hook` is configured in AllCops.RSpec.Aliases.Hooks' do + include_context 'config' + + let(:all_cops_config) do + { 'RSpec' => { 'Aliases' => { 'Hooks' => %w[custom_hook] } } } + end + + it 'adds an offense for `expect` with block in `custom_hook` hook' do + expect_offense(<<-RUBY) + custom_hook do + expect { something }.to eq('foo') + ^^^^^^ Do not use `expect` in `custom_hook` hook + end + RUBY + end + end end From 32f85c4c1f1cd398cdf9735a325c1686a40103fe Mon Sep 17 00:00:00 2001 From: Viacheslav Mefodin Date: Sat, 30 May 2020 15:48:32 +0300 Subject: [PATCH 3/4] Make concepts to consider config It fixes ScatteredSetup cop specs as well. --- lib/rubocop/cop/rspec/scattered_setup.rb | 2 +- lib/rubocop/rspec/concept.rb | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/rubocop/cop/rspec/scattered_setup.rb b/lib/rubocop/cop/rspec/scattered_setup.rb index f54a0a482..75891bab0 100644 --- a/lib/rubocop/cop/rspec/scattered_setup.rb +++ b/lib/rubocop/cop/rspec/scattered_setup.rb @@ -42,7 +42,7 @@ def on_block(node) end def repeated_hooks(node) - hooks = RuboCop::RSpec::ExampleGroup.new(node) + hooks = RuboCop::RSpec::ExampleGroup.new(node, config) .hooks .select(&:knowable_scope?) .group_by { |hook| [hook.name, hook.scope, hook.metadata] } diff --git a/lib/rubocop/rspec/concept.rb b/lib/rubocop/rspec/concept.rb index 9a66ede9d..fbb590b9f 100644 --- a/lib/rubocop/rspec/concept.rb +++ b/lib/rubocop/rspec/concept.rb @@ -6,10 +6,12 @@ module RSpec class Concept include Language include Language::NodePattern + include ConfigShortcuts extend NodePattern::Macros - def initialize(node) + def initialize(node, config = nil) @node = node + @config = config end def eql?(other) @@ -29,6 +31,7 @@ def to_node protected attr_reader :node + attr_reader :config end end end From bc0edb81e0c9d829de2a907eabd61a5eb00c470a Mon Sep 17 00:00:00 2001 From: Viacheslav Mefodin Date: Sat, 30 May 2020 15:54:36 +0300 Subject: [PATCH 4/4] Make EmptyHook cop to consider hook aliases --- lib/rubocop/cop/rspec/empty_hook.rb | 8 ++++--- spec/rubocop/cop/rspec/empty_hook_spec.rb | 26 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/lib/rubocop/cop/rspec/empty_hook.rb b/lib/rubocop/cop/rspec/empty_hook.rb index ea5618032..c7bcca4f9 100644 --- a/lib/rubocop/cop/rspec/empty_hook.rb +++ b/lib/rubocop/cop/rspec/empty_hook.rb @@ -27,12 +27,14 @@ class EmptyHook < Cop MSG = 'Empty hook detected.' - def_node_matcher :empty_hook?, <<~PATTERN - (block $#{Hooks::ALL.send_pattern} _ nil?) + def_node_matcher :empty_block?, <<~PATTERN + (block $#{Patterns::SEND_PATTERN} _ nil?) PATTERN def on_block(node) - empty_hook?(node) do |hook| + return unless hook?(node) + + empty_block?(node) do |hook| add_offense(hook) end end diff --git a/spec/rubocop/cop/rspec/empty_hook_spec.rb b/spec/rubocop/cop/rspec/empty_hook_spec.rb index 0c01480cf..dd2202771 100644 --- a/spec/rubocop/cop/rspec/empty_hook_spec.rb +++ b/spec/rubocop/cop/rspec/empty_hook_spec.rb @@ -240,4 +240,30 @@ RUBY end end + + # TODO: These tests just demonstrate that hooks aliases work in rewritten cops + context 'with `custom_hook` hook' do + it 'does NOT detect offense for empty NOT configured hook' do + expect_no_offenses(<<~RUBY) + custom_hook {} + RUBY + end + + context 'when `custom_hook` is configured in AllCops.RSpec.Aliases.Hooks' do + include_context 'config' + + let(:all_cops_config) do + { 'RSpec' => { 'Aliases' => { 'Hooks' => %w[custom_hook] } } } + end + + it 'detects offense for empty configured hook' do + expect_offense(<<~RUBY) + custom_hook {} + ^^^^^^^^^^^ Empty hook detected. + RUBY + + expect_correction('') + end + end + end end