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 = "http://www.example.com?a=b&c=d"
    assert_equal "http://www.example.com?a=b&c=d", url_for(:a =>
'b', :c => 'd')
    assert_equal "http://www.example.com?a=b&c=d", url_for(:a =>
'b', :c => 'd', :escape => true)
    assert_equal "http://www.example.com?a=b&c=d", 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.