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

Deprecate assigns() and assert_template in controller testing #18950

Closed
dhh opened this Issue Feb 15, 2015 · 39 comments

Comments

Projects
None yet
@dhh
Member

dhh commented Feb 15, 2015

Testing what instance variables are set by your controller is a bad idea. That's grossly overstepping the boundaries of what the test should know about. You can test what cookies are set, what HTTP code is returned, how the view looks, or what mutations happened to the DB, but testing the innards of the controller is just not a good idea.

The same goes for assert_template. This ties the test to the internal structure of how your views are organized. That's not what the test should be testing. It should be testing what DOM elements it expect to be there. Rearranging your templates and partials should not cause these tests to break. Deprecate and recommend using assert_select instead.

@dhh dhh added the actionpack label Feb 15, 2015

@dhh dhh added this to the 5.0.0 milestone Feb 15, 2015

@ajgrover

This comment has been minimized.

Show comment
Hide comment
@ajgrover

ajgrover Feb 24, 2015

Contributor

Just sent a PR with deprecation warnings. I tried to follow the style in other actionpack warnings.

Contributor

ajgrover commented Feb 24, 2015

Just sent a PR with deprecation warnings. I tried to follow the style in other actionpack warnings.

@renanoronfle

This comment has been minimized.

Show comment
Hide comment
@renanoronfle

renanoronfle Feb 25, 2015

I agree testing what instance variables are set is a bad idea, but i'm not sure to test what mutations happened to the DB, because this is not innards of the controller too.
I think the better way is test what HTTP code is returned

renanoronfle commented Feb 25, 2015

I agree testing what instance variables are set is a bad idea, but i'm not sure to test what mutations happened to the DB, because this is not innards of the controller too.
I think the better way is test what HTTP code is returned

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 25, 2015

Member

That’s more a matter of whether you’re testing the controller or doing a full system test. I’m very sympathetic to system testing, but the black box can also appear in shades of grey for greater efficiency or other concerns.

On Feb 25, 2015, at 10:33, Renan Oronfle notifications@github.com wrote:

i agree testing what instance variables are set is a bad idea, but i'm not sure to test what mutations happened to the DB, because this is not innards of the controller too.
I think the better way is test what HTTP code is returned


Reply to this email directly or view it on GitHub.

Member

dhh commented Feb 25, 2015

That’s more a matter of whether you’re testing the controller or doing a full system test. I’m very sympathetic to system testing, but the black box can also appear in shades of grey for greater efficiency or other concerns.

On Feb 25, 2015, at 10:33, Renan Oronfle notifications@github.com wrote:

i agree testing what instance variables are set is a bad idea, but i'm not sure to test what mutations happened to the DB, because this is not innards of the controller too.
I think the better way is test what HTTP code is returned


Reply to this email directly or view it on GitHub.

@renanoronfle

This comment has been minimized.

Show comment
Hide comment
@renanoronfle

renanoronfle Feb 25, 2015

Ok, but if you check mutations happened in DB, are you overstepping controllers tests?
For test full system you can test in controller the http code and make a model test to check DB mutations.
I understand your point, at present moment my tests check DB mutations, but your commentary made me think if is right.

renanoronfle commented Feb 25, 2015

Ok, but if you check mutations happened in DB, are you overstepping controllers tests?
For test full system you can test in controller the http code and make a model test to check DB mutations.
I understand your point, at present moment my tests check DB mutations, but your commentary made me think if is right.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 25, 2015

Member

I don’t think so. Controllers don’t own the model. They do own their ivars, though. So I see no issue there.

On Feb 25, 2015, at 13:52, Renan Oronfle notifications@github.com wrote:

Ok, but if you check mutations happened in DB, are you overstepping controllers tests?
For test full system you can test in controller the http code and make a model test to check DB mutations.
I understand your point, at present moment my tests check DB mutations, but your commentary made me think if is right.


Reply to this email directly or view it on GitHub.

Member

dhh commented Feb 25, 2015

I don’t think so. Controllers don’t own the model. They do own their ivars, though. So I see no issue there.

On Feb 25, 2015, at 13:52, Renan Oronfle notifications@github.com wrote:

Ok, but if you check mutations happened in DB, are you overstepping controllers tests?
For test full system you can test in controller the http code and make a model test to check DB mutations.
I understand your point, at present moment my tests check DB mutations, but your commentary made me think if is right.


Reply to this email directly or view it on GitHub.

@listrophy

This comment has been minimized.

Show comment
Hide comment
@listrophy

