Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NestedGroups counts levels differently since version 1.9.0 #270

Closed
mvz opened this issue Jan 2, 2017 · 8 comments · Fixed by #271
Closed

NestedGroups counts levels differently since version 1.9.0 #270

mvz opened this issue Jan 2, 2017 · 8 comments · Fixed by #271

Comments

@mvz
Copy link
Contributor

mvz commented Jan 2, 2017

With version 1.8.0, the following does not cause a NestedGroups offense:

RSpec.describe Foo do
  context 'bar' do
    it 'baz' do
      pass
    end
  end

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

With 1.9.0, it seems the nested is counted differently, since it registers an offense, even thought the MaxNesting parameter default has stayed the same, at 2.

This seems to be a bug or regressions, since this changes is not marked in the change log.
@backus
Copy link
Collaborator

backus commented Jan 2, 2017

Thanks for the report @mvz I think you're right I'll take a look

@backus
Copy link
Collaborator

backus commented Jan 2, 2017

@mvz using your example code I bisected this regression down to bd88c07. I'll see if I can fix and push a patch release

backus added a commit that referenced this issue Jan 2, 2017
@backus
Fix bad NestedGroups top level describe behavior
cdf70b7 
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.
backus added a commit that referenced this issue Jan 2, 2017
@backus
Fix bad NestedGroups top level describe behavior
642f286 
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.
@backus backus mentioned this issue Jan 2, 2017
Merged
backus added a commit that referenced this issue Jan 2, 2017
@backus
Fix bad NestedGroups top level describe behavior
d2daef9 
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.
@backus backus mentioned this issue Jan 2, 2017
Closed
backus added a commit that referenced this issue Jan 2, 2017
@backus
Fix bad NestedGroups top level describe behavior
73377d6 
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.
@mvz
Copy link
Contributor Author

mvz commented Jan 3, 2017

Thanks, @backus!

@backus
Copy link
Collaborator

backus commented Jan 3, 2017

@mvz could you try out #274 before I release it?

@mvz
Copy link
Contributor Author

mvz commented Jan 3, 2017

@backus sure, hold on.

@backus
Copy link
Collaborator

backus commented Jan 3, 2017

Thanks :)

@mvz
Copy link
Contributor Author

mvz commented Jan 3, 2017

Looks good, @backus.

@backus
Copy link
Collaborator

backus commented Jan 3, 2017

Thanks @mvz

markburns pushed a commit to markburns/rubocop-rails-ddd that referenced this issue Nov 7, 2017
@backus
Fix bad NestedGroups top level describe behavior
a8ab721 
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 rubocop#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 rubocop#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix bad NestedGroups top level describe behavior rubocop/rubocop-rspec
2 participants
@mvz @backus