ActionController::TestCase controller construction

[Aaron Patterson]

In the case a developer has not constructed a controller, the setup
method of ActionController::TestCase will attempt to construct a
controller object. If it cannot construct a controller object, it
silently fails.

I added a warning in this case, and I'd like to eventually deprecate the
behavior. I can't think of why anyone would want to use
ActionController::TestCase and *not* test a controller. Does anyone
know a reason *why* we would do this?

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L534-542

--
Aaron Patterson
http://tenderlovemaking.com/

[Matt Jones]

Maybe I'm missing something, but doesn't the call to setup_controller_request_and_response happen *before* any user-defined setup methods get called? In that case, it's intended to let users do unusual things (that don't set, or set to nonsense, controller_class) and then set up their own controller object.

There are some related commits that seem relevant:

https://github.com/rails/rails/commit/13950a8cc94ab93bb20976f2797b1df9740849b3
https://github.com/rails/rails/commit/ee82e1c3015392c87c88ee32003763210a75d1ec
https://github.com/rails/rails/commit/529a3ee50eeb7db5a78afeb5da38fd71b42e7ea2

I'd say there's a good chance that all of these changes are intended to support doing things like this:

https://www.relishapp.com/rspec/rspec-rails/docs/controller-specs/anonymous-controller

by handling the case where the controller-under-test isn't a named constant.

--Matt Jones

[Aaron Patterson]

On Sun, Jul 29, 2012 at 11:54:33AM -0400, Matt Jones wrote:
>
> On Jul 28, 2012, at 11:44 PM, Aaron Patterson wrote:
>
> > In the case a developer has not constructed a controller, the setup
> > method of ActionController::TestCase will attempt to construct a
> > controller object. If it cannot construct a controller object, it
> > silently fails.
> >
> > I added a warning in this case, and I'd like to eventually deprecate the
> > behavior. I can't think of why anyone would want to use
> > ActionController::TestCase and *not* test a controller. Does anyone
> > know a reason *why* we would do this?
> >
> > https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L534-542
>
> Maybe I'm missing something, but doesn't the call to setup_controller_request_and_response happen *before* any user-defined setup methods get called? In that case, it's intended to let users do unusual things (that don't set, or set to nonsense, controller_class) and then set up their own controller object.

Yes, I think that is true. However, there is the `controller_class=`
and the `tests` class method that you can use when AC::TC cannot intuit the
controller class from your test class name:

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L390-393

If you needed a dynamic anonymous controllers, you could just implement
the `controller_class` method on your test case, e.g.:

class FooTest < ActionController::TestCase
def self.controller_class
# new anonymous subclass on every test
Class.new(ActionController::Base)
end
end

> There are some related commits that seem relevant:
>
> https://github.com/rails/rails/commit/13950a8cc94ab93bb20976f2797b1df9740849b3
> https://github.com/rails/rails/commit/ee82e1c3015392c87c88ee32003763210a75d1ec
> https://github.com/rails/rails/commit/529a3ee50eeb7db5a78afeb5da38fd71b42e7ea2
>
> I'd say there's a good chance that all of these changes are intended to support doing things like this:
>
> https://www.relishapp.com/rspec/rspec-rails/docs/controller-specs/anonymous-controller

I could be mistaken, but I don't think RSpec uses AC::TC behavior
methods. Maybe Mr. Chelimsky can enlighten us.

> by handling the case where the controller-under-test isn't a named constant.

As I demoed above, the controller doesn't need to be a named constant,
you just need to implement the correct factory method. So I'm still at
a loss why we would want to support "unconstructable" controllers.

The reason I want to get rid of this code is because there is currently
a code path that allows `@controller` to be nil during the test run.
This is annoying because there are *many* methods[1] that depend on this
instance variable, yet we cannot guarantee that the instance variable is
set.

If this is truly something that is for RSpec only, then perhaps this
behavior should be pushed to RSpec rather than maintained in Rails.

1. https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L414-525

[David Chelimsky]

rspec-rails overrides the `controller_class` method, providing its own based on the object passed to `describe` [1]. For anonymous controller specs, it generates a subclass of ApplicationController (by default) or a user defined base class [2].

I'm not clear on what you're proposing to change, but as long as Rails continues to generate a controller using `controller_class`, rspec-rails should (I think) continue to work as it does without any changes. Of course, I'd love to verify that if you do make any such changes.

That help?

[1] https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/controller_example_group.rb#L17-19

[2] https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/controller_example_group.rb#L56-74

[Aaron Patterson]

Perfect. That's what I would expect. Will `controller_class` ever
return something that can't be constructed via `new`?

Basically, I'd like to get rid of this rescue:

https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L538-540

> That help?

Sure does! Thanks! <3<3

[David Chelimsky]

Not by rspec, but I can't account for end users doing silly things :)

 

Basically, I'd like to get rid of this rescue:

  https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L538-540

That seems completely fine. Let it error out if there's a problem instantiating.

Cheers,
David

[Aaron Patterson]

On Sun, Jul 29, 2012 at 02:49:55PM -0700, David Chelimsky wrote:
> > Perfect. That's what I would expect. Will `controller_class` ever
> > return something that can't be constructed via `new`?
> >
>
> Not by rspec, but I can't account for end users doing silly things :)

Yes, and I presume end users would like to be notified when they do
silly things. :-)

> > Basically, I'd like to get rid of this rescue:
> >
> >
> > https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/test_case.rb#L538-540
> >
>
> That seems completely fine. Let it error out if there's a problem
> instantiating.

Great, thanks!