Controller.url_for stub in ActionPack / UrlHelperTest doesn't do anything useful

Hi,

I've been trying to patch a [perceived] bug in UrlHelper#current_page? when I realized that the UrlHelperTest's controller stub was causing any call to url_for in that test to always the URL we're on.

Consider the test test_url_for_escapes_urls:

  def test_url_for_escapes_urls     @controller.url = "Example Domain;     assert_equal "Example Domain, url_for(:a => 'b', :c => 'd')     assert_equal "Example Domain, url_for(:a => 'b', :c => 'd', :escape => true)     assert_equal "Example Domain, url_for(:a => 'b', :c => 'd', :escape => false)   end

If you change any of the url_for calls from :c => 'd' to :c => 'e', the test still passes, even though it clearly shouldn't. This is because UrlHelper#url_for calls <controller>.url_for, and in the tests, @controller.url_for (declared in the setup method) simply returns the URL we're on.

I have a solution started that looks like this:

  def setup     @controller = Class.new do       include ActionController::UrlWriter       attr_accessor :url, :request       alias :old_url_for :url_for       def url_for(options)         options[:host] = "www.example.com" unless options[:host]         options[:controller] = "foo" unless options[:controller]         options[:action] = "bar" unless options[:action]         old_url_for(options)       end     end     @controller = @controller.new     @controller.url = "http://www.example.com"   end

If you make this change, you'll get about 6 failing tests; the tests need to be updated.

Does this approach make sense? If I correct and improve the tests, would this approach get the blessing of a committer? If not, could someone suggest what I should do to make this right?

Thanks! Paul.

If you make this change, you'll get about 6 failing tests; the tests need to be updated.

What makes the tests fail? are the assertions wrong or?

Does this approach make sense? If I correct and improve the tests, would this approach get the blessing of a committer? If not, could someone suggest what I should do to make this right?

The general approach you're talking about sounds perfectly fine, let us know when you have a patch ready :slight_smile:

Hi,

Sorry it took so long to get around to this. I just had my first block of free time where I could get into the code. I appreciate your responding to my message, and I hope I can convince you of the changes below!

> If you make this change, you'll get about 6 failing tests; the tests > need to be updated.

What makes the tests fail? are the assertions wrong or?

Usually it's minor: the leading http://www.example.com shouldn't be in the assertion because it's not truly returned in url_for unless :only_path => false. In some cases, the test was actually just flat wrong.

I am including my whole diff below, and I'll explain each change in turn.

I hope to hear from you with any suggestions before I submit this as a patch.

Index: test/template/url_helper_test.rb

+1

This looks very well thought out, thanks for spending your time improving Rails tests!

Sorry it took so long to get around to this. I just had my first block of free time where I could get into the code. I appreciate your responding to my message, and I hope I can convince you of the changes below!

This is awesome work, thanks a bunch for the explanation! My only advice is that when you submit your patch consider including a few comments near assertions which took you a while to figure out.