listrophy Mar 5, 2015

When testing a unit, you want to see the result of calling a function. That result comprises a) return values and b) external state modifications. In nearly any other case, internal (ivar) modification should not be tested, but this is not true with controllers. Why? Because the next high-level step of the render chain is to copy the controller's ivars* to the view. That's an external—albeit delayed—state change, and the fact that it occurs with ivars is merely an implementation detail/convenience.

If there was an accepted, default way to pass information to views without using ivars, then I'd agree with removing assigns(). Until such a mechanism is "The Rails Way" (yes, one can do it already with mechanisms like decent_exposure, but the guides do not encourage that), assigns is crucial, especially when trying to fix some "bad" controller code.

  • Well, some of them, at least.

listrophy commented Mar 5, 2015

When testing a unit, you want to see the result of calling a function. That result comprises a) return values and b) external state modifications. In nearly any other case, internal (ivar) modification should not be tested, but this is not true with controllers. Why? Because the next high-level step of the render chain is to copy the controller's ivars* to the view. That's an external—albeit delayed—state change, and the fact that it occurs with ivars is merely an implementation detail/convenience.

If there was an accepted, default way to pass information to views without using ivars, then I'd agree with removing assigns(). Until such a mechanism is "The Rails Way" (yes, one can do it already with mechanisms like decent_exposure, but the guides do not encourage that), assigns is crucial, especially when trying to fix some "bad" controller code.

  • Well, some of them, at least.
@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Mar 5, 2015

Contributor

This is an honest question. With the removal of these two checks, what is the benefit of continuing to use a controller test over an integration test that tests a single controller?

Contributor

cupakromer commented Mar 5, 2015

This is an honest question. With the removal of these two checks, what is the benefit of continuing to use a controller test over an integration test that tests a single controller?

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 5, 2015

Member

@cupakromer performance. ATM controller tests are still a bit faster then integration tests. We would like those to have the same performance, so we can only have one.

Member

arthurnn commented Mar 5, 2015

@cupakromer performance. ATM controller tests are still a bit faster then integration tests. We would like those to have the same performance, so we can only have one.

@phoet

This comment has been minimized.

Show comment
Hide comment
@phoet

phoet Mar 9, 2015

Contributor

i think that @listrophy has some valid points there. in case i actually want to test the way a controller accesses views or sets instance variables (because maybe i use it as some kind of internal API), what would be the recommended way?

Contributor

phoet commented Mar 9, 2015

i think that @listrophy has some valid points there. in case i actually want to test the way a controller accesses views or sets instance variables (because maybe i use it as some kind of internal API), what would be the recommended way?

@dmitry

This comment has been minimized.

Show comment
Hide comment
@dmitry

dmitry Mar 9, 2015

Contributor

I agree with @listrophy and @phoet. It should be possible to test what messages are passing to external interface (messages - instance variables, external interface - views/templates).

Contributor

dmitry commented Mar 9, 2015

I agree with @listrophy and @phoet. It should be possible to test what messages are passing to external interface (messages - instance variables, external interface - views/templates).

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 9, 2015

Member

You can of course do whatever you want. Ruby is open. But Rails does not have to encourage this behavior, and won't, starting from the next version.

Member

dhh commented Mar 9, 2015

You can of course do whatever you want. Ruby is open. But Rails does not have to encourage this behavior, and won't, starting from the next version.

@listrophy

This comment has been minimized.

Show comment
Hide comment
@listrophy

listrophy Mar 9, 2015

@dhh how, specifically, do you think we should test the interface between controllers and views? Strictly integration testing? just trying to get clarification here.

listrophy commented Mar 9, 2015

@dhh how, specifically, do you think we should test the interface between controllers and views? Strictly integration testing? just trying to get clarification here.

@calebthompson

This comment has been minimized.

Show comment
Hide comment
@calebthompson

calebthompson Mar 9, 2015

Contributor

Since this is for 5.0, could we remove and also deprecate for the remainder of the 4.X lifecycle?

Contributor

calebthompson commented Mar 9, 2015

Since this is for 5.0, could we remove and also deprecate for the remainder of the 4.X lifecycle?

@calebthompson

This comment has been minimized.

Show comment
Hide comment
@calebthompson

calebthompson Mar 9, 2015

Contributor

@listrophy I think that's the point. You shouldn't test that interface as their being separate is an implementation detail of your framework, not something you should care about.

Contributor

calebthompson commented Mar 9, 2015

