Rename private TestHelpers methods (#1661)

* Rename private TestHelpers#controller to avoid conflicts

In #1641, @jacob-carlborg-apoex provided a test case showing
how our private #controller method can create confusion
when writing tests. This PR renames this private method to
a private prefixed format that should prevent such conflicts.

Closes #1641, #1646

* increase test timer

docs/CHANGELOG.md

@@ -18,6 +18,10 @@ nav_order: 5
 
     *Joel Hawksley*
 
+* BREAKING: Rename private TestHelpers#controller, #build_controller, #request, and #preview_class to avoid conflicts. Note: While these methods were undocumented and marked as private, they was easily accessible in tests. As such, we're cautiously considering this to be a breaking change.
+
+    *Joel Hawksley*
+
 * Avoid loading ActionView::Base during Rails initialization. Originally submitted in #1528.
 
     *Jonathan del Strother*

lib/view_component/system_test_helpers.rb

@@ -15,7 +15,7 @@ module ViewComponent
 
       file = Tempfile.new(["rendered_#{fragment.class.name}", ".html"], "tmp/view_components/")
       begin
-        file.write(controller.render_to_string(html: fragment.to_html.html_safe, layout: layout))
+        file.write(__vc_test_helpers_controller.render_to_string(html: fragment.to_html.html_safe, layout: layout))
         file.rewind
 
         block.call("/_system_test_entrypoint?file=#{file.path.split("/").last}")

lib/view_component/test_helpers.rb

@@ -47,9 +47,9 @@ module ViewComponent
       @page = nil
       @rendered_content =
         if Rails.version.to_f >= 6.1
-          controller.view_context.render(component, args, &block)
+          __vc_test_helpers_controller.view_context.render(component, args, &block)
         else
-          controller.view_context.render_component(component, &block)
+          __vc_test_helpers_controller.view_context.render_component(component, &block)
         end
 
       Nokogiri::HTML.fragment(@rendered_content)
@@ -74,8 +74,8 @@ module ViewComponent
     # @param from [ViewComponent::Preview] The class of the preview to be rendered.
     # @param params [Hash] Parameters to be passed to the preview.
     # @return [Nokogiri::HTML]
-    def render_preview(name, from: preview_class, params: {})
-      previews_controller = build_controller(Rails.application.config.view_component.preview_controller.constantize)
+    def render_preview(name, from: __vc_test_helpers_preview_class, params: {})
+      previews_controller = __vc_test_helpers_build_controller(Rails.application.config.view_component.preview_controller.constantize)
 
       # From what I can tell, it's not possible to overwrite all request parameters
       # at once, so we set them individually here.
@@ -105,26 +105,11 @@ module ViewComponent
     # ```
     def render_in_view_context(*args, &block)
       @page = nil
-      @rendered_content = controller.view_context.instance_exec(*args, &block)
+      @rendered_content = __vc_test_helpers_controller.view_context.instance_exec(*args, &block)
       Nokogiri::HTML.fragment(@rendered_content)
     end
     ruby2_keywords(:render_in_view_context) if respond_to?(:ruby2_keywords, true)
 
-    # @private
-    def controller
-      @controller ||= build_controller(Base.test_controller.constantize)
-    end
-
-    # @private
-    def request
-      @request ||=
-        begin
-          request = ActionDispatch::TestRequest.create
-          request.session = ActionController::TestSession.new
-          request
-        end
-    end
-
     # Set the Action Pack request variant for the given block:
     #
     # ```ruby
@@ -135,12 +120,12 @@ module ViewComponent
     #
     # @param variant [Symbol] The variant to be set for the provided block.
     def with_variant(variant)
-      old_variants = controller.view_context.lookup_context.variants
+      old_variants = __vc_test_helpers_controller.view_context.lookup_context.variants
 
-      controller.view_context.lookup_context.variants = variant
+      __vc_test_helpers_controller.view_context.lookup_context.variants = variant
       yield
     ensure
-      controller.view_context.lookup_context.variants = old_variants
+      __vc_test_helpers_controller.view_context.lookup_context.variants = old_variants
     end
 
     # Set the controller to be used while executing the given block,
@@ -154,12 +139,12 @@ module ViewComponent
     #
     # @param klass [ActionController::Base] The controller to be used.
     def with_controller_class(klass)
-      old_controller = defined?(@controller) && @controller
+      old_controller = defined?(@__vc_test_helpers_controller) && @__vc_test_helpers_controller
 
-      @controller = build_controller(klass)
+      @__vc_test_helpers_controller = __vc_test_helpers_build_controller(klass)
       yield
     ensure
-      @controller = old_controller
+      @__vc_test_helpers_controller = old_controller
     end
 
     # Set the URL of the current request (such as when using request-dependent path helpers):
@@ -172,34 +157,47 @@ module ViewComponent
     #
     # @param path [String] The path to set for the current request.
     def with_request_url(path)
-      old_request_path_info = request.path_info
-      old_request_path_parameters = request.path_parameters
-      old_request_query_parameters = request.query_parameters
-      old_request_query_string = request.query_string
-      old_controller = defined?(@controller) && @controller
+      old_request_path_info = __vc_test_helpers_request.path_info
+      old_request_path_parameters = __vc_test_helpers_request.path_parameters
+      old_request_query_parameters = __vc_test_helpers_request.query_parameters
+      old_request_query_string = __vc_test_helpers_request.query_string
+      old_controller = defined?(@__vc_test_helpers_controller) && @__vc_test_helpers_controller
 
       path, query = path.split("?", 2)
-      request.path_info = path
-      request.path_parameters = Rails.application.routes.recognize_path_with_request(request, path, {})
-      request.set_header("action_dispatch.request.query_parameters", Rack::Utils.parse_nested_query(query))
-      request.set_header(Rack::QUERY_STRING, query)
+      __vc_test_helpers_request.path_info = path
+      __vc_test_helpers_request.path_parameters = Rails.application.routes.recognize_path_with_request(__vc_test_helpers_request, path, {})
+      __vc_test_helpers_request.set_header("action_dispatch.request.query_parameters", Rack::Utils.parse_nested_query(query))
+      __vc_test_helpers_request.set_header(Rack::QUERY_STRING, query)
       yield
     ensure
-      request.path_info = old_request_path_info
-      request.path_parameters = old_request_path_parameters
-      request.set_header("action_dispatch.request.query_parameters", old_request_query_parameters)
-      request.set_header(Rack::QUERY_STRING, old_request_query_string)
-      @controller = old_controller
-    end
-
-    # @private
-    def build_controller(klass)
-      klass.new.tap { |c| c.request = request }.extend(Rails.application.routes.url_helpers)
+      __vc_test_helpers_request.path_info = old_request_path_info
+      __vc_test_helpers_request.path_parameters = old_request_path_parameters
+      __vc_test_helpers_request.set_header("action_dispatch.request.query_parameters", old_request_query_parameters)
+      __vc_test_helpers_request.set_header(Rack::QUERY_STRING, old_request_query_string)
+      @__vc_test_helpers_controller = old_controller
     end
 
+    # Note: We prefix private methods here to prevent collisions in consumer's tests.
     private
 
-    def preview_class
+    def __vc_test_helpers_controller
+      @__vc_test_helpers_controller ||= __vc_test_helpers_build_controller(Base.test_controller.constantize)
+    end
+
+    def __vc_test_helpers_request
+      @__vc_test_helpers_request ||=
+        begin
+          out = ActionDispatch::TestRequest.create
+          out.session = ActionController::TestSession.new
+          out
+        end
+    end
+
+    def __vc_test_helpers_build_controller(klass)
+      klass.new.tap { |c| c.request = __vc_test_helpers_request }.extend(Rails.application.routes.url_helpers)
+    end
+
+    def __vc_test_helpers_preview_class
       result = if respond_to?(:described_class)
         raise "`render_preview` expected a described_class, but it is nil." if described_class.nil?
 

test/sandbox/app/components/render_check_component.html.erb

@@ -1 +0,0 @@
-Rendered

test/sandbox/app/components/render_check_component.rb

@@ -2,6 +2,10 @@
 
 class RenderCheckComponent < ViewComponent::Base
   def render?
-    !view_context.cookies[:shown]
+    !view_context.cookies[:hide]
+  end
+
+  def call
+    "Rendered"
   end
 end

test/sandbox/test/integration_test.rb

@@ -214,7 +214,7 @@ class IntegrationTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_includes response.body, "Rendered"
 
-    cookies[:shown] = true
+    cookies[:hide] = true
 
     get "/render_check"
     assert_response :success

test/sandbox/test/rendering_test.rb

@@ -403,19 +403,6 @@ class RenderingTest < ViewComponent::TestCase
     assert_text("Content")
   end
 
-  def test_render_check
-    render_inline(RenderCheckComponent.new)
-
-    assert_text("Rendered")
-
-    controller.view_context.cookies[:shown] = true
-
-    render_inline(RenderCheckComponent.new)
-
-    assert_no_text("Rendered")
-    refute_component_rendered
-  end
-
   def test_assert_select
     render_inline(MyComponent.new)
 
@@ -854,7 +841,7 @@ class RenderingTest < ViewComponent::TestCase
     end
 
     with_request_url "/products" do
-      assert_equal "/products", request.path
+      assert_equal "/products", __vc_test_helpers_request.path
     end
   end
 
@@ -875,13 +862,13 @@ class RenderingTest < ViewComponent::TestCase
     end
 
     with_request_url "/products?mykey=myvalue&otherkey=othervalue" do
-      assert_equal "/products", request.path
-      assert_equal "mykey=myvalue&otherkey=othervalue", request.query_string
-      assert_equal "/products?mykey=myvalue&otherkey=othervalue", request.fullpath
+      assert_equal "/products", __vc_test_helpers_request.path
+      assert_equal "mykey=myvalue&otherkey=othervalue", __vc_test_helpers_request.query_string
+      assert_equal "/products?mykey=myvalue&otherkey=othervalue", __vc_test_helpers_request.fullpath
     end
 
     with_request_url "/products?mykey[mynestedkey]=myvalue" do
-      assert_equal({"mynestedkey" => "myvalue"}, request.parameters["mykey"])
+      assert_equal({"mynestedkey" => "myvalue"}, __vc_test_helpers_request.parameters["mykey"])
     end
   end
 

test/sandbox/test/test_helper_test.rb

@@ -23,7 +23,7 @@ class TestHelperTest < ViewComponent::TestCase
     warden = Minitest::Mock.new
     warden.expect(:authenticate!, true)
 
-    request.env["warden"] = warden
+    __vc_test_helpers_request.env["warden"] = warden
 
     with_request_url "/constraints_with_env" do
       render_inline(ControllerInlineComponent.new(message: "request.env is valid"))

test/test_helper.rb

@@ -48,7 +48,7 @@ end
 # Reduce extra logs produced by puma booting up
 Capybara.server = :puma, {Silent: true}
 # Increase the max wait time to appease test failures due to timeouts.
-Capybara.default_max_wait_time = 10
+Capybara.default_max_wait_time = 30
 
 def with_config_option(option_name, new_value, config_entrypoint: Rails.application.config.view_component)
   old_value = config_entrypoint.public_send(option_name)