ActionController::TestCase: reset instance variables after each request

[ghiculescu]

ActionController::TestCase keeps a @controller instance variable, which represents the controller being tested. At the end of each request inside a test, its params and format are reset. But any other instance variables set in the test aren't reset. This creates a problem if you do something like this:

class UserController
  def show
    @user ||= User.find(params[:id])
    render plain: @user.name
  end
end

# ActionController::TestCase subclass

test "gets the user" do
  get :show, params: { id: users(:one).id }
  assert "one", response.body

  get :show, params: { id: users(:two).id }
  assert "two", response.body # fails; response.body is "one"
end

The second assertion will fail, because @user won't be re-assigned in the second test (due to ||=). This example is a bit contrived, but it shows how instance variables persisting between requests can lead to surprising outcomes.

This PR fixes this by clearing all instance variables that were created on the controller while a request was processed. This now matches the behaviour of ActionDispatch::IntegrationTest.

Questions

• Is this behaviour intended? I don't know why it would be, but if it is I can update https://api.rubyonrails.org/classes/ActionController/TestCase.html instead of this change.
• Does this need a config switch? To me it's just a bug, and the caveat below should mean it won't break most legitimate uses of this. But if we want a Rails update to never break any existing tests then it needs to be something you can opt out of. I think this should be enabled by default.

Caveats

It explicitly excludes instance variables that were created before any requests were run. And it leaves the instance variable around until the next request in the test. This means that you can still do this:

# ActionController::TestCase subclass

test "gets the user" do
  @controller.user = users(:one) # assuming `Controller#user` users an ivar internally, you can set the ivar here...

  get :show_current
  assert "one", response.body

  assert_equal users(:one), @controller.user # and you can read the ivar here
end

[byroot]

Couldn't we instantiate a new controller instead?

[ghiculescu]

No, because then you can’t set ivars on the controller before any request runs.

While that’s not a good idea it’s possible and I’m sure lots of tests in the wild do it.

An example use case would be setting the current user.

[byroot]

Hum, my memory is fuzzy on this, but I think historically you weren't supposed to call two controller actions in a single tests. But to be honest I've seen this pattern a lot so it would be better to properly support it.

[ghiculescu]

Yeah I agree. You aren’t meant to but there’s nothing in the framework that stops you, so in practice it happens all the time.

[byroot]

@ghiculescu yeah sorry about that one, I didn't really forget about this PR, it's just one of these things were you can't really think of a good solution. I'll think more about it and come back tomorrow.

[ghiculescu]

Thanks. Was hoping to sneak it into 7 but no worries if it's too late.

[byroot]

If we find some decent solution I'll see with Rafael.

What I mean by no good solution is that I can see issues both ways with restoring the state or not.

[byroot]

Sorry, I was kinda busy today and I forgot to get back to this PR.

My main concerns is that some of the pre-existing instance variables may be mutated etc, so really this solution is far from being 100% reliable.

But if we're being reasonable, it's way better than not doing it, and your added documentation recommending not to make more than one request per test is a nice touch.

Let's add some changelog entry, and it's 👍 from me.

[byroot]

And apologies for the delays.

[ghiculescu]

And apologies for the delays.

No worries! No rush on this stuff, I appreciate the feedback.

[byroot]

cc @rafaelfranca can we backport this this 7-0-stable?

[rafaelfranca]

Done!