Rails core - Feb 2008 - 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.


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> 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 :)



-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

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!

On Feb 9, 3:14 am, "Michael Koziarski" <mich...@koziarski.com>
wrote:
> > 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
==================================================================---
test/template/url_helper_test.rb	(revision 8906)
+++ test/template/url_helper_test.rb	(working copy)
@@ -9,9 +9,14 @@

   def setup
     @controller = Class.new do
+      include ActionController::UrlWriter
       attr_accessor :url, :request
+      alias :old_url_for :url_for
       def url_for(options)
-        url
+        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

This is the big change. This way we use the actual UrlWriter#url for
our controller''s url_for, rather than just a stub. Of course, I had to
pre-seed some of the values (:host, :controller and :action) because
url_for barfs when they''re not present
(ActionController::RoutingError: Need controller and action!).

@@ -20,9 +25,9 @@

   def test_url_for_escapes_urls
     @controller.url = "http://www.example.com?a=b&c=d"
-    assert_equal "http://www.example.com?a=b&amp;c=d", url_for(:a
=>
''b'', :c => ''d'')
-    assert_equal "http://www.example.com?a=b&amp;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)
+    assert_equal "/foo/bar?a=b&amp;c=d", url_for(:a =>
''b'', :c =>
''d'')
+    assert_equal "/foo/bar?a=b&amp;c=d", url_for(:a =>
''b'', :c =>
''d'', :escape => true)
+    assert_equal "/foo/bar?a=b&c=d", url_for(:a =>
''b'', :c =>
''d'', :escape => false)
   end

In this case I removed the leading http://www.example.com; everything
else stayed the same. The reason is that url_for doesn''t actually
return the proto & domain unless you pass in :only_path => false. This
happens a few more times below.


@@ -134,7 +139,7 @@
   end

   def test_link_with_nil_html_options
-    assert_dom_equal "<a
href=\"http://www.example.com\">Hello</a>",
link_to("Hello", {:action => ''myaction''}, nil)
+    assert_dom_equal "<a
href=\"/foo/myaction\">Hello</a>",
link_to("Hello", {:action => ''myaction''}, nil)
   end

As above, the proto & domain don''t actually appear in the response.
Further, this assertion was just flat wrong, because if we''re pointing
to action ''myaction'', shouldn''t the action appear in
the URL on the LH
side of the assert_dom_equal?


@@ -216,7 +221,7 @@

   def test_link_to_unless
     assert_equal "Showing", link_to_unless(true, "Showing",
:action
=> "show", :controller => "weblog")
-    assert_dom_equal "<a
href=\"http://www.example.com\">Listing</
a>", link_to_unless(false, "Listing", :action =>
"list", :controller
=> "weblog")
+    assert_dom_equal "<a
href=\"/weblog/list\">Listing</a>",
link_to_unless(false, "Listing", :action => "list",
:controller =>
"weblog")
     assert_equal "Showing", link_to_unless(true, "Showing",
:action
=> "show", :controller => "weblog", :id => 1)
     assert_equal "<strong>Showing</strong>",
link_to_unless(true,
"Showing", :action => "show", :controller =>
"weblog", :id => 1) { |
name, options, html_options|
       "<strong>#{name}</strong>"

Again, it seems to me that this assertion was just plain wrong. If
link_to_unless is supposed to return a link, then the link that it
should return should at least include the controller and action
specified.


@@ -231,19 +236,19 @@

   def test_link_to_if
     assert_equal "Showing", link_to_if(false, "Showing",
:action =>
"show", :controller => "weblog")
-    assert_dom_equal "<a
href=\"http://www.example.com\">Listing</
a>", link_to_if(true, "Listing", :action =>
"list", :controller =>
"weblog")
+    assert_dom_equal "<a
href=\"/weblog/list\">Listing</a>",
link_to_if(true, "Listing", :action => "list",
:controller =>
"weblog")
     assert_equal "Showing", link_to_if(false, "Showing",
:action =>
"show", :controller => "weblog", :id => 1)
   end

Same as above. This assertion appears to be wrong.


   def test_link_unless_current
-    @controller.request = RequestMock.new("http://www.example.com/
weblog/show")
+    @controller.request = RequestMock.new("/weblog/show",
"http://",
"www.example.com")
-     @controller.url = "http://www.example.com/weblog/show"
     assert_equal "Showing",
link_to_unless_current("Showing",
{ :action => "show", :controller => "weblog" })
     assert_equal "Showing",
link_to_unless_current("Showing", "http://
www.example.com/weblog/show")

-     @controller.request = RequestMock.new("http://www.example.com/
weblog/show")
+    @controller.request = RequestMock.new("/weblog/show",
"http://",
"www.example.com")
-     @controller.url = "http://www.example.com/weblog/list"
-    assert_equal "<a href=\"http://www.example.com/weblog/list
\">Listing</a>",
+    assert_equal "<a
href=\"/weblog/list\">Listing</a>",
       link_to_unless_current("Listing", :action =>
"list", :controller => "weblog")
     assert_equal "<a href=\"http://www.example.com/weblog/list
\">Listing</a>",
       link_to_unless_current("Listing", "http://www.example.com/
weblog/list")

This was a bit of a challenging fix. Basically, unless the RequestMock
was broken down into its constituent parts (request_uri, proto,
domain) the current_page? method wouldn''t correctly identify if the
request was for the page we were on. I also determined that setting
the @controller.url appears to be noise in this context, and simply
removing it does well enough. After that, the only thing that was
actually wrong in the assertion was the path.

The funny thing about the changes above is that no changes were
required to any of the non-test code. I actually used the non-test
code as a reference for how things "should" behave in order to fix the
test so it expects the [correct] results that the code was already
giving.

All the changes above are just to make all the tests more effective.

I look forward to your comments!

Paul.


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Tue, Feb 19, 2008 at 6:05 PM, Paul Doerwald <doerwald@gmail.com>
wrote:
>  All the changes above are just to make all the tests more effective.
>
>  I look forward to your comments!
+1

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

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

>  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.


-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---