Rubocop-rspec: RSpec/DescribedClass mis-identifies inside of rspec-rails anonymous controllers

[benhutton]

Versions:

• rubocop — 0.74.0
• rubocop-rspec — 1.34.1

RSpec/DescribedClass will correct code like this:

RSpec.describe SomeConcern, type: :controller do
  controller(ApplicationController) do
    include SomeConcern
  end
  ...
end

to this:

RSpec.describe SomeConcern, type: :controller do
  controller(ApplicationController) do
    include described_class
  end
  ...
end

Resulting in this error:

An error occurred while loading ./spec/controllers/concerns/some_concern.rb.
Failure/Error: include described_class

NameError:
  undefined local variable or method `described_class' for #<Class:0x000056272150a908>
  Did you mean?  descendants

For reference, please see https://relishapp.com/rspec/rspec-rails/v/3-8/docs/controller-specs/anonymous-controller

[pirj]

Thanks for reporting, I'll take a look.

[pirj]

It works fine for some blocks, so I'd rather not make any code changes:

class A
  def self.described_class
    'A'
  end

  def self.y(&block)
    block.call
  end

  y do
    puts self # => A
    puts described_class # => A
  end

  def self.x(&block)
    Class.new(&block)
  end

  x do
    puts self # => #<Class:0x00...
    puts described_class # fails
  end
end

There's no way to statically determine what kind of block it is.

It's a known caveat, and there's a configuration option to work this around, SkipBlocks:

RSpec/DescribedClass:
  SkipBlocks: true

I suggest you add a .rubocop.yml file with this snippet above only, and put it to a directory where the specs for controller concerns reside. That should solve the issue you're experiencing and will keep the detection for rest of the code base intact.

Please let me know if it works for you, @benhutton, @anthony-robin.

[anthony-robin]

Thanks @pirj ! The SkipBlocks: true option solved offenses I had 👍

[pirj]

@benhutton Did adding a directory-local RuboCop configuration file solve the issue you're experiencing?

[benhutton]

@pirj right now, I'm preferring to use a single-line # rubocop:disable comment to get things working. Maybe I'm being too greedy. I think in general this cop is doing great stuff and wish there was a way it could be made smarter! I wonder if adding a block blacklist configuration would make sense? That way, we can tell it to ignore controller do blocks and nothing else (until that becomes necessary). My guess is that MOST blocks work correctly and a select few cause problems.

[pirj]

@benhutton Unfortunately, there's no other way to set configuration options per-directory right now.

You mean that situation when in a controller test there a non-controller do block that is using a class name instead of described_class, and you'd like to detect it?

Do you think it is worth it?

Speaking of described_class, you might be interested in an ongoing discussion.

[benhutton]

@pirj I'm not talking about configuring per-directory. I'd do it globally.

That discussion is interesting. And I think this adds an interesting detail: you can't even actually use described_class consistently / all the time.

That said, I don't do the things that make it hard to use. It always works for me (except in these controller specs!). So I would probably keep it around. Maybe with a described_module alias.

For this particular cop, it feels really tricky, right? The cop _cannot_ do what it purports to do reliably. So it's unsafe. So maybe I would disable the whole cop by default and then go with the SkipBlocks option you have now, and maybe some other knobs, to try to get it working flexibly? I suppose it really depends on how decisions like this get made for other cops with similar caveats...

[pirj]

The mention of controller do caveat is in cop's documentation.

As far as I know, it's a common way to configure cops in directories.

There is a TODO to mark this cop as "Unsafe to autocorrect" on cop metadata roadmap.

Is there anything that can be done in the scope of this ticket, @benhutton ?

[benhutton]

@pirj from what I can tell, the caveat is in the cop's _specs_, not in the actual docs (https://rubocop-rspec.readthedocs.io/en/latest/cops_rspec/#rspecdescribedclass). Thus, it's a known and tested-for caveat, but not one that is well-communicated to users. So I'd say that the safest thing to do would be to enhance the docs clarifying _why_ the skip blocks option exists and when/how to use it?

[pirj]

Fair enough. I was completely sure that it was in the docs, I've definitely seen it somewhere, my bad, and please accept my apologies.

Please take a look #802, @benhutton. Do you think this would be enough to avoid confusion?

Just for history: #59

[benhutton]

yes! that is great! thanks so much @pirj