From 0b62b7621c12fb0d543b146df11815070a607bfe Mon Sep 17 00:00:00 2001 From: ydah Date: Sun, 5 Oct 2025 16:49:26 +0900 Subject: [PATCH] Add new cop `RSpec/RedundantPending` This cop detects redundant `pending` or `skip` calls inside examples that are already marked as skipped. When an example is skipped using `xit`, `xspecify`, `xexample`, or skip/pending metadata, adding a `pending` or `skip` call inside the example body is redundant and creates unnecessary duplication. It's common to see code like this: ```ruby xspecify do pending 'Need to upgrade to the latest HTTP gem version' expect(pinger.call).to eq(204) end xit 'answers success status' do pending 'Need to upgrade HTTP gem before this will work again' expect(pinger.call).to eq(200) end ``` In these cases: - The example is already skipped via `xspecify`/`xit` - The `pending` call inside is redundant and never executed - This creates confusion about where the skip/pending is actually applied - The reason message in `pending` is not displayed (the example is already skipped at method level) This cop flags such redundant calls and suggests either: 1. Remove the redundant `pending`/`skip` call entirely, OR 2. Use a regular example method (`it`, `specify`) if the pending call is actually needed The cop detects redundancy in the following cases: 1. Skipped example methods: - `xit`, `xspecify`, `xexample` 2. Skip/pending metadata: - Symbol metadata: `it 'test', :skip do` - Symbol metadata: `it 'test', :pending do` - Hash metadata: `it 'test', skip: true do` - Hash metadata: `it 'test', skip: 'reason' do` - Hash metadata: `it 'test', pending: 'reason' do` 3. Both `pending` and `skip` calls: - Detects both `pending 'reason'` and `skip 'reason'` The cop does NOT flag these legitimate uses: 1. Regular examples with pending/skip: ```ruby it 'does something' do pending 'not yet implemented' expect(something).to be_truthy end ``` 2. Conditional skip/pending: ```ruby xit 'does something' do setup_something skip 'setup failed' if setup_failed? # Not first statement expect(something).to be_truthy end ``` 3. Metadata with `false` value: ```ruby it 'does something', skip: false do pending 'conditional pending' expect(something).to be_truthy end ``` - Checks only the first statement in the example body - Supports both regular blocks (`do...end`) and numblocks - Handles examples with single or multiple statements - Handles one-liner syntax: `xit('test') { pending 'reason' }` - Reduces confusion about where examples are actually skipped - Eliminates redundant code - Makes skip/pending reasons visible (use metadata instead of body) - Improves test suite maintainability --- .rubocop.yml | 1 + CHANGELOG.md | 1 + config/default.yml | 6 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_rspec.adoc | 70 ++++++ lib/rubocop/cop/rspec/redundant_pending.rb | 106 ++++++++ lib/rubocop/cop/rspec_cops.rb | 1 + .../cop/rspec/redundant_pending_spec.rb | 230 ++++++++++++++++++ 8 files changed, 416 insertions(+) create mode 100644 lib/rubocop/cop/rspec/redundant_pending.rb create mode 100644 spec/rubocop/cop/rspec/redundant_pending_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 4362ff878..3b3f93099 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -294,3 +294,4 @@ Performance/ZipWithoutBlock: {Enabled: true} RSpec/IncludeExamples: {Enabled: true} RSpec/LeakyLocalVariable: {Enabled: true} +RSpec/RedundantPending: {Enabled: false} diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e0528474..5cac34fb6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Master (Unreleased) - Add new cop `RSpec/LeakyLocalVariable`. ([@lovro-bikic]) +- Add new cop `RSpec/RedundantPending`. ([@ydah]) - Bump RuboCop requirement to +1.81. ([@ydah]) - Fix a false positive for `RSpec/LetSetup` when `let!` used in outer scope. ([@ydah]) diff --git a/config/default.yml b/config/default.yml index f8678689c..cccb34aa2 100644 --- a/config/default.yml +++ b/config/default.yml @@ -815,6 +815,12 @@ RSpec/RedundantAround: VersionAdded: '2.19' Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantAround +RSpec/RedundantPending: + Description: Checks for redundant `pending` or `skip` inside skipped examples. + Enabled: pending + VersionAdded: "<>" + Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantPending + RSpec/RedundantPredicateMatcher: Description: Checks for redundant predicate matcher. Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index fb43db294..15062ef92 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -85,6 +85,7 @@ * xref:cops_rspec.adoc#rspecreceivemessages[RSpec/ReceiveMessages] * xref:cops_rspec.adoc#rspecreceivenever[RSpec/ReceiveNever] * xref:cops_rspec.adoc#rspecredundantaround[RSpec/RedundantAround] +* xref:cops_rspec.adoc#rspecredundantpending[RSpec/RedundantPending] * xref:cops_rspec.adoc#rspecredundantpredicatematcher[RSpec/RedundantPredicateMatcher] * xref:cops_rspec.adoc#rspecremoveconst[RSpec/RemoveConst] * xref:cops_rspec.adoc#rspecrepeateddescription[RSpec/RepeatedDescription] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index ee503bb65..d92c8ae20 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/docs/modules/ROOT/pages/cops_rspec.adoc @@ -5156,6 +5156,76 @@ end * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantAround +[#rspecredundantpending] +== RSpec/RedundantPending + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| No +| <> +| - +|=== + +Checks for redundant `pending` or `skip` inside skipped examples. + +When an example is already skipped using `xit`, `xspecify`, `xexample`, +or `skip` metadata, adding `pending` or `skip` inside the example body +is redundant. + +[#examples-rspecredundantpending] +=== Examples + +[source,ruby] +---- +# bad +xit 'does something' do + pending 'not yet implemented' + expect(something).to be_truthy +end + +# bad +xspecify do + pending 'not yet implemented' + expect(something).to be_truthy +end + +# bad +it 'does something', :skip do + pending 'not yet implemented' + expect(something).to be_truthy +end + +# bad +it 'does something', skip: true do + skip 'not yet implemented' + expect(something).to be_truthy +end + +# good +xit 'does something' do + expect(something).to be_truthy +end + +# good +it 'does something', skip: 'not yet implemented' do + expect(something).to be_truthy +end + +# good +it 'does something' do + pending 'not yet implemented' + expect(something).to be_truthy +end +---- + +[#references-rspecredundantpending] +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantPending + [#rspecredundantpredicatematcher] == RSpec/RedundantPredicateMatcher diff --git a/lib/rubocop/cop/rspec/redundant_pending.rb b/lib/rubocop/cop/rspec/redundant_pending.rb new file mode 100644 index 000000000..fcf3b8232 --- /dev/null +++ b/lib/rubocop/cop/rspec/redundant_pending.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for redundant `pending` or `skip` inside skipped examples. + # + # When an example is already skipped using `xit`, `xspecify`, `xexample`, + # or `skip` metadata, adding `pending` or `skip` inside the example body + # is redundant. + # + # @example + # # bad + # xit 'does something' do + # pending 'not yet implemented' + # expect(something).to be_truthy + # end + # + # # bad + # xspecify do + # pending 'not yet implemented' + # expect(something).to be_truthy + # end + # + # # bad + # it 'does something', :skip do + # pending 'not yet implemented' + # expect(something).to be_truthy + # end + # + # # bad + # it 'does something', skip: true do + # skip 'not yet implemented' + # expect(something).to be_truthy + # end + # + # # good + # xit 'does something' do + # expect(something).to be_truthy + # end + # + # # good + # it 'does something', skip: 'not yet implemented' do + # expect(something).to be_truthy + # end + # + # # good + # it 'does something' do + # pending 'not yet implemented' + # expect(something).to be_truthy + # end + # + class RedundantPending < Base + MSG = 'Redundant `%s` inside already skipped example. ' \ + 'Remove `%s` or use regular example method.' + + # @!method skipped_example?(node) + def_node_matcher :skipped_example?, <<~PATTERN + { + (any_block (send _ #Examples.skipped ...) ...) + } + PATTERN + + # @!method skipped_by_metadata?(node) + def_node_matcher :skipped_by_metadata?, <<~PATTERN + { + (any_block (send _ #Examples.all ... <(sym {:skip :pending}) ...>) ...) + (any_block (send _ #Examples.all ... (hash <(pair (sym {:skip :pending}) !false) ...>)) ...) + } + PATTERN + + # @!method pending_or_skip_call?(node) + def_node_matcher :pending_or_skip_call?, <<~PATTERN + (send nil? ${:pending :skip} ...) + PATTERN + + def on_block(node) + check_example(node) + end + alias on_numblock on_block + + private + + def check_example(node) + return unless skipped_example?(node) || skipped_by_metadata?(node) + return unless node.body + + find_pending_or_skip(node.body) do |method_name| + message = format(MSG, method: method_name) + add_offense(node.body, message: message) + end + end + + def find_pending_or_skip(body, &block) + first_statement = if body.begin_type? + body.children.first + else + body + end + + pending_or_skip_call?(first_statement, &block) + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index d64ca9e51..db9c2437b 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -83,6 +83,7 @@ require_relative 'rspec/receive_messages' require_relative 'rspec/receive_never' require_relative 'rspec/redundant_around' +require_relative 'rspec/redundant_pending' require_relative 'rspec/redundant_predicate_matcher' require_relative 'rspec/remove_const' require_relative 'rspec/repeated_description' diff --git a/spec/rubocop/cop/rspec/redundant_pending_spec.rb b/spec/rubocop/cop/rspec/redundant_pending_spec.rb new file mode 100644 index 000000000..2de7d47a3 --- /dev/null +++ b/spec/rubocop/cop/rspec/redundant_pending_spec.rb @@ -0,0 +1,230 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::RedundantPending do + it 'registers an offense for pending inside xit' do + expect_offense(<<~RUBY) + xit 'does something' do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for skip inside xit' do + expect_offense(<<~RUBY) + xit 'does something' do + skip 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `skip` inside already skipped example. Remove `skip` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending inside xspecify' do + expect_offense(<<~RUBY) + xspecify do + pending 'Need to upgrade to the latest HTTP gem version' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(pinger.call).to eq(204) + end + RUBY + end + + it 'registers an offense for pending inside xexample' do + expect_offense(<<~RUBY) + xexample 'does something' do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending inside example with :skip metadata' do + expect_offense(<<~RUBY) + it 'does something', :skip do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for skip inside example with :skip metadata' do + expect_offense(<<~RUBY) + it 'does something', :skip do + skip 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `skip` inside already skipped example. Remove `skip` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending inside example with :pending metadata' do + expect_offense(<<~RUBY) + it 'does something', :pending do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending inside example with ' \ + 'skip: true metadata' do + expect_offense(<<~RUBY) + it 'does something', skip: true do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for skip inside example with ' \ + 'skip: "reason" metadata' do + expect_offense(<<~RUBY) + it 'does something', skip: 'not ready' do + skip 'duplicate reason' + ^^^^^^^^^^^^^^^^^^^^^^^ Redundant `skip` inside already skipped example. Remove `skip` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending inside example with ' \ + 'pending: true metadata' do + expect_offense(<<~RUBY) + it 'does something', pending: true do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + end + RUBY + end + + it 'does not register an offense for pending inside regular it' do + expect_no_offenses(<<~RUBY) + it 'does something' do + pending 'not yet implemented' + expect(something).to be_truthy + end + RUBY + end + + it 'does not register an offense for skip inside regular it' do + expect_no_offenses(<<~RUBY) + it 'does something' do + skip 'not yet implemented' + expect(something).to be_truthy + end + RUBY + end + + it 'does not register an offense for pending inside regular specify' do + expect_no_offenses(<<~RUBY) + specify do + pending 'not yet implemented' + expect(something).to be_truthy + end + RUBY + end + + it 'does not register an offense when skip/pending is not ' \ + 'the first statement' do + expect_no_offenses(<<~RUBY) + xit 'does something' do + setup_something + pending 'not yet implemented' + expect(something).to be_truthy + end + RUBY + end + + it 'does not register an offense for example with skip: false metadata' do + expect_no_offenses(<<~RUBY) + it 'does something', skip: false do + pending 'not yet implemented' + expect(something).to be_truthy + end + RUBY + end + + it 'does not register an offense for example with pending: false metadata' do + expect_no_offenses(<<~RUBY) + it 'does something', pending: false do + skip 'not yet implemented' + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending in numblock with xit' do + expect_offense(<<~RUBY) + xit do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(_1).to be_truthy + end + RUBY + end + + it 'registers an offense when example body has multiple statements' do + expect_offense(<<~RUBY) + xit 'does something' do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + expect(something).to be_truthy + expect(another).to be_falsey + end + RUBY + end + + it 'does not register an offense for xit without body' do + expect_no_offenses(<<~RUBY) + xit 'does something' + RUBY + end + + it 'does not register an offense for xit with empty body' do + expect_no_offenses(<<~RUBY) + xit 'does something' do + end + RUBY + end + + it 'registers an offense when body is a single pending statement' do + expect_offense(<<~RUBY) + xit 'does something' do + pending 'not yet implemented' + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + end + RUBY + end + + it 'registers an offense when body is a single skip statement' do + expect_offense(<<~RUBY) + xspecify do + skip 'not ready yet' + ^^^^^^^^^^^^^^^^^^^^ Redundant `skip` inside already skipped example. Remove `skip` or use regular example method. + end + RUBY + end + + it 'does not register an offense when body is a single non-skip/pending ' \ + 'statement' do + expect_no_offenses(<<~RUBY) + xit 'does something' do + expect(something).to be_truthy + end + RUBY + end + + it 'registers an offense for pending in one-liner example body' do + expect_offense(<<~RUBY) + xit('does something') { pending 'not ready' } + ^^^^^^^^^^^^^^^^^^^ Redundant `pending` inside already skipped example. Remove `pending` or use regular example method. + RUBY + end +end