RSpec/InstanceVariable declaration inside an example

[ngouy]

is that a wanted behavior that when I declare an instance variable, this cop registers an offense?

example:

it "does something" do
  create(:questionnaire) do |questionnaire|
    @answers = create_list(:answer, 4)
  end
  
  expect(somePolicy.resolve(current_user)).to eq @answers
end

If I want to avoid an offense, I must

it "does something" do
  answers = nil
  create(:questionnaire) do |questionnaire|
    answers = create_list(:answer, 4)
  end
  
  expect(somePolicy.resolve(current_user)).to eq answers
end

Not that this is complicated or else. But capturing values into variables in a block without having to declare them before is handy.

Do you think it's a corner case? or is that wanted? Should we consider update the cop, or having an option?

[pirj]

Can be easier - block can return the return value of their last statement, so in theory this could be just:

it "does something" do
  answers = 
    create(:questionnaire) do |questionnaire|
      create_list(:answer, 4)
    end
  
  expect(somePolicy.resolve(current_user)).to eq answers
end

Would it work? I'm not sure - is create a FactoryBoy method? I haven't seen nested syntax before.

[ngouy]

indeed, but my above example was a very simple one amount multiple cases Sometimes (and that's when this is even more relevant) I instantiate multiple instances variables in the block.

is create a FactoryBoy method yes, sorry for the confusion, the idea is that you assign the questionnaire to the answers, mb

But let's have something like that for example

it "does something" do
  answers = nil
  responses = nil
  create(:questionnaire) do |questionnaire|
    answers = create_list(:answer, 4, questionnaire: questionnaire) 
    responses = create_list(:responses, 4, questionnaire: questionnaire)
  end
  
  expect(somePolicy.resolve(current_user)).to eq answers
end

or another example

it "does something"
  allow(MyService).to receive(:perform) do |new_user, new_house|
    @created_user = new_user
    @created_house = new_house
  end.and_call_original
  
  send_request
  
  expect(response["user"]["id"]).to eq @created_user.id
  expect(response["user"]["house_ids"]).to eq [@created_house.id]
end

I don't know

I was just wondering if we have a good reason to prevent this kind of things.

As I understand it, having an instance variables outside of an example can create leaks. But for the ones inside that's not really the case So if we also want to forbid it, wouldn't it be for different reasons? If so I was wondering if it could be another cop or an option to the existing one

[Darhazer]

What about just using the relations:

it "does something" do
  questionnaire = create(:questionnaire) do |questionnaire|
    create_list(:answer, 4, questionnaire: questionnaire) 
    create_list(:responses, 4, questionnaire: questionnaire)
  end
  
  expect(somePolicy.resolve(current_user)).to eq questionnaire.answers
  expect(something_else).to eq questionnaire.responses
end

Ok, that's really more about FactoryBot, and not general instance variables. Let's asume we can't use those relationships for whatever reason. Move setup to let?

let(:questionnaire) { create(:questionnaire) }
let!(:answers) { create_list(:answer, 4, questionnaire: questionnaire)

Now, getting the values with wich the stub was called is more interesting example indeed. Except for exploiting some let pattern like:

let(:memo) { {} }

it "does something"
  allow(MyService).to receive(:perform) do |new_user, new_house|
    memo[:user] = new_user
    memo[:house] = new_house
  end.and_call_original
  
  send_request
  
  expect(response["user"]["id"]).to eq memo[:user].id
  expect(response["user"]["house_ids"]).to eq [memo[:house].id]
end

I don't really have a solution. Perhaps you can disable the cop for such usages.

[Darhazer]

As for the reasoning, you are right - if it is assigned only in the example, there is no leak between the examples. I'm not sure what was the reasoning for AssigmentOnly option, but we can consider extending with an option to allow locally scoped instance variables.

@bquorning WDYT

[ngouy]

As for the factory example yes. I was just trying to highlight the issue I have but the simplest example I gave can be dealt with otherwise. Thought it's not really the point I try to expose. MB trying to oversimplify.

The memo is a good one, haven't really thought about it. But it's """sad""" to have to use a small hack because one cop that has been designed for a specific reason is overlapping over legit use-cases I'm not saying it's bad design, but again, I'm here to question if this is something worth digging, and if it's worth adjusting the current cop

But I agree, disabling the cop is ultimately always something we can do 👍

thank you all for the quick feedback

[ngouy]

Another thing to consider could be to have a system of "ignored_pattern"

So we can still have the cop ON, and target the variable we want to cop to ignore

But TBH It feels like those 2 things are not exclusive. Update the cop with an option to not target example variable, + add another option to ignore pattern/list

[pirj]

How about

it "does something" do
  questionnaire = create(:questionnaire)
  answers = create_list(:answer, 4, questionnaire: questionnaire) 
  responses = create_list(:responses, 4, questionnaire: questionnaire)
  
  expect(somePolicy.resolve(current_user)).to eq answers
  expect(something_else).to eq responses
end

It feels that the create(...) do |...| construct is redundant, as usually foo(...).tap do |...| is where a local variable would do just fine.

Just for the reference, I could find the following usages of create ... do in the famous GETTING_STARTED.md

create(:user) do |user|
  user.posts.create(attributes_for(:post))
end

def user_with_posts(posts_count: 5)
  FactoryBot.create(:user) do |user|
    FactoryBot.create_list(:post, posts_count, user: user)
  end
end

    ##   # factory yielding its result to a block
    ##   create(:post) do |post|
    ##     create(:comment, post: post)
    ##   end

[ngouy]

@pirj 💯

Forget about the factory example. As I said it was a bad attempt to expose the issue. But that issue is very real for other use cases Another example could be

it "does something"
  allow(MyService).to receive(:perform) do |new_user, new_house|
    @created_user = new_user
    @created_house = new_house
  end.and_call_original
  
  send_request
  
  expect(response["user"]["id"]).to eq @created_user.id
  expect(response["user"]["house_ids"]).to eq [@created_house.id]
end

Also even if I'm open to always get better at doing things, and having better practices, having to change my "relatively legit" code for a cop that have a bigger AOE that what it was design for (which is avoid leaks) seems hacky

I can do it, yes. Or I can disable the cop too.

• Could the cop could be retrained to "only" what is tries to fight instead. If my code is bad for other reasons than leaks, I'm fine, but maybe then we need another cop, or different options in the existing one, rather than trying to make my "legit" code fit a cop.
• Or maybe my use-cases are bad too and maybe instances variable should be avoided altogether. But then I think it's worth mentioning in the reason of why this cop is good(But RN I'm not convinced)

[Darhazer]

As I mentioned, I agree with @ngouy reasoning on this one. All bad examples in the cop, the assigment happens in a hook. Perhaps we have to clarify it in the rspec style guide first.

[pirj]

The subject under test is some request, and it makes a call to a service with certain values, and we tap in to get those values and make sure the response contains them. I can only say that the above example breaks isolation and boundaries. From my perspective, a good test of that request would stub the call to the service and would return some value that the request processor expects it to return. But I clearly understand that in real-life scenario such tests might be an overkill.

The problem with instance variables is not that they leak because they don't.

It is that a mistyped @fo0 won't blow up, while a fo0 would, and it takes an effort to find such typos.

Another problem with the above example is that it fails:

module MyService
  def self.perform(a, b)
    a + b
  end
end

module RequestHandler
  def self.handle
    c = MyService.perform(1, 2)
    {a: 1, b: 2, c: c}
  end
end

RSpec.describe RequestHandler do
  it do
    allow(MyService).to receive(:perform) do |a, b|
      @a = a
      @b = b
    end.and_call_original

    response = RequestHandler.handle

    expect(response).to include(a: @a, b: @b)
  end
end

  1) RequestHandler
     Failure/Error:
       allow(MyService).to receive(:perform) do |a, b|
         @a = a
         @b = b
       end.and_call_original

     RuntimeError:
       WARNING: You're overriding a previous stub implementation of `perform`. Called from /Users/pirj/source/rubocop-rspec/spec/a_spec.rb:19:in `block (2 levels) in <top (required)>'.. Called from /Users/pirj/source/rubocop-rspec/spec/a_spec.rb:19:in `block (2 levels) in <top (required)>'.
     # ./spec/a_spec.rb:19:in `block (2 levels) in <top (required)>'

A correct way is:

    allow(MyService).to receive(:perform).and_wrap_original do |_method, a, b|
      @a = a
      @b = b
    end

And it is still possible to write this example without using instance variables:

RSpec.describe RequestHandler do
  it do
    allow(MyService).to receive(:perform).and_call_original

    response = RequestHandler.handle

    a = response[:a]
    b = response[:b]
    c = response[:c]
    expect(c).to eq(a + b)

    expect(MyService).to have_received(:perform).with(a, b)
  end
end

The main reason I'm being stubborn with this is that a small leak may result in an avalanche of cases. Case where using an instance variable would subjectively simplify the spec code. In the end, resulting in a guideline/cop full of exceptions for such cases.

[ngouy]

gotcha, thanks for the advice

I guess I just need guidance on how I write my specs 😂