-
-
Notifications
You must be signed in to change notification settings - Fork 283
Add new cop RSpec/RedundantPending
#2125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
|
|
||
| RSpec/IncludeExamples: {Enabled: true} | ||
| RSpec/LeakyLocalVariable: {Enabled: true} | ||
| RSpec/RedundantPending: {Enabled: false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse skipped_in_metadata? from the SkipOrPending mixin?
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.