Cop idea: prevent closure variables

[thijsnado]

A common mistake I see is using closure variables instead of let statements. An example of this:

describe User do
  user = FactoryBot.create(:user)

  it "exists" do
    expect(user).not_to be_nil
  end
end

The issue with above example is now a user exists in the DB for the rest of the tests since it is not part of example transaction.

I'd be willing to work on a PR for this if you think this would be a good addition to this gem.

[pirj]

Good catch! This create runs even before before(:suite) hooks.

Few things to consider:

• calls without assignment to variables
• fabrication gem
• model class create! calls (can this be subject to false positives due to legitimate calls?)
• direct calls to create/build, since it's quite popular to config.include FactoryBot::Syntax::Methods

[thijsnado]

I get the feeling this should possibly be two rules:

1. No local variables outside before/it blocks.
2. No singleton method calls outside before/it blocks (with some configuration for allowed singletons).

I'll likely split this into two PR's.

[pirj]

I'd start with the second one as more impactful. Also, I remember that variables can be quite handy in some cases, e.g.:

describe SomeConcern do
  klazz = Class.new do # a variable, not a constant because of LeakyConstant; could be `before(:context)`, but an instance var would be required then
    include SomeConcern
  end

  # examples follow
end

[thijsnado]

I've always done the klass thing as a let statement. Perhaps a bit inefficient since it gets defined every test example. Is that the reason you would prefer local variable instead of let there?

[pirj]

One example of a legitimate usage of a variable https://github.com/rubocop-hq/rubocop/pull/8447#discussion_r467528726

inefficient since it gets defined every test example. Is that the reason

No, just because variables are simpler.

[bquorning]

The problem in the user = FactoryBot.create(:user) is not the local variable, but the fact that global state is changed without being reset afterwards.

[thijsnado]

@pirj I think I'm going to punt on "direct calls to create/build, since it's quite popular to config.include FactoryBot::Syntax::Methods". The reason why is if you use these methods outside of an example, FactoryBot already gives you a descriptive error message stating that you should only use them within examples.

[pirj]

Sounds great @thijsnado 👍

[thijsnado]

@pirj put up a PR, I'm getting the following failure in CI: https://app.circleci.com/pipelines/github/rubocop-hq/rubocop-rspec/460/workflows/bea51799-651f-4806-8016-12f297c9112f/jobs/16335, it looks un-related to my changes though. Any thoughts?

[pirj]

@thijsnado No worries, there's a fix https://github.com/rubocop-hq/rubocop-rspec/pull/1014 It's caused by https://github.com/rubocop-hq/rubocop/issues/8561