Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support devise using ViewComponent::TestCase #1849

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Support devise using ViewComponent::TestCase #1849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5c1edde
This updates test_helpers.rb to support Devise tests by ensuring the …
fugufish Sep 11, 2023
d3ad78f
fix silly indentation changes
fugufish Sep 11, 2023
73281ef
update changelog
fugufish Sep 11, 2023
94dd746
revert autoformat change
fugufish Sep 12, 2023
57b562f
remove include
fugufish Sep 12, 2023
2fa7ada
better method of handling devise request
fugufish Sep 12, 2023
b7e0c99
better way of handling devise
fugufish Sep 12, 2023
0041fb4
revert formatting artifact
fugufish Sep 12, 2023
275ea47
Merge branch 'main' into fix_devise_support
fugufish Sep 12, 2023
633e682
Update docs/guide/testing.md
fugufish Sep 12, 2023
f24369a
Update docs/guide/testing.md
fugufish Sep 12, 2023
41e1169
clear the @vc_render_preview_controller after each test run
fugufish Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ nav_order: 5

## main

* Add support for `Devise::Test::ControllerHelpers` by resolving `NoMethodError: undefined method env for nil:NilClass`

*fugufish*

* Refer to `helpers` in `NameError` message in development and test environments.

*Simon Fish*
Expand Down
16 changes: 15 additions & 1 deletion lib/view_component/test_helpers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def render_inline(component, **args, &block)
# @param params [Hash] Parameters to be passed to the preview.
# @return [Nokogiri::HTML]
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)
previews_controller = __vc_render_preview_controller

# From what I can tell, it's not possible to overwrite all request parameters
# at once, so we set them individually here.
Expand Down Expand Up @@ -225,6 +225,16 @@ def vc_test_request
end
end

def before_setup
@request = __vc_render_preview_controller.request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@BlakeWilliams BlakeWilliams Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@fugufish fugufish Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@fugufish fugufish Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fails setting up the request object.

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

Copy link
Contributor Author

@fugufish fugufish Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

super
end

def teardown
super
@vc_render_preview_controller = nil
end

# Note: We prefix private methods here to prevent collisions in consumer's tests.
private

Expand All @@ -244,5 +254,9 @@ def __vc_test_helpers_preview_class
rescue NameError
raise NameError, "`render_preview` expected to find #{result}, but it does not exist."
end

def __vc_render_preview_controller
@vc_render_preview_controller ||= __vc_test_helpers_build_controller(Rails.application.config.view_component.preview_controller.constantize)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a #teardown method that clears the variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
end