Browse files

Simplify the implementation of assert_redirected_to to normalise the …

…urls before comparing. Also allows for a simpler implementation of redirect_to without most of the recursion.

Also allows for assert_redirected_to @some_record
  • Loading branch information...
1 parent db58391 commit c3aaba0180f0710094d974b4ba4659bce81446df @NZKoz NZKoz committed Jun 23, 2008
View
5 actionpack/CHANGELOG
@@ -18,6 +18,11 @@
* Made ActionView::Base#render_file private [Josh Peek]
+* Refactor and simplify the implementation of assert_redirected_to. Arguments are now normalised relative to the controller being tested, not the root of the application. [Michael Koziarski]
+
+ This could cause some erroneous test failures if you were redirecting between controllers
+ in different namespaces and wrote your assertions relative to the root of the application.
+
* Remove follow_redirect from controller functional tests.
If you want to follow redirects you can use integration tests. The functional test
View
96 actionpack/lib/action_controller/assertions/response_assertions.rb
@@ -56,75 +56,18 @@ def assert_response(type, message = nil)
# # assert that the redirection was to the named route login_url
# assert_redirected_to login_url
#
+ # # assert that the redirection was to the url for @customer
+ # assert_redirected_to @customer
+ #
def assert_redirected_to(options = {}, message=nil)
clean_backtrace do
assert_response(:redirect, message)
return true if options == @response.redirected_to
- ActionController::Routing::Routes.reload if ActionController::Routing::Routes.empty?
-
- begin
- url = {}
- original = { :expected => options, :actual => @response.redirected_to.is_a?(Symbol) ? @response.redirected_to : @response.redirected_to.dup }
- original.each do |key, value|
- if value.is_a?(Symbol)
- value = @controller.respond_to?(value, true) ? @controller.send(value) : @controller.send("hash_for_#{value}_url")
- end
-
- unless value.is_a?(Hash)
- request = case value
- when NilClass then nil
- when /^\w+:\/\// then recognized_request_for(%r{^(\w+://.*?(/|$|\?))(.*)$} =~ value ? $3 : nil)
- else recognized_request_for(value)
- end
- value = request.path_parameters if request
- end
-
- if value.is_a?(Hash) # stringify 2 levels of hash keys
- if name = value.delete(:use_route)
- route = ActionController::Routing::Routes.named_routes[name]
- value.update(route.parameter_shell)
- end
-
- value.stringify_keys!
- value.values.select { |v| v.is_a?(Hash) }.collect { |v| v.stringify_keys! }
- if key == :expected && value['controller'] == @controller.controller_name && original[:actual].is_a?(Hash)
- original[:actual].stringify_keys!
- value.delete('controller') if original[:actual]['controller'].nil? || original[:actual]['controller'] == value['controller']
- end
- end
-
- if value.respond_to?(:[]) && value['controller']
- value['controller'] = value['controller'].to_s
- if key == :actual && value['controller'].first != '/' && !value['controller'].include?('/')
- new_controller_path = ActionController::Routing.controller_relative_to(value['controller'], @controller.class.controller_path)
- value['controller'] = new_controller_path if value['controller'] != new_controller_path && ActionController::Routing.possible_controllers.include?(new_controller_path) && @response.redirected_to.is_a?(Hash)
- end
- value['controller'] = value['controller'][1..-1] if value['controller'].first == '/' # strip leading hash
- end
- url[key] = value
- end
-
- @response_diff = url[:actual].diff(url[:expected]) if url[:actual]
- msg = build_message(message, "expected a redirect to <?>, found one to <?>, a difference of <?> ", url[:expected], url[:actual], @response_diff)
-
- assert_block(msg) do
- url[:expected].keys.all? do |k|
- if k == :controller then url[:expected][k] == ActionController::Routing.controller_relative_to(url[:actual][k], @controller.class.controller_path)
- else parameterize(url[:expected][k]) == parameterize(url[:actual][k])
- end
- end
- end
- rescue ActionController::RoutingError # routing failed us, so match the strings only.
- msg = build_message(message, "expected a redirect to <?>, found one to <?>", options, @response.redirect_url)
- url_regexp = %r{^(\w+://.*?(/|$|\?))(.*)$}
- eurl, epath, url, path = [options, @response.redirect_url].collect do |url|
- u, p = (url_regexp =~ url) ? [$1, $3] : [nil, url]
- [u, (p.first == '/') ? p : '/' + p]
- end.flatten
+ redirected_to_after_normalisation = normalize_argument_to_redirection(@response.redirected_to)
+ options_after_normalisation = normalize_argument_to_redirection(options)
- assert_equal(eurl, url, msg) if eurl && url
- assert_equal(epath, path, msg) if epath && path
- end
+ assert_equal redirected_to_after_normalisation, options_after_normalisation,
+ "Expected response to be a redirect to <#{options_after_normalisation}> but was a redirect to <#{redirected_to_after_normalisation}>"
end
end
@@ -150,23 +93,24 @@ def assert_template(expected = nil, message=nil)
end
private
- # Recognizes the route for a given path.
- def recognized_request_for(path, request_method = nil)
- path = "/#{path}" unless path.first == '/'
-
- # Assume given controller
- request = ActionController::TestRequest.new({}, {}, nil)
- request.env["REQUEST_METHOD"] = request_method.to_s.upcase if request_method
- request.path = path
-
- ActionController::Routing::Routes.recognize(request)
- request
- end
# Proxy to to_param if the object will respond to it.
def parameterize(value)
value.respond_to?(:to_param) ? value.to_param : value
end
+
+ def normalize_argument_to_redirection(fragment)
+ after_routing = @controller.url_for(fragment)
+ if after_routing =~ %r{^\w+://.*}
+ after_routing
+ else
+ # FIXME - this should probably get removed.
+ if after_routing.first != '/'
+ after_routing = '/' + after_routing
+ end
+ @request.protocol + @request.host_with_port + after_routing
+ end
+ end
end
end
end
View
32 actionpack/lib/action_controller/base.rb
@@ -1042,29 +1042,31 @@ def redirect_to(options = {}, response_status = {}) #:doc:
status = 302
end
+ response.redirected_to= options
+ logger.info("Redirected to #{options}") if logger && logger.info?
+
case options
when %r{^\w+://.*}
- raise DoubleRenderError if performed?
- logger.info("Redirected to #{options}") if logger && logger.info?
- response.redirect(options, interpret_status(status))
- response.redirected_to = options
- @performed_redirect = true
-
+ redirect_to_full_url(options, status)
when String
- redirect_to(request.protocol + request.host_with_port + options, :status=>status)
-
+ redirect_to_full_url(request.protocol + request.host_with_port + options, status)
when :back
- request.env["HTTP_REFERER"] ? redirect_to(request.env["HTTP_REFERER"], :status=>status) : raise(RedirectBackError)
-
- when Hash
- redirect_to(url_for(options), :status=>status)
- response.redirected_to = options
-
+ if referer = request.headers["Referer"]
+ redirect_to(referer, :status=>status)
+ else
+ raise RedirectBackError
+ end
else
- redirect_to(url_for(options), :status=>status)
+ redirect_to_full_url(url_for(options), status)
end
end
+ def redirect_to_full_url(url, status)
+ raise DoubleRenderError if performed?
+ response.redirect(url, interpret_status(status))
+ @performed_redirect = true
+ end
+
# Sets a HTTP 1.1 Cache-Control header. Defaults to issuing a "private" instruction, so that
# intermediate caches shouldn't cache the response.
#
View
10 actionpack/test/controller/action_pack_assertions_test.rb
@@ -232,7 +232,6 @@ def test_assert_redirect_to_named_route
process :redirect_to_named_route
assert_redirected_to 'http://test.host/route_one'
assert_redirected_to route_one_url
- assert_redirected_to :route_one_url
end
end
@@ -253,9 +252,6 @@ def test_assert_redirect_to_named_route_failure
assert_raise(Test::Unit::AssertionFailedError) do
assert_redirected_to route_two_url
end
- assert_raise(Test::Unit::AssertionFailedError) do
- assert_redirected_to :route_two_url
- end
end
end
@@ -432,14 +428,16 @@ def test_assert_redirection_with_extra_controller_option
assert_redirected_to :controller => 'action_pack_assertions', :action => "flash_me", :id => 1, :params => { :panda => 'fun' }
end
- def test_redirected_to_url_leadling_slash
+ def test_redirected_to_url_leading_slash
process :redirect_to_path
assert_redirected_to '/some/path'
end
+
def test_redirected_to_url_no_leadling_slash
process :redirect_to_path
assert_redirected_to 'some/path'
end
+
def test_redirected_to_url_full_url
process :redirect_to_path
assert_redirected_to 'http://test.host/some/path'
@@ -459,7 +457,7 @@ def test_assert_redirection_with_symbol
def test_redirected_to_with_nested_controller
@controller = Admin::InnerModuleController.new
get :redirect_to_absolute_controller
- assert_redirected_to :controller => 'content'
+ assert_redirected_to :controller => '/content'
get :redirect_to_fellow_controller
assert_redirected_to :controller => 'admin/user'
View
2 actionpack/test/controller/components_test.rb
@@ -119,7 +119,7 @@ def test_flash
def test_component_redirect_redirects
get :calling_redirected
- assert_redirected_to :action => "being_called"
+ assert_redirected_to :controller=>"callee", :action => "being_called"
end
def test_component_multiple_redirect_redirects
View
19 actionpack/test/controller/redirect_test.rb
@@ -168,21 +168,6 @@ def test_simple_redirect_using_options
assert_redirected_to :action => "other_host", :only_path => false, :host => 'other.test.host'
end
- def test_redirect_error_with_pretty_diff
- get :host_redirect
- assert_response :redirect
- begin
- assert_redirected_to :action => "other_host", :only_path => true
- rescue Test::Unit::AssertionFailedError => err
- expected_msg, redirection_msg, diff_msg = err.message.scan(/<\{[^\}]+\}>/).collect { |s| s[2..-3] }
- assert_match %r("only_path"=>false), redirection_msg
- assert_match %r("host"=>"other.test.host"), redirection_msg
- assert_match %r("action"=>"other_host"), redirection_msg
- assert_match %r("only_path"=>false), diff_msg
- assert_match %r("host"=>"other.test.host"), diff_msg
- end
- end
-
def test_module_redirect
get :module_redirect
assert_response :redirect
@@ -235,9 +220,11 @@ def test_redirect_to_record
get :redirect_to_existing_record
assert_equal "http://test.host/workshops/5", redirect_to_url
+ assert_redirected_to Workshop.new(5, false)
get :redirect_to_new_record
assert_equal "http://test.host/workshops", redirect_to_url
+ assert_redirected_to Workshop.new(5, true)
end
def test_redirect_to_nil
@@ -283,7 +270,7 @@ def test_module_redirect
def test_module_redirect_using_options
get :module_redirect
assert_response :redirect
- assert_redirected_to :controller => 'redirect', :action => "hello_world"
+ assert_redirected_to :controller => '/redirect', :action => "hello_world"
end
end
end

0 comments on commit c3aaba0

Please sign in to comment.