@listrophy I think that's the point. You shouldn't test that interface as their being separate is an implementation detail of your framework, not something you should care about.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 9, 2015

Member

If you want to continue to test ivars, you can just do #instance_variable_get.

And yes, this is a change for Rails 5.0.

The change may well be that we’re dropping controller tests entirely in favor of integration tests, now that the latter is faster on rails/master than the former was on 4.2.

On Mar 9, 2015, at 12:22, Bradley Grzesiak notifications@github.com wrote:

@dhh how, specifically, do you think we should test the interface between controllers and views? Strictly integration testing? just trying to get clarification here.


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 9, 2015

If you want to continue to test ivars, you can just do #instance_variable_get.

And yes, this is a change for Rails 5.0.

The change may well be that we’re dropping controller tests entirely in favor of integration tests, now that the latter is faster on rails/master than the former was on 4.2.

On Mar 9, 2015, at 12:22, Bradley Grzesiak notifications@github.com wrote:

@dhh how, specifically, do you think we should test the interface between controllers and views? Strictly integration testing? just trying to get clarification here.


Reply to this email directly or view it on GitHub.

@listrophy

This comment has been minimized.

Show comment
Hide comment
@listrophy

listrophy Mar 9, 2015

Honestly, I don't actually want to test ivars. I (sometimes) want to test what data is getting sent from controllers to views. It just so happens that the main way to do that right now is through ivars.

And to be completely fair, I usually don't write controller unit tests. I still think it's important to not shut that door in cases where it's useful, such as all those brownfield projects out there. If such functionality were pulled into a gem rather than just removed, you wouldn't hear me complain.

listrophy commented Mar 9, 2015

Honestly, I don't actually want to test ivars. I (sometimes) want to test what data is getting sent from controllers to views. It just so happens that the main way to do that right now is through ivars.

And to be completely fair, I usually don't write controller unit tests. I still think it's important to not shut that door in cases where it's useful, such as all those brownfield projects out there. If such functionality were pulled into a gem rather than just removed, you wouldn't hear me complain.

@cupakromer

This comment has been minimized.

Show comment
Hide comment
@cupakromer

cupakromer Mar 10, 2015

Contributor

I agree with @listrophy, I'd rather not test ivars. As has been pointed out, it is necessary when attempting to isolate the controller's contract with the view. Honestly, for me, what the controller passes into the view and what the view decides to do, or not do, with that information are fairly separate concerns. As long as the base contract is met.

These two parts of Rails have always been tightly coupled, I'm guessing due to ERB's behavior for how it evals on an object. I have recently notice Rails even takes extra steps to prevent certain ivars from crossing this "invisible" barrier.

If the future route is to drop controller testing in favor of integration testing you won't hear me complain. Nor if this behavior is extract into a gem as suggested above.

Contributor

cupakromer commented Mar 10, 2015

I agree with @listrophy, I'd rather not test ivars. As has been pointed out, it is necessary when attempting to isolate the controller's contract with the view. Honestly, for me, what the controller passes into the view and what the view decides to do, or not do, with that information are fairly separate concerns. As long as the base contract is met.

These two parts of Rails have always been tightly coupled, I'm guessing due to ERB's behavior for how it evals on an object. I have recently notice Rails even takes extra steps to prevent certain ivars from crossing this "invisible" barrier.

If the future route is to drop controller testing in favor of integration testing you won't hear me complain. Nor if this behavior is extract into a gem as suggested above.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 25, 2015

Contributor

iVars essentially serve as the recommended "API" between controllers and views. It's how controllers pass data to views, as instructed and given by examples in almost all the Rails docs, it seems to be the way Rails wants you to do it.

So I'd disagree that controllers "own" their iVars, in Rails design the iVars serve as the interface between controllers and views.

I don't see any good way to 'unit' test a controller without testing iVars as well as templates rendered. The output of a controller is essentially just those, iVars and templates rendered.

If you only test HTTP status codes and HTML returned, as output, I think you're basically doing a "full system test," you might as well be doing an integration test. Which, part of this thread suggests that's what Rails means to encourage, do not unit test controllers, only do integration tests. Myself, I find unit testing controllers to be useful -- most especially when delivering a controller (or controller mix-in) in an engine. When delivered via an engine (eg like devise), we want to test inputs and outputs for a contract the engine provides very carefully, make sure they are exactly as specified. It would be difficult for me if they were to go away.

But if you mean to promote "only integration tests for rails", I guess that's consistent, that's basically what happens when you can't test iVars or templates rendered anymore, whether you use the Rails integration test base class or the functional/controller test base class, you're basically doing an integration test now.

