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