Add Rails/PersistenceCalledOutsideExample cop.

[thijsnado]

Prevents persistence calls from being called outside example or hook. https://github.com/rubocop-hq/rubocop-rspec/issues/991

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded in default/config.yml to the next minor version.

[pirj]

Looks good.
I've run it against https://github.com/pirj/real-world-rspec, let me quickly analyze the offences (as quickly as it's possible with ~9k offences).

[pirj]

False positives:

1. Factories

FactoryBot.define do
  factory :project do
    submitted_by { create(:user) }
  end

2. Methods

describe Organisation, type: :model do
  def setup_contribution_data
    most_pr_user = create(:user)

3. String/Array methods

RSpec.describe 'I18n' do
  def test_translations
    reference = locale_keys[locales.delete("en")]

4. Default method arguments (executed in the example scope)

describe Admin::LogEntriesController, type: :controller do
  describe "POST create" do
    def post_create(action: "create", logeable: create(:customer))

5. Non-spec files

# real-world-rspec/administrate/spec/example_app/config/initializers/disable_xml_params.rb
if Rails::VERSION::MAJOR < 5
  ActionDispatch::ParamsParser::DEFAULT_PARSERS.delete(Mime::XML)
end

6. Usage of forbidden instance methods on classes

    FileUtils.touch "README"

7. Usage of forbidden instance methods without an explicit receiver

      touch path/"bin"/file

8. RSpec aliases and let wrappers
   This is going to be resolved in RSpec aliases runtime matchers #956 that allows users to describe their RSpec DSL, but only in 2.0.
   2.0 will hopefully happen soon, but if we release the cop in 1.44 there might be an unpleasant period for users being forced to disable the cop before they are able to tune it to avoid false positives.

describe "users/grades" do
  context "as a teacher" do
    let_once(:course) { Course.create!(workflow_state: "available") }

9. Method calls with arguments where Active Record won't typically take an argument

          yml_config[:user] = yml_config.delete :username

10. Calls from globally defined hooks

# real-world-rspec/spree/frontend/spec/spec_helper.rb
  config.before do
    create(:taxon, permalink: 'bestsellers')
  end

I've only scratched the surface, and have not found a single valid offence.

Please don't get me wrong. I think this will make a very useful cop. But we'll have to relax it a bit to avoid false positives.

WDYT of:

• reducing its scope to those forbidden calls directly under example groups (and inside iterators directly under example groups)
• reduce method checks to:
• • create/create!/build/... for class receivers
• • create/build/build_stubbed/attributes_for/create_list/build_list/build_stubbed_list for an implicit receiver (and FactoryBot/FactoryGirl const)
• • save!/save for local var/instance var receivers

Also, first_or_create/first_or_create! is missed among forbidden methods.

https://github.com/pirj/real-world-rspec is a nice sandbox to test the cop, I suggest using it extensively to reduce false positives to the minimum while still keeping real offences detectable.

[thijsnado]

Okay, so for "reducing its scope to those forbidden calls directly under example groups (and inside iterators directly under example groups)", right now I detect whether something is in an allowed block. Would your suggestion be to assume all blocks are allowed unless specifically forbidden, such as iterator blocks? So something like let_once would be allowed but 5.times or each would be explicitly forbidden? I figure I'd forbid all built in ruby iterator methods for Enumerable and Array. I'd have to allow things like:

describe User do
  before do
    5.times do
      User.create!
    end
  end
end

but not

describe User do
  5.times do
    User.create!
  end
end

Another alternative would be to have an AllowedBlockNames config for custom names but that seems like it would be a duplication of #956. I think I'm going to go with the ForbiddenBlockNames approach.

[thijsnado]

Another alternative would be to make assumptions about receiver. Other than the initial RSpec.describe, rspec blocks usually have a nil receiver. We could forbid calls inside describe blocks in same way LeakyConstantDeclaration does but allow any calls where the ancestor is a receiverless block:

# bad
describe User
  5.times do # has a receiver
    User.create!
  end
end

# good
describe User
  my_custom_before_block # no receiver
    User.create!
  end
end

The downsides of this approach would be false negatives for ExampleGroups and SharedExamples but I think that is consistent with behavior precedented by LeakyConstantDeclaration and would be fixed when #956 is merged with no need to modify this cop.

[pirj]

forbidden calls directly under example groups (and inside iterators directly under example groups)

Small correction, iterators and conditionals.

RSpec.describe do
  if pre_create
    user = create(:user)
  end
  ...
end

[pirj]

forbidden calls directly under example groups (and inside iterators directly under example groups)

I'd have to allow things like:

Not sure if the detecting the receiver would help.
I had the following in my mind:

        def on_send(node)
          return if example_group?(node.ancestors(:block).first)

The above won't skip iterators and conditional blocks, but I guess it's not too hard to add that support.

[thijsnado]

I think you meant unless instead of if but essentially, find the first ancestor block which is not an iterator or conditional block and if it is part of example group (and is a persistence call) flag it as a violation. Otherwise treat it as allowed since we don't know that block is an alias or not.

[thijsnado]

Here is an example of what I got so far:

# frozen_string_literal: true

module RuboCop
  module Cop
    module RSpec
      class PersistenceCalledOutsideExample < Base
        MSG = 'Persistence called outside of example.'

        def on_send(node)
          return unless persistent_call?(node)
          return unless inside_describe_block?(node)
          return if inside_method_definition?(node)
          return if allowed_receiver?(node)
          return if allowed_method?(node)

          add_offense(node)
        end

        private

        def persistent_call?(node)
          forbidden_methods.include?(node.method_name.to_s)
        end

        def inside_describe_block?(node)
          spec_group?(node.each_ancestor(:block).find(&method(:not_iterator_or_conditional_block?)))
        end

        def not_iterator_or_conditional_block?(node)
          !%i(each).include?(node.method_name)
        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'] || []
        end

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

        def inside_example_scope?(node)
          node.each_ancestor(:block).any?(&method(:example_scope?))
        end

        def inside_method_definition?(node)
          node.each_ancestor(:def).any?
        end
      end
    end
  end
end

This seems to work but will have to get a long list of method names. I'll start with methods that accept blocks on Enumerable, Enumerator, Array, Hash, String and IO and make it configurable for people where the default list doesn't work. In version 2.0 we can deprecate this list and explicitly check for Language settings to see if in an example scoped block.

[pirj]

return if example_group?(node.ancestors(:block).first)

I think you meant unless instead of if but essentially

Exactly.

This seems to work but will have to get a long list of method names. I'll start with methods that accept blocks on Enumerable, Enumerator, Array, Hash, String and IO

There's an indefinite list actually, and matching their combinations with with_object, with_index, etc and chains is a hard task.
We can approach it from the other side, and consider a block illegal for those definitions if it's not an RSpec DSL block. Because ...

we don't know that block is an alias or not.

we actually have knowledge about that. A pull request that makes RSpec DSL alias configuration configurable on RuboCop RSpec side is about to be merged. I believe you can already rely on that. See #956 for more info.

[pirj]

I would probably call this cop Rails/PersistenceCalledOutsideExample.
RuboCop soon introduces nested departments, so it will end up being an RSpec/Rails/PersistenceCalledOutsideExample.

[thijsnado]

I'm temporarily changing this PR to a draft while I make these change. I'll ping you when I'm ready for re-review.

[thijsnado]

@pirj I've made the following changes:

• Only flag errors inside describe blocks.
• Allow configuration of AllowedBlockNames as a workaround for those who use custom rspec blocks.
• Flag some methods as taking no arguments (touch, delete, etc.). This was inspired by https://github.com/rubocop-hq/rubocop-rails/blob/master/lib/rubocop/cop/rails/skips_model_validations.rb#L41 approach.
• Don't flag errors within procs or lambdas.

With the following AllowedBlockNames I was able to run against https://github.com/pirj/real-world-rspec without any false positives:

- let_once
- let_it_be
- subject_once
- fab!
- before_all
- given
- given!
- background
- scenario
- where

Trying to restrict calls based on receiver didn't seem necessary after these changes since there were no more false positives and seems like doing so may cause false negatives, for example: klass.create! would look like a local variable but point to an AR instance.

I kept the default AllowedBlockNames empty. Do you think we should add any of the ones to the default values? I also am getting errors for code-climate. I could probably bust on_send into two parts exempt_from_violation? (is allowed method, block, or reciever) and has_violation? (in describe block and has persistence call) but seems like that would be appeasing the linter in a way that does not necessarily make the code more readable.

[pirj]

Sorry, it's taking me longer than usual to review. Hope to get to it soon.

[pirj]

Another attempt to digest and classify 47880 files inspected, 6580 offenses detected from real-world-rspec.

1. (no need to fix, but delays this PR up until rubocop-rspec 2.0-beta) Custom setup blocks are (correctly) ignored. This will be fixed with RSpec aliases runtime matchers #956 by allowing to configure custom RSpec DSL methods. Examples:

RSpec.feature "Quantity Promotions", js: true do
  background do
    create(:store)

    let_it_be(:project) { create(:project, :repository) }

      before_all do
        design1.update!(relative_position: 2)

      fab!(:badge) { Fabricate(:badge, name: 'Minion') }

    let_once(:course) { Course.create!(workflow_state: "available") }

  given(:variant) { create :variant }