It will be easy to work around the missing assigns with instance_variable_get, indeed, but the missing assert_template is going to be harder, perhaps not possible to do in any reliable way without getting into Rails internals that can change with every release.

Contributor

jrochkind commented Mar 25, 2015

iVars essentially serve as the recommended "API" between controllers and views. It's how controllers pass data to views, as instructed and given by examples in almost all the Rails docs, it seems to be the way Rails wants you to do it.

So I'd disagree that controllers "own" their iVars, in Rails design the iVars serve as the interface between controllers and views.

I don't see any good way to 'unit' test a controller without testing iVars as well as templates rendered. The output of a controller is essentially just those, iVars and templates rendered.

If you only test HTTP status codes and HTML returned, as output, I think you're basically doing a "full system test," you might as well be doing an integration test. Which, part of this thread suggests that's what Rails means to encourage, do not unit test controllers, only do integration tests. Myself, I find unit testing controllers to be useful -- most especially when delivering a controller (or controller mix-in) in an engine. When delivered via an engine (eg like devise), we want to test inputs and outputs for a contract the engine provides very carefully, make sure they are exactly as specified. It would be difficult for me if they were to go away.

But if you mean to promote "only integration tests for rails", I guess that's consistent, that's basically what happens when you can't test iVars or templates rendered anymore, whether you use the Rails integration test base class or the functional/controller test base class, you're basically doing an integration test now.

It will be easy to work around the missing assigns with instance_variable_get, indeed, but the missing assert_template is going to be harder, perhaps not possible to do in any reliable way without getting into Rails internals that can change with every release.

@msaspence

This comment has been minimized.

Show comment
Hide comment
@msaspence

msaspence Mar 26, 2015

How should I test my controllers in isolation of my views? Is that not the advantage of the MVC model?

This feels like a very heavy handed way of strong arming people into writing integration tests rather than unit tests. I appreciate that Rails is "convention over configuration", and perhaps the convention should be to to write integration tests but I should be able to choose to unit test controllers, just as easily.

I know I could probably reimplement these myself if I so wished such is the nature of ruby, but it feels a bit like fighting against the framework. I can't help but feel that there are better ways to point people towards integration tests without removing these helpers.

msaspence commented Mar 26, 2015

How should I test my controllers in isolation of my views? Is that not the advantage of the MVC model?

This feels like a very heavy handed way of strong arming people into writing integration tests rather than unit tests. I appreciate that Rails is "convention over configuration", and perhaps the convention should be to to write integration tests but I should be able to choose to unit test controllers, just as easily.

I know I could probably reimplement these myself if I so wished such is the nature of ruby, but it feels a bit like fighting against the framework. I can't help but feel that there are better ways to point people towards integration tests without removing these helpers.

@Tekhne

This comment has been minimized.

Show comment
Hide comment
@Tekhne

Tekhne Mar 27, 2015

One of the foundations of object-oriented programming is the idea that encapsulated objects pass messages between each other to produce systemic behavior. When I'm testing that those objects are correctly implemented, I want to know that the messages they are sending to collaborators are happening at the right time, and in the right way. For better or worse, Rails controllers send messages to templates/views via ivars (instead of method calls). Therefore, I need to test that ivars are being set. I can certainly do that with things like #instance_variable_get, but I would prefer that #assigns and #assert_template stay in place.

Tekhne commented Mar 27, 2015

One of the foundations of object-oriented programming is the idea that encapsulated objects pass messages between each other to produce systemic behavior. When I'm testing that those objects are correctly implemented, I want to know that the messages they are sending to collaborators are happening at the right time, and in the right way. For better or worse, Rails controllers send messages to templates/views via ivars (instead of method calls). Therefore, I need to test that ivars are being set. I can certainly do that with things like #instance_variable_get, but I would prefer that #assigns and #assert_template stay in place.

@shekibobo

This comment has been minimized.

Show comment
Hide comment
@shekibobo

shekibobo Mar 27, 2015

Since iVars are considered a public interface for the views, and nobody really likes that we have to access what is considered private variables from outside the class, would it make sense to instead change the access of those iVars to be delivered from a public assigns method on the controller? Or perhaps from dynamically generated methods based on the assigned name? Assignment is a fundamental responsibility of Rails controllers, and to eliminate the method of testing because it breaks the rules that the views and controllers are already breaking seems misguided. Consider this usage:

# posts_controller.rb
def index
  assign :posts, Post.published
