From 20e37fbe085959b09b1b46c777213dfceabed7ec Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Sun, 7 Jun 2020 20:45:52 +0300 Subject: [PATCH 1/6] RSpec/SubjectStub. Refactor and decrease complexity --- lib/rubocop/cop/rspec/subject_stub.rb | 81 ++++++++++++--------------- 1 file changed, 35 insertions(+), 46 deletions(-) diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index 7454a32a6..d86a51568 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/lib/rubocop/cop/rspec/subject_stub.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require 'set' + module RuboCop module Cop module RSpec @@ -74,71 +76,58 @@ class SubjectStub < Cop PATTERN def on_block(node) + # process only describe/context/... example groups return unless example_group?(node) - find_subject_stub(node) do |stub| + # skip already processed example group + # it's processed if is nested in one of the processed example groups + return unless (processed_example_groups & node.ancestors).empty? + + # add example group to already processed + processed_example_groups << node + + # find all custom subjects e.g. subject(:foo) { ... } + @named_subjects = find_all_named_subjects(node) + + # look for method expectation matcher + find_subject_expectations(node) do |stub| add_offense(stub) end end private - # Find subjects within tree and then find (send) nodes for that subject - # - # @param node [RuboCop::Node] example group - # - # @yield [RuboCop::Node] message expectations for subject - def find_subject_stub(node, &block) - find_subject(node) do |subject_name, context| - find_subject_expectation(context, subject_name, &block) - end + def processed_example_groups + @processed_example_groups ||= Set[] end - # Find a subject message expectation - # - # @param node [RuboCop::Node] - # @param subject_name [Symbol] name of subject - # - # @yield [RuboCop::Node] message expectation - def find_subject_expectation(node, subject_name, &block) - # Do not search node if it is an example group with its own subject. - return if example_group?(node) && redefines_subject?(node) - - # Yield the current node if it is a message expectation. - yield(node) if message_expectation?(node, subject_name) + def find_all_named_subjects(node) + named_subjects = {} - # Recurse through node's children looking for a message expectation. - node.each_child_node do |child| - find_subject_expectation(child, subject_name, &block) + node.each_descendant(:block) do |child| + name = subject(child) + named_subjects[child.parent.parent] = name if name end + + named_subjects end - # Check if node's children contain a subject definition - # - # @param node [RuboCop::Node] - # - # @return [Boolean] - def redefines_subject?(node) - node.each_child_node.any? do |child| - subject(child) || redefines_subject?(child) + def find_subject_expectations(node, subject_name = nil, &block) + # if it's a new example group - check whether new named subject is + # defined there + if example_group?(node) + subject_name = @named_subjects[node] || subject_name end - end - # Find a subject definition - # - # @param node [RuboCop::Node] - # @param parent [RuboCop::Node,nil] - # - # @yieldparam subject_name [Symbol] name of subject being defined - # @yieldparam parent [RuboCop::Node] parent of subject definition - def find_subject(node, parent: nil, &block) - # An implicit subject is defined by RSpec when no subject is declared - subject_name = subject(node) || :subject + # check default :subject and then named one (if it's present) + expectation_detected = message_expectation?(node, :subject) || \ + (subject_name && message_expectation?(node, subject_name)) - yield(subject_name, parent) if parent + return yield(node) if expectation_detected + # Recurse through node's children looking for a message expectation. node.each_child_node do |child| - find_subject(child, parent: node, &block) + find_subject_expectations(child, subject_name, &block) end end end From 817a77e71ade072358795049e1143168e7311cfa Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Mon, 8 Jun 2020 00:10:05 +0300 Subject: [PATCH 2/6] RSpec/SubjectStub. Code review. Refactor #find_all_named_subjects method --- lib/rubocop/cop/rspec/subject_stub.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index d86a51568..a88856355 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/lib/rubocop/cop/rspec/subject_stub.rb @@ -102,14 +102,10 @@ def processed_example_groups end def find_all_named_subjects(node) - named_subjects = {} - - node.each_descendant(:block) do |child| + node.each_descendant(:block).each_with_object({}) do |child, h| name = subject(child) - named_subjects[child.parent.parent] = name if name + h[child.parent.parent] = name if name end - - named_subjects end def find_subject_expectations(node, subject_name = nil, &block) From 7e76bd7be16c710bf934d5cb0716e72b4a07cd14 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Mon, 8 Jun 2020 15:57:01 +0300 Subject: [PATCH 3/6] RSpec/SubjectStub. Code review. Handle case with several explicit subjects in example group --- lib/rubocop/cop/rspec/subject_stub.rb | 37 ++++++++------------- spec/rubocop/cop/rspec/subject_stub_spec.rb | 20 +++++++++++ 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index a88856355..0ebbc2918 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/lib/rubocop/cop/rspec/subject_stub.rb @@ -76,20 +76,12 @@ class SubjectStub < Cop PATTERN def on_block(node) - # process only describe/context/... example groups return unless example_group?(node) - - # skip already processed example group - # it's processed if is nested in one of the processed example groups return unless (processed_example_groups & node.ancestors).empty? - # add example group to already processed processed_example_groups << node + @explicit_subjects = find_all_explicit_subjects(node) - # find all custom subjects e.g. subject(:foo) { ... } - @named_subjects = find_all_named_subjects(node) - - # look for method expectation matcher find_subject_expectations(node) do |stub| add_offense(stub) end @@ -98,32 +90,31 @@ def on_block(node) private def processed_example_groups - @processed_example_groups ||= Set[] + @processed_example_groups ||= Set.new end - def find_all_named_subjects(node) + def find_all_explicit_subjects(node) node.each_descendant(:block).each_with_object({}) do |child, h| name = subject(child) - h[child.parent.parent] = name if name + if name + h[child.parent.parent] ||= [] + h[child.parent.parent] << name + end end end - def find_subject_expectations(node, subject_name = nil, &block) - # if it's a new example group - check whether new named subject is - # defined there - if example_group?(node) - subject_name = @named_subjects[node] || subject_name + def find_subject_expectations(node, subject_names = [], &block) + if example_group?(node) && @explicit_subjects[node] + subject_names = @explicit_subjects[node] end - # check default :subject and then named one (if it's present) - expectation_detected = message_expectation?(node, :subject) || \ - (subject_name && message_expectation?(node, subject_name)) - + expectation_detected = (subject_names + [:subject]).any? do |name| + message_expectation?(node, name) + end return yield(node) if expectation_detected - # Recurse through node's children looking for a message expectation. node.each_child_node do |child| - find_subject_expectations(child, subject_name, &block) + find_subject_expectations(child, subject_names, &block) end end end diff --git a/spec/rubocop/cop/rspec/subject_stub_spec.rb b/spec/rubocop/cop/rspec/subject_stub_spec.rb index 09f2c0fc4..728d1fc7d 100644 --- a/spec/rubocop/cop/rspec/subject_stub_spec.rb +++ b/spec/rubocop/cop/rspec/subject_stub_spec.rb @@ -20,6 +20,26 @@ RUBY end + it 'flags when subject is stubbed and there are several named subjects ' \ + 'in the same example group', :wip do + expect_offense(<<-RUBY) + describe Foo do + subject(:foo) { described_class.new } + subject(:bar) { described_class.new } + subject(:baz) { described_class.new } + + before do + allow(bar).to receive(:bar).and_return(baz) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test. + end + + it 'uses expect twice' do + expect(foo.bar).to eq(baz) + end + end + RUBY + end + it 'flags when subject is mocked' do expect_offense(<<-RUBY) describe Foo do From 341ae440ff17e2bc1dadf3c025c46698152f0cd2 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Tue, 9 Jun 2020 00:15:28 +0300 Subject: [PATCH 4/6] RSpec/SubjectStub. Code review. Use #on_top_level_describe instead of #on_block callback --- lib/rubocop/cop/rspec/subject_stub.rb | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index 0ebbc2918..1c57d2cca 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/lib/rubocop/cop/rspec/subject_stub.rb @@ -22,6 +22,8 @@ module RSpec # end # class SubjectStub < Cop + include RuboCop::RSpec::TopLevelDescribe + MSG = 'Do not stub methods of the object under test.' # @!method subject(node) @@ -75,24 +77,16 @@ class SubjectStub < Cop } ...) PATTERN - def on_block(node) - return unless example_group?(node) - return unless (processed_example_groups & node.ancestors).empty? - - processed_example_groups << node - @explicit_subjects = find_all_explicit_subjects(node) + def on_top_level_describe(node, _args) + @explicit_subjects = find_all_explicit_subjects(node.parent) - find_subject_expectations(node) do |stub| + find_subject_expectations(node.parent) do |stub| add_offense(stub) end end private - def processed_example_groups - @processed_example_groups ||= Set.new - end - def find_all_explicit_subjects(node) node.each_descendant(:block).each_with_object({}) do |child, h| name = subject(child) From 358d1296129570f599677cc4789e2f96273c082d Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 10 Jun 2020 01:30:49 +0300 Subject: [PATCH 5/6] Revert "RSpec/SubjectStub. Code review. Use #on_top_level_describe instead of #on_block callback" This reverts commit 341ae440ff17e2bc1dadf3c025c46698152f0cd2. --- lib/rubocop/cop/rspec/subject_stub.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index 1c57d2cca..0ebbc2918 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/lib/rubocop/cop/rspec/subject_stub.rb @@ -22,8 +22,6 @@ module RSpec # end # class SubjectStub < Cop - include RuboCop::RSpec::TopLevelDescribe - MSG = 'Do not stub methods of the object under test.' # @!method subject(node) @@ -77,16 +75,24 @@ class SubjectStub < Cop } ...) PATTERN - def on_top_level_describe(node, _args) - @explicit_subjects = find_all_explicit_subjects(node.parent) + def on_block(node) + return unless example_group?(node) + return unless (processed_example_groups & node.ancestors).empty? + + processed_example_groups << node + @explicit_subjects = find_all_explicit_subjects(node) - find_subject_expectations(node.parent) do |stub| + find_subject_expectations(node) do |stub| add_offense(stub) end end private + def processed_example_groups + @processed_example_groups ||= Set.new + end + def find_all_explicit_subjects(node) node.each_descendant(:block).each_with_object({}) do |child, h| name = subject(child) From 956efea7b7c5dffaa22cccecaf501321dd10d7f5 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Wed, 10 Jun 2020 02:02:09 +0300 Subject: [PATCH 6/6] RSpec/SubjectStub. Code review. Remove excessive condition and fix searching of outer example group --- lib/rubocop/cop/rspec/subject_stub.rb | 14 ++++++++------ spec/rubocop/cop/rspec/subject_stub_spec.rb | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index 0ebbc2918..3b60cf1b4 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/lib/rubocop/cop/rspec/subject_stub.rb @@ -96,17 +96,19 @@ def processed_example_groups def find_all_explicit_subjects(node) node.each_descendant(:block).each_with_object({}) do |child, h| name = subject(child) - if name - h[child.parent.parent] ||= [] - h[child.parent.parent] << name + next unless name + + outer_example_group = child.each_ancestor.find do |a| + example_group?(a) end + + h[outer_example_group] ||= [] + h[outer_example_group] << name end end def find_subject_expectations(node, subject_names = [], &block) - if example_group?(node) && @explicit_subjects[node] - subject_names = @explicit_subjects[node] - end + subject_names = @explicit_subjects[node] if @explicit_subjects[node] expectation_detected = (subject_names + [:subject]).any? do |name| message_expectation?(node, name) diff --git a/spec/rubocop/cop/rspec/subject_stub_spec.rb b/spec/rubocop/cop/rspec/subject_stub_spec.rb index 728d1fc7d..712fb84fe 100644 --- a/spec/rubocop/cop/rspec/subject_stub_spec.rb +++ b/spec/rubocop/cop/rspec/subject_stub_spec.rb @@ -21,7 +21,7 @@ end it 'flags when subject is stubbed and there are several named subjects ' \ - 'in the same example group', :wip do + 'in the same example group' do expect_offense(<<-RUBY) describe Foo do subject(:foo) { described_class.new }