diff --git a/.rubocop.yml b/.rubocop.yml index e8625243b..b823401d6 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -292,4 +292,5 @@ Performance/ZipWithoutBlock: {Enabled: true} # Enable our own pending cops. +RSpec/ChangeWithoutExpect: {Enabled: true} RSpec/IncludeExamples: {Enabled: true} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a6335b37..4def932d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Fix issue when `Style/ContextWording` is configured with a Prefix being interpreted as a boolean, like `on`. ([@sakuro]) - Add new `RSpec/IncludeExamples` cop to enforce using `it_behaves_like` over `include_examples`. ([@dvandersluis]) - Change `RSpec/ScatteredSetup` to allow `around` hooks to be scattered. ([@ydah]) +- Add new `RSpec/ChangeWithoutExpect` cop. ([@ydah]) ## 3.5.0 (2025-02-16) diff --git a/config/default.yml b/config/default.yml index 38fc04255..a7dd86ccc 100644 --- a/config/default.yml +++ b/config/default.yml @@ -199,6 +199,12 @@ RSpec/ChangeByZero: Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ChangeByZero NegatedMatcher: ~ +RSpec/ChangeWithoutExpect: + Description: Checks for `change` matcher usage without an `expect` block. + Enabled: pending + VersionAdded: "<>" + Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ChangeWithoutExpect + RSpec/ClassCheck: Description: Enforces consistent use of `be_a` or `be_kind_of`. StyleGuide: "#is-a-vs-kind-of" diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 08329092b..b8e115f1f 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -13,6 +13,7 @@ * xref:cops_rspec.adoc#rspecbenil[RSpec/BeNil] * xref:cops_rspec.adoc#rspecbeforeafterall[RSpec/BeforeAfterAll] * xref:cops_rspec.adoc#rspecchangebyzero[RSpec/ChangeByZero] +* xref:cops_rspec.adoc#rspecchangewithoutexpect[RSpec/ChangeWithoutExpect] * xref:cops_rspec.adoc#rspecclasscheck[RSpec/ClassCheck] * xref:cops_rspec.adoc#rspeccontainexactly[RSpec/ContainExactly] * xref:cops_rspec.adoc#rspeccontextmethod[RSpec/ContextMethod] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 38c800a42..f75b6a3ab 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/docs/modules/ROOT/pages/cops_rspec.adoc @@ -553,6 +553,75 @@ expect { run } * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ChangeByZero +[#rspecchangewithoutexpect] +== RSpec/ChangeWithoutExpect + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| No +| <> +| - +|=== + +Checks for `change` matcher usage without an `expect` block. + +This cop only considered a matcher if it is chained with +`from`, `by`, `by_at_least`, or `by_at_most`. + +[#examples-rspecchangewithoutexpect] +=== Examples + +[source,ruby] +---- +# bad +it 'changes the count' do + change(Counter, :count).by(1) +end + +# bad +it 'changes the count' do + change(Counter, :count).by_at_least(1) +end + +# bad +it 'changes the count' do + change(Counter, :count).by_at_most(1) +end + +# bad +it 'changes the count' do + change(Counter, :count).from(0) +end + +# bad +it 'changes the count' do + change(Counter, :count).from(0).to(1) +end + +# good - no chained matchers +it 'changes the count' do + change(Counter, :count) +end + +# good - ignore not change matcher +it 'changes the count' do + change(Counter, :count).from(0).by(1) +end + +# good - within expect block +it 'changes the count' do + expect { subject }.to change(Counter, :count).by(1) +end +---- + +[#references-rspecchangewithoutexpect] +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ChangeWithoutExpect + [#rspecclasscheck] == RSpec/ClassCheck diff --git a/lib/rubocop/cop/rspec/change_without_expect.rb b/lib/rubocop/cop/rspec/change_without_expect.rb new file mode 100644 index 000000000..76472497b --- /dev/null +++ b/lib/rubocop/cop/rspec/change_without_expect.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for `change` matcher usage without an `expect` block. + # + # This cop only considered a matcher if it is chained with + # `from`, `by`, `by_at_least`, or `by_at_most`. + # + # @example + # # bad + # it 'changes the count' do + # change(Counter, :count).by(1) + # end + # + # # bad + # it 'changes the count' do + # change(Counter, :count).by_at_least(1) + # end + # + # # bad + # it 'changes the count' do + # change(Counter, :count).by_at_most(1) + # end + # + # # bad + # it 'changes the count' do + # change(Counter, :count).from(0) + # end + # + # # bad + # it 'changes the count' do + # change(Counter, :count).from(0).to(1) + # end + # + # # good - no chained matchers + # it 'changes the count' do + # change(Counter, :count) + # end + # + # # good - ignore not change matcher + # it 'changes the count' do + # change(Counter, :count).from(0).by(1) + # end + # + # # good - within expect block + # it 'changes the count' do + # expect { subject }.to change(Counter, :count).by(1) + # end + # + class ChangeWithoutExpect < RuboCop::Cop::Base + include RangeHelp + + MSG = 'Use `change` matcher within an `expect` block.' + RESTRICT_ON_SEND = [:change].freeze + SINGLE_RESTRICTED_METHODS = %i[by by_at_least by_at_most from].freeze + + # @!method expectation?(node) + def_node_search :expectation?, <<~PATTERN + (send + { + (block (send nil? :expect ...) ...) + (send nil? :expect ...) + } + {:to :not_to} + ...) + PATTERN + + def on_send(node) + return if within_expectation?(node) + return unless offensive_chain?(node) + + add_offense(node) + end + + private + + def offensive_chain?(node) + chain_methods = chain_methods(node) + return false if chain_methods.empty? + + # Case 1: single restricted method (by, by_at_least, by_at_most, from) + if chain_methods.size == 1 && + SINGLE_RESTRICTED_METHODS.include?(chain_methods.first) + return true + end + + # Case 2: exactly from().to() pattern + if chain_methods.size == 2 && + chain_methods.first == :from && + chain_methods.last == :to + return true + end + + false + end + + def chain_methods(node) + methods = [] + chain = node + + while chain.parent.send_type? + chain = chain.parent + methods << chain.method_name + end + + methods + end + + def within_expectation?(node) + node.each_ancestor(:send).any? do |ancestor| + expectation?(ancestor) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index cf2879e11..c13fa6fbd 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -11,6 +11,7 @@ require_relative 'rspec/be_nil' require_relative 'rspec/before_after_all' require_relative 'rspec/change_by_zero' +require_relative 'rspec/change_without_expect' require_relative 'rspec/class_check' require_relative 'rspec/contain_exactly' require_relative 'rspec/context_method' diff --git a/spec/rubocop/cop/rspec/change_without_expect_spec.rb b/spec/rubocop/cop/rspec/change_without_expect_spec.rb new file mode 100644 index 000000000..0da8a2933 --- /dev/null +++ b/spec/rubocop/cop/rspec/change_without_expect_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::ChangeWithoutExpect, :config do + it 'registers an offense for a `change` call with only `by` ' \ + 'without an `expect` block' do + expect_offense(<<~RUBY) + it 'changes the count' do + change(Counter, :count).by(1) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `change` matcher within an `expect` block. + end + RUBY + end + + it 'registers an offense for a `change` call with only `by_at_least` ' \ + 'without an `expect` block' do + expect_offense(<<~RUBY) + it 'changes the count' do + change(Counter, :count).by_at_least(1) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `change` matcher within an `expect` block. + end + RUBY + end + + it 'registers an offense for a `change` call with only `by_at_most` ' \ + 'without an `expect` block' do + expect_offense(<<~RUBY) + it 'changes the count' do + change(Counter, :count).by_at_most(1) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `change` matcher within an `expect` block. + end + RUBY + end + + it 'registers an offense for a `change` call with only `from` ' \ + 'without an `expect` block' do + expect_offense(<<~RUBY) + it 'changes the count' do + change(Counter, :count).from(0) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `change` matcher within an `expect` block. + end + RUBY + end + + it 'registers an offense for a `change` call with exactly `from().to()` ' \ + 'without an `expect` block' do + expect_offense(<<~RUBY) + it 'changes the count' do + change(Counter, :count).from(0).to(1) + ^^^^^^^^^^^^^^^^^^^^^^^ Use `change` matcher within an `expect` block. + end + RUBY + end + + it 'does not register an offense for a `change` call with `from().by()` ' \ + 'without an `expect` block' do + expect_no_offenses(<<~RUBY) + it 'changes the count' do + change(Counter, :count).from(0).by(1) + end + RUBY + end + + it 'does not register an offense for a simple `change` call ' \ + 'without chains' do + expect_no_offenses(<<~RUBY) + it 'changes the count' do + change(Counter, :count) + end + RUBY + end + + it 'does not register an offense for a `change` call with unsupported ' \ + 'chain method' do + expect_no_offenses(<<~RUBY) + it 'changes the count' do + change(Counter, :count).some_other_method(123) + end + RUBY + end + + it 'does not register an offense for `change` with chains within an ' \ + '`expect` block' do + expect_no_offenses(<<~RUBY) + it 'changes the count' do + expect { subject }.to change(Counter, :count).by(1) + end + RUBY + end + + it 'does not register an offense for `change` with from-to chain ' \ + 'within an `expect` block' do + expect_no_offenses(<<~RUBY) + it 'changes the count' do + expect { subject }.to change(Counter, :count).from(0).to(1) + end + RUBY + end + + it 'does not register an offense for `change` with chains ' \ + 'within an `expect` with not_to' do + expect_no_offenses(<<~RUBY) + it 'does not change the count' do + expect { subject }.not_to change(Counter, :count).by(1) + end + RUBY + end +end