end
# index.html.erb
<% posts.each do |post| %>
  <%= render post %>
<% end %>

<!-- alternatively -->
<% assigns(:posts).each do |post| %>
  <%= render post %>
<% end %>

This way, we get a nice interface for the views to get things in the controller without accessing it's iVars, we can get an assigns method in the views (and directly on the controller) to access them by name or see what assigns are available, and we can still test this fundamentally important part of controllers through an interface that is consistent for all interactors.

shekibobo commented Mar 27, 2015

Since iVars are considered a public interface for the views, and nobody really likes that we have to access what is considered private variables from outside the class, would it make sense to instead change the access of those iVars to be delivered from a public assigns method on the controller? Or perhaps from dynamically generated methods based on the assigned name? Assignment is a fundamental responsibility of Rails controllers, and to eliminate the method of testing because it breaks the rules that the views and controllers are already breaking seems misguided. Consider this usage:

# posts_controller.rb
def index
  assign :posts, Post.published
end
# index.html.erb
<% posts.each do |post| %>
  <%= render post %>
<% end %>

<!-- alternatively -->
<% assigns(:posts).each do |post| %>
  <%= render post %>
<% end %>

This way, we get a nice interface for the views to get things in the controller without accessing it's iVars, we can get an assigns method in the views (and directly on the controller) to access them by name or see what assigns are available, and we can still test this fundamentally important part of controllers through an interface that is consistent for all interactors.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Mar 27, 2015

Member

For those missing assigns take a look at assert_select here: https://github.com/rails/rails-dom-testing.
That will let you test the interface of your views instead of what instance variables were used to build it.

The Rails team has heard your points, but we'll go ahead with the deprecations.
Thanks everyone ❤️

Kasper

Den 27/03/2015 kl. 16.11 skrev Joshua Kovach notifications@github.com:

Since iVars are considered a public interface for the views, and nobody really likes that we have to access what is considered private variables from outside the class, would it make sense to instead change the access of those iVars to be delivered from a public assigns method on the controller? Or perhaps from dynamically generated methods based on the assigned name? Assignment is a fundamental responsibility of Rails controllers, and to eliminate the method of testing because it breaks the rules that the views and controllers are already breaking seems misguided. Consider this usage:

posts_controller.rb

def index
assign :posts, Post.published
end

index.html.erb

<% posts.each do |post| %>
<%= render post %>
<% end %>

<% assigns(:posts).each do |post| %>
<%= render post %>
<% end %>
This way, we get a nice interface for the views to get things in the controller without accessing it's iVars, we can get an assigns method in the views (and directly on the controller) to access them by name or see what assigns are available, and we can still test this fundamentally important part of controllers through an interface that is consistent for all interactors.


Reply to this email directly or view it on GitHub.

Member

kaspth commented Mar 27, 2015

For those missing assigns take a look at assert_select here: https://github.com/rails/rails-dom-testing.
That will let you test the interface of your views instead of what instance variables were used to build it.

The Rails team has heard your points, but we'll go ahead with the deprecations.
Thanks everyone ❤️

Kasper

Den 27/03/2015 kl. 16.11 skrev Joshua Kovach notifications@github.com:

Since iVars are considered a public interface for the views, and nobody really likes that we have to access what is considered private variables from outside the class, would it make sense to instead change the access of those iVars to be delivered from a public assigns method on the controller? Or perhaps from dynamically generated methods based on the assigned name? Assignment is a fundamental responsibility of Rails controllers, and to eliminate the method of testing because it breaks the rules that the views and controllers are already breaking seems misguided. Consider this usage:

posts_controller.rb

def index
assign :posts, Post.published
end

index.html.erb

<% posts.each do |post| %>
<%= render post %>
<% end %>

<% assigns(:posts).each do |post| %>
<%= render post %>
<% end %>
This way, we get a nice interface for the views to get things in the controller without accessing it's iVars, we can get an assigns method in the views (and directly on the controller) to access them by name or see what assigns are available, and we can still test this fundamentally important part of controllers through an interface that is consistent for all interactors.


Reply to this email directly or view it on GitHub.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 27, 2015

Contributor

The assert_select approach lets you test the controler+view as an integrated unit.

Those of us unhappy with that want to test the controller as a unit isolated from the view. This is particularly useful/important when delivering an engine, where the controller (or mix-in to local controller) might be used with a locally overridden view -- you want the controller, as an isolated unit separated from the view, to have a contractual interface, so the local app can provide a view depending on that interface. Devise would be one example.

