Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Michael Koziarski July 12th, 2008 @ 10:47 AM

  • State changed from “new” to “committed”

  I've restored most of this functionality in:

  http://github.com/rails/rails/co...

  I'm still not sold on the test case you submitted originally, you're coupling your assertion to the implementation of your routes. But hopefully this is a decent enough middle ground.

  If you feel strongly about it, grab me in irc the next time you see me and we can talk about it a little more.

• You flagged this item as spam.
  

  Andrew Vit July 12th, 2008 @ 10:57 PM

  Thanks for the quick patch... I was just trying it now, but it doesn't really change what I illustrated. If the controller says redirect_to string and the test doesn't have all the url params derived by the controller, then it can't know the whole url string.

  I guess it basically means either don't use url helpers in controllers if you wanna test it, or hack around these limitations in the tests.

  I don't know if you're right to say that my example assertion is too coupled to the routes... If anything, I think I'm trying to keep my test concerns within the bounds of the request logic, and leave the implementation of the routes to themselves.

  The whole thing boils down to one's opinion of what we should actually be testing here:

  1. Should functional tests concern themselves only with the external appearance of the URL, and just verify that we're redirecting to a specific address?
  2. Or, should we test that the redirect connects to the right endpoint (action & params), ignoring the format of the URLs as a separate issue--an abstraction that's done for the HTTP round-trip, and left up to the routes?

  I personally still prefer the idea of comparing the hashes because that represents our functional interface. The url strings are the external representation, and to me the format of those is a separate concern.

  Just to show where I'm coming from, I have to admit that I never even considered that I was redirecting to either a string or a params hash... at some point it gets converted to a string anyway, then back to a hash again on the other side. Either way, this black box magically _just works_, so why shouldn't it work in our tests? :-)

  It's not a real biggie, but since it worked before I'd like to leave it open for discussion...

• 

  Michael Koziarski July 13th, 2008 @ 07:53 AM

  I still kinda think the assertion that you're making isn't right, especially when you have access to assigns in order to make the full assertion.

  It's not about the appearance of the URL either, you can pass any argument to assert_redirected_to which resolves to the same place as they would in redirect_to

    redirect_to @customer
    assert_redirected_to customer_url(@customer)
    assert_redirected_to customer_url(:id=>@customer)
    assert_redirected_to @customer
    assert_redirected_to :action=>'show', :id=>@customer
  

  If you change your controller to redirect_to :action=>'edit' it goes somewhere completely different to where it does now. Which makes me think that the assertion you're making is a little broken.

  Similarly if you changed the edit_foo_url named route to point to some other action (ludicrous proposition for edit, but bear with me). Your controller doesn't change, it's still redirecting to the 'edit' url, but your assertion fails because the action is only specified in routes.rb

  With all the tools like mocking and assigns available to you, I can't see a situation where you need to do it the way you were. But I fully admit I could be missing something.

• You flagged this item as spam.
  

  Andrew Vit July 13th, 2008 @ 09:27 AM

  Ok, let's leave it unless anyone else has an opinion. My closing arguments:

  "you can pass any argument to assert_redirected_to which resolves to the same place as they would in redirect_to"

  Understood, but then it's just a question of repeating yourself in the tests exactly, and you can't really isolate the params/route segments that are relevant and ignore the rest.

  "If you change your controller to redirect_to :action=>'edit' it goes somewhere completely different to where it does now."

  Yes, and the assertion would fail as expected, no? Why would you call that broken?

  "Similarly if you changed the edit_foo_url named route to point to some other action...your assertion fails because the action is only specified in routes.rb"

  I would prefer that it throw an error if it can't find a new route to the asserted action, instead of a false pass because the URL looks the same but now points to a different action.

  "you have access to assigns"

  I still don't like this idea--assigns is for the view on the far side of the redirect. Isn't this like saying "assert that I was redirected to the id that I was redirected to"?

  Mocking will have to be the solution then.

• 

  Michael Koziarski July 13th, 2008 @ 09:31 AM

  I still don't like this idea--assigns is for the view on the far side of the redirect. Isn't this like saying "assert that I was redirected to the id that I was redirected to"?

  I think you misunderstand assigns, it's not for the view on the far side of the redirect, it's for the action that was just executed. So because you have

    @foo = Foo.create!
    redirect_to @foo
  

  You can use

    assert_redirected_to assigns(:foo)
  

• You flagged this item as spam.
  

  Andrew Vit July 13th, 2008 @ 09:33 AM

  Thanks... that does clear up a part of it.

• 

  Michael Koziarski July 13th, 2008 @ 09:42 AM

  I'll bounce this ticket off a few people and see if they have strong opinions on it.