diff --git a/CHANGELOG.md b/CHANGELOG.md index a79a02da0..ff8aa2618 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - Add `RSpec/RedundantAround` cop. ([@r7kamura]) - Add `RSpec/Rails/TravelAround` cop. ([@r7kamura]) - Add `RSpec/ContainExactly` and `RSpec/MatchArray` cops. ([@faucct]) +- Fix a false positive for `RSpec/PendingWithoutReason` when not inside example and pending/skip with block. ([@ydah], [@pirj]) +- Fix a false positive for `RSpec/PendingWithoutReason` when `skip` is passed a block inside example. ([@ydah], [@pirj]) ## 2.18.1 (2023-01-19) diff --git a/lib/rubocop/cop/rspec/pending_without_reason.rb b/lib/rubocop/cop/rspec/pending_without_reason.rb index 08111a52c..15554ed2b 100644 --- a/lib/rubocop/cop/rspec/pending_without_reason.rb +++ b/lib/rubocop/cop/rspec/pending_without_reason.rb @@ -59,61 +59,90 @@ module RSpec class PendingWithoutReason < Base MSG = 'Give the reason for pending or skip.' - # @!method pending_by_example_method?(node) - def_node_matcher :pending_by_example_method?, block_pattern(<<~PATTERN) - #Examples.pending + # @!method skipped_in_example?(node) + def_node_matcher :skipped_in_example?, <<~PATTERN + { + (send nil? ${#Examples.skipped #Examples.pending}) + (block (send nil? ${#Examples.skipped}) ...) + (numblock (send nil? ${#Examples.skipped}) ...) + } PATTERN - # @!method pending_by_metadata_without_reason?(node) - def_node_matcher :pending_by_metadata_without_reason?, <<~PATTERN - (send #rspec? {#ExampleGroups.all #Examples.all} ... {<(sym :pending) ...> (hash <(pair (sym :pending) true) ...>)}) + # @!method skipped_by_example_method?(node) + def_node_matcher :skipped_by_example_method?, <<~PATTERN + (send nil? ${#Examples.skipped #Examples.pending} ...) PATTERN - # @!method skipped_by_example_method?(node) - def_node_matcher :skipped_by_example_method?, block_pattern(<<~PATTERN) - #Examples.skipped + # @!method metadata_without_reason?(node) + def_node_matcher :metadata_without_reason?, <<~PATTERN + (send #rspec? + {#ExampleGroups.all #Examples.all} ... + { + <(sym ${:pending :skip}) ...> + (hash <(pair (sym ${:pending :skip}) true) ...>) + } + ) PATTERN # @!method skipped_by_example_group_method?(node) - def_node_matcher( - :skipped_by_example_group_method?, - block_pattern(<<~PATTERN) - #ExampleGroups.skipped - PATTERN - ) - - # @!method skipped_by_metadata_without_reason?(node) - def_node_matcher :skipped_by_metadata_without_reason?, <<~PATTERN - (send #rspec? {#ExampleGroups.all #Examples.all} ... {<(sym :skip) ...> (hash <(pair (sym :skip) true) ...>)}) + def_node_matcher :skipped_by_example_group_method?, <<~PATTERN + (send #rspec? ${#ExampleGroups.skipped} ...) PATTERN - # @!method without_reason?(node) - def_node_matcher :without_reason?, <<~PATTERN - (send nil? ${:pending :skip}) + # @!method pending_step_without_reason?(node) + def_node_matcher :pending_step_without_reason?, <<~PATTERN + (send nil? {:skip :pending}) PATTERN def on_send(node) - if pending_without_reason?(node) - add_offense(node, message: 'Give the reason for pending.') - elsif skipped_without_reason?(node) - add_offense(node, message: 'Give the reason for skip.') - elsif without_reason?(node) && example?(node.parent) - add_offense(node, - message: "Give the reason for #{node.method_name}.") + on_pending_by_metadata(node) + return unless (parent = parent_node(node)) + + if example_group?(parent) || block_node_example_group?(node) + on_skipped_by_example_method(node) + on_skipped_by_example_group_method(node) + elsif example?(parent) + on_skipped_by_in_example_method(node, parent) end end private - def pending_without_reason?(node) - pending_by_example_method?(node.block_node) || - pending_by_metadata_without_reason?(node) + def parent_node(node) + node_or_block = node.block_node || node + return unless (parent = node_or_block.parent) + + parent.begin_type? && parent.parent ? parent.parent : parent + end + + def block_node_example_group?(node) + node.block_node && + example_group?(node.block_node) && + explicit_rspec?(node.receiver) + end + + def on_skipped_by_in_example_method(node, _direct_parent) + skipped_in_example?(node) do |pending| + add_offense(node, message: "Give the reason for #{pending}.") + end end - def skipped_without_reason?(node) - skipped_by_example_group_method?(node.block_node) || - skipped_by_example_method?(node.block_node) || - skipped_by_metadata_without_reason?(node) + def on_pending_by_metadata(node) + metadata_without_reason?(node) do |pending| + add_offense(node, message: "Give the reason for #{pending}.") + end + end + + def on_skipped_by_example_method(node) + skipped_by_example_method?(node) do |pending| + add_offense(node, message: "Give the reason for #{pending}.") + end + end + + def on_skipped_by_example_group_method(node) + skipped_by_example_group_method?(node) do + add_offense(node, message: 'Give the reason for skip.') + end end end end diff --git a/lib/rubocop/rspec/language.rb b/lib/rubocop/rspec/language.rb index 48bac6479..625751b48 100644 --- a/lib/rubocop/rspec/language.rb +++ b/lib/rubocop/rspec/language.rb @@ -20,17 +20,29 @@ class << self end # @!method rspec?(node) - def_node_matcher :rspec?, '{(const {nil? cbase} :RSpec) nil?}' + def_node_matcher :rspec?, '{#explicit_rspec? nil?}' + + # @!method explicit_rspec?(node) + def_node_matcher :explicit_rspec?, '(const {nil? cbase} :RSpec)' # @!method example_group?(node) - def_node_matcher :example_group?, block_pattern('#ExampleGroups.all') + def_node_matcher :example_group?, <<~PATTERN + { + #{block_pattern('{#ExampleGroups.all}')} + #{numblock_pattern('{#ExampleGroups.all}')} + } + PATTERN # @!method shared_group?(node) def_node_matcher :shared_group?, block_pattern('#SharedGroups.all') # @!method spec_group?(node) - def_node_matcher :spec_group?, - block_pattern('{#SharedGroups.all #ExampleGroups.all}') + def_node_matcher :spec_group?, <<~PATTERN + { + #{block_pattern('{#SharedGroups.all #ExampleGroups.all}')} + #{numblock_pattern('{#SharedGroups.all #ExampleGroups.all}')} + } + PATTERN # @!method example_group_with_body?(node) def_node_matcher :example_group_with_body?, <<-PATTERN diff --git a/spec/rubocop/cop/rspec/pending_without_reason_spec.rb b/spec/rubocop/cop/rspec/pending_without_reason_spec.rb index ab10160aa..05a9797c1 100644 --- a/spec/rubocop/cop/rspec/pending_without_reason_spec.rb +++ b/spec/rubocop/cop/rspec/pending_without_reason_spec.rb @@ -1,185 +1,321 @@ # frozen_string_literal: true -RSpec.describe RuboCop::Cop::RSpec::PendingWithoutReason do - context 'when pending by pending step with reason' do +RSpec.describe RuboCop::Cop::RSpec::PendingWithoutReason, :ruby27 do + context 'when pending/skip has a reason inside an example' do it 'registers no offense' do - expect_no_offenses(<<~RUBY) - it 'does something' do - pending 'reason' + expect_no_offenses(<<~'RUBY') + RSpec.describe Foo do + it 'does something' do + pending 'reason' + pending "#{reason}" + pending `echo #{reason}` + skip 'reason' + skip "#{reason}" + skip `echo #{reason}` + end end RUBY end end - context 'when skipped by skip step with reason' do + context 'when pending/skip by metadata on example with reason' do it 'registers no offense' do expect_no_offenses(<<~RUBY) - it 'does something' do - skip 'reason' + RSpec.describe Foo do + it 'does something', pending: 'reason' do + end + it 'does something', skip: 'reason' do + end + it 'does something', pending: 'reason' do + _1 + end + it 'does something', skip: 'reason' do + _1 + end end RUBY end end - context 'when pending by metadata on example with reason' do + context 'when pending/skip by metadata on example group with reason' do it 'registers no offense' do expect_no_offenses(<<~RUBY) - it 'does something', pending: 'reason' do + describe 'something', pending: 'reason' do end - RUBY - end - end - - context 'when skipped by metadata on example with reason' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - it 'does something', skip: 'reason' do + describe 'something', skip: 'reason' do end - RUBY - end - end - - context 'when pending by pending step without reason ' \ - 'and not inside example' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - FactoryBot.define do - factory :task do - pending - end + describe 'something', pending: 'reason' do + _1 + end + describe 'something', skip: 'reason' do + _1 end RUBY end end - context 'when skipped by skip step without reason ' \ - 'and not inside example' do + context 'when pending/skip not inside an example' do it 'registers no offense' do expect_no_offenses(<<~RUBY) FactoryBot.define do factory :task do + pending skip + pending { true } + skip { true } end end RUBY end end - context 'when pending with receiver' do + context 'when pending/skip with receiver' do it 'registers no offense' do expect_no_offenses(<<~RUBY) - it 'does something' do - Foo.pending + RSpec.describe Foo do + it 'does something' do + Foo.pending + Foo.skip + end + it 'does something' do + _1.pending + _1.skip + end end RUBY end end - context 'when skip with receiver' do + context 'when pending/skip is an argument' do it 'registers no offense' do expect_no_offenses(<<~RUBY) - it 'does something' do - Foo.skip + RSpec.describe Foo do + it 'does something' do + expect('foo').to eq(pending) + expect('foo').to eq(skip) + foo(bar, pending, skip) + foo(bar, pending: pending, skip: skip) + is_expected.to match_array([foo, pending, skip, bar]) + list = [skip, pending] + foo(list) + end end RUBY end end - context 'when pending is argument of methods' do - it 'registers no offense' do - expect_no_offenses(<<~RUBY) - it 'does something' do - expect('foo').to eq pending - foo(bar, pending) - foo(bar, pending: pending) - is_expected.to match_array [foo, pending, bar] + context 'when pending/skip by example method with block' do + it 'registers offense' do + expect_offense(<<~RUBY) + RSpec.describe Foo do + pending 'does something' do + ^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + end + skip 'does something' do + ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + end + skip 'does something' do + ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + _1 + end end RUBY end end - context 'when skip is argument of methods' do + context 'when pending inside an example with a block' do it 'registers no offense' do + # `ArgumentError` is raised when block is given to `pending` inside + # an example. expect_no_offenses(<<~RUBY) - it 'does something' do - expect('foo').to eq skip - foo(bar, skip) - foo(bar, skip: skip) - is_expected.to match_array [foo, skip, bar] + RSpec.describe Foo do + it 'does something' do + pending 'does something' do + do_something + end + pending 'does something' do + _1.do_something + end + end end RUBY end end - context 'when pending/skip is argument of methods' do + context 'when skip inside an example with a block' do it 'registers no offense' do + # RSpec ignores the block expect_no_offenses(<<~RUBY) - it 'does something' do - list = [skip, pending] - foo(list) + RSpec.describe Foo do + it 'does something' do + skip 'does something' do + do_something + end + skip 'does something' do + _1.do_something + end + end end RUBY end end - context 'when pending by example method' do + context 'when pending/skip by metadata on example without reason' do it 'registers offense' do expect_offense(<<~RUBY) - pending 'does something' do - ^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + RSpec.describe Foo do + it 'does something', :pending do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + end + it 'does something', :skip do + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + end + it 'does something', pending: true do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + end + it 'does something', skip: true do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + end + it 'does something', :pending do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + _1 + end + it 'does something', :skip do + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + _1 + end end RUBY end end - context 'when pending by metadata on example without reason' do + context 'when pending/skip by metadata on example group without reason' do it 'registers offense' do expect_offense(<<~RUBY) - it 'does something', :pending do - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + describe 'something', :pending do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + end + describe 'something', :skip do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + end + describe 'something', pending: true do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + end + describe 'something', skip: true do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. end - RUBY - end - end - - context 'when pending by metadata on example group without reason' do - it 'registers offense' do - expect_offense(<<~RUBY) describe 'something', :pending do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + _1 + end + describe 'something', :skip do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + _1 end RUBY end end - context 'when pending by hash metadata on example group without reason' do + context 'when pending/skip without reason' do it 'registers offense' do expect_offense(<<~RUBY) - describe 'something', pending: true do - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + RSpec.describe Foo do + pending + ^^^^^^^ Give the reason for pending. + skip + ^^^^ Give the reason for skip. + end + RSpec.describe Foo do + context 'when something' do + pending + ^^^^^^^ Give the reason for pending. + end + context 'when something' do + skip + ^^^^ Give the reason for skip. + end + end + RSpec.describe Foo do + it 'does something' do + pending + ^^^^^^^ Give the reason for pending. + end + it 'does something' do + skip + ^^^^ Give the reason for skip. + end end RUBY end end - context 'when pending by pending step without reason' do + context 'when pending/skip without reason with other statement' do it 'registers offense' do expect_offense(<<~RUBY) - it 'does something' do + RSpec.describe Foo do + before { buzz! } pending ^^^^^^^ Give the reason for pending. + skip + ^^^^ Give the reason for skip. + context 'when something' do + let(:foo) { 'bar' } + pending + ^^^^^^^ Give the reason for pending. + skip + ^^^^ Give the reason for skip. + it 'does something' do + do_something + pending + ^^^^^^^ Give the reason for pending. + skip + ^^^^ Give the reason for skip. + do_something + end + end end RUBY end + + context 'with a numblock' do + it 'registers offense' do + expect_offense(<<~RUBY) + RSpec.describe Foo do + pending + ^^^^^^^ Give the reason for pending. + skip + ^^^^ Give the reason for skip. + _1 + context 'when something' do + _1 + pending + ^^^^^^^ Give the reason for pending. + skip + ^^^^ Give the reason for skip. + it 'does something' do + _1 + skip + ^^^^ Give the reason for skip. + pending + ^^^^^^^ Give the reason for pending. + _1 + end + end + end + RUBY + end + end end context 'when pending/skip inside conditional' do it 'registers no offense' do expect_no_offenses(<<~RUBY) - it 'does something' do - pending if RUBY_VERSION < '3.0' - if RUBY_VERSION < '3.0' - skip + RSpec.describe Foo do + it 'does something' do + pending if RUBY_VERSION < '3.0' + if RUBY_VERSION < '3.0' + skip + end end end RUBY @@ -189,49 +325,68 @@ context 'when skipped by example group method' do it 'registers offense' do expect_offense(<<~RUBY) - xdescribe 'something' do - ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + RSpec.describe 'Foo' do + xdescribe 'something' do + ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + end + xdescribe 'something' do + ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + _1.do_something + end end RUBY end end - context 'when skipped by example method' do + context 'when skipped by top-level example group' do it 'registers offense' do expect_offense(<<~RUBY) - skip 'does something' do - ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + RSpec.xdescribe 'something' do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. end - RUBY - end - end - - context 'when skipped by metadata on example without reason' do - it 'registers offense' do - expect_offense(<<~RUBY) - it 'does something', skip: true do - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + RSpec.xdescribe 'something' do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + _1.do_something end RUBY end end - context 'when skipped by metadata on example group without reason' do + context 'when skipped by example method' do it 'registers offense' do expect_offense(<<~RUBY) - describe 'something', skip: true do - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + RSpec.describe 'Foo' do + skip 'does something' do + ^^^^^^^^^^^^^^^^^^^^^ Give the reason for skip. + end + xit 'something' do + ^^^^^^^^^^^^^^^ Give the reason for xit. + end + xit 'something' do + ^^^^^^^^^^^^^^^ Give the reason for xit. + _1.do_something + end + pending 'does something' do + ^^^^^^^^^^^^^^^^^^^^^^^^ Give the reason for pending. + _1 + end end RUBY end end - context 'when skipped by skip step without reason' do - it 'registers offense' do - expect_offense(<<~RUBY) - it 'does something' do - skip - ^^^^ Give the reason for skip. + context 'when skipped inside an example' do + it 'registers no offense' do + expect_no_offenses(<<~RUBY) + RSpec.describe Foo do + it 'does something' do + skip 'does something' do + do_something + end + skip 'does something' do + _1 + end + end end RUBY end