But okay, it's clear that Rails team does not see the value in testing controllers as an isolated unit separate from views, but thinks it's preferable to do systems or integration tests instead, or controller+view 'functional' tests.

Hopefully there's enough people who disagree that there will be capacity to create a third party plugin that restore the ability to test controller as an isolated unit, and maintain it across versions of rails that may otherwise break it. We will see.

Contributor

jrochkind commented Mar 27, 2015

The assert_select approach lets you test the controler+view as an integrated unit.

Those of us unhappy with that want to test the controller as a unit isolated from the view. This is particularly useful/important when delivering an engine, where the controller (or mix-in to local controller) might be used with a locally overridden view -- you want the controller, as an isolated unit separated from the view, to have a contractual interface, so the local app can provide a view depending on that interface. Devise would be one example.

But okay, it's clear that Rails team does not see the value in testing controllers as an isolated unit separate from views, but thinks it's preferable to do systems or integration tests instead, or controller+view 'functional' tests.

Hopefully there's enough people who disagree that there will be capacity to create a third party plugin that restore the ability to test controller as an isolated unit, and maintain it across versions of rails that may otherwise break it. We will see.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 28, 2015

Member

I'd be happy to see a plugin that provides this testing approach. Just because it's not in core doesn't mean you can't use it. If someone wants to do the work, we can update the deprecation notice to point to this plugin.

On Mar 27, 2015, at 18:51, Jonathan Rochkind notifications@github.com wrote:

The assert_select approach lets you test the controler+view as an integrated unit.

Those of us unhappy with that want to test the controller a unit isolated from the view. This is particularly useful/important when delivering an engine, where the controller (or mix-in to local controller) might be used with a locally overridden view -- you want the controller, as an isolated unit separated from the view, to have a contractual interface, so the local app can provide a view depending on that interface. Devise would be one example.

But okay, it's clear that Rails team does not see the value in testing controllers as an isolated unit separate from views, but thinks it's preferable to do systems or integration tests instead, or controller+view 'functional' tests.

Hopefully there's enough people who disagree that there will be capacity to create a third party plugin that restore the ability to test controller as an isolated unit, and maintain it across versions of rails that may otherwise break it. We will see.


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 28, 2015

I'd be happy to see a plugin that provides this testing approach. Just because it's not in core doesn't mean you can't use it. If someone wants to do the work, we can update the deprecation notice to point to this plugin.

On Mar 27, 2015, at 18:51, Jonathan Rochkind notifications@github.com wrote:

The assert_select approach lets you test the controler+view as an integrated unit.

Those of us unhappy with that want to test the controller a unit isolated from the view. This is particularly useful/important when delivering an engine, where the controller (or mix-in to local controller) might be used with a locally overridden view -- you want the controller, as an isolated unit separated from the view, to have a contractual interface, so the local app can provide a view depending on that interface. Devise would be one example.

But okay, it's clear that Rails team does not see the value in testing controllers as an isolated unit separate from views, but thinks it's preferable to do systems or integration tests instead, or controller+view 'functional' tests.

Hopefully there's enough people who disagree that there will be capacity to create a third party plugin that restore the ability to test controller as an isolated unit, and maintain it across versions of rails that may otherwise break it. We will see.


Reply to this email directly or view it on GitHub.

jprince pushed a commit to jprince/sleepers-and-keepers that referenced this issue Jan 1, 2016

Josh Prince
Upgrade to Rails 5 (beta)
Why:
Making the application "reactive" so that multiple users can participate in a draft, with real-time
updates pushed to their clients, necessitates the use of Rails 5's ActionCable.

How:
- Various config file updates
- General gem housekeeping. Includes updating the Devise and Capybara gems to their tips, since
  neither gem has released a Rails 5 compatible version yet. Also removes several gems that weren't used.
- In Rails 5, HTTP request parameters are no longer duck-typed to a hash (ActionController::Parameters
  are instead a sub-class of hash). As a result, conversion is needed to call some hash methods on them.
  The `deep_snake_case_params` method is also modified to ensure that these parameters get converted correctly.
