-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Add Rails/PersistenceCalledOutsideExample cop. #1016
Changes from all commits
e10c878
fff77ea
7ff1fd8
b2b6827
fabdf5a
a51cbd1
d3a94f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -671,3 +671,25 @@ Rails/HttpStatus: | |
- symbolic | ||
VersionAdded: '1.23' | ||
StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Rails/HttpStatus | ||
|
||
Rails/PersistenceCalledOutsideExample: | ||
Description: Checks for persistence calls outside example blocks. | ||
Enabled: true | ||
VersionAdded: '1.43' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1.44 |
||
AllowedMethods: [] | ||
ForbiddenMethods: | ||
- create | ||
- create! | ||
- build | ||
- create_list | ||
- build_list | ||
- save | ||
- save! | ||
- create_or_find_by | ||
- create_or_find_by! | ||
- find_or_create_by | ||
- find_or_create_by! | ||
- first_or_create | ||
- first_or_create! | ||
- Fabricate | ||
StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Rails/PersistenceCalledOutsideExample |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module RSpec | ||
module Rails | ||
# Checks for persistence calls outside example blocks. | ||
# | ||
# Prevents persistence calls outside examples which usually run | ||
# with some sort of cleanup hook. Saving outside of example causes | ||
# records to leak into other tests. | ||
# | ||
# @ example | ||
# # bad - records created outside example group | ||
# describe User do | ||
# User.create! | ||
# end | ||
# | ||
# describe User do | ||
# user = User.new | ||
# user.save! | ||
# end | ||
# | ||
# # good - records created inside an example group | ||
# describe User do | ||
# it do | ||
# User.create! | ||
# end | ||
# end | ||
# | ||
# describe User do | ||
# it do | ||
# user = User.new | ||
# user.save! | ||
# end | ||
# end | ||
class PersistenceCalledOutsideExample < Base | ||
MSG = 'Persistence called outside of example.' | ||
|
||
def on_send(node) | ||
return if inside_example_scope?(node) || | ||
inside_method_definition?(node) || | ||
inside_proc_or_lambda?(node) || | ||
Comment on lines
+41
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe extract those three into |
||
allowed_method?(node) | ||
return unless inside_describe_block?(node) | ||
return unless persistent_call?(node) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven’t done benchmarks, but it looks like changing the order of these early def on_send(node)
return if allowed_method?
return unless persistent_call?
return unless inside_describe_block?
return if lazily_executed? # as per @pirj's suggestion
add_offense(node)
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Speaking on the performance, it might be better to start from an example group, and check recursively block/send nodes, breaking on valid scopes, instead of triggering the code on each send node. |
||
|
||
add_offense(node) | ||
end | ||
|
||
private | ||
|
||
def inside_example_scope?(node) | ||
node.each_ancestor(:block).any?(&method(:example_scope?)) | ||
end | ||
|
||
def example_scope?(node) | ||
example?(node) || | ||
let?(node) || | ||
hook?(node) || | ||
subject?(node) | ||
end | ||
|
||
def inside_method_definition?(node) | ||
node.each_ancestor(:def).any? | ||
end | ||
|
||
def inside_proc_or_lambda?(node) | ||
node.each_ancestor(:block).any?(&:lambda_or_proc?) | ||
end | ||
|
||
def allowed_method?(node) | ||
allowed_methods.include?(node.method_name.to_s) | ||
end | ||
|
||
def allowed_methods | ||
cop_config['AllowedMethods'] || [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't get why we need both AllowedMethods and ForbiddenMethods |
||
end | ||
|
||
def inside_describe_block?(node) | ||
node.each_ancestor(:block).any?(&method(:spec_group?)) | ||
end | ||
|
||
def persistent_call?(node) | ||
method_name = node.method_name.to_s | ||
|
||
forbidden_methods.include?(method_name) | ||
end | ||
|
||
def forbidden_methods | ||
cop_config['ForbiddenMethods'] || [] | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
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 think we need to add
Safe: false
as well, as it would produce false positives