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