Add Capybara assertion support to Controller, Integration, Mailer, and View tests

[seanpdoyle]

Motivation / Background

This is a re-submission of #41291.

Capybara provides a wide-range of selectors, which
can be extended even further.

While Capybara is capable of exercising browser-based, JavaScript-enabled pages, its assertions and selectors excel in JavaScript-free static HTML environments.

HTML-only assertions make Capybara a compelling alternative to the rails-dom-testing assertions utilized by ActionController::TestCase, ActionDispatch::IntegrationTest, and ActionView::TestCase.

For example, Capybara provides a :button selector, which can be
invoked by assert_selector :button, "Button Text" or assert_button "Button Text". Additionally, Capybara's button selector supports
resolution based on ARIA attributes like aria-label and
[role="button"]. The assertions provided by rails-dom-testing do
not.

It's possible to recreate those assertion characteristics gem-side in
rails-dom-testing, internally Rails-side, or application-side.

Given the fact that capybara is already a Rails testing dependency
(through System Tests), there is an opportunity re-use that dependency across all HTML test cases.

Detail

This commit introduces framework-specific configuration values:

• config.action_controller.html_assertions for ActionController::TestCase
• config.action_dispatch.html_assertions for ActionDispatch::IntegrationTest
• config.action_mailer.html_assertions for ActionMailer::TestCase
• config.action_view.html_assertions for ActionView::TestCase

When set to :capybara, those tests include framework-scoped CapybaraAssertions modules that transitively include Capybara::Minitest::Assertions, and define the required #page method to parse the HTML into a Capybara::Node::Simple instance.

When set to :rails_dom_testing, those tests include a framework-scoped RailsDomTestingAssetrions module to preserve the existing behavior.

They all default to :rails_dom_testing.

Additional Information

I've forked eileencodes/integration_performance_test and updated it to work with this branch:

eileencodes/integration_performance_test@master...seanpdoyle:rails/action-dispatch-integration-capybara

The results show that asserting with Capybara::Minitest::Assertions is roughly 1.11x slower than Rails::Dom::Testing::Assertions

Calculating -------------------------------------
ActionController::TestCase: rails-dom-testing
                       139.000  i/100ms
ActionController::TestCase: capybara/minitest
                       132.000  i/100ms
-------------------------------------------------
ActionController::TestCase: rails-dom-testing
                          1.479k (± 1.5%) i/s -      7.506k
ActionController::TestCase: capybara/minitest
                          1.328k (± 0.8%) i/s -      6.732k

Comparison:
ActionController::TestCase: rails-dom-testing:     1479.1 i/s
ActionController::TestCase: capybara/minitest:     1327.6 i/s - 1.11x slower

Calculating -------------------------------------
ActionDispatch::IntegrationTest: rails-dom-testing
                       121.000  i/100ms
ActionDispatch::IntegrationTestINDEX: capybara/minitest
                       112.000  i/100ms
-------------------------------------------------
ActionDispatch::IntegrationTest: rails-dom-testing
                          1.237k (± 0.7%) i/s -      6.292k
ActionDispatch::IntegrationTestINDEX: capybara/minitest
                          1.117k (± 1.1%) i/s -      5.600k

Comparison:
ActionDispatch::IntegrationTest: rails-dom-testing:     1237.2 i/s
ActionDispatch::IntegrationTestINDEX: capybara/minitest:     1117.0 i/s - 1.11x slower

Run options: --seed 59692

# Running:

....

Finished in 0.004782s, 836.4701 runs/s, 2509.4103 assertions/s.
4 runs, 12 assertions, 0 failures, 0 errors, 0 skips

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[rails-bot]

This pull request 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.

[nimmolo]

Hey, just want to say this looks like a desirable PR.
Can this be re-opened please?

[nimmolo]

@seanpdoyle maybe the lack of context is the reason for the lack of further discussion on this PR. It's such a useful implementation... and it seems to me you've made requested modifications and done the benchmarking.

Would it make sense to abandon this newer PR, and consolidate these changes into the older PR branch, so reviewers and core committers have the reminders of the work and review that has already gone into this?

[cpjmcquillan]

@seanpdoyle awesome work, +1-ing that it would be great if this was reconsidered.

Oh, and thanks again for all your fantastic contributions to the community!

[nimmolo]

thanks @p8, @cpjmcquillan and @seanpdoyle !

[seanpdoyle]

Unfortunately, #41291 was closed due to inactivity, and as a result, the context for this discussion must be split between two pull requests.

Due to that split, I need to ping @eileencodes @rafaelfranca @zzak, since you were all involved in the original discussion.

Given the concerns about the performance impacts of the original changeset, I've re-structured the proposal to a point where it's entirely opt-in.

Is this approach more viable than the original?

[rafaelfranca]

I think so, but I also think is more confusing. Why would someone chose this new list of assertions over the existing ones? Rails is opinionated, and having two set of assertions for the same kind of tests seems that we don't have an opinion in the matter.

I still don't understand why we wouldn't improve rails-dom-testing. If the capybara assertions were not slower I'd even say we could just stop using rails-dom-testing, but that doesn't seems to be the case since those capybara assertions are almost 2 times slower in the worse case.

[seanpdoyle]

@rafaelfranca thank you for jumping back into this conversation! I'm sorry for the long pause and the change in venue.

Why would someone chose this new list of assertions over the existing ones?

Capybara's built-in selectors are more robust and flexible than what rails-dom-testing provides. Built-in selectors like :button or :field are more sophisticated than their String selector rails-dom-testing counterparts. For example, Capybara can find a <button> element based on its text content, [type] value, and presence or absence of the [disabled] attribute:

# <button>Submit</button>

assert_button "Submit"
assert page.find(:button, "Submit")

# <button type="reset">Reset</button>

assert_button type: "reset"
assert page.find(:button, type: "reset")

# <button disabled>Submit</button>
assert_no_button "Submit"
page.find(:button, "Submit") #=> raises!

assert_button "Submit", disabled: true
assert page.find(:button, disabled: true)

# <button>Submit</button>
# <button type="button">Cancel</button>

assert_button { _1.text.include?("Submit") && _1["type"] != "button" }

rails-dom-testing lacks built-in abstractions, and instead relies on String selectors. This leaves the burden of responsibility with applications to develop their own abstractions and helpers. Without those abstractions, assertions about <button> elements might resemble:

# <button>Submit</button>

assert_select "button", "Submit"

# <button type="reset">Reset</button>

assert_select 'button[type="reset"]'

# <button disabled>Submit</button>
assert_no_select "button:not([disabled])", "Submit"

assert_select "button[disabled]", "Submit"

Some selectors can be mapped directly one-to-one from Capybara to rails-dom-testing. However, it gets tricky for elements that derive their "text" from somewhere other than the element's .textContent. Take, for example, an <input type="submit"> or <input type="button">. Capybara's built-in selectors are capable of distinguishing between the two:

# <input type="submit" value="Submit">
# <input type="button" value="Submit">
# <button>Submit</button>
# <button type="button">Submit</button>

assert_button "Submit", count: 4
assert_button "Submit", type: "button", count: 2

Calls to assert_select, however, must account for the textContent-[value] differences and the input-button differences. While it's true that applications or rails-dom-testing itself could come up with abstractions to paper over those differences, that effort would duplicate what Capybara already provides.

Another feature of Capybara's selectors that would be challenging to recreate in rails-dom-testing is its support for locating :field (and :fillable_field, and :file) based on corresponding <label> text or [aria-label] value.

# <label for="a_text_field">A text field</label>
# <input id="a_text_field" value="an initial value">

assert_field "A text field", with: "an initial value"

The accessibility benefits alone feel valuable enough to warrant a migration. Personally, I find support for asserting that form controls are properly labelled without the need for JavaScript-enabled System Tests very compelling.

having two set of assertions for the same kind of tests seems that we don't have an opinion in the matter.

I think we're in agreement here. My perspective is that since most applications already depend on Capybara for driving their System Tests, integrating those predictable and robust selectors into their Rack Test driven integration tests could halve their maintenance burden. Choosing Capybara over rails-dom-testing means they only need to create the abstraction once, and use it anywhere their test harness interacts with HTML. Some of those applications might already have their own suite of domain-specific bespoke Capybara selectors (or depend on third-party selectors like those provided by capybara_accessible_selectors). Should they also maintain a collection of bespoke rails-dom-testing selectors, helpers, and abstractions for their Integration test suite?

If the capybara assertions were not slower I'd even say we could just stop using rails-dom-testing, but that doesn't seems to be the case since those capybara assertions are almost 2 times slower in the worse case.

This is an unfortunate concern. My original proposal considered replacing rails-dom-testing, but felt that would be too disruptive. I wonder what kind of effort it would require to make Capybara a more performant option. From my perspective, I consider optimizing Capybara a more cost-effective use of time and effort than reverse engineering some of Capybara's features back into rails-dom-testing.

[rafaelfranca]

To me, given all options capybara helpers provide over our own helpers I can't see why Rails should not provide that as default, other than the performance concerns. We spent a considerable amount of time trying to make ActionDispatch::IntegrationTest as fast as ActionController::TestCase to open the avenue of deprecating/removing the latter, so making capybara helpers the default in IntegrationTest would goes against that.

But, if we can make Capybara helpers as fast as the ones we have today, I don't see a reason why we should not do that. I totally agree that if not too hard, we should spend time optimizing Capybara instead of reimplementing it.

[eileencodes]

My perspective is that since most applications already depend on Capybara for driving their System Tests

I'm not sure this is a true statement. I can see why it seems that way, but any apps upgrading wouldn't be forced to add Capybara unless they wanted system tests and any application that's generated as just an API skips system tests. It's also pretty easy to remove. So while we can say a fair amount of applications already depend on Capybara it's much less of a requirement than replacing rails-dom-testing with Capybara would be. We'd be making a hard requirement on Capybara for default integration testing, whereas right now it's only a hard requirement for system testing.

But, if we can make Capybara helpers as fast as the ones we have today, I don't see a reason why we should not do that.

I think this is a big concern of mine as well, since I worked on making integration tests faster so they were in line with functional/controller tests. I wouldn't be supportive of undoing that work.

The other side of it is that currently we control rails-dom-testing but we have no control over how and when Capybara fixes bugs or adds features. I'm sure that it's well maintained but it is a complex codebase and would make it harder for the core team to have influence into how it affects the framework and testing environments. There is a non-zero chance it would increase the maintenance burden for us. This is fine for system testing because there aren't many other alternatives we could plug and play into Rails, and system testing isn't your default go to when testing the majority of your framework (mainly because system testing is pretty slow and should be mainly used for testing how Rails and JS interact).

I can tell you from first hand experience that since I'm not that familiar with Capybara it's been difficult for me to decide when to merge changes into Rails that "fix" system tests. I'm often relying on the PR author to have done due diligence into the right fix because I don't know Capybara well enough.

TL;DR:

My main concerns for moving to Capybara from rails-dom-testing are maintenance burden and performance. Improving Capybara perf gets rid of one my concerns.

[seanpdoyle]

Thank you two for your perspectives. While I'm personally interested in exploring ways to optimize Capybara, I'm curious about the viability of this Pull Request in its current form.

The Capybara assertions are opt-in, since the ActionDispatch::Assertions::CapybaraAssertions module isn't mixed into ActionDispatch::IntegrationTest by default.

This changeset isn't replacing or deprecating rails-dom-testing. It also isn't introducing a Capybara dependency where one did not exist before (right?). Does the file's presence mean that it's autoloaded in a way that carries along the Capybara dependency from the inline gem "capybara" line?

This diff is the least disruptive version of these changes I could imagine. If the team isn't interested in accepting them, could we instead determine a more public interface so that a third-party gem could extend ActionDispatch::IntegrationTest without overriding the very private _mock_session method?

[rafaelfranca]

could we instead determine a more public interface so that a third-party gem could extend ActionDispatch::IntegrationTest without overriding the very private _mock_session method?

I'm up for that.

While I can see this PR as a good compromise it goes against the opinionated nature of Rails. We are including 2 solutions inside the framework, when we usually are very opinionated on what should be used. An API to allow people to build on top of our system gives people to flexibility to chose, while keeping our opinion.

[seanpdoyle]

@rafaelfranca @eileencodes I've reconsidered the initial proposal implemented by e428beb.

In its place, c571a1f introduces a config.FRAMEWORK.assertions configuration value to control the assertions used by ActionController::TestCase, ActionDispatch::IntegrationTest, and ActionView::TestCase.

I've updated the PR's description to encompass the changes, and I've included changes to Guides documentation.

Since performance impacts were a concern, I've included some rough benchmarks in the PR description. Swapping :capybara for :rails_dom_testing is 1.11x slower.

Is this approach an improvement on the initial proposal?