From 58185617d0bd10981acbf41c26178d5d69c7182c Mon Sep 17 00:00:00 2001 From: John Backus Date: Mon, 19 Dec 2016 18:06:41 -0800 Subject: [PATCH 1/2] Add ScatteredSetup cop --- CHANGELOG.md | 1 + config/default.yml | 4 + lib/rubocop-rspec.rb | 2 + lib/rubocop/cop/rspec/scattered_setup.rb | 51 ++++++++++ lib/rubocop/rspec/example_group.rb | 31 ++++++- lib/rubocop/rspec/hook.rb | 42 +++++++++ .../rubocop/cop/rspec/scattered_setup_spec.rb | 92 +++++++++++++++++++ 7 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 lib/rubocop/cop/rspec/scattered_setup.rb create mode 100644 lib/rubocop/rspec/hook.rb create mode 100644 spec/rubocop/cop/rspec/scattered_setup_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a40892f5..49f17f656 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * Add autocorrect support for `SingleArgumentMessageChain` cop. ([@bquorning][]) * Rename `NestedGroups`' configuration key from `MaxNesting` to `Max` in order to be consistent with other cop configuration. ([@backus][]) * Add `RepeatedExample` cop for detecting repeated examples within example groups. ([@backus][]) +* Add `ScatteredSetup` cop for enforcing that only one `before`, `around`, and `after` hook are used per example group scope. ([@backus][]) ## 1.9.1 (2017-01-02) diff --git a/config/default.yml b/config/default.yml index 7b283bfaf..bc403baa5 100644 --- a/config/default.yml +++ b/config/default.yml @@ -149,6 +149,10 @@ RSpec/SingleArgumentMessageChain: Description: Checks that chains of messages contain more than one element. Enabled: true +RSpec/ScatteredSetup: + Description: Checks for separated hooks in the same example group. + Enabled: true + RSpec/SubjectStub: Description: Checks for stubbed test subjects. Enabled: true diff --git a/lib/rubocop-rspec.rb b/lib/rubocop-rspec.rb index 8ed092934..92045535e 100644 --- a/lib/rubocop-rspec.rb +++ b/lib/rubocop-rspec.rb @@ -13,6 +13,7 @@ require 'rubocop/rspec/language/node_pattern' require 'rubocop/rspec/example_group' require 'rubocop/rspec/example' +require 'rubocop/rspec/hook' require 'rubocop/cop/rspec/cop' RuboCop::RSpec::Inject.defaults! @@ -44,6 +45,7 @@ require 'rubocop/cop/rspec/not_to_not' require 'rubocop/cop/rspec/repeated_description' require 'rubocop/cop/rspec/repeated_example' +require 'rubocop/cop/rspec/scattered_setup' require 'rubocop/cop/rspec/single_argument_message_chain' require 'rubocop/cop/rspec/subject_stub' require 'rubocop/cop/rspec/verified_doubles' diff --git a/lib/rubocop/cop/rspec/scattered_setup.rb b/lib/rubocop/cop/rspec/scattered_setup.rb new file mode 100644 index 000000000..6a0e6a1db --- /dev/null +++ b/lib/rubocop/cop/rspec/scattered_setup.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for separated hooks in the same example group. + # + # Unify `before`, `after`, and `around` hooks when possible. + # + # @example + # # bad + # describe Foo do + # before { setup1 } + # before { setup2 } + # end + # + # # good + # describe Foo do + # before do + # setup1 + # setup2 + # end + # end + # + class ScatteredSetup < Cop + include RuboCop::RSpec::TopLevelDescribe + + MSG = 'Do not define multiple hooks in the same example group.'.freeze + + def on_block(node) + return unless example_group?(node) + + analyzable_hooks(node).each do |repeated_hook| + add_offense(repeated_hook, :expression) + end + end + + def analyzable_hooks(node) + RuboCop::RSpec::ExampleGroup.new(node) + .hooks + .reject(&:unknown_scope?) + .group_by { |hook| [hook.name, hook.scope] } + .values + .reject(&:one?) + .flatten + .map(&:to_node) + end + end + end + end +end diff --git a/lib/rubocop/rspec/example_group.rb b/lib/rubocop/rspec/example_group.rb index 8ed42cfef..d660f5cf0 100644 --- a/lib/rubocop/rspec/example_group.rb +++ b/lib/rubocop/rspec/example_group.rb @@ -18,6 +18,13 @@ class ExampleGroup def_node_matcher :scope_change?, (ExampleGroups::ALL + SharedGroups::ALL).block_pattern + # @!method hook(node) + # + # Detect if node is `before`, `after`, `around` + def_node_matcher :hook, <<-PATTERN + (block {$(send nil #{Hooks::ALL.node_pattern_union} ...)} ...) + PATTERN + def initialize(node) @node = node end @@ -26,13 +33,33 @@ def examples examples_in_scope(node).map(&Example.public_method(:new)) end + def hooks + hooks_in_scope(node).map(&Hook.public_method(:new)) + end + private attr_reader :node - def examples_in_scope(node) + def hooks_in_scope(node) + node.each_child_node.flat_map do |child| + find_hooks(child) + end + end + + def find_hooks(node) + return [] if scope_change?(node) || example?(node) + + if hook(node) + [node] + else + hooks_in_scope(node) + end + end + + def examples_in_scope(node, &blk) node.each_child_node.flat_map do |child| - find_examples(child) + find_examples(child, &blk) end end diff --git a/lib/rubocop/rspec/hook.rb b/lib/rubocop/rspec/hook.rb new file mode 100644 index 000000000..d295e3f94 --- /dev/null +++ b/lib/rubocop/rspec/hook.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module RuboCop + module RSpec + # Wrapper for RSpec hook + class Hook + extend RuboCop::NodePattern::Macros + + def initialize(node) + @node = node + end + + def name + node.method_name + end + + def scope + scope_argument ? scope_argument.to_a.first : :each + end + + def unknown_scope? + return false unless scope_argument + + !scope_argument.sym_type? + end + + def to_node + node + end + + protected + + attr_reader :node + + private + + def scope_argument + node.method_args.first + end + end + end +end diff --git a/spec/rubocop/cop/rspec/scattered_setup_spec.rb b/spec/rubocop/cop/rspec/scattered_setup_spec.rb new file mode 100644 index 000000000..842127052 --- /dev/null +++ b/spec/rubocop/cop/rspec/scattered_setup_spec.rb @@ -0,0 +1,92 @@ +describe RuboCop::Cop::RSpec::ScatteredSetup do + subject(:cop) { described_class.new } + + it 'flags multiple hooks in the same example group' do + expect_violation(<<-RUBY) + describe Foo do + before { bar } + ^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + before { baz } + ^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + end + RUBY + end + + it 'flags multiple before(:all) hooks in the same example group' do + expect_violation(<<-RUBY) + describe Foo do + before(:all) { bar } + ^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + before(:all) { baz } + ^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + end + RUBY + end + + it 'does not flag different hooks' do + expect_no_violations(<<-RUBY) + describe Foo do + before { bar } + after { baz } + around { qux } + end + RUBY + end + + it 'does not flag different hook types' do + expect_no_violations(<<-RUBY) + describe Foo do + before { bar } + before(:all) { baz } + before(:suite) { baz } + end + RUBY + end + + it 'does not flag hooks in different example groups' do + expect_no_violations(<<-RUBY) + describe Foo do + before { bar } + + describe '.baz' do + before { baz } + end + end + RUBY + end + + it 'does not flag hooks in different shared contexts' do + expect_no_violations(<<-RUBY) + describe Foo do + shared_context 'one' do + before { bar } + end + + shared_context 'two' do + before { baz } + end + end + RUBY + end + + it 'does not flag similar method names inside of examples' do + expect_no_violations(<<-RUBY) + describe Foo do + before { bar } + + it 'uses an instance method called before' do + expect(before { tricky }).to_not confuse_rubocop_rspec + end + end + RUBY + end + + it 'does not break if a hook is not given a symbol literal' do + expect_no_violations(<<-RUBY) + describe Foo do + scope = :each + before(scope) { example_setup } + end + RUBY + end +end From 1963e86df40ce94ba78fc59b62935bc143525f7d Mon Sep 17 00:00:00 2001 From: John Backus Date: Thu, 12 Jan 2017 01:36:31 -0800 Subject: [PATCH 2/2] Extract RuboCop::RSpec::Concept --- config/default.yml | 2 +- lib/rubocop-rspec.rb | 1 + lib/rubocop/cop/rspec/scattered_setup.rb | 6 +-- lib/rubocop/rspec/example.rb | 26 +--------- lib/rubocop/rspec/example_group.rb | 11 +---- lib/rubocop/rspec/hook.rb | 39 ++++++++------- lib/rubocop/rspec/interface.rb | 33 +++++++++++++ .../rubocop/cop/rspec/scattered_setup_spec.rb | 22 +++++---- spec/rubocop/rspec/hook_spec.rb | 49 +++++++++++++++++++ 9 files changed, 122 insertions(+), 67 deletions(-) create mode 100644 lib/rubocop/rspec/interface.rb create mode 100644 spec/rubocop/rspec/hook_spec.rb diff --git a/config/default.yml b/config/default.yml index bc403baa5..9e3865485 100644 --- a/config/default.yml +++ b/config/default.yml @@ -150,7 +150,7 @@ RSpec/SingleArgumentMessageChain: Enabled: true RSpec/ScatteredSetup: - Description: Checks for separated hooks in the same example group. + Description: Checks for setup scattered across multiple hooks in an example group. Enabled: true RSpec/SubjectStub: diff --git a/lib/rubocop-rspec.rb b/lib/rubocop-rspec.rb index 92045535e..c207559c1 100644 --- a/lib/rubocop-rspec.rb +++ b/lib/rubocop-rspec.rb @@ -11,6 +11,7 @@ require 'rubocop/rspec/util' require 'rubocop/rspec/language' require 'rubocop/rspec/language/node_pattern' +require 'rubocop/rspec/interface' require 'rubocop/rspec/example_group' require 'rubocop/rspec/example' require 'rubocop/rspec/hook' diff --git a/lib/rubocop/cop/rspec/scattered_setup.rb b/lib/rubocop/cop/rspec/scattered_setup.rb index 6a0e6a1db..bdd82b91e 100644 --- a/lib/rubocop/cop/rspec/scattered_setup.rb +++ b/lib/rubocop/cop/rspec/scattered_setup.rb @@ -3,7 +3,7 @@ module RuboCop module Cop module RSpec - # Checks for separated hooks in the same example group. + # Checks for setup scattered across multiple hooks in an example group. # # Unify `before`, `after`, and `around` hooks when possible. # @@ -23,8 +23,6 @@ module RSpec # end # class ScatteredSetup < Cop - include RuboCop::RSpec::TopLevelDescribe - MSG = 'Do not define multiple hooks in the same example group.'.freeze def on_block(node) @@ -38,7 +36,7 @@ def on_block(node) def analyzable_hooks(node) RuboCop::RSpec::ExampleGroup.new(node) .hooks - .reject(&:unknown_scope?) + .select { |hook| hook.knowable_scope? && hook.valid_scope? } .group_by { |hook| [hook.name, hook.scope] } .values .reject(&:one?) diff --git a/lib/rubocop/rspec/example.rb b/lib/rubocop/rspec/example.rb index ce8ba3435..3f2d78c79 100644 --- a/lib/rubocop/rspec/example.rb +++ b/lib/rubocop/rspec/example.rb @@ -3,17 +3,11 @@ module RuboCop module RSpec # Wrapper for RSpec examples - class Example - extend RuboCop::NodePattern::Macros - + class Example < Concept def_node_matcher :extract_doc_string, '(send _ _ $str ...)' def_node_matcher :extract_metadata, '(send _ _ _ $...)' def_node_matcher :extract_implementation, '(block send args $_)' - def initialize(node) - @node = node - end - def doc_string extract_doc_string(definition) end @@ -26,20 +20,6 @@ def implementation extract_implementation(node) end - def eql?(other) - node.eql?(other.node) - end - - alias == eql? - - def hash - [self.class, node].hash - end - - def to_node - node - end - def definition if node.send_type? node @@ -47,10 +27,6 @@ def definition node.children.first end end - - protected - - attr_reader :node end end end diff --git a/lib/rubocop/rspec/example_group.rb b/lib/rubocop/rspec/example_group.rb index d660f5cf0..580c0ac38 100644 --- a/lib/rubocop/rspec/example_group.rb +++ b/lib/rubocop/rspec/example_group.rb @@ -3,10 +3,7 @@ module RuboCop module RSpec # Wrapper for RSpec example groups - class ExampleGroup - include Language - extend NodePattern::Macros - + class ExampleGroup < Concept def_node_matcher :example?, Examples::ALL.block_pattern # @!method scope_change?(node) @@ -25,10 +22,6 @@ class ExampleGroup (block {$(send nil #{Hooks::ALL.node_pattern_union} ...)} ...) PATTERN - def initialize(node) - @node = node - end - def examples examples_in_scope(node).map(&Example.public_method(:new)) end @@ -39,8 +32,6 @@ def hooks private - attr_reader :node - def hooks_in_scope(node) node.each_child_node.flat_map do |child| find_hooks(child) diff --git a/lib/rubocop/rspec/hook.rb b/lib/rubocop/rspec/hook.rb index d295e3f94..57d6f4daf 100644 --- a/lib/rubocop/rspec/hook.rb +++ b/lib/rubocop/rspec/hook.rb @@ -3,37 +3,40 @@ module RuboCop module RSpec # Wrapper for RSpec hook - class Hook - extend RuboCop::NodePattern::Macros - - def initialize(node) - @node = node - end + class Hook < Concept + STANDARDIZED_SCOPES = %i(each context suite).freeze + private_constant(:STANDARDIZED_SCOPES) def name node.method_name end - def scope - scope_argument ? scope_argument.to_a.first : :each - end - - def unknown_scope? - return false unless scope_argument + def knowable_scope? + return true unless scope_argument - !scope_argument.sym_type? + scope_argument.sym_type? end - def to_node - node + def valid_scope? + STANDARDIZED_SCOPES.include?(scope) end - protected - - attr_reader :node + def scope + case scope_name + when nil, :each, :example then :each + when :context, :all then :context + when :suite then :suite + else + scope_name + end + end private + def scope_name + scope_argument.to_a.first + end + def scope_argument node.method_args.first end diff --git a/lib/rubocop/rspec/interface.rb b/lib/rubocop/rspec/interface.rb new file mode 100644 index 000000000..39776f124 --- /dev/null +++ b/lib/rubocop/rspec/interface.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module RuboCop + module RSpec + # Wrapper for RSpec DSL methods + class Concept + include Language + extend NodePattern::Macros + + def initialize(node) + @node = node + end + + def eql?(other) + node.eql?(other.node) + end + + alias == eql? + + def hash + [self.class, node].hash + end + + def to_node + node + end + + protected + + attr_reader :node + end + end +end diff --git a/spec/rubocop/cop/rspec/scattered_setup_spec.rb b/spec/rubocop/cop/rspec/scattered_setup_spec.rb index 842127052..ad305d72b 100644 --- a/spec/rubocop/cop/rspec/scattered_setup_spec.rb +++ b/spec/rubocop/cop/rspec/scattered_setup_spec.rb @@ -12,6 +12,19 @@ RUBY end + it 'flags multiple hooks of the same scope with different symbols' do + expect_violation(<<-RUBY) + describe Foo do + before { bar } + ^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + before(:each) { baz } + ^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + before(:example) { baz } + ^^^^^^^^^^^^^^^^^^^^^^^^ Do not define multiple hooks in the same example group. + end + RUBY + end + it 'flags multiple before(:all) hooks in the same example group' do expect_violation(<<-RUBY) describe Foo do @@ -80,13 +93,4 @@ end RUBY end - - it 'does not break if a hook is not given a symbol literal' do - expect_no_violations(<<-RUBY) - describe Foo do - scope = :each - before(scope) { example_setup } - end - RUBY - end end diff --git a/spec/rubocop/rspec/hook_spec.rb b/spec/rubocop/rspec/hook_spec.rb new file mode 100644 index 000000000..ebd1ceace --- /dev/null +++ b/spec/rubocop/rspec/hook_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::RSpec::Hook do + include RuboCop::Sexp + + def hook(source) + described_class.new(parse_source(source).ast) + end + + it 'extracts name' do + expect(hook('around(:each) { }').name).to be(:around) + end + + it 'does not break if a hook is not given a symbol literal' do + expect(hook('before(scope) { example_setup }').knowable_scope?).to be(false) + end + + it 'knows the scope of a hook with a symbol literal' do + expect(hook('before { example_setup }').knowable_scope?).to be(true) + end + + it 'ignores other arguments to hooks' do + expect(hook('before(:each, :metadata) { example_setup }').scope) + .to be(:each) + end + + it 'classifies nonstandard hook arguments as invalid' do + expect(hook('before(:nothing) { example_setup }').valid_scope?).to be(false) + end + + it 'classifies :each as a valid hook argument' do + expect(hook('before(:each) { example_setup }').valid_scope?).to be(true) + end + + shared_examples 'standardizes scope' do |source, scope| + it "interprets #{source} as having scope #{scope}" do + expect(hook(source).scope).to equal(scope) + end + end + + include_examples 'standardizes scope', 'before(:each) { }', :each + include_examples 'standardizes scope', 'around(:example) { }', :each + include_examples 'standardizes scope', 'after { }', :each + + include_examples 'standardizes scope', 'before(:all) { }', :context + include_examples 'standardizes scope', 'around(:context) { }', :context + + include_examples 'standardizes scope', 'after(:suite) { }', :suite +end