Rubocop-rspec: `describe`/`context` inside module

[bquorning]

Today I discovered code like this in a spec file:

module Foo
  RSpec.describe Bar do
  end
end

which I found a bit strange. I would rewrite that as

RSpec.describe Foo::Bar do
end

Is this a common enough problem that we need a cop it?

[pirj]

Haven't' seen this so far.

[pirj]

Actually, I have. https://github.com/rspec/rspec-expectations/blob/master/spec/rspec/expectations/configuration_spec.rb#L3

This allows to write:

allow(Expectations::Syntax.default_should_host).to

instead of a more verbose:

allow(RSpec::Expectations::Syntax.default_should_host).to

While fixing RuboCop offences in that repository, I tried fixing this as well (due to Metrics/ModuleLength), and it turned out not to bring any improvements except for reduced indentation.

[dgollahon]

I've actually seen this in multiple codebases I've worked on. See my comment here: https://github.com/rubocop-hq/rubocop-rspec/pull/381#discussion_r107826572

[pirj]

Did you have DescribedClass's pending spec in mind @bquorning ?
I've fixed the cop, going to submit it soon.

[bquorning]

No, I hadn’t noticed that spec. But yes, that is an example of the issue I mentioned.

[pirj]

As DescribedClass has been merged and the nasty pending spec is green, and the approach of namespacing specs with modules is quite usable with no visible downsides, do you think there's still something to address in the context of this issue, @bquorning?

[dgollahon]

Still seems worthwhile to me for consistency's sake. I would have liked something to autocorrect one project that used a mix of this form and the standard form.

Also, you are still in the context of that class which seems strange to me in a testing context. You would also have to edit more places if a class becomes a module or vice-versa and errors about invalid constant references might be a bit more confusing since when you surround a test in a module the module will get created if it does not yet exist. That said, these are refactoring problems you just have in ruby and I'm not sure it's that strong of an argument against it and it also gains you some reference brevity. If you're not leaking constants, it's probably not a big deal, beyond consistency/aesthetics but it's not something I would personally do.

[dgollahon]

Now that I think of it, it would probably have to be an unsafe autocorrect because you can't tell which constant lookups were relying on the module scope.

[pirj]

Are you talking about some specific cop @dgollahon ?

[dgollahon]

@pirj I was taking this issue as a cop proposal: DescribeInsideModule or something like that which could flag and, possibly, autocorrect @bquorning's example from the first snippet to the second.

[dgollahon]

Though, if they dump described_class https://github.com/rubocop-hq/rubocop-rspec/pull/784#issuecomment-515514685, this may be a moot point.

[pirj]

Actually, I think it's still a good idea to have such a cop. Even though if it happens that there's no described_class anymore, some specs might still benefit from being put inside a module, or the other way around.

RSpec Core team's coding style slightly differ, they don't seem to use subjects, described_class that much, but class names are used, and if they are namespaced, there's a place for improvisation, e.g.

module RSpec
  module Expectations
    RSpec.describe Configuration do
      let(:config) { Configuration.new }

vs

RSpec.describe RSpec::Expectations::Configuration do
  let(:config) { RSpec::Expectations::Configuration.new }

Both styles have their benefits, and I don't see any major flaw in either of them.

Such usage, I suppose, is mostly due to the reluctance to use one-liner syntax, abuse subject intention to be explicit in expectations.

Note that while the examples below demonstrate how the subject helper can be used
as a user-facing concept, we recommend that you reserve it for support of custom
matchers and/or extension libraries that hide its use from examples.

explicit subject doccumentation

[rpbaptist]

I'm fine with a cop for this, as along as I can configure it to enforce the style to be the other way around.

[kellysutton]

I can tell a small story as to why this should be a cop to consider. Specifically, re-opening modules in tests can confuse the Rails autoloading mechanism. (Note: This is all pre-zeitwork on Rails 5.2 and earlier. I'm not sure if Rails 6+ with zeitwerk will change this.)

If you write:

module Foo
  module Bar
    RSpec.describe Baz do 
      # ...
    end
  end
end

And then you have code that refers to a constant defined on Foo::Bar, e.g. Foo::Bar::WIDGETS, this test code will fail if eagerloading is disabled. Why? Rails autoloading thinks it's already loaded the Foo::Bar module because it's defined.

Even though app/models/foo/bar.rb might define the WIDGETS constant, our test will not load the file because our test file has already defined the module. To confuse the matter more, turning on eager loading will cause the test to pass, because all code in app/ will be loaded before the test executes.

Not reopening the module, e.g.

RSpec.describe Foo::Bar::Baz do 
  # ...
end 

will cause the test to pass in both an eagerloaded and not eagerloaded configuration.

I realize that this is a Rails-specific issue, so I'm not sure if this gem is the best place to put such a cop or not. Let me know if I can further clarify the issue above or if my explanation was confusing. I may open a PR to this effect. Thanks!

[pirj]

I've just realized we've merged a cop quite long ago. https://github.com/rubocop-hq/rubocop-rspec/pull/831
Is there anything actionable left?

[bquorning]

Good point, I totally missed that as well. I’m closing this one as fixed by #831.