Skip to content

Commit

Permalink
Fix bad NestedGroups top level describe behavior
Browse files Browse the repository at this point in the history
The changes in bd88c07 changes the node pattern for NestedGroups from

    (block (send nil [SELECTORS UNION PATTERN] ...) (args) ...)

to

    (block (send _ [SELECTORS UNION PATTERN] ...) ...)

which led to issue #270 being reported. While this did introduce a
change in behavior, it helped reveal an inconsistency in the
implementation. With the change mentioned above, the following code
generated an offense when it didn't in 1.8.0

    RSpec.describe Foo do
      context 'bar' do
        it 'baz' do
          expect(foo).to_not be(1)
          expect(foo).not_to be(1)
        end
      end

      describe '#qux' do
        context 'when quuz' do
          it 'zyxxy' do
            pass
          end
        end
      end
    end

the following nearly equivalent code *did* generate an offense in 1.8
though:

    describe Foo do
      context 'bar' do
        it 'baz' do
          expect(foo).to_not be(1)
          expect(foo).not_to be(1)
        end
      end

      describe '#qux' do
        context 'when quuz' do
          it 'zyxxy' do
            pass
          end
        end
      end
    end

The problem was that the previous node pattern would only start
counting nesting at the first bare `describe` or `context` and would
not count `RSpec.describe`. It was also probably much less efficient
since it was searching through all nodes inside every example group
instead of only searching from top level describes.

This change fixes #270 by starting a search at the top level describe
and properly handle `RSpec.describe` like `describe`.

This change also convinced me that the current default for NestedGroups
is too aggressive. Almost all of the specs that I write use
`RSpec.describe` which means that rubocop-rspec was more lenient for my
tests. The default MaxNesting is now 3 instead of 2.
  • Loading branch information
backus committed Jan 2, 2017
1 parent 9630411 commit 642f286
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 11 deletions.
2 changes: 2 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
require: rubocop-rspec

inherit_from: .rubocop_todo.yml

AllCops:
DisplayCopNames: true
TargetRubyVersion: 2.2
Expand Down
15 changes: 15 additions & 0 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
require: rubocop-rspec

# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2017-01-02 14:45:11 -0800 using RuboCop version 0.44.1.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
# versions of RuboCop, may require this file to be generated again.

# Offense count: 2
# Configuration parameters: MaxNesting.
RSpec/NestedGroups:
Exclude:
- 'spec/project/changelog_spec.rb'
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Master (unreleased)

* Fix unintentional regression change in `NestedGroups` reported in #270. ([@backus][])
* Change `MaxNesting` for `NestedGroups` from 2 to 3. ([@backus][])

## 1.9.0 (2016-12-29)

* Add `MessageSpies` cop for enforcing consistent style of either `expect(...).to have_received` or `expect(...).to receive`, intended as a replacement for the `MessageExpectation` cop. ([@bquorning][])
Expand Down
2 changes: 1 addition & 1 deletion config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ RSpec/NamedSubject:
RSpec/NestedGroups:
Description: Checks for nested example groups.
Enabled: true
MaxNesting: 2
MaxNesting: 3

RSpec/NotToNot:
Description: Checks for consistent method usage for negating expectations.
Expand Down
9 changes: 3 additions & 6 deletions lib/rubocop/cop/rspec/nested_groups.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,8 @@ class NestedGroups < Cop

def_node_search :find_contexts, ExampleGroups::ALL.block_pattern

def on_block(node)
describe, = described_constant(node)
return unless describe

find_nested_contexts(node) do |context|
def on_top_level_describe(node, _)
find_nested_contexts(node.parent) do |context|
add_offense(context.children.first, :expression)
end
end
Expand All @@ -113,7 +110,7 @@ def find_nested_contexts(node, nesting: 1, &block)
end

def max_nesting
Integer(cop_config.fetch('MaxNesting', 2))
Integer(cop_config.fetch('MaxNesting', 3))
end
end
end
Expand Down
7 changes: 3 additions & 4 deletions spec/rubocop/cop/rspec/nested_groups_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
describe MyClass do
context 'when foo' do
context 'when bar' do
^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded
context 'when baz' do
^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded
end
Expand All @@ -17,7 +16,6 @@
context 'when qux' do
context 'when norf' do
^^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded
end
end
end
Expand All @@ -35,14 +33,15 @@ class MyThingy
RUBY
end

context 'when MaxNesting is configured as 3' do
let(:cop_config) { { 'MaxNesting' => '3' } }
context 'when MaxNesting is configured as 2' do
let(:cop_config) { { 'MaxNesting' => '2' } }

it 'only flags third level of nesting' do
expect_violation(<<-RUBY)
describe MyClass do
context 'when foo' do
context 'when bar' do
^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded
context 'when baz' do
^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded
end
Expand Down

0 comments on commit 642f286

Please sign in to comment.