Cop Idea: Ban @controller = ... in controller specs

[ndbroadbent]

Is your feature request related to a problem? Please describe.

We just ran into an issue where I was doing this for an ApplicationController spec to test some helper methods:

class TestApplicationController < ApplicationController
  def test
    head :ok
  end
end

RSpec.describe ApplicationController do
  draw_test_routes! { get '/test', to: 'test_application#test' }

  before(:each) do
    @controller = TestApplicationController.new

It turns out that this is a huge footgun, and caused an order-dependent test failure. By setting @controller, RSpec is no longer able to clean up after the test, and it breaks view rendering (even for feature specs!)

We spent a very long time trying to figure out why some tests were suddenly failing on CI after a Rails/RSpec upgrade.

Describe the solution you'd like

A new rule that bans any instance of @controller = in spec/controllers

Additional context

This article describes the issue in great detail: https://www.vector-logic.com/blog/posts/what-the-spec-order-dependent-test-failure

[pirj]

Nice research!
Do you think this can be fixed on rspec-rails side by eg using @_controller?

[ndbroadbent]

Hi @pirj, setting @controller actually does change the controller that rspec uses when running the spec - @_controller would have no effect. This is more about banning this practice since it's unsafe - a better way is to use an anonymous controller

[pirj]

I understand, and I’ve carefully read your article. Do you think rspec-rails is using @controller instance variable underneath? Were you able to identify where exactly in its source code? Do you think this problem can be fixed on rspec-rails side by changing that variable name to something that end users would very unlikely use in their specs to assign the controller to, like @_controller?

[ndbroadbent]

Oh sorry the actual issue is where people are intentionally overwriting the @controller variable to change the behavior of the test, not accidentally using @controller by mistake. In my case, I was intentionally setting the @controller variable because I knew that's what rspec-rails was using - if they were using @_controller then I would have set that instead. As you can see from the article, it's dangerous to do this since it breaks the cleanup mechanism and rspec isn't able to tidy up after the test finishes. I wasn't aware of that issue before and I think I might have copied this @controller = ... code example from someone on StackOverflow or a forum post.

[pirj]

Understood. Makes total sense. Would you like to contribute such a cop?