Support devise using ViewComponent::TestCase

[fugufish]

What are you trying to accomplish?

This updates ViewComponent::TestHelpers to better detect and support Devise. It resolves the issue described in this discussion #371.

What approach did you choose and why?

Detect whether or not Devise::Test::ControllerHelpers, and if so, set up the @request instance variable and include Devise::Test::ControllerHelpers. The @request variable MUST be present before Devise's own setup hook is called, the only way to ensure this is to create setup initializing the controller and setting the @request instance variable before inclusion.

Anything you want to highlight for special attention from reviewers?

I'd like not to have to add janky included callbacks, but this is the only way that seems to work with Devise.

[BlakeWilliams]

Thanks for making those changes, I think this is getting close but there's a few small things I think we want to address first.

[BlakeWilliams]

This is slightly changing behavior since we're now memoizing this value. Is this okay to memoize or do we need to reset it each render? I think we want to keep the old behavior and have it re-evaluate per-call to render_preview so each call has a fresh controller instance?

[fugufish]

Added a #teardown method that clears the variable.

[BlakeWilliams]

I mentioned it above, but not every test needs a preview controller. Can we revert this back to the previous, lazy loaded implementation to avoid extra work in test suites?

[fugufish]

Unfortunately to support devise I don't see this being possible. We can't predict when render_preview is going to be used. The entire problem is that to support devise, setup needs to happen before render_preview is ever called. If we don't memoize, then the test may still work in most cases, but keep in mind it wont be accurate, as you're warden request environ would be a different instance than your render_preview request environ.

One possible alternative that might meet your requirements is a new concern, which sets up a test environment for devise and render_preview. That way a user is actively opting in essentially stating "I am using this pattern", and then update documentation to the concern's existence.

Honestly if it weren't for the fact that this project already has a bunch of minitests built before I started working on it, I'd just move the whole thing over to rspec and be done with it :D

[fugufish]

Something like

module ViewComponent::TestHelpers::DevisePreviewHelper
   def self.included(base)
      base.instance_eval do
        setup do
          @request = __vc_render_preview_controller.request
        end
         
        teardown do
          @request = nil
          @vc_render_preview_controller = nil
        end
          
      # ....
   end
    
    def __vc_render_preview_controller
      @vc_render_preview_controller ||= super
    end
      
    
end

[BlakeWilliams]

To help clarify things, are devise helpers only broken for previews or for previews + render_inline?

[fugufish]

Just checked, it works for render_inline. I believe it is because render_preview uses an actual controller instance, causing it to pick up Warden's hooks.

[BlakeWilliams]

This isn't correct since this is creating the request from the preview controller, not using the same request the request method uses. Can we update this to use the proper request?

[fugufish]

I'm not sure I follow. On line 78 we have previews_controller = __vc_render_preview_controller, #request tracks to ViewComponent::Base#request which memoizes the request.

If you debug the call you can see that the request objects are indeed the same.

@request -> #<ActionDispatch::TestRequest:0x000000010c675838>
preview_controller.request -> #<ActionDispatch::TestRequest:0x000000010c675838>

These are identical. Am I missing something?

[BlakeWilliams]

Suggested change

	@request = __vc_render_preview_controller.request
	@request = vc_test_request

That wasn't completely clear in hindsight, sorry about that.

We shouldn't use __vc_render_preview_controller.request here because we don't need to couple the @request to the preview controller or how it's constructed. Also not every test requires a preview controller to be present either, so we should skip instantiating one except where necessary.

[fugufish]

Actually we do. The Warden context is not correctly set up in ActionDispatch::TestRequest.create. I tested this myself with one of your earlier suggestions.

[BlakeWilliams]

We don't always use the preview controller instance to render since users can pass their own controller class to us and render_inline uses its own controller.

What does Devise need the controller instance for?

[fugufish]

It fails setting up the request object.

https://github.com/heartcombo/devise/blob/f6e73e5b5c8f519f4be29ac9069c6ed8a2343ce4/lib/devise/test/controller_helpers.rb#L44

[fugufish]

@BlakeWilliams to be clear, this path is ONLY triggered if you call current_user in any of your components. Which is kindof edge case ish right now. I actually refactored so that current_user is only ever passed into components when they need it, so this PR actually wont even apply to my case anymore.

That said, I read somewhere that the long term goal of ViewComponent is that it would be used for page rendering as well, which will make this a lot less of an edge case.

[fugufish]

how can I retrigger test suite, looks like a flakey test

[BlakeWilliams]

Not relevant to this PR but we really should address that flaky test. I can re-run it real quick.

[fugufish]

@BlakeWilliams are we considering this PR dead on arrival?