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 3 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
28 changes: 24 additions & 4 deletions lib/view_component/test_helpers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,27 @@ def refute_component_rendered
if ENV["DEBUG"]
warn(
"WARNING in `ViewComponent::TestHelpers`: Add `capybara` " \
"to Gemfile to use Capybara assertions."
"to Gemfile to use Capybara assertions."
)
end

# :nocov:
end

def self.included(base)
if defined?(Devise::Test::ControllerHelpers)
# the `@request` instance variable is used by Devise::Test::ControllerHelpers and must
# be in a `setup` block before including `Devise::Test::ControllerHelpers`
base.instance_eval do
setup do
@request = __vc_render_preview_controller.request
end
end

base.include(Devise::Test::ControllerHelpers)
fugufish marked this conversation as resolved.
Show resolved Hide resolved
end
end

# Returns the result of a render_inline call.
#
# @return [ActionView::OutputBuffer]
Expand Down Expand Up @@ -75,7 +89,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 @@ -103,11 +117,12 @@ def render_preview(name, from: __vc_test_helpers_preview_class, params: {})
#
# assert_text("Hello, World!")
# ```
def render_in_view_context(*args, &block)
def render_in_view_context(*, &block)
@page = nil
@rendered_content = vc_test_controller.view_context.instance_exec(*args, &block)
@rendered_content = vc_test_controller.view_context.instance_exec(*, &block)
fugufish marked this conversation as resolved.
Show resolved Hide resolved
Nokogiri::HTML.fragment(@rendered_content)
end

ruby2_keywords(:render_in_view_context) if respond_to?(:ruby2_keywords, true)

# Set the Action Pack request variant for the given block:
Expand Down Expand Up @@ -226,6 +241,7 @@ def vc_test_request
end

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

private

def __vc_test_helpers_build_controller(klass)
Expand All @@ -244,5 +260,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