-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
[NestedGroups] Reset nesting to 1 on describe block #207
[NestedGroups] Reset nesting to 1 on describe block #207
Conversation
@Krystosterone Thanks for the PR! I have some questions / concerns about this approach. Why should it reset at every describe block? It seems wrong to me that this cop now, with the default configuration, does not flag this: RSpec.describe MyClass do
describe '1' do
describe '2' do
describe '3' do
describe '4' do
describe '5' do
context '6' do
describe '7' do
end
end
end
end
end
end
end
end Have you considered either of the following options?
RSpec.describe MyClass, '#foobar' do
context 'when boop' do # this is now within the allowed levels of nesting!
end
end
|
@mockdeep would you like to weigh in here? |
I've definitely done the 3 level approach that @Krystosterone mentions before. It makes sense if you're testing all of your methods in the same file: describe MyClass do
describe '#my_one_method' do
context 'when stuff' do
end
end
describe '#my_other_method' do
context 'when something' do
end
end
end I'm not sure what the best approach for |
If people use RSpec.describe Foo do
describe '#bar' do
context 'when doing something' do # does not register a violation
end
end
end RSpec.describe Foo do
describe 'when I do not describe a method' do
context 'when doing something' do # registers a violation
end
end
end RSpec.describe Foo do
describe '#bar' do
describe '#baz' do # registers a violation
end
end
end RSpec.describe Foo do
describe '#bar' do
describe 'invalid usage of describe' do # registers a violation
end
end
end |
In my case, 3 is also usually the accepted nesting, and not only when I am describing pure ruby methods, but also in feature tests, where describe 'Users request' do
describe 'POST sign_in' do
context 'when credentials are valid' do
# ...
end
context 'when credentials are not valid' do
# ...
end
end
end If we have to rely on convention (or style), I think it is safer to just accept two Cheers |
Just wanted to give this a bump; it's not too huge but I do have a few specs that trigger this cop in a way that seems unfair, including one that's almost exactly like @waiting-for-dev's example. For my specs, the best way for it to go would be allowing |
@waiting-for-dev @ChallahuAkbar a separate issue with |
@backus it looks good for me. I also think that 3 should be the default. Thanks! |
Never heard from you @ChallahuAkbar. Would still be interested in whether #271 satisfied your problem. I'm going to close this PR for now since we are trying to keep the open issue count lower |
Whoops, yeah, I'm happy with the way the behavior is now. |
The reason for restricting nesting is presumably to flag complex test setups that indicate a smell in the tested code. [Three levels are allowed](rubocop/rubocop-rspec#207 (comment)) to accomodate this common style: ```ruby describe MyClass do describe '#a_method' do context 'some application state' do # examples end context 'other application state' do # examples end end end ``` My experience has been that a slightly more complex setup in a Rails application (particularly for model and controller specs) is more a fact of life than a code smell. Four levels accommodates code like: ```ruby describe MyController, type: :controller do context 'when logged in as admin' do include_context 'as admin' do ... end context 'when logged is as other_role' do include_context 'as other_role' do ... end end ``` Using my current application as a gague, my sense is that the additional level of nesting addresses the common cases.
PR that introduced the cop: #156
Issue that spawned the idea of the cop: #177
I recently updated to the newest
rubocop-rspec
and ran into a problem of rapidly triggering this cop when having a nesteddescribe
for my methods. For example, it would trigger:which I don't think is a legitimate case.
@mockdeep 's original comment:
While I agree that nesting should be limited, I also think that it should only start counting at the very nearest
describe
block (but I might be mistaken).