- With Rails 5 the core team seems to be moving away from controller testing
  (rails/rails#18950), especially testing of iVars, in favor of integrations specs.
  Removed the only controller spec (with some logic/testing moved to a model) accordingly.

bf4 added a commit to bf4/active_model_serializers that referenced this issue Jan 4, 2016

Remove `assert_template` from Test::SerializerTest
Rails 5 removed this assertion after considering it not
a good testing practice. rails/rails#18950

Rather that add a gem to our Rails 5 matrix to support it,
the assertion is made that the template is rendering using
active support notifications.

Also, to clarify that the action 'render_template' is unrelated to the
event name '!render_template.action_view', I renamed the actions
so that would not look like event names.

bf4 added a commit to bf4/active_model_serializers that referenced this issue Jan 13, 2016

Remove `assert_template` from Test::SerializerTest
Rails 5 removed this assertion after considering it not
a good testing practice. rails/rails#18950

Rather that add a gem to our Rails 5 matrix to support it,
the assertion is made that the template is rendering using
active support notifications.

Also, to clarify that the action 'render_template' is unrelated to the
event name '!render_template.action_view', I renamed the actions
so that would not look like event names.

bf4 added a commit to bf4/active_model_serializers that referenced this issue Jan 14, 2016

Remove `assert_template` from Test::SerializerTest
Rails 5 removed this assertion after considering it not
a good testing practice. rails/rails#18950

Rather that add a gem to our Rails 5 matrix to support it,
the assertion is made that the template is rendering using
active support notifications.

Also, to clarify that the action 'render_template' is unrelated to the
event name '!render_template.action_view', I renamed the actions
so that would not look like event names.

bf4 added a commit to bf4/active_model_serializers that referenced this issue Jan 14, 2016

Remove `assert_template` from Test::SerializerTest
Rails 5 removed this assertion after considering it not
a good testing practice. rails/rails#18950

Rather that add a gem to our Rails 5 matrix to support it,
the assertion is made that the template is rendering using
active support notifications.

Also, to clarify that the action 'render_template' is unrelated to the
event name '!render_template.action_view', I renamed the actions
so that would not look like event names.

bf4 added a commit to bf4/active_model_serializers that referenced this issue Jan 14, 2016

Remove `assert_template` from Test::SerializerTest
Rails 5 removed this assertion after considering it not
a good testing practice. rails/rails#18950

Rather that add a gem to our Rails 5 matrix to support it,
the assertion is made that the template is rendering using
active support notifications.

Also, to clarify that the action 'render_template' is unrelated to the
event name '!render_template.action_view', I renamed the actions
so that would not look like event names.
@stevecj

This comment has been minimized.

Show comment
Hide comment
@stevecj

stevecj Jun 15, 2016

I know this is a closed issue and I pretty much agree with not asserting assignments, but I'm not making sense of the logic of not being able to assert the template.

I use Integration tests or Feature specs to test end-to-end, and then I want to be able to test alternative paths against the controller itself. I should not need to have my tests know about the HTML rendered by a template just to find out which template the controller decided to render. If a view is non-trivial and might break, then I'll usually want to test that separately from the controller.

To me, rendering a template is an outcome of the controller, not an "innard".

stevecj commented Jun 15, 2016

I know this is a closed issue and I pretty much agree with not asserting assignments, but I'm not making sense of the logic of not being able to assert the template.

I use Integration tests or Feature specs to test end-to-end, and then I want to be able to test alternative paths against the controller itself. I should not need to have my tests know about the HTML rendered by a template just to find out which template the controller decided to render. If a view is non-trivial and might break, then I'll usually want to test that separately from the controller.

To me, rendering a template is an outcome of the controller, not an "innard".

@Xenofex

This comment has been minimized.

Show comment
Hide comment
@Xenofex

Xenofex Jun 24, 2016

Agree with @jrochkind : ivar is the output of the controller and the input of the view, and controller testing should be there. A lot of web MVC frameworks use return value as the bridge between controllers and views. In that case, testing the return values of the controller methods seems fair.

I'm pretty happy that Rails didn't choose to use return value as the bridge between controllers and views, for which being too restricted.

I think besides the controller in an engine which could probably have another view at the client developer's hand, even if a view belongs exclusively to a controller which is the 95% of the case, I still want to test the controller without the view. Not being able to call the private/protected method during a test doesn't mean that those methods are really 'private' to the controller's internal usage. That's simply because the controller is designed to be calling the methods inside itself. The public interface is mostly the framework's concern. I even want to expose all the non action methods of a controller during a test, or at least those protected ones. For instance, checking controller's current_user method for testing my customized login/registration flow, is much more intuitive than asserting the templates, for the controller's correctness.

Xenofex commented Jun 24, 2016

Agree with @jrochkind : ivar is the output of the controller and the input of the view, and controller testing should be there. A lot of web MVC frameworks use return value as the bridge between controllers and views. In that case, testing the return values of the controller methods seems fair.

I'm pretty happy that Rails didn't choose to use return value as the bridge between controllers and views, for which being too restricted.

I think besides the controller in an engine which could probably have another view at the client developer's hand, even if a view belongs exclusively to a controller which is the 95% of the case, I still want to test the controller without the view. Not being able to call the private/protected method during a test doesn't mean that those methods are really 'private' to the controller's internal usage. That's simply because the controller is designed to be calling the methods inside itself. The public interface is mostly the framework's concern. I even want to expose all the non action methods of a controller during a test, or at least those protected ones. For instance, checking controller's current_user method for testing my customized login/registration flow, is much more intuitive than asserting the templates, for the controller's correctness.

fwoelm added a commit to OpenlyOne/alpha.upshift.network that referenced this issue Jul 2, 2016

Remove controller specs
Controller specs were failing because assert() is deprecated in Rails 5.0.

"As long as your controllers are slim and your models are fat, I think you can
get away without writing controller specs most of the time except for the API,
which is where request specs come in."
Source: https://www.reddit.com/r/rails/comments/34g0o6/
request_spec_vs_feature_spec_vs_controller_spec/cqueh5q

Rails is thinking about dropping controller tests entirely, in favor of
integration tests. See:
rails/rails#18950 (comment)

Made the decision to drop controller testing.

AEgan added a commit to rubyforgood/share_christmas that referenced this issue Aug 30, 2016

Update organization controller specs
This updates the organization controller tests to reflect the changes
to the controller. This also updates the ability file to avoid a
postgres error when checking for roles in rollify.

Worth noting that I avoided `assigns` because it is becoming an
antipattern. rails/rails#18950

fizz added a commit to fizz/junket that referenced this issue Sep 26, 2016

Workaround because Rails deprecated assigns()
"Testing what instance variables are set by your controller is a bad idea" - DHH.

Nonetheless, rails-rspec still does, so I installed the extracted gem rails-controller-testing.
C.f. rails/rails#18950

hugocorbucci added a commit to hugocorbucci/presentation-generator that referenced this issue Sep 27, 2016

Upgrading rails and mongoid.
Removing some controller tests as per rails/rails#18950
We were not really making use of it so...

@ammancilla ammancilla referenced this issue Jan 9, 2018

Closed

Enhancement #1

2 of 3 tasks complete
@vanboom

This comment has been minimized.

Show comment
Hide comment
@vanboom

vanboom Jun 14, 2018

Totally disagree with this philosophy like others above. Controller variables are not internals of the controller, they are the interface between the controller and the view and should be rigorously tested. Integration tests do not allow you to test specifically enough.

Simple example: a view formats a date <%= @startdate.to_s(:long) %> Is it not reasonable to test the controller to ensure that @startdate is never nil?

Furthermore, please do not change the method of controller to view interface in order to justify this deprecation. Setting an ivar in the controller and using it in the view is one of the beautiful RoR conventions that enable great productivity imho. Poll the community and you will find everyone using the gem and moving forward into Rails 5 with controller tests that use assigns(...).

vanboom commented Jun 14, 2018

Totally disagree with this philosophy like others above. Controller variables are not internals of the controller, they are the interface between the controller and the view and should be rigorously tested. Integration tests do not allow you to test specifically enough.

Simple example: a view formats a date <%= @startdate.to_s(:long) %> Is it not reasonable to test the controller to ensure that @startdate is never nil?

Furthermore, please do not change the method of controller to view interface in order to justify this deprecation. Setting an ivar in the controller and using it in the view is one of the beautiful RoR conventions that enable great productivity imho. Poll the community and you will find everyone using the gem and moving forward into Rails 5 with controller tests that use assigns(...).

@robzolkos

This comment has been minimized.

Show comment
Hide comment
@robzolkos

robzolkos Jun 14, 2018

Contributor

@vanboom Why can't you test that @startdate is set in the view? As you said, they are the interface between controller and view. Rails by default lets you check the assignment in the view and optionally (for those that prefer the original implementation there is the gem). There can only be one default.

Poll the community and you will find everyone

And if you polled the community there are many that prefer the new way, myself included. It works great.

Contributor

robzolkos commented Jun 14, 2018

@vanboom Why can't you test that @startdate is set in the view? As you said, they are the interface between controller and view. Rails by default lets you check the assignment in the view and optionally (for those that prefer the original implementation there is the gem). There can only be one default.

Poll the community and you will find everyone

And if you polled the community there are many that prefer the new way, myself included. It works great.

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