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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* Add `IgnoredMetadata` configuration option to `RSpec/DescribeClass`. ([@Rafix02][])
* Fix false positives in `RSpec/EmptyExampleGroup`. ([@pirj][])
* Fix a false positive for `RSpec/EmptyExampleGroup` when example is defined in an `if` branch. ([@koic][])
* Add RSpec/Rails/PersistenceCalledOutsideExample cop.([@thijsnado][])

## 1.43.2 (2020-08-25)

Expand Down Expand Up @@ -565,3 +566,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@biinari]: https://github.com/biinari
[@koic]: https://github.com/koic
[@Rafix02]: https://github.com/Rafix02
[@thijsnado]: https://github.com/thijsnado
22 changes: 22 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@Darhazer Darhazer Sep 28, 2020

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

VersionAdded: '1.43'
Copy link
Member

Choose a reason for hiding this comment

The 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
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,6 @@
=== Department xref:cops_rails.adoc[Rails]

* xref:cops_rails.adoc#railshttpstatus[Rails/HttpStatus]
* xref:cops_rails.adoc#railspersistencecalledoutsideexample[Rails/PersistenceCalledOutsideExample]

// END_COP_LIST
61 changes: 61 additions & 0 deletions docs/modules/ROOT/pages/cops_rails.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,64 @@ it { is_expected.to have_http_status :error }
=== References

* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Rails/HttpStatus

== Rails/PersistenceCalledOutsideExample

|===
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Enabled
| Yes
| No
| 1.43
| -
|===

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

=== Configurable attributes

|===
| Name | Default value | Configurable values

| AllowedMethods
| `[]`
| Array

| 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`
| Array
|===

=== References

* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract those three into lazily_executed? (pretty sure a better for such a method name exists) to appease CodeClimate?

allowed_method?(node)
return unless inside_describe_block?(node)
return unless persistent_call?(node)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 returns would be good for performance. I think persistent_call? and allowed_method? are significantly faster than traversing node ancestors. How about e.g.

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

Copy link
Member

Choose a reason for hiding this comment

The 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'] || []
Copy link
Member

Choose a reason for hiding this comment

The 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
1 change: 1 addition & 0 deletions lib/rubocop/cop/rspec_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
rescue LoadError
# Rails/HttpStatus cannot be loaded if rack/utils is unavailable.
end
require_relative 'rspec/rails/persistence_called_outside_example'

require_relative 'rspec/align_left_let_brace'
require_relative 'rspec/align_right_let_brace'
Expand Down