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
RSpec/SubjectStub. Refactor and decrease complexity #925
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
20e37fb
RSpec/SubjectStub. Refactor and decrease complexity
andrykonchin 817a77e
RSpec/SubjectStub. Code review. Refactor #find_all_named_subjects method
andrykonchin 7e76bd7
RSpec/SubjectStub. Code review. Handle case with several explicit sub…
andrykonchin 341ae44
RSpec/SubjectStub. Code review. Use #on_top_level_describe instead of…
andrykonchin 358d129
Revert "RSpec/SubjectStub. Code review. Use #on_top_level_describe in…
andrykonchin 956efea
RSpec/SubjectStub. Code review. Remove excessive condition and fix se…
andrykonchin File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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 file contains 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
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.
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.
How big are your specs so this makes a significant difference?
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.
I ran Rubocop on its own source code so the test suite isn't large enough.
Regarding significance... This check avoids repeated analyzing of nested
context
/describe
and I assume (I didn't test this TBH) it decrease the execution time by a factor of 2/3/4/... it depends on how many levels of nesting there are in the spec file.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.
I would prefer to avoid optimizations while sacrificing simplicity if we don't have enough proof that it gets us significant performance benefits. Also, long specs are hard to read and change. I'm not saying that we should deliberately make our tools slow to induce frustration when working with such specs, but primarily aiming to work with that kind of specs is not our aim.
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.
Regarding the performance testing in the wild - you can pick some projects out of this list, I find https://github.com/discourse/discourse/ specifically good.
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.
Checked this optimization on Gitlab source code and got the following results:
Command to reproduce:
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.
You can use
on_top_level describe
and then search for example groups, in order to avoid double lookups. E.g. once you are in an example group, you can break from going to nested example groups,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.
@Darhazer This is a truly amazing proposal.
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.
But then it turns out that RuboCop::RSpec::TopLevelDescribe#on_send which triggers on_top_level_descrive, is one of the slowest methods
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.
Ha, fair enough. Never looked closely at
TopLevelDescribe
. Frankly, I'm having a hard time understanding why it's implemented that way. Should be: "callon_top_level_describe
if method name is:describe
, and there are no ancestors of the type:block
with a method name:describe
". It'd better keep a set of already detected TLDs to quickly return fromon_send
if(node.ancestors & top_level_describes).any?
just like Andrew has implemented in this pull request.Luckily, it doesn't fit our purposes in this pull request anyway, since it's only for
describe
:while we need something like
TopLevelExampleGroup
, probably implemented in a way I suggest above, but focusing on all example group methods, not only:describe
.Would you like to give this idea a shot in the scope of this pull request, or do you feel this improvement that you've done is good on its own already, @andrykonchin?
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.
I would love to optimize
TopLevelDescribe
but think it deserves a separate PR.Currently I am working on optimizing cops from the main
rubocop
gem but will definitely look atrubocop-rspec
after that.P.S.
Switching from manual caching to
#on_top_level_describe
callback decreased profiler samples forSubjectStub
cop from 0.9% to 0% so it isn't as bad as you may think 😉.