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

Cop Idea: Disallow Controller Specs #1116

Closed
jherdman opened this issue Jan 8, 2021 · 9 comments
Closed

Cop Idea: Disallow Controller Specs #1116

jherdman opened this issue Jan 8, 2021 · 9 comments

Comments

@jherdman
Copy link

jherdman commented Jan 8, 2021

Our project is fairly old and is still in the process of converting over controller specs into request specs. It'd be really helpful to have a cop that disallows the introduction of new controller specs.

@pirj
Copy link
Member

pirj commented Jan 8, 2021

How do you tell a controller spec from a request spec?
An obvious way is by the use of assigns and render_template that come from rails-controller-testing gem, but controller specs can live without it.
By type: :controller metadata? But what if infer_spec_type_from_file_location! is turned on?
By the presence of _controller_spec.rb in the file name?

@jherdman
Copy link
Author

jherdman commented Jan 8, 2021

Canonically: the path. spec/requests vs spec/controllers

@jherdman
Copy link
Author

jherdman commented Jan 8, 2021

IMHO the meta data is fine too.

@pirj
Copy link
Member

pirj commented Jan 8, 2021

I've just recently seen requests specs residing in spec/controllers (a decision was made not to change their location during the conversion), and controller specs shamelessly occupying in spec/requests directory.

There's also a major confusion between the two (see rubocop/rspec-style-guide#113) as both of them inherit from ActionDispatch::IntegrationTest in Rails, but rspec-rails hasn't followed yet, and still uses ActionController::TestCase::Behavior as the base class for controller tests.

So we could come up with a cop that would prevent from writing type: :controller on a top-level example group, and putting files into spec/controllers, but there are ways around that.

I'd prefer to apply an effort and help rspec-rails to use the right base class. Now is the best moment to do that, rspec-rails 4.1 that is a companion to Rails 6.1 is about to be released.

@pirj
Copy link
Member

pirj commented Jan 4, 2022

An idea to implement such a cop is very tempting, but it would be easier to add a warning on RSpec Rails side.

@pirj pirj closed this as completed Jan 4, 2022
@rafaelsales
Copy link

rafaelsales commented Sep 14, 2023

@pirj Rubocop makes it easy to introduce a rule in phases -- allow existing offenses and warn new offenses.

I can't see how that can be easily done in rspec-rails without reinventing rubocop in a way.

Identifying based on path or metadata type should be attack the majority of cases, which in the end will have already helped projects move into the right direction.

@pirj
Copy link
Member

pirj commented Sep 14, 2023

I can't see how that can be easily done in rspec-rails

module RSpec::Rails::ControllerExampleGroup
def self.included(base)
warn “included in #{base}”
end
end

no?

@rafaelsales
Copy link

On a codebase with 600 controller specs, I need an easier way to manage a transition. Rubocop offers include/exclude patterns to start enforcing new code only.

@pirj
Copy link
Member

pirj commented Sep 15, 2023

What your suggestion is for the cop exactly, @rafaelsales ? I guess since nobody is going to write the cop for you, you can send a pull request and let’s see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants