From 00345a883a27616117dbd267d0bccdd52915b004 Mon Sep 17 00:00:00 2001 From: Alex Coomans Date: Mon, 11 Dec 2023 15:43:42 -0600 Subject: [PATCH] Add new RSpec/RepeatedSubjectCall cop --- .rubocop.yml | 2 + CHANGELOG.md | 2 + config/default.yml | 6 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rspec.adoc | 40 ++++++ .../cop/rspec/repeated_subject_call.rb | 121 ++++++++++++++++++ lib/rubocop/cop/rspec_cops.rb | 1 + .../cop/rspec/repeated_subject_call_spec.rb | 91 +++++++++++++ 8 files changed, 264 insertions(+) create mode 100644 lib/rubocop/cop/rspec/repeated_subject_call.rb create mode 100644 spec/rubocop/cop/rspec/repeated_subject_call_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index fec4631b4..d8de390d9 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -179,6 +179,8 @@ RSpec/RedundantAround: Enabled: true RSpec/RedundantPredicateMatcher: Enabled: true +RSpec/RepeatedSubjectCall: + Enabled: true RSpec/SkipBlockInsideExample: Enabled: true RSpec/SortMetadata: diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d4470237..aabe1d5ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Add new `RSpec/RedundantPredicateMatcher` cop. ([@ydah]) - Add support for correcting "it will" (future tense) for `RSpec/ExampleWording`. ([@jdufresne]) +- Add new `RSpec/RepeatedSubjectCall` cop. ([@drcapulet]) ## 2.25.0 (2023-10-27) @@ -836,6 +837,7 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features. [@deivid-rodriguez]: https://github.com/deivid-rodriguez [@dgollahon]: https://github.com/dgollahon [@dmitrytsepelev]: https://github.com/dmitrytsepelev +[@drcapulet]: https://github.com/drcapulet [@drowze]: https://github.com/Drowze [@dswij]: https://github.com/dswij [@dvandersluis]: https://github.com/dvandersluis diff --git a/config/default.yml b/config/default.yml index 0429572b1..7c3852d8f 100644 --- a/config/default.yml +++ b/config/default.yml @@ -807,6 +807,12 @@ RSpec/RepeatedIncludeExample: VersionAdded: '1.44' Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedIncludeExample +RSpec/RepeatedSubjectCall: + Description: Checks for repeated calls to subject missing that it is memoized. + Enabled: pending + VersionAdded: "<>" + Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedSubjectCall + RSpec/ReturnFromStub: Description: Checks for consistent style of stub's return setting. Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index b2878c1a9..356f2b0ec 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -86,6 +86,7 @@ * xref:cops_rspec.adoc#rspecrepeatedexamplegroupbody[RSpec/RepeatedExampleGroupBody] * xref:cops_rspec.adoc#rspecrepeatedexamplegroupdescription[RSpec/RepeatedExampleGroupDescription] * xref:cops_rspec.adoc#rspecrepeatedincludeexample[RSpec/RepeatedIncludeExample] +* xref:cops_rspec.adoc#rspecrepeatedsubjectcall[RSpec/RepeatedSubjectCall] * xref:cops_rspec.adoc#rspecreturnfromstub[RSpec/ReturnFromStub] * xref:cops_rspec.adoc#rspecscatteredlet[RSpec/ScatteredLet] * xref:cops_rspec.adoc#rspecscatteredsetup[RSpec/ScatteredSetup] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 51960efc7..8c956259c 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/docs/modules/ROOT/pages/cops_rspec.adoc @@ -4792,6 +4792,46 @@ end * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedIncludeExample +== RSpec/RepeatedSubjectCall + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| No +| <> +| - +|=== + +Checks for repeated calls to subject missing that it is memoized. + +=== Examples + +[source,ruby] +---- +# bad +it do + subject + expect { subject }.to not_change { A.count } +end + +it do + expect { subject }.to change { A.count } + expect { subject }.to not_change { A.count } +end + +# good +it do + expect { my_method }.to change { A.count } + expect { my_method }.to not_change { A.count } +end +---- + +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RepeatedSubjectCall + == RSpec/ReturnFromStub |=== diff --git a/lib/rubocop/cop/rspec/repeated_subject_call.rb b/lib/rubocop/cop/rspec/repeated_subject_call.rb new file mode 100644 index 000000000..b1a4b16fd --- /dev/null +++ b/lib/rubocop/cop/rspec/repeated_subject_call.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for repeated calls to subject missing that it is memoized. + # + # @example + # # bad + # it do + # subject + # expect { subject }.to not_change { A.count } + # end + # + # it do + # expect { subject }.to change { A.count } + # expect { subject }.to not_change { A.count } + # end + # + # # good + # it do + # expect { my_method }.to change { A.count } + # expect { my_method }.to not_change { A.count } + # end + # + class RepeatedSubjectCall < Base + include TopLevelGroup + + BLOCK_MSG = 'Calls to subject are memoized, this block is misleading' + + # @!method subject?(node) + # Find a named or unnamed subject definition + # + # @example anonymous subject + # subject?(parse('subject { foo }').ast) do |name| + # name # => :subject + # end + # + # @example named subject + # subject?(parse('subject(:thing) { foo }').ast) do |name| + # name # => :thing + # end + # + # @param node [RuboCop::AST::Node] + # + # @yield [Symbol] subject name + def_node_matcher :subject?, <<-PATTERN + (block + (send nil? + { #Subjects.all (sym $_) | $#Subjects.all } + ) args ...) + PATTERN + + # @!method subject_calls(node, method_name) + def_node_search :subject_calls, <<~PATTERN + (send nil? %) + PATTERN + + def on_top_level_group(node) + @subjects_by_node = detect_subjects_in_scope(node) + + detect_offenses_in_block(node) + end + + private + + def detect_offense(example_node, subject_node) + walker = subject_node + + while walker.parent? && walker.parent != example_node.body + walker = walker.parent + + if walker.block_type? && walker.method?(:expect) + add_offense(walker, message: BLOCK_MSG) + return + end + end + end + + def detect_offenses_in_block(node, subject_names = []) + subject_names = [*subject_names, *@subjects_by_node[node]] + + if example?(node) + return detect_offenses_in_example(node, subject_names) + end + + node.each_child_node(:send, :def, :block, :begin) do |child| + detect_offenses_in_block(child, subject_names) + end + end + + def detect_offenses_in_example(node, subject_names) + return unless node.body + + subjects_used = Hash.new(false) + + subject_calls(node.body, Set[*subject_names, :subject]).each do |call| + if subjects_used[call.method_name] + detect_offense(node, call) + else + subjects_used[call.method_name] = true + end + end + end + + def detect_subjects_in_scope(node) + node.each_descendant(:block).with_object({}) do |child, h| + name = subject?(child) + next unless name + + outer_example_group = child.each_ancestor(:block).find do |a| + example_group?(a) + end + + (h[outer_example_group] ||= []) << name + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 70add9ab0..8ab6cf807 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -112,6 +112,7 @@ require_relative 'rspec/repeated_example_group_body' require_relative 'rspec/repeated_example_group_description' require_relative 'rspec/repeated_include_example' +require_relative 'rspec/repeated_subject_call' require_relative 'rspec/return_from_stub' require_relative 'rspec/scattered_let' require_relative 'rspec/scattered_setup' diff --git a/spec/rubocop/cop/rspec/repeated_subject_call_spec.rb b/spec/rubocop/cop/rspec/repeated_subject_call_spec.rb new file mode 100644 index 000000000..4bf24dae5 --- /dev/null +++ b/spec/rubocop/cop/rspec/repeated_subject_call_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::RepeatedSubjectCall do + it 'registers an offense for a singular block' do + expect_offense(<<-RUBY) + RSpec.describe Foo do + it do + subject + expect { subject }.to not_change { Foo.count } + ^^^^^^^^^^^^^^^^^^ Calls to subject are memoized, this block is misleading + end + end + RUBY + end + + it 'registers an offense for repeated blocks' do + expect_offense(<<-RUBY) + RSpec.describe Foo do + it do + expect { subject }.to change { Foo.count } + expect { subject }.to not_change { Foo.count } + ^^^^^^^^^^^^^^^^^^ Calls to subject are memoized, this block is misleading + end + end + RUBY + end + + it 'registers an offense for nested blocks' do + expect_offense(<<-RUBY) + RSpec.describe Foo do + it do + expect(subject.a).to eq(3) + nested_block do + expect { on_shard(:europe) { subject } }.to not_change { Foo.count } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Calls to subject are memoized, this block is misleading + end + end + end + RUBY + end + + it 'registers an offense for custom subjects' do + expect_offense(<<-RUBY) + RSpec.describe Foo do + subject(:bar) { do_something } + + it do + bar + expect { bar }.to not_change { Foo.count } + ^^^^^^^^^^^^^^ Calls to subject are memoized, this block is misleading + end + end + RUBY + end + + it 'registers no offenses for no block' do + expect_no_offenses(<<~RUBY) + RSpec.describe Foo do + it do + expect(subject.a).to eq(3) + expect(subject.b).to eq(4) + end + end + RUBY + end + + it 'registers no offenses for block first' do + expect_no_offenses(<<~RUBY) + RSpec.describe Foo do + it do + expect { subject }.to change { Foo.count } + expect(subject.b).to eq(4) + end + end + RUBY + end + + it 'registers no offenses different subjects' do + expect_no_offenses(<<-RUBY) + RSpec.describe Foo do + subject { do_something_else } + subject(:bar) { do_something } + + it do + expect { bar }.to not_change { Foo.count } + expect { subject }.to not_change { Foo.count } + end + end + RUBY + end +end