Add new cop RSpec/RedundantPending
#2125
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
pirj
reviewed
Oct 6, 2025
There was a problem hiding this comment.
Can't inline comment 🤦
I wonder, how many callbacks we'll get for on_block, and for each we would have to check those two node matchers, skipped_example? and skipped_by_metadata?. Won't this impact the performance?
Wouldn't checking send nil? { :skip :pending }, finding its nearest ancestor (any)block element and making those checks against it reduce the work to be done?
I wonder, can this happen in the wild? Have you by a change ran the cop against some real-world repos?
bquorning
reviewed
Oct 6, 2025
|
|
||
| RSpec/IncludeExamples: {Enabled: true} | ||
| RSpec/LeakyLocalVariable: {Enabled: true} | ||
| RSpec/RedundantPending: {Enabled: false} |
There was a problem hiding this comment.
Suggested change
| RSpec/RedundantPending: {Enabled: false} | |
| RSpec/RedundantPending: {Enabled: true} |
| PATTERN | ||
|
|
||
| # @!method skipped_by_metadata?(node) | ||
| def_node_matcher :skipped_by_metadata?, <<~PATTERN |
There was a problem hiding this comment.
Can we reuse skipped_in_metadata? from the SkipOrPending mixin?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This cop detects redundant
pendingorskipcalls inside examples that are already marked as skipped. When an example is skipped usingxit,xspecify,xexample, or skip/pending metadata, adding apendingorskipcall inside the example body is redundant and creates unnecessary duplication.It's common to see code like this:
In these cases:
xspecify/xitpendingcall inside is redundant and never executedpendingis not displayed (the example is already skipped at method level)This cop flags such redundant calls and suggests either:
pending/skipcall entirely, ORit,specify) if the pending call is actually neededThe cop detects redundancy in the following cases:
Skipped example methods:
xit,xspecify,xexampleSkip/pending metadata:
it 'test', :skip doit 'test', :pending doit 'test', skip: true doit 'test', skip: 'reason' doit 'test', pending: 'reason' doBoth
pendingandskipcalls:pending 'reason'andskip 'reason'The cop does NOT flag these legitimate uses:
Regular examples with pending/skip:
ruby it 'does something' do pending 'not yet implemented' expect(something).to be_truthy endConditional skip/pending:
ruby xit 'does something' do setup_something skip 'setup failed' if setup_failed? # Not first statement expect(something).to be_truthy endMetadata with
falsevalue:ruby it 'does something', skip: false do pending 'conditional pending' expect(something).to be_truthy endChecks only the first statement in the example body
Supports both regular blocks (
do...end) and numblocksHandles 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
Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).If you have created a new cop:
config/default.yml.Enabled: pendinginconfig/default.yml.Enabled: truein.rubocop.yml.VersionAdded: "<<next>>"indefault/config.yml.If you have modified an existing cop's configuration options:
VersionChanged: "<<next>>"inconfig/default.yml.