Helpers from ApplicationHelper are included when they shouldn't be #831

Closed
jacob-carlborg opened this Issue Oct 2, 2013 · 16 comments

Comments

Projects
None yet
3 participants

We have an Rails application with two application controllers, one standard, ApplicationController and one namespaced to Admin::ApplicationController. Both of these two inherit from ActionController::Base, so they are completely separated.

When I normally run this application ApplictionHelper is only included when ApplicationController is used and Admin::ApplictionHelper is only included when Admin::ApplicationController is used. But when I run the specs for this application ApplictionHelper is always included, regardless if I test a view or helper which is namespaced to admin or not.

The issue is here:

https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/helper_example_group.rb#L21

And here:

https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/view_example_group.rb#L21

Owner

JonRowe commented Oct 2, 2013

@alindeman or @dchelimsky can you shed light on why we do this?

Hmm, actually, those two lines may not be the problem. I removed both of them and ApplicationHelper is still included. This might be a problem in Rails.

BTW, I'm using RSpec Rails 2.11.4 and Rails 3.2.11.

Owner

JonRowe commented Oct 2, 2013

Well AFAIK the default Rails behaviour is to include ApplicationHelper in all controllers by default...

Not if the application controller is namespaced and I call clear_helpers.

I've done some more investigation. Calling clear_helpers in the controller is what removes the helper methods coming from the application helper that is not namespaced.

The problem seems to be in Rails. The controller used in a view spec has nothing to do with the controller used when viewing the site. Instead the controller in the view spec is an instance of this class: ActionView::TestCase::TestController. Of course, this controller doesn't clear all helpers, like I do in my controllers.

I managed to monkey patch Rails to set the controller class in the view spec. Now I can use a controller looking like this:

class Admin::ApplicationController < ActionView::TestCase::TestController
  clear_helpers
end

Now I've instead encountered what I thought was my original problem, this:

https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/view_example_group.rb#L21

The above code was added in this commit: a4200ef. I guess it was added to work correctly when using a config, like this:

config.action_controller.include_all_helpers = false

But the helpers should not be added if clear_helpers has been called. �I guess I could monkey patch that as well.

Owner

JonRowe commented Oct 3, 2013

For the most part we won't monkey patch Rails to fix their issues, we open PR's to help them, is there anything left that is actually an issue within RSpec?

For the most part we won't monkey patch Rails to fix their issues, we open PR's to help them

I understand that.

is there anything left that is actually an issue within RSpec?

Yes, this code should not be called if clear_helpers has been called: https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/example/view_example_group.rb#L21. I can see if I can do a pull request.

I created an issue for Rails, rails/rails#12432. I've been thinking a bit about this. The standard Rails test system works differently when it comes to views. To test the views it seems one needs to use functional tests. As far as I can see, they're basically not making any difference between tests for controller and tests for views.

This might actually be an RSpec problem after all, I'm not sure.

Owner

JonRowe commented Oct 4, 2013

If your view tests are isolated from your controller, then clear_helpers is never invoked so we don't know, if your doing integration tests with your controllers then clear_helpers should be invoked by that.

I just want the correct helpers to be available to the view. I guess I probably should have created an engine, or something like that.

Owner

JonRowe commented Oct 5, 2013

I'm open to a PR that adds a clear_helpers or set_helpers type setup to this so you can clear them and set the appropriate ones you wish to use.

Cool, I see what I can do.

Member

cupakromer commented Apr 28, 2014

Ping, @jacob-carlborg is this still something you're interested in?

@cupakromer no, most likely not. Sorry.

Member

cupakromer commented Apr 28, 2014

Close as this is no longer requested.

cupakromer closed this Apr 28, 2014

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