rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

Add `RSpec/Rails/InferredSpecType` cop

Open r7kamura opened this issue 2 years ago • 3 comments

In many Rails apps, I often see test code like RSpec.describe MyModel, type: :model even though config.infer_spec_type_from_file_location! is enabled.

This makes it difficult to distinguish whether they are being overwritten as needed or written even though they are not needed.

To solve this problem, it woule be nice to have a cop that issues offenses for redundant type metadata.

  • https://relishapp.com/rspec/rspec-rails/docs/directory-structure

Before submitting the PR make sure the following are checked:

  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [x] Updated documentation.
  • [x] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • [x] The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • [x] Added the new cop to config/default.yml.
  • [x] The cop is configured as Enabled: pending in config/default.yml.
  • [x] The cop is configured as Enabled: true in .rubocop.yml.
  • [x] The cop documents examples of good and bad code.
  • [x] The tests assert both that bad code is reported and that good code is not reported.
  • [x] Set VersionAdded in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

  • [ ] Set VersionChanged in config/default.yml to the next major version.

r7kamura avatar Aug 19 '22 09:08 r7kamura

Those two comments suggest that automatic inferring is legacy [1, 2]. I don't have an idea if infer_spec_type_from_file_location! will be deprecated or removed and when. define_derived_metadata provides the same, and at some point it may appear in RSpec Rails' spec/rails_helper.rb generator and would replace infer_spec_type_from_file_location!.

With this in mind, I'm on the fence with the decision what to do with this cop, as it doesn't seem to enforce the routine that RSpec suggests.

pirj avatar Sep 12 '22 09:09 pirj

I have renamed some of the required CI checks, which unfortunately means that you will need to rebase this branch to make CI pass.

bquorning avatar Sep 12 '22 13:09 bquorning

Then, it might be better to provide two styles, one that omits the type whenever possible according to infer_spec_type_from_file_location! and another that always specifies it explicitly.

r7kamura avatar Sep 12 '22 21:09 r7kamura

Thank you, @r7kamura !

pirj avatar Oct 20 '22 07:10 pirj

Those two comments suggest that automatic inferring is legacy [1, 2]. I don't have an idea if infer_spec_type_from_file_location! will be deprecated or removed and when. define_derived_metadata provides the same, and at some point it may appear in RSpec Rails' spec/rails_helper.rb generator and would replace infer_spec_type_from_file_location!.

With this in mind, I'm on the fence with the decision what to do with this cop, as it doesn't seem to enforce the routine that RSpec suggests.

Was this consideration resolved? I'm reluctant to let this cop autocorrect my specs because of this.

mvz avatar Oct 24 '22 07:10 mvz

Note that view specs are usually described as a string (e.g. RSpec.describe "users/new.html.erb").

#90 fixed an issue with the RSpec/DescribeClass cop - when argument type: :view is present, the cop should not register the RSpec/DescribeClass offense.

In #110 it is discussed that even though the infer_spec_type_from_file_location! is active, the cop still registers an RSpec/DescribeClass offense for view specs.

Since rubocop can't read RSpec's configuration, either:

  • Keep type: :view and disable the RSpec/Rails/InferredSpecType for view specs
RSpec/Rails/InferredSpecType:
  Exclude:
    - "spec/views/**/*_spec.rb"

or,

  • Disable RSpec/DescribeClass for view specs
RSpec/DescribeClass:
  Exclude:
    - "spec/views/**/*_spec.rb"

Open to hearing other approaches to this problem. Maybe a test could be added to cover future changes in these two cops.

davideluque avatar Nov 01 '22 15:11 davideluque

@davideluque I have simply disabled the entire InferredSpecType cop, since spec type inference is supposed to be deprecated.

mvz avatar Nov 01 '22 15:11 mvz

Personally I'm quite confused that you're adding a Rails specific thing to rubocop-rspec. infer_spec_type_from_file_location! comes from rspec-rails and not everyone uses ruby with rails. IMO you should introduce rubocop-rspec-rails to define such cops.

knapo avatar Nov 02 '22 09:11 knapo

We have this on our radar, see https://github.com/rubocop/rubocop-rspec/discussions/1440

But it’s unlikely that this will happen soon.

You can say the same about FactoryBot, and Capybara. For now, rubocop-rspec is an umbrella project for all those cops.

pirj avatar Nov 02 '22 15:11 pirj

Fair enough! Thanks for the explanation @pirj

knapo avatar Nov 03 '22 08:11 knapo