RSpec/InstanceVariable should ignore static (literal) classes

[AlexWayfer]

We have some specs where we're defining classes inside specs like this:

describe 'Foo' do
  class Bar
    def initialize(name)
      @name = name
    end

    def execute
      p @name
    end
  end
end

RSpec/InstanceVariable should not fire in this case.

[dgollahon]

@AlexWayfer I think you're right that RSpec/InstanceVariable is probably not the right mechanism to flag this, but it should actually still get flagged by something as written.

When you define Bar using class inside of a describe, it is not scoped to a describe. You are permanently introducing Bar into the global scope and it will be available across all specs.

See #197 for more details.

I'm assuming this is still broken for dynamic class declarations that do not pollute the global scope where accessing instance variables would be legitimate (using Class.new { ... }) so we should at least confirm and fix that.

[AlexWayfer]

When you define Bar using class inside of a describe, it is not scoped to a describe. You are permanently introducing Bar into the global scope and it will be available across all specs.

Yep, I know. And I don't know how to define scoped constants normally, beside ugly (and sometimes slow) stub_const. There are approaches like self::Bar = Class.new or let(:Bar), but they don't work for any case (constant nested usage, especially comparing, like klass < Bar).

[dgollahon]

I commonly do something like

let(:bar) do
	Class.new(WhateverParentClass) do
		...
	end
end

it 'works' do
	expect(bar.new).to be_whatever
end

or within an example just using a local variable

it 'works' do
	bar =
		Class.new(WhateverParentClass) do
			...
		end
	
  expect(bar.new).to be_whatever
end

If you need a generated constant within a constant for some reason you can use const_set on your dynamic class, though it can be a little hairy if you're doing something complex.

let(:bar) do
	Class.new(WhateverParentClass) do
		const_set(:BAZ, 5)
	end
end

it 'works' do
	expect(bar::BAZ).to eql(5)
end

And you can still inherit from your dynamic class using:

baz = Class.new(bar)

etc.

Usually I try to think carefully about what I'm doing if any of this is necessary and make sure I haven't created a difficult to test design when I didn't need to.

Either way, I think it's generally worth remembering that a class doesn't actually have to be assigned to a constant, though if you feel you need one stub_const is probably a better bet because it gives you some kind of test isolation.

I think rubocop-rspec should definitely try to discourage the introduction of constants in test files, but the instance variable cop probably shouldn't care about instance variables inside whatever classes you define.

[pirj]

Since LeakyConstantDeclaration cop was added to flag constant definitions.

RSpec/InstanceVariable is completely ignorant to:

RSpec.describe do
  let(:bar) do
    Class.new do
      def initialize(name)
        @name = name
      end

      def execute
        p @name
      end
    end
  end
  it {}
end

Do you think this might be closed @dgollahon ?

[dgollahon]

This is still, technically, a false-positive because if you chose not to enable the LeakyConstantDeclaration cop (or did an inline disable for one case) then this is still flagging something in a context it wasn't meant to and you'd need to add a disable statement.

That said, this is a pretty uncommon situation and I don't know if we should direct any energy at fixing the false-positive. If you / others think we should just not worry about it I do think it's a little less important now that we're flagging the constant declaration.

[dgollahon]

May also be relevant: https://github.com/rubocop-hq/rubocop-rspec/pull/381#issuecomment-288871191

If you stub the constant, then create it (and that works as illustrated here^ I haven't tried to define a class like this in a very long time so i'm not 100% on the legitimacy of this) you'd have a false positive still.

[pirj]

Mixed-style will only lead to confusion especially in cases when constant stubbing and class reopening are scattered:

before do
  stub_const('Foo', Class.new)
end

context 'when Foo has some specific method defined' do
  class Foo
    def some_method
    end
  end
  # ...
end

Wouldn't it be better to use class_eval:

context 'when Foo has some specific method defined' do
  Foo.class_eval do # will also work with "sacrificial" anonymous classes
    def some_method
    end
  end
  # ...
end

In case it's not scattered, the whole class can be defined in a single place with Class.new do: https://github.com/rubocop-hq/rubocop-rspec/pull/765#issuecomment-493705348

[dgollahon]

@pirj I'm not saying one should do it the way I referenced, just that someone might (although I'm not actually sure it's leak-free in all situations...). I guess it would still be flagged by the LeakyConstantDeclaration cop though so maybe that aspect is not a big deal. Still, the instance variable cop is only meant to discourage ivars in the spec's scope but I don't know if we need to be that meticulous about detection. I suppose this depends mostly on how independent our cops should be designed to be and how aggressively we want to avoid false-positives in any context.

~~I still think on style/format consistency grounds it's still a reasonable thing to have a preference on though and would probably make a good cop.~~ EDIT: mixed up the cop I was talking about while multi-tasking issues, first paragraph still applies.

[pirj]

how independent our cops should be designed

Now I think I understand what you mean.

So speaking about this specific case in question, when class is defined directly in example group, LeakyConstantDeclaration is disabled, and instance vars are used, do you think it makes sense to detect scope change (like we do in DescribedClass) and ignore instance variables there? I like that, it makes total sense to me.