2. Unable to configure with AllowedReceivers:

    given(:action) do
      Spree::Promotion::Actions::CreateQuantityAdjustments.create(

AllowedReceivers don't support namespaced constants, does it?

            allowed_receivers.include?(node.receiver.const_name)

That's basically all of it.

In general, I still think that:

• create/build/create_list/build_list with an implicit or FactoryBot/FactoryGirl receiver
• save!/save with instance var receiver
• create[!]/create_or_find_by[!]/find_or_create_by[!]/first_or_create[!] with a constant receiver
• Fabricate

is sufficient to detect creation of records before test suite initialization. I can't think of a situation where there's an attempt to delete/update/touch/increment records that we should care of. Do you?

build_stubbed can be a problem with factory hooks, if something is created in an after(:stub), but that doesn't sound too realistic. I see this often in after(:create), but not in after(:stub).

[pirj]

Sorry, I've missed your comment about the recent changes you've made.

for example: klass.create! would look like a local variable but point to an AR instance.

Agree.
Also, on my current project, FactoryBot is aliased to Oleg, so it's Oleg.create 🤷

With the following AllowedBlockNames I was able to run against https://github.com/pirj/real-world-rspec without any false positives

Checking again 👍

[pirj]

No false positives. But no real offences either :D
With that much information on hand we can only theoretize.

This cop is ~90 LoC. It's a maintenance cost. I have little doubt that it's perfect, most probably bugs of some kind will appear.

I would rather prefer to extend its functionality to cover more cases than to reduce its functionality to avoid false positives. Method call detection except create*/build* seems unnecessary to me.

Customization option is nice, but with #956 coming up it would better use the new approach, that would also simplify the cop.

I'd love to hear what @bquorning and @Darhazer think.

[bquorning]

I have mixed feelings about cops that check method names against a list of “disallowed methods”. Since we can only do static analysis, we rarely know if the receiver is an ActiveRecord class or instance, or whether I’m using the Factory or Builder patterns and my objects implement #create or #build without persisting.

I wonder if this entire class of problems (AR related ones, not mutation of global state in general unfortunately) could be solved by disallowing database access during spec setup? Or having separate rw and read-only connections, and only allowing read-only during spec setup? (I guess this is more a question for @pirj and rspec-rails)

[pirj]

I wonder if this entire class of problems could be solved by disallowing database access during spec setup? Or having separate rw and read-only connections, and only allowing read-only during spec setup?

rspec-rails is a wrapper around Rails, so in Minitest e could have the exact same problems:

class PostsTest < ApplicationTestCase
  author = Author.create!(...) 

  test "..." do
    # ...
  end
end

and since establishing the connection (and with Rails 6 connections can differ per-model), it gets outside of the responsibility of RSpec Rails.

Since establishing the connection happens somewhere deep in Rails internals, and is used to e.g. get the list of the attributes, and this happens lazily, I can't think of a way to intentionnally breaking it for a Author.create!-alike statements in the runtime.

So handling this nasty case in static analysis, even under the risk of false positives, is the only option I can think of.

[thijsnado]

So in terms of things we want to change:

1. Reduce amount of disallowed methods:

Reduce methods to: create, build, create_list, build_list for FactoryBot
save[!], update[!], create[!], create_or_find_by[!], find_or_create_by[!], first_or_create[!] to handle built in rails methods
And finally Fabricate.

I included update[!] since it can be done after a find_or_initialize call even if record is un-saved.

Are we in agreement that detecting the receiver is not needed or should I put in some work on restricting receiver also, for example, only flagging create_list if we know it is from FactoryBot?

2. Fix AllowedReceivers to work with namespaces

3. Get rid of AllowedBlockNames since it won't be needed when version 2.0 comes out.

Is there anything else?

[pirj]

Unable to configure with AllowedReceivers

I don't really think this option is necessary at all. We're detecting calls made outside of the suite context, and I think they'd better go to spec/support or spec_helper.rb in any case.

The example I've found:

    given(:action) do
      Spree::Promotion::Actions::CreateQuantityAdjustments.create(

is not yet detected as being in the example context because we lack the detection of given as a hook, but this will be resolved in another pull request.

From my perspective, 1 & 3, plus get rid of AllowedReceivers.

Do you feel this is a reasonable approach?

I might miss something. real-world-rspec is not a definitive source of how RSpec is used in the wild. It's there to check if we're not introducing something that will upset our users that write specs in a reasonable way. Most of their specs don't suffer from such blunders, so it's not a good playground to find real offences. And I'm not sure what is apart from the projects we're working on on a daily basis.

[thijsnado]

Hey @pirj I've done 1 & 3 as well as removing AllowedRecievers. I was also able to get rid of ForbiddenMethodsWithoutArguments since none of those methods were included in the list we wanted to keep.

[pirj]

Looks great!
Thanks a lot for this incredible effort!

@Darhazer @bquorning WDYT?

[pirj]

1.44

[pirj]

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

[bquorning]

I’d love to hear @Darhazer‘s opinion too.

[bquorning]

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

[Darhazer]

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.

[Darhazer]

I think we need to add Safe: false as well, as it would produce false positives

[Darhazer]

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.

[Darhazer]

I don't get why we need both AllowedMethods and ForbiddenMethods

[Darhazer]

I have mixed feelings on the cop as well. It's a real problem to solve, but it's hard to do in a static analysis, especially with a single-file analysis. Such a cop is bound to produce false positives. The problem would be better handled at run time.
On the other hand, the majority of the usages should be in hooks or setup helpers anyway, so perhaps chances of false positives are quite small. As @pirj mentioned, he couldn't find any real offenses in order to see for false positives. Which also questions how useful the cop would be for the general public.

[pirj]

The problem would be better handled at run time.

I can only think of before(:suite) { DatabaseCleaner.clean_with(:deletion/:truncation) }.

couldn't find any real offenses

I must confess, it hit my by surprise that such usage is not equivalent to before(:context). So I can't say if I wouldn't step into this trap myself when writing a spec.
Luckily, those codebases I've run this cop on, are written by smart people, and checked with a second pair of eyes. Not always the case, though.

[ydah]

The Rails department was extracted to rubocop-rspec_rails. It would be great if you could create this PR again there.