Cop Idea: redundant `require '(spec|rails)_helper'`

[gondalez]

Background

Often each spec file has a require 'spec_helper' as its first line.

This is redundant if you have a .rspec file that looks like this:

--require rails_helper

Problem

To not have a .rspec file like this can cause unexpected behaviour when you collect test coverage on the spec files themselves. This can lead to flaky coverage reporting.

Specifically, if you configure simplecov per the docs and start coverage in spec_helper.rb then the first spec file run will not be included in coverage reports.

I found this out the hard way 😅

Solution

Per @olliebennett's https://github.com/rubocop/rubocop-rspec/issues/612#issuecomment-450860963:

Add a new cop that sees require 'spec_helper' and require 'rails_helper' as a violation. It should explain that having --require x_helper in .rspec is preferred.

# bad
require 'spec_helper'

RSpec.describe Xyz do
  # ...
end

# bad
require 'rails_helper'

RSpec.describe Xyz do
  # ...
end

# good
RSpec.describe Xyz do
  # ...
end

[pirj]

Having spec_helper and rails_helper separate is a deliberate decision, but it’s not mandatory at all to always include the rails_helper

I agree that requiring spec_helper is redundant and can be extracted to .rspec. But it should be opt-in to recommend the same for rails_helper. I’ve seen projects with a bunch of bunch of spec helper files, and one common which is always loaded is the spec_helper. Others, like cop_helper, and rails_helpers should not be required from .rspec by default. But if the cop provides such an option - nice.

Would you like to contribute such a cop?

[gondalez]

Hi @pirj :)

Sure, I can try my hand!

So with your suggestion, perhaps the cop is more like the following?

EnforcedStyle: spec_helper (default)

# bad
require 'spec_helper'

RSpec.describe Xyz do
  # ...
end

# good
require 'rails_helper'

RSpec.describe Xyz do
  # ...
end

# good
RSpec.describe Xyz do
  # ...
end

EnforcedStyle: rails_helper

# bad
require 'spec_helper'

RSpec.describe Xyz do
  # ...
end

# bad
require 'rails_helper'

RSpec.describe Xyz do
  # ...
end

# good
RSpec.describe Xyz do
  # ...
end

[bquorning]

Should we read the existing .rspec file to determine if people use another file name than spec_helper.rb? I wouldn’t want to enforce one valid name.

[gondalez]

That sounds sensible to me 👍

It would drop the need for EnforcedStyle which is nice!

So I would find all --require entries in the .rspec file and list as a violation any matching requires in specs.

The only issue I can think of is that rspec-rails' rails_helper template has a require 'spec_helper.rb' out of the box.

So if any specs have require 'spec_helper' and only --require rails_helper is in the .rspec file, they won't be seen as a violation 🤔

How about this? :

• require 'spec_helper' is always a violation (this would prompt a move to using .rspec when it is not already being used)
• any other --requires that have a matching .rspec entry are also violations

[pirj]

Sounds reasonable, as the rspec-core’s project initializer does add --require spec_helper to .rspec.