From dcce01132de9734c9f7a6973bfff1b16b07ef84a Mon Sep 17 00:00:00 2001 From: David Chelimsky Date: Sat, 19 May 2012 16:28:14 -0500 Subject: [PATCH] Raise Assertion instead of RoutingError for routing assertion failures. Before this change, assert_recognizes, assert_generates, and assert_routing raised ActionController::RoutingError when they failed to recognize the route. This commit changes them to raise Assertion instead. This aligns with convention for logical failures, and supports reporting tools that care about the difference between logical failures and errors e.g. the summary at the end of a test run. - Fixes #5899 --- actionpack/CHANGELOG.md | 3 +++ .../testing/assertions/routing.rb | 20 ++++++++++++------- actionpack/test/controller/resources_test.rb | 10 +++++----- .../test/dispatch/routing_assertions_test.rb | 12 +++++------ 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index e625bbe7af..0f0e7b0975 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -186,6 +186,9 @@ * `ActionView::Helpers::TextHelper#highlight` now defaults to the HTML5 `mark` element. *Brian Cardarella* +* `assert_generates`, `assert_recognizes`, and `assert_routing` all raise + `Assertion` instead of `RoutingError` *David Chelimsky* + ## Rails 3.2.3 (March 30, 2012) ## diff --git a/actionpack/lib/action_dispatch/testing/assertions/routing.rb b/actionpack/lib/action_dispatch/testing/assertions/routing.rb index 567ca0c392..41fa3a4b95 100644 --- a/actionpack/lib/action_dispatch/testing/assertions/routing.rb +++ b/actionpack/lib/action_dispatch/testing/assertions/routing.rb @@ -69,11 +69,9 @@ module ActionDispatch # assert_generates "changesets/12", { :controller => 'scm', :action => 'show_diff', :revision => "12" } def assert_generates(expected_path, options, defaults={}, extras = {}, message=nil) if expected_path =~ %r{://} - begin + fail_on(URI::InvalidURIError) do uri = URI.parse(expected_path) expected_path = uri.path.to_s.empty? ? "/" : uri.path - rescue URI::InvalidURIError => e - raise ActionController::RoutingError, e.message end else expected_path = "/#{expected_path}" unless expected_path.first == '/' @@ -189,14 +187,12 @@ module ActionDispatch request = ActionController::TestRequest.new if path =~ %r{://} - begin + fail_on(URI::InvalidURIError) do uri = URI.parse(path) request.env["rack.url_scheme"] = uri.scheme || "http" request.host = uri.host if uri.host request.port = uri.port if uri.port request.path = uri.path.to_s.empty? ? "/" : uri.path - rescue URI::InvalidURIError => e - raise ActionController::RoutingError, e.message end else path = "/#{path}" unless path.first == "/" @@ -205,11 +201,21 @@ module ActionDispatch request.request_method = method if method - params = @routes.recognize_path(path, { :method => method, :extras => extras }) + params = fail_on(ActionController::RoutingError) do + @routes.recognize_path(path, { :method => method, :extras => extras }) + end request.path_parameters = params.with_indifferent_access request end + + def fail_on(exception_class) + begin + yield + rescue exception_class => e + raise MiniTest::Assertion, e.message + end + end end end end diff --git a/actionpack/test/controller/resources_test.rb b/actionpack/test/controller/resources_test.rb index 9fc875014c..de1bff17eb 100644 --- a/actionpack/test/controller/resources_test.rb +++ b/actionpack/test/controller/resources_test.rb @@ -109,7 +109,7 @@ class ResourcesTest < ActionController::TestCase expected_options = {:controller => 'messages', :action => 'show', :id => '1.1.1'} with_restful_routing :messages do - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes(expected_options, :path => 'messages/1.1.1', :method => :get) end end @@ -660,15 +660,15 @@ class ResourcesTest < ActionController::TestCase options = { :controller => controller_name.to_s } collection_path = "/#{controller_name}" - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes(options.merge(:action => 'update'), :path => collection_path, :method => :patch) end - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes(options.merge(:action => 'update'), :path => collection_path, :method => :put) end - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes(options.merge(:action => 'destroy'), :path => collection_path, :method => :delete) end end @@ -1353,7 +1353,7 @@ class ResourcesTest < ActionController::TestCase end def assert_not_recognizes(expected_options, path) - assert_raise ActionController::RoutingError, Assertion do + assert_raise Assertion do assert_recognizes(expected_options, path) end end diff --git a/actionpack/test/dispatch/routing_assertions_test.rb b/actionpack/test/dispatch/routing_assertions_test.rb index 517354ae58..aea4489852 100644 --- a/actionpack/test/dispatch/routing_assertions_test.rb +++ b/actionpack/test/dispatch/routing_assertions_test.rb @@ -54,21 +54,21 @@ class RoutingAssertionsTest < ActionController::TestCase end def test_assert_recognizes_with_hash_constraint - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes({ :controller => 'secure_articles', :action => 'index' }, 'http://test.host/secure/articles') end assert_recognizes({ :controller => 'secure_articles', :action => 'index', :protocol => 'https://' }, 'https://test.host/secure/articles') end def test_assert_recognizes_with_block_constraint - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes({ :controller => 'block_articles', :action => 'index' }, 'http://test.host/block/articles') end assert_recognizes({ :controller => 'block_articles', :action => 'index' }, 'https://test.host/block/articles') end def test_assert_recognizes_with_query_constraint - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_recognizes({ :controller => 'query_articles', :action => 'index', :use_query => 'false' }, '/query/articles', { :use_query => 'false' }) end assert_recognizes({ :controller => 'query_articles', :action => 'index', :use_query => 'true' }, '/query/articles', { :use_query => 'true' }) @@ -87,14 +87,14 @@ class RoutingAssertionsTest < ActionController::TestCase end def test_assert_routing_with_hash_constraint - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_routing('http://test.host/secure/articles', { :controller => 'secure_articles', :action => 'index' }) end assert_routing('https://test.host/secure/articles', { :controller => 'secure_articles', :action => 'index', :protocol => 'https://' }) end def test_assert_routing_with_block_constraint - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_routing('http://test.host/block/articles', { :controller => 'block_articles', :action => 'index' }) end assert_routing('https://test.host/block/articles', { :controller => 'block_articles', :action => 'index' }) @@ -107,7 +107,7 @@ class RoutingAssertionsTest < ActionController::TestCase end assert_routing('/artikel', :controller => 'articles', :action => 'index') - assert_raise(ActionController::RoutingError) do + assert_raise(Assertion) do assert_routing('/articles', { :controller => 'articles', :action => 'index' }) end end