Make with_routing test helper work for integration tests

Adds support for with_routing test helpers in ActionDispatch::IntegrationTest.
Previously, this helper didn't work in an integration context because
the rack app and integration session under test were not mutated.
Because controller tests are integration tests by default, we should
support test routes for these kinds of test cases as well.

actionpack/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Add support for `with_routing` test helper in `ActionDispatch::IntegrationTest`
+
+    *Gannon McGibbon*
+
 *   Remove deprecated support to set `Rails.application.config.action_dispatch.show_exceptions` to `true` and `false`.
 
     *Rafael Mendonça França*

actionpack/lib/action_dispatch/testing/assertions/routing.rb

@@ -11,6 +11,65 @@ module Assertions
     module RoutingAssertions
       extend ActiveSupport::Concern
 
+      module WithIntegrationRouting # :nodoc:
+        extend ActiveSupport::Concern
+
+        module ClassMethods
+          def with_routing(&block)
+            old_routes = nil
+            old_integration_session = nil
+
+            setup do
+              old_routes = app.routes
+              old_integration_session = integration_session
+              create_routes(&block)
+            end
+
+            teardown do
+              reset_routes(old_routes, old_integration_session)
+            end
+          end
+        end
+
+        def with_routing(&block)
+          old_routes = app.routes
+          old_integration_session = integration_session
+          create_routes(&block)
+        ensure
+          reset_routes(old_routes, old_integration_session)
+        end
+
+        private
+          def create_routes
+            app = self.app
+            routes = ActionDispatch::Routing::RouteSet.new
+            rack_app = app.config.middleware.build(routes)
+            https = integration_session.https?
+            host = integration_session.host
+
+            app.instance_variable_set(:@routes, routes)
+            app.instance_variable_set(:@app, rack_app)
+            @integration_session = Class.new(ActionDispatch::Integration::Session) do
+              include app.routes.url_helpers
+              include app.routes.mounted_helpers
+            end.new(app)
+            @integration_session.https! https
+            @integration_session.host! host
+            @routes = routes
+
+            yield routes
+          end
+
+          def reset_routes(old_routes, old_integration_session)
+            old_rack_app = app.config.middleware.build(old_routes)
+
+            app.instance_variable_set(:@routes, old_routes)
+            app.instance_variable_set(:@app, old_rack_app)
+            @integration_session = old_integration_session
+            @routes = old_routes
+          end
+      end
+
       module ClassMethods
         # A helper to make it easier to test different route configurations.
         # This method temporarily replaces @routes with a new RouteSet instance
@@ -44,6 +103,26 @@ def setup # :nodoc:
         super
       end
 
+      # A helper to make it easier to test different route configurations.
+      # This method temporarily replaces @routes with a new RouteSet instance.
+      #
+      # The new instance is yielded to the passed block. Typically the block
+      # will create some routes using <tt>set.draw { match ... }</tt>:
+      #
+      #   with_routing do |set|
+      #     set.draw do
+      #       resources :users
+      #     end
+      #     assert_equal "/users", users_path
+      #   end
+      #
+      def with_routing(&block)
+        old_routes, old_controller = @routes, @controller
+        create_routes(&block)
+      ensure
+        reset_routes(old_routes, old_controller)
+      end
+
       # Asserts that the routing of the given +path+ was handled correctly and that the parsed options (given in the +expected_options+ hash)
       # match +path+. Basically, it asserts that \Rails recognizes the route given by +expected_options+.
       #
@@ -167,26 +246,6 @@ def assert_routing(path, options, defaults = {}, extras = {}, message = nil)
         assert_generates(path.is_a?(Hash) ? path[:path] : path, generate_options, defaults, extras, message)
       end
 
-      # A helper to make it easier to test different route configurations.
-      # This method temporarily replaces @routes with a new RouteSet instance.
-      #
-      # The new instance is yielded to the passed block. Typically the block
-      # will create some routes using <tt>set.draw { match ... }</tt>:
-      #
-      #   with_routing do |set|
-      #     set.draw do
-      #       resources :users
-      #     end
-      #     assert_equal "/users", users_path
-      #   end
-      #
-      def with_routing(&block)
-        old_routes, old_controller = @routes, @controller
-        create_routes(&block)
-      ensure
-        reset_routes(old_routes, old_controller)
-      end
-
       # ROUTES TODO: These assertions should really work in an integration context
       def method_missing(selector, *args, &block)
         if defined?(@controller) && @controller && defined?(@routes) && @routes && @routes.named_routes.route_defined?(selector)

actionpack/lib/action_dispatch/testing/integration.rb

@@ -657,6 +657,7 @@ module Behavior
       included do
         include ActionDispatch::Routing::UrlFor
         include UrlOptions # don't let UrlFor override the url_options method
+        include ActionDispatch::Assertions::RoutingAssertions::WithIntegrationRouting
         ActiveSupport.run_load_hooks(:action_dispatch_integration_test, self)
         @@app = nil
       end

actionpack/test/abstract_unit.rb

@@ -91,15 +91,23 @@ class TestCase
 end
 
 class RoutedRackApp
+  class Config < Struct.new(:middleware)
+  end
+
   attr_reader :routes
 
   def initialize(routes, &blk)
     @routes = routes
-    @stack = ActionDispatch::MiddlewareStack.new(&blk).build(@routes)
+    @stack = ActionDispatch::MiddlewareStack.new(&blk)
+    @app = @stack.build(@routes)
   end
 
   def call(env)
-    @stack.call(env)
+    @app.call(env)
+  end
+
+  def config
+    Config.new(@stack)
   end
 end
 
@@ -150,19 +158,6 @@ def self.stub_controllers(config = ActionDispatch::Routing::RouteSet::DEFAULT_CO
     yield DeadEndRoutes.new(config)
   end
 
-  def with_routing(&block)
-    temporary_routes = ActionDispatch::Routing::RouteSet.new
-    old_app, self.class.app = self.class.app, self.class.build_app(temporary_routes)
-    old_routes = SharedTestRoutes
-    silence_warnings { Object.const_set(:SharedTestRoutes, temporary_routes) }
-
-    yield temporary_routes
-  ensure
-    self.class.app = old_app
-    remove!
-    silence_warnings { Object.const_set(:SharedTestRoutes, old_routes) }
-  end
-
   def with_autoload_path(path)
     path = File.join(__dir__, "fixtures", path)
     Zeitwerk.with_loader do |loader|

actionpack/test/controller/flash_test.rb

@@ -377,6 +377,14 @@ def get(path, **options)
       super(path, **options)
     end
 
+    def app
+      @app ||= self.class.build_app do |middleware|
+        middleware.use ActionDispatch::Session::CookieStore, key: SessionKey
+        middleware.use ActionDispatch::Flash
+        middleware.delete ActionDispatch::ShowExceptions
+      end
+    end
+
     def with_test_route_set
       with_routing do |set|
         set.draw do
@@ -385,12 +393,6 @@ def with_test_route_set
           end
         end
 
-        @app = self.class.build_app(set) do |middleware|
-          middleware.use ActionDispatch::Session::CookieStore, key: SessionKey
-          middleware.use ActionDispatch::Flash
-          middleware.delete ActionDispatch::ShowExceptions
-        end
-
         yield
       end
     end

actionpack/test/controller/integration_test.rb

@@ -172,15 +172,17 @@ class RackLintIntegrationTest < ActionDispatch::IntegrationTest
         get "/", to: ->(_) { [200, {}, [""]] }
       end
 
-      @app = self.class.build_app(set) do |middleware|
-        middleware.unshift Rack::Lint
-      end
-
       get "/"
 
       assert_equal 200, status
     end
   end
+
+  def app
+    @app ||= self.class.build_app do |middleware|
+      middleware.unshift Rack::Lint
+    end
+  end
 end
 
 # Tests that integration tests don't call Controller test methods for processing.

actionpack/test/dispatch/request/query_string_parsing_test.rb

@@ -157,6 +157,12 @@ def test_array_parses_without_nil
   end
 
   private
+    def app
+      @app ||= self.class.build_app do |middleware|
+        middleware.use(EarlyParse)
+      end
+    end
+
     def assert_parses(expected, actual)
       with_routing do |set|
         set.draw do
@@ -164,9 +170,6 @@ def assert_parses(expected, actual)
             get ":action", to: ::QueryStringParsingTest::TestController
           end
         end
-        @app = self.class.build_app(set) do |middleware|
-          middleware.use(EarlyParse)
-        end
 
         get "/parse", params: actual
         assert_response :ok

actionpack/test/dispatch/request_id_test.rb

@@ -55,6 +55,10 @@ def index
     end
   end
 
+  setup do
+    @header = "X-Request-Id"
+  end
+
   test "request id is passed all the way to the response" do
     with_test_route_set do
       get "/"
@@ -70,25 +74,28 @@ def index
   end
 
   test "using a custom request_id header key" do
-    with_test_route_set(header: "X-Tracer-Id") do
+    @header = "X-Tracer-Id"
+    with_test_route_set do
       get "/"
       assert_match(/\w+/, @response.headers["X-Tracer-Id"])
     end
   end
 
   private
+    def app
+      @app ||= self.class.build_app do |middleware|
+        middleware.use Rack::Lint
+        middleware.use ActionDispatch::RequestId, header: @header
+        middleware.use Rack::Lint
+      end
+    end
+
     def with_test_route_set(header: "X-Request-Id")
       with_routing do |set|
         set.draw do
           get "/", to: ::RequestIdResponseTest::TestController.action(:index)
         end
 
-        @app = self.class.build_app(set) do |middleware|
-          middleware.use Rack::Lint
-          middleware.use ActionDispatch::RequestId, header: header
-          middleware.use Rack::Lint
-        end
-
         yield
       end
     end

actionpack/test/dispatch/routing_assertions_test.rb

@@ -12,7 +12,7 @@ class SecureBooksController < BooksController; end
 class BlockBooksController < BooksController; end
 class QueryBooksController < BooksController; end
 
-class RoutingAssertionsTest < ActionController::TestCase
+module RoutingAssertionsSharedTests
   def setup
     root_engine = Class.new(Rails::Engine) do
       def self.name
@@ -98,28 +98,28 @@ def test_assert_recognizes_with_method
   end
 
   def test_assert_recognizes_with_hash_constraint
-    assert_raise(Assertion) do
+    assert_raise(Minitest::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(Assertion) do
+    assert_raise(Minitest::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(Assertion) do
+    assert_raise(Minitest::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")
   end
 
   def test_assert_recognizes_raises_message
-    err = assert_raise(Assertion) do
+    err = assert_raise(Minitest::Assertion) do
       assert_recognizes({ controller: "secure_articles", action: "index" }, "http://test.host/secure/articles", {}, "This is a really bad msg")
     end
 
@@ -145,28 +145,28 @@ def test_assert_recognizes_with_engine_and_method
   end
 
   def test_assert_recognizes_with_engine_and_hash_constraint
-    assert_raise(Assertion) do
+    assert_raise(Minitest::Assertion) do
       assert_recognizes({ controller: "secure_books", action: "index" }, "http://test.host/shelf/secure/books")
     end
     assert_recognizes({ controller: "secure_books", action: "index", protocol: "https://" }, "https://test.host/shelf/secure/books")
   end
 
   def test_assert_recognizes_with_engine_and_block_constraint
-    assert_raise(Assertion) do
+    assert_raise(Minitest::Assertion) do
       assert_recognizes({ controller: "block_books", action: "index" }, "http://test.host/shelf/block/books")
     end
     assert_recognizes({ controller: "block_books", action: "index" }, "https://test.host/shelf/block/books")
   end
 
   def test_assert_recognizes_with_engine_and_query_constraint
-    assert_raise(Assertion) do
+    assert_raise(Minitest::Assertion) do
       assert_recognizes({ controller: "query_books", action: "index", use_query: "false" }, "/shelf/query/books", use_query: "false")
     end
     assert_recognizes({ controller: "query_books", action: "index", use_query: "true" }, "/shelf/query/books", use_query: "true")
   end
 
   def test_assert_recognizes_raises_message_with_engine
-    err = assert_raise(Assertion) do
+    err = assert_raise(Minitest::Assertion) do
       assert_recognizes({ controller: "secure_books", action: "index" }, "http://test.host/shelf/secure/books", {}, "This is a really bad msg")
     end
 
@@ -182,7 +182,7 @@ def test_assert_routing
   end
 
   def test_assert_routing_raises_message
-    err = assert_raise(Assertion) do
+    err = assert_raise(Minitest::Assertion) do
       assert_routing("/thisIsNotARoute", { controller: "articles", action: "edit", id: "1" }, { id: "1" }, {}, "This is a really bad msg")
     end
 
@@ -198,14 +198,14 @@ def test_assert_routing_with_extras
   end
 
   def test_assert_routing_with_hash_constraint
-    assert_raise(Assertion) do
+    assert_raise(Minitest::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(Assertion) do
+    assert_raise(Minitest::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")
@@ -218,13 +218,15 @@ def test_with_routing
       end
 
       assert_routing("/artikel", controller: "articles", action: "index")
-      assert_raise(Assertion) do
+      assert_raise(Minitest::Assertion) do
         assert_routing("/articles", controller: "articles", action: "index")
       end
     end
   end
 
-  class WithRoutingTest < ActionController::TestCase
+  module WithRoutingSharedTests
+    extend ActiveSupport::Concern
+
     def before_setup
       @routes = ActionDispatch::Routing::RouteSet.new
       @routes.draw do
@@ -234,15 +236,17 @@ def before_setup
       super
     end
 
-    with_routing do |routes|
-      routes.draw do
-        resources :articles, path: "artikel"
+    included do
+      with_routing do |routes|
+        routes.draw do
+          resources :articles, path: "artikel"
+        end
       end
     end
 
     def test_with_routing_for_the_entire_test_file
       assert_routing("/artikel", controller: "articles", action: "index")
-      assert_raise(Assertion) do
+      assert_raise(Minitest::Assertion) do
         assert_routing("/articles", controller: "articles", action: "index")
       end
     end
@@ -254,15 +258,58 @@ def test_with_routing_for_entire_test_file_can_be_overwritten_for_individual_tes
         end
 
         assert_routing("/articolo", controller: "articles", action: "index")
-        assert_raise(Assertion) do
+        assert_raise(Minitest::Assertion) do
           assert_routing("/artikel", controller: "articles", action: "index")
         end
       end
 
       assert_routing("/artikel", controller: "articles", action: "index")
-      assert_raise(Assertion) do
+      assert_raise(Minitest::Assertion) do
         assert_routing("/articolo", controller: "articles", action: "index")
       end
     end
   end
 end
+
+class RoutingAssertionsControllerTest < ActionController::TestCase
+  include RoutingAssertionsSharedTests
+
+  class WithRoutingTest < ActionController::TestCase
+    include RoutingAssertionsSharedTests::WithRoutingSharedTests
+  end
+end
+
+class RoutingAssertionsIntegrationTest < ActionDispatch::IntegrationTest
+  include RoutingAssertionsSharedTests
+
+  test "https and host settings are set on new session" do
+    https!
+    host! "newhost.com"
+
+    with_routing do |routes|
+      routes.draw {  }
+      assert_predicate integration_session, :https?
+      assert_equal "newhost.com", integration_session.host
+    end
+  end
+
+  class WithRoutingTest < ActionDispatch::IntegrationTest
+    include RoutingAssertionsSharedTests::WithRoutingSharedTests
+  end
+
+  class WithRoutingSettingsTest < ActionDispatch::IntegrationTest
+    setup do
+      https!
+      host! "newhost.com"
+    end
+
+    with_routing do |routes|
+      routes.draw {  }
+    end
+
+    test "https and host settings are set on new session" do
+      assert_predicate integration_session, :https?
+      assert_equal "newhost.com", integration_session.host
+    end
+  end
+end

actionpack/test/dispatch/server_timing_test.rb

@@ -120,6 +120,12 @@ def call(env)
   end
 
   private
+    def app
+      @app ||= self.class.build_app do |middleware|
+        @middlewares.each { |m| middleware.use m }
+      end
+    end
+
     def with_test_route_set
       with_routing do |set|
         set.draw do
@@ -128,10 +134,6 @@ def with_test_route_set
           post "/", to: ::ServerTimingTest::TestController.action(:create)
         end
 
-        @app = self.class.build_app(set) do |middleware|
-          @middlewares.each { |m| middleware.use m }
-        end
-
         yield
       end
     end

actionpack/test/dispatch/session/cache_store_test.rb

@@ -204,6 +204,14 @@ def test_drop_session_in_the_legacy_id_as_well
   end
 
   private
+    def app
+      @app ||= self.class.build_app do |middleware|
+        @cache = ActiveSupport::Cache::MemoryStore.new
+        middleware.use ActionDispatch::Session::CacheStore, key: "_session_id", cache: @cache
+        middleware.delete ActionDispatch::ShowExceptions
+      end
+    end
+
     def with_test_route_set
       with_routing do |set|
         set.draw do
@@ -212,12 +220,6 @@ def with_test_route_set
           end
         end
 
-        @app = self.class.build_app(set) do |middleware|
-          @cache = ActiveSupport::Cache::MemoryStore.new
-          middleware.use ActionDispatch::Session::CacheStore, key: "_session_id", cache: @cache
-          middleware.delete ActionDispatch::ShowExceptions
-        end
-
         yield
       end
     end

actionpack/test/dispatch/session/cookie_store_test.rb

@@ -142,7 +142,9 @@ def test_disregards_tampered_sessions
   end
 
   def test_does_not_set_secure_cookies_over_http
-    with_test_route_set(secure: true) do
+    cookie_options(secure: true)
+
+    with_test_route_set do
       get "/set_session_value"
       assert_response :success
       assert_nil headers["Set-Cookie"]
@@ -161,7 +163,9 @@ def test_properly_renew_cookies
   end
 
   def test_does_set_secure_cookies_over_https
-    with_test_route_set(secure: true) do
+    cookie_options(secure: true)
+
+    with_test_route_set do
       get "/set_session_value", headers: { "HTTPS" => "on" }
 
       assert_response :success
@@ -306,7 +310,9 @@ def test_setting_session_id_to_nil_is_respected
   end
 
   def test_session_store_with_expire_after
-    with_test_route_set(expire_after: 5.hours) do
+    cookie_options(expire_after: 5.hours)
+
+    with_test_route_set do
       # First request accesses the session
       time = Time.local(2008, 4, 24)
 
@@ -333,7 +339,9 @@ def test_session_store_with_expire_after
   end
 
   def test_session_store_with_expire_after_does_not_accept_expired_session
-    with_test_route_set(expire_after: 5.hours) do
+    cookie_options(expire_after: 5.hours)
+
+    with_test_route_set do
       # First request accesses the session
       time = Time.local(2017, 11, 12)
 
@@ -361,7 +369,9 @@ def test_session_store_with_expire_after_does_not_accept_expired_session
   end
 
   def test_session_store_with_explicit_domain
-    with_test_route_set(domain: "example.es") do
+    cookie_options(domain: "example.es")
+
+    with_test_route_set do
       get "/set_session_value"
       assert_match(/domain=example\.es/, headers["Set-Cookie"])
       headers["Set-Cookie"]
@@ -376,14 +386,18 @@ def test_session_store_without_domain
   end
 
   def test_session_store_with_nil_domain
-    with_test_route_set(domain: nil) do
+    cookie_options(domain: nil)
+
+    with_test_route_set do
       get "/set_session_value"
       assert_no_match(/domain=/, headers["Set-Cookie"])
     end
   end
 
   def test_session_store_with_all_domains
-    with_test_route_set(domain: :all) do
+    cookie_options(domain: :all)
+
+    with_test_route_set do
       get "/set_session_value"
       assert_match(/domain=example\.com/, headers["Set-Cookie"])
     end
@@ -397,14 +411,18 @@ def test_session_store_with_all_domains
   end
 
   test "explicit same_site sets SameSite" do
-    with_test_route_set(same_site: :strict) do
+    cookie_options(same_site: :strict)
+
+    with_test_route_set do
       get "/set_session_value"
       assert_set_cookie_attributes("_myapp_session", "SameSite=Strict")
     end
   end
 
   test "explicit nil same_site omits SameSite" do
-    with_test_route_set(same_site: nil) do
+    cookie_options(same_site: nil)
+
+    with_test_route_set do
       get "/set_session_value"
       assert_not_set_cookie_attributes("_myapp_session", "SameSite")
     end
@@ -428,7 +446,18 @@ def get(path, **options)
       super
     end
 
-    def with_test_route_set(options = {})
+    def cookie_options(options = {})
+      (@cookie_options ||= { key: SessionKey }).merge!(options)
+    end
+
+    def app
+      @app ||= self.class.build_app do |middleware|
+        middleware.use ActionDispatch::Session::CookieStore, cookie_options
+        middleware.delete ActionDispatch::ShowExceptions
+      end
+    end
+
+    def with_test_route_set
       with_routing do |set|
         set.draw do
           ActionDispatch.deprecator.silence do
@@ -436,13 +465,6 @@ def with_test_route_set(options = {})
           end
         end
 
-        options = { key: SessionKey }.merge!(options)
-
-        @app = self.class.build_app(set) do |middleware|
-          middleware.use ActionDispatch::Session::CookieStore, options
-          middleware.delete ActionDispatch::ShowExceptions
-        end
-
         yield
       end
     end

actionpack/test/dispatch/session/mem_cache_store_test.rb

@@ -187,6 +187,16 @@ def test_prevents_session_fixation
   end
 
   private
+    def app
+      @app ||= self.class.build_app do |middleware|
+        middleware.use ActionDispatch::Session::MemCacheStore,
+          key: "_session_id", namespace: "mem_cache_store_test:#{SecureRandom.hex(10)}",
+          memcache_server: ENV["MEMCACHE_SERVERS"] || "localhost:11211",
+          socket_timeout: 60
+        middleware.delete ActionDispatch::ShowExceptions
+      end
+    end
+
     def with_test_route_set
       with_routing do |set|
         set.draw do
@@ -195,14 +205,6 @@ def with_test_route_set
           end
         end
 
-        @app = self.class.build_app(set) do |middleware|
-          middleware.use ActionDispatch::Session::MemCacheStore,
-            key: "_session_id", namespace: "mem_cache_store_test:#{SecureRandom.hex(10)}",
-            memcache_server: ENV["MEMCACHE_SERVERS"] || "localhost:11211",
-            socket_timeout: 60
-          middleware.delete ActionDispatch::ShowExceptions
-        end
-
         yield
       end
     end