RSpec and ViewComponent::TestHelpers

[rmacklin]

Opening this issue based on the discussions started in https://github.com/github/view_component/issues/292#issuecomment-612737914

Do we really need TestHelpers to have a dependency on Capybara?

I think the reason for including Capybara::Minitest::Assertions was to give access to those assertions inside instances of ViewComponent::TestCase (those methods leverage the page definition we supply).

makes sense in the Minitest case. How much does this logic help if you're using RSpec or some other testing library? The testing pattern if very different there and you wouldn't be inheriting from ViewComponent::TestCase and would be requiring capybara/rspec yourself.

I think you're right that things are a bit murky w.r.t. rspec. We're including capybara's minitest assertions module in ViewComponent::TestHelpers: https://github.com/github/view_component/blob/d3952cb35c82734b5119bc2c173e99cc08219d5c/lib/view_component/test_helpers.rb#L7

but we also have documentation suggesting that ViewComponent::TestHelpers should be included in rspec component configuration (where it seems like the minitest assertions would be out of place): https://github.com/github/view_component/blob/dc0c0c1671dc116a3c2b822a5031b4045c2eb569/README.md#L553-L562

That said, there definitely are consumers of this library that use rspec and I haven't seen them raise an issue about this being a problem so far. I suspect they are just not using those methods. I don't have a lot of rspec experience, but I also noticed that the capybara README section on rspec mentions:

Capybara matchers are also supported in view specs

I wonder if folks have been testing components in view specs where they can already leverage capybara's rspec matchers.

Anyway, might be better to open a new issue to discuss the rspec stuff since this one about removing the capybara dependency has been closed.

and https://github.com/github/view_component/pull/299#discussion_r409022216:

For RSpec the IMO the "best" solution is to separate TestHelpers into two pieces. RSpec users get value out of render_inline and the supporting code. We're using that today. What RSpec doesn't need is the Capybara Assertions etc. For RSpec you're going to pull in the rspec Capybara helpers explicitly. It would be entirely normal/expected to configure RSpec like this;

require 'capybara/rspec'
require "view_component/test_helpers"

RSpec.configure do |config|
  config.include ViewComponent::TestHelpers, type: :component
end

I wonder should we just push Capybara yup to ViewComponent::TestCase ? Anyone using Minitest is going to inherit from that. Meanwhile other frameworks just get what they need.

[rmacklin]

One thought re:

I don't have a lot of rspec experience, but I also noticed that the capybara README section on rspec mentions:

Capybara matchers are also supported in view specs

I wonder if folks have been testing components in view specs where they can already leverage capybara's rspec matchers.

---

The example code from capybara's README below that sentence is:

RSpec.describe "todos/show.html.erb", type: :view do
  it "displays the todo title" do
    assign :todo, Todo.new(title: "Buy milk")

    render

    expect(rendered).to have_css("header h1", text: "Buy milk")
  end
end

This seems to leverage the rendered instance variable from ActionView::TestCase::Behavior: https://github.com/rails/rails/blob/9303873e57dd86a82ef69d58ed7e7af6bc7ed344/actionview/lib/action_view/test_case.rb#L122 which has a public reader (and writer): https://github.com/rails/rails/blob/9303873e57dd86a82ef69d58ed7e7af6bc7ed344/actionview/lib/action_view/test_case.rb#L55

I wonder if it's worth renaming @raw: https://github.com/github/view_component/blob/d3952cb35c82734b5119bc2c173e99cc08219d5c/lib/view_component/test_helpers.rb#L21 to @rendered and providing a reader, so that the same code from the example:

expect(rendered).to have_css("header h1", text: "Buy milk")

would work after you've called render_inline.

[joelhawksley]

@rmacklin I could see it being nice to support the @rendered API. Would you be up for taking on a PR?

[rmacklin]

I'll take a look at this tomorrow!

[rmacklin]

@joelhawksley So it looks like all that's necessary to make the example code from the rspec section of Capybara's README:

expect(rendered).to have_css("header h1", text: "Buy milk")

is:

diff --git a/lib/view_component/test_helpers.rb b/lib/view_component/test_helpers.rb
index efcb986..cb9b3c3 100644
--- a/lib/view_component/test_helpers.rb
+++ b/lib/view_component/test_helpers.rb
@@ -7,7 +7,7 @@ module ViewComponent
       include Capybara::Minitest::Assertions

       def page
-        Capybara::Node::Simple.new(@raw)
+        Capybara::Node::Simple.new(@rendered)
       end

       def refute_component_rendered
@@ -18,9 +18,9 @@ module ViewComponent
     end

     def render_inline(component, **args, &block)
-      @raw = controller.view_context.render(component, args, &block)
+      @rendered = controller.view_context.render(component, args, &block)

-      Nokogiri::HTML.fragment(@raw)
+      Nokogiri::HTML.fragment(@rendered)
     end

     def controller

It's not even necessary for ViewComponent::TestHelpers to provide a reader for @rendered because rspec view specs include ActionView::TestCase::Behavior so they have the reader via https://github.com/rails/rails/blob/9303873e57dd86a82ef69d58ed7e7af6bc7ed344/actionview/lib/action_view/test_case.rb#L55

That said, I'm second guessing the idea a bit. I think it might be cleaner to change it to e.g. @rendered_component and provide a reader for that, so that we don't interfere with ActionView::TestCase::Behavior's @rendered. (For example, if someone wanted to test a regular view and a component in the same test case it seems like @rendered could run into conflicts. I'm not sure if that's a reasonable thing to do in the same test case, but it seems like we could avoid it altogether by using a different name, and I like the extra explicitness of rendered_component.)

If we do that, then the example from Capybara's README would only need to be changed to:

expect(rendered_component).to have_css("header h1", text: "Buy milk")

I'll open a PR with this change and we can continue the discussion about this particular change there. (I think this GitHub Issue is still broader in scope than just that change.)

[rmacklin]

I'll open a PR with this change

Opened https://github.com/github/view_component/pull/344

[fsateler]

Hi,

I'd like to echo the request to decouple capybara. We don't use it, and we get an annoying warning on every rails test invocation :(

EDIT: the warning comes up once per rails test run, not once per individual test.

[joelhawksley]

@fsateler could you share the warning you're seeing?

[fsateler]

@joelhawksley

WARNING in ViewComponent::TestHelpers: You must add capybara to your Gemfile to use Capybara assertions.

[joelhawksley]

@rmacklin what do you think we should do here? I can see it being annoying to get the error on every test invocation.

[rmacklin]

I'm fine with removing the warning. One question:

The README originally mentioned that Capybara matchers were only made available if the application had a capybara dependency: https://github.com/github/view_component/pull/298/files#diff-04c6e90faac2675aa89e2176d2eec7d8 but that's been edited out of the current README: https://github.com/github/view_component/blob/48cb5b6092017ef9c8593519ec8c748ac6df5c63/README.md#L452

If we're going to remove the warning, is there some updated copy you'd like to use in the README to clarify this point?

[joelhawksley]

@rmacklin I did not mean to remove that information from the README. Would you mind adding it back in?

I'm find with removing the warning.

[rmacklin]

Nevermind, the current README mentions it: https://github.com/github/view_component/blob/fe8b6f24aa0635115e20920353a40b683b76cef7/README.md#L548

I think I was looking at an outdated version in my fork 🙈

Anyway, I've opened a PR here: https://github.com/github/view_component/pull/393

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.