Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RSpec/Rails/InferredSpecType cop #1365

Merged
merged 3 commits into from
Oct 20, 2022
Merged

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Aug 19, 2022

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.


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • 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:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop is configured as Enabled: true in .rubocop.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • 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.

@pirj
Copy link
Member

pirj commented Sep 12, 2022

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.

@bquorning
Copy link
Collaborator

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

@r7kamura
Copy link
Contributor Author

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 r7kamura force-pushed the feature/infer branch 3 times, most recently from b3104a4 to 87f3ef5 Compare October 11, 2022 23:44
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a small change to config/default.yml to prevent overriding the default list of inferences. Otherwise -

Just perfect, thank you so much!

@r7kamura r7kamura force-pushed the feature/infer branch 2 times, most recently from 51dff6b to f770015 Compare October 14, 2022 21:27
@pirj pirj requested review from bquorning and Darhazer October 15, 2022 06:06
@pirj pirj force-pushed the feature/infer branch 2 times, most recently from d0fc60e to 4969f5f Compare October 15, 2022 06:18
@r7kamura r7kamura force-pushed the feature/infer branch 2 times, most recently from 891be9b to 1f30ff5 Compare October 19, 2022 21:55
@pirj pirj requested a review from bquorning October 19, 2022 21:56
@pirj pirj merged commit 9806c0f into rubocop:master Oct 20, 2022
@pirj
Copy link
Member

pirj commented Oct 20, 2022

Thank you, @r7kamura !

@r7kamura r7kamura deleted the feature/infer branch October 20, 2022 08:10
@mvz
Copy link
Contributor

mvz commented Oct 24, 2022

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.

@davideluque
Copy link

davideluque commented Nov 1, 2022

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.

@mvz
Copy link
Contributor

mvz commented Nov 1, 2022

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

@knapo
Copy link

knapo commented Nov 2, 2022

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.

@pirj
Copy link
Member

pirj commented Nov 2, 2022

We have this on our radar, see #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.

@knapo
Copy link

knapo commented Nov 3, 2022

Fair enough! Thanks for the explanation @pirj

bfad added a commit to bfad/rubocop-rspec_rails that referenced this pull request Nov 23, 2024
At the time this cop was added, [it was noted][PR Comment] that this was
legacy behavior left as the default for people migrating to RSpec 3. Now
RSpec 7.1.0 has [removed this default][PR] to make it clear that this is
deprecated / legacy behavior, let's delete this cop.

[PR Comment]: rubocop/rubocop-rspec#1365 (comment)
[PR]: rspec/rspec-rails#2804
bfad added a commit to bfad/rubocop-rspec_rails that referenced this pull request Nov 23, 2024
At the time this cop was added, [it was noted][PR Comment] that this was
legacy behavior left as the default for people migrating to RSpec 3. Now
RSpec 7.1.0 has [removed this default][PR] to make it clear that this is
deprecated / legacy behavior, let's delete this cop.

[PR Comment]: rubocop/rubocop-rspec#1365 (comment)
[PR]: rspec/rspec-rails#2804
pirj pushed a commit to bfad/rubocop-rspec_rails that referenced this pull request Nov 25, 2024
At the time this cop was added, [it was noted][PR Comment] that this was
legacy behavior left as the default for people migrating to RSpec 3. Now
RSpec 7.1.0 has [removed this default][PR] to make it clear that this is
deprecated / legacy behavior, let's delete this cop.

[PR Comment]: rubocop/rubocop-rspec#1365 (comment)
[PR]: rspec/rspec-rails#2804
bfad added a commit to bfad/rubocop-rspec_rails that referenced this pull request Nov 25, 2024
At the time this cop was added, [it was noted][PR Comment] that this was
legacy behavior left as the default for people migrating to RSpec 3. Now
RSpec 7.1.0 has [removed this default][PR] to make it clear that this is
deprecated / legacy behavior, let's delete this cop.

[PR Comment]: rubocop/rubocop-rspec#1365 (comment)
[PR]: rspec/rspec-rails#2804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants