mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
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
This commit is contained in:
parent
db58391079
commit
c3aaba0180
6 changed files with 50 additions and 114 deletions
|
@ -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
|
||||
|
|
|
@ -56,75 +56,18 @@ module ActionController
|
|||
# # 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?
|
||||
redirected_to_after_normalisation = normalize_argument_to_redirection(@response.redirected_to)
|
||||
options_after_normalisation = normalize_argument_to_redirection(options)
|
||||
|
||||
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
|
||||
|
||||
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 @@ module ActionController
|
|||
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
|
||||
|
|
|
@ -1042,29 +1042,31 @@ module ActionController #:nodoc:
|
|||
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.
|
||||
#
|
||||
|
|
|
@ -232,7 +232,6 @@ class ActionPackAssertionsControllerTest < Test::Unit::TestCase
|
|||
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 @@ class ActionPackAssertionsControllerTest < Test::Unit::TestCase
|
|||
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 @@ class ActionPackAssertionsControllerTest < Test::Unit::TestCase
|
|||
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 @@ class ActionPackAssertionsControllerTest < Test::Unit::TestCase
|
|||
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'
|
||||
|
|
|
@ -119,7 +119,7 @@ class ComponentsTest < Test::Unit::TestCase
|
|||
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
|
||||
|
|
|
@ -168,21 +168,6 @@ class RedirectTest < Test::Unit::TestCase
|
|||
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 @@ class RedirectTest < Test::Unit::TestCase
|
|||
|
||||
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 @@ module ModuleTest
|
|||
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
|
||||
|
|
Loading…
Reference in a new issue