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 2 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/PersistenceCalledOutsideExample cop.([@thijsnado][])
pirj marked this conversation as resolved.
Show resolved Hide resolved

## 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
48 changes: 48 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -671,3 +671,51 @@ 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

AllowedBlockNames: []
AllowedReceivers: []
AllowedMethods: []
ForbiddenMethods:
- create
- create!
- create_or_find_by
- create_or_find_by!
- decrement!
- delete_all
- delete_by
- destroy_all
- destroy_by
- find_or_create_by
- find_or_create_by!
- first_or_create
- first_or_create!
- increment!
- save
- save!
- toggle!
- touch_all
- update
- update!
- update_all
- update_attribute
- update_attributes
- update_column
- update_columns
- attributes_for
- attributes_for_list
- build
- build_list
- build_stubbed
- build_stubbed_list
- create_list
- Fabricate
ForbiddenMethodsWithoutArguments:
- delete
- destroy
- destroy!
- touch
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
73 changes: 73 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,76 @@ 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

| AllowedBlockNames
| `[]`
| Array

| AllowedReceivers
| `[]`
| Array

| AllowedMethods
| `[]`
| Array

| ForbiddenMethods
| `create`, `create!`, `create_or_find_by`, `create_or_find_by!`, `decrement!`, `delete_all`, `delete_by`, `destroy_all`, `destroy_by`, `find_or_create_by`, `find_or_create_by!`, `first_or_create`, `first_or_create!`, `increment!`, `save`, `save!`, `toggle!`, `touch_all`, `update`, `update!`, `update_all`, `update_attribute`, `update_attributes`, `update_column`, `update_columns`, `attributes_for`, `attributes_for_list`, `build`, `build_list`, `build_stubbed`, `build_stubbed_list`, `create_list`, `Fabricate`
| Array

| ForbiddenMethodsWithoutArguments
| `delete`, `destroy`, `destroy!`, `touch`
| Array
|===

=== References

* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Rails/PersistenceCalledOutsideExample
128 changes: 128 additions & 0 deletions lib/rubocop/cop/rspec/rails/persistence_called_outside_example.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
# 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.'

# rubocop:disable Metrics/CyclomaticComplexity
# rubocop:disable Metrics/PerceivedComplexity
def on_send(node)
return if inside_example_scope?(node)
return if inside_method_definition?(node)
return if inside_proc_or_lambda?(node)
return if inside_allowed_block?(node)
return if allowed_receiver?(node)
return if allowed_method?(node)
return unless inside_describe_block?(node)
return unless persistent_call?(node)
pirj marked this conversation as resolved.
Show resolved Hide resolved
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
# rubocop:enable Metrics/CyclomaticComplexity
# rubocop:enable Metrics/PerceivedComplexity

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 inside_allowed_block?(node)
node.each_ancestor(:block).any?(&method(:allowed_block?))
end

def allowed_block?(node)
allowed_block_names = (cop_config['AllowedBlockNames'] || [])
allowed_block_names.include?(node.method_name.to_s)
end

def allowed_receiver?(node)
return unless node.receiver.respond_to?(:const_name)

allowed_receivers.include?(node.receiver.const_name)
end

def allowed_receivers
cop_config['AllowedReceivers'] || []
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) ||
forbidden_methods_without_arguments.include?(method_name) &&
!node.arguments?
end

def forbidden_methods
cop_config['ForbiddenMethods'] || []
end

def forbidden_methods_without_arguments
cop_config['ForbiddenMethodsWithoutArguments'] || []
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