Add an assertion for testing redirect in routes.rb #2488

Closed
mattwynne opened this Issue Aug 10, 2011 · 25 comments

Projects

None yet
@mattwynne

I'd like to use the redirect method in my routes configuration, but I'd also like to be able to cover it with unit tests for the routing.

I can't see an assertion for the routing in [1] that will test a redirect in the routes. Did I miss something?

[1] https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/testing/assertions/routing.rb

@matthuhiggins

I've been using integration tests

class RedirectsTest < ActionController::IntegrationTest
  test 'redirect' do
    get '/foo'
    assert_redirected_to '/bar'
  end
end
@mattwynne

That's how I've worked around it too, but I'd like to keep all my routing tests in one place if possible.

I'd be happy to try and work on a patch if someone can give me some pointers where to start.

@pjammer
pjammer commented Sep 10, 2011

I've just started looking into this today, myself. One of the other forums mentioned that "functional tests don't actually use the routes" ruby-forum so i was looking into what he meant? Maybe this person means like integration tests, but we all know that assert_routing works in functional tests quite nice. I think we should be able to see this style of assertion (and refute) as well in functional tests.

Question is how, as you say mattwynne. any luck with a patch? I'd be happy to help too or come up with one, I just don't want to ride your coat tails, as you obviously beat me to the punch on the whole issue. :-)

@mattwynne

I haven't started looking at a patch, no.

@isaacsanders
Contributor

@mattwynne Is this still an issue?

@mattwynne

Well, as far as I know it hasn't been implemented. I would be glad to help implement it, but I would need some pointers from someone with more knowledge of the codebase. If nobody has time to do that, please just close the ticket.

@steveklabnik
Member

I wouldn't mind helping, but I'm not 100% sure where you should start looking, either.

@isaacsanders
Contributor

I think that it may be part of Rack::Test, but that is only a slight feeling from something that I may have read at some point.

@steveklabnik
Member

this then this then this

@mattwynne

@steveklabnik isn't that stuff for integration testing though? I raised this ticket because I'd like to be able to spec redirect routing config the same as I can other routing config, with focussed unit tests.

@isaacsanders
Contributor

Yes, but It can be used effectively for testing what you are looking for.

@pixeltrix
Member

@mattwynne do you have an example of your unit test? Are you using ActiveSupport::TestCase?

@steveklabnik
Member

@matwynne I meant that it might give you pointers on where to start looking. And to confirm that @isaacsanders did see something about it in Rack::Test.

@isaacsanders
Contributor

Thanks Steve, I knew I wasn't that crazy. On this count.

@pixeltrix
Member

FYI, assert_routing underneath the hood uses recognize_path which only matches ‘simple routes’ (i.e. hash based ones) - however it would be possible to enhance recognize_path to return the Rack response that a redirect generates. Once you have that you would be able to assert the contents of it.

If you don't think you're able to make the changes can you give me an example of how it'd be used and I can make the enhancement myself.

@pixeltrix
Member

Actually, leave this to me - I've got the bare bones of an implementation worked out.

@pixeltrix pixeltrix was assigned Apr 30, 2012
@mattwynne

@pixeltrix. Here's an example. Suppose I have this in my routes:

 match ':owner/file/:file', :to => redirect("/%{owner}/docs/%{file}")

I'd like to be able to put this in my spec/routing/routing_spec.rb:

it "redirects legacy requests for files" do
  { :get => "/matt/file/foo" }.should redirect_to("/matt/docs/foo")
end
@georgeguimaraes
Contributor

My main problem with the redirect method is the lack of environment check like relative_url_root. I ended up using something like:

  match '/admin', :to => redirect("#{Rails.configuration.relative_url_root}/admin/projects")

Do you think this is a problem as well?

@andrewferk
Contributor

There is a problem with not being able to test redirects in routing, as it can lead to false positives. If you have this routing:

get 'help' => redirect('http://help.example.org/')
get 'help' => 'help#index'

and this test:

test "should recognize /help as help#index" do
  assert_recognizes({:controller => 'help', :action => 'index'}, '/help')
end

the test passes, but actually, it should recognize /help as a redirect. Obviously, this may not be a real world example, but it's about as barebones as i could make it to show the issue.

@andrewferk
Contributor

I have been playing around with this for the last five hours, and @pixeltrix was correct about recognize_path being the culprit. It seems the entire redirect feature in routing is not supported by recognize_path.

I have modified recognize_path and assert_recognizes to support url redirects in routing. Here's a diff.

This would allow for:

get 'help' => redirect("http://example.org/")
assert_recognizes("http://example.org/", "/help")

Does that seem reasonable?

@steveklabnik
Member

Thanks for putting in the time on this, @andrewferk! I don't see anything wrong with the patch, but that's a part of Rails I'm not super familiar with, so we'll see what @pixeltrix says.

Since you have the patch already, why not open up a PR so we can discuss the implementation there?

@andrewferk
Contributor

Done deal man: #6922.

@mattwynne mattwynne added the stale label Apr 23, 2014
@rafaelfranca
Member

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@laurocaetano laurocaetano removed the stale label May 2, 2014
@mikegee
mikegee commented Jun 8, 2014

The conversation has moved to #6922. I recommend closing this issue.

@sgrif
Member
sgrif commented Jun 15, 2014

Closing. If you'd like to discuss the PR, please do so at #6922. If you'd like to discuss a feature request, please email the rails-core mailing group

@sgrif sgrif closed this Jun 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment