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.

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

Introduce Node#has_element? #2700

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Aug 16, 2023

While other, more specific, selectors and matches exist, it's very common for test suites to fall back to CSS's support for attribute matching.

For example, it's common for suites to find <img> elements by their [src] value through concatenating a CSS Selector string:

expect(page).to have_css("img[src=#{image_path}]")

Sometimes, including double quotes into the String isn't necessary, but when it is, it leads to escaping, combined ' and " usage, alternative syntaxes like %{...}, or calls to String#inspect:

expect(page).to have_css("img[src=\"#{image_path}\"]")
expect(page).to have_css('img[src="#{image_path}"]')
expect(page).to have_css(%{img[src="#{image_path}"]})
expect(page).to have_css("img[src=#{image_path.inspect}]")

The :element selector thrives in circumstances like this:

expect(page).to have_selector :element, "img", src: image_path

In my experience, it's uncommon for teams to even be aware that the :element selector exists, let alone make longer-form calls like have_selector that require both the :element and "img" argument.

This commit proposes the Node#has_element? and Node#has_no_element? methods to power have_element, assert_element, etc. The hope is that the generated Ruby documentation will popularize the use of the :element selector.

With this new method, the calls above are much more concise:

expect(page).to have_element "img", src: image_path
assert_element "img", src: image_path

They even support regular expressions in a way that was tricky before:

expect(page).to have_element "img", src: /image.png/
assert_element "img", src: /image.png/

@seanpdoyle seanpdoyle force-pushed the node-has_element branch 4 times, most recently from 9dd66fa to db4abfe Compare August 16, 2023 22:22
lib/capybara/node/matchers.rb Outdated Show resolved Hide resolved
@twalpole
Copy link
Member

The reason this hasn't been done previously is because of the difficulty in communicating the edge cases around the system option collisions and peoples general inability to understand the difference between properties and attributes. That combined with my belief we should be getting rid of the predicates rather than adding more has me leaning towards no on this, but I'll leave it open while I think about it

While other, more specific, selectors and matches exist, it's very
common for test suites to fall back to CSS's support for attribute
matching.

For example, it's common for suites to find `<img>` elements by their
`[src]` value through concatenating a CSS Selector string:

```ruby
have_css("img[src=#{image_path}]")
```

Sometimes, including double quotes into the String isn't necessary, but
when it is, it leads to escaping, combined `'` and `"` usage,
alternative syntaxes like `%{...}`, or calls to `String#inspect`:

```ruby
have_css("img[src=\"#{image_path}\"]")
have_css('img[src="#{image_path}"]')
have_css(%{img[src="#{image_path}"]})
have_css("img[src=#{image_path.inspect}]")
```

The `:element` selector thrives in circumstances like this:

```ruby
have_selector :element, "img", src: image_path
```

In my experience, it's uncommon for teams to even _be aware_ that the
`:element` selector exists, let alone make longer-form calls like
`have_selector` that require both the `:element` and `"img"` argument.

This commit proposes the `Node#has_element?` and `Node#has_no_element?`
methods to power `have_element`, `assert_element`, etc. The hope is that
the generated Ruby documentation will popularize the use of the
`:element` selector.
@seanpdoyle
Copy link
Contributor Author

the difficulty in communicating the edge cases around the system option collisions

I'm not sure I understand what you mean. Would you be able to share an example?

peoples general inability to understand the difference between properties and attributes

When I introduced these changes, I only had Rack Test, structure, and attributes in mind. I hadn't considered access to properties. Since these new methods boil down to short-hands for the :element selector itself, are there changes we could make to it that would make these aliases more viable?

we should be getting rid of the predicates

Is the idea that reducing the number of aliases like have_link and have_button by replacing them with calls like have_selector :link and have_selector :button would be more verbose, but ultimately simplified? I could understand that point for most short-hands, but the absence of the :element short-hand has always surprised me.

@twalpole
Copy link
Member

As an example, what exactly does page.has_element? "input", info: "abc", minimum: 3, min: 4, class: "class1 class2" match?

On the predicates question, I'm referring to only the predicate methods (those ending in ?) not the matchers. There really is no good reason for using the predicates in tests, and yet people continue to do so.

@seanpdoyle
Copy link
Contributor Author

@twalpole would this be a more acceptable changeset if it only introduced the assert_element and have_element assertions, and skipped over introducing a new predicate method?

@seanpdoyle seanpdoyle requested a review from twalpole September 22, 2023 17:08
@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Nov 3, 2023

@twalpole as an alternative to playing whack-a-mole with selectors and helpers, I wonder if you're interested in expanding Capybara's public API to make this something that consumer applications or libraries can do on their own.

Something like:

class ATestCase < ActiveSupport::TestCase
  include Capybara::Minitest::Assertions

  define_assertions_for_selector :element
end

RSpec.describe MyClass do
  define_matchers_for_selector :element
end

Capybara could use that same macro internally. I think it might be helpful to provide simplified user-facing porcelain. I've made the mistake of defining an assertion like:

def assert_element(...)
  assert_selector(:element, ...)
end

def assert_no_element(...)
  assert_no_selector(:element, ...)
end

While this works for most cases, it ignores baking-in support for the subject argument (the first positional argument), which (even as a long-time Capybara user) I had no idea about until last week.

I'm happy to explore this in a separate PR. Is that something that would be a compromise on the continuum between adding a new predicate method and making no changes?

@seanpdoyle
Copy link
Contributor Author

@twalpole I've opened #2715 to explore providing a helpers to consumer application's test suites to expand the kinds of assertions.

@twalpole
Copy link
Member

I've changed my mind on this, I'm going to merge just for consistency in the v3.x branch --- can revist cleanup when we get to 4.x

@twalpole twalpole merged commit fafcb58 into teamcapybara:master Dec 11, 2023
@seanpdoyle seanpdoyle deleted the node-has_element branch December 11, 2023 20:17

it 'should be true if the given element is on the page' do
expect(@session).to have_element('a', id: 'foo')
expect(@session).to have_element('a', text: 'A link', href: '/with_simple_html')
Copy link
Contributor

Choose a reason for hiding this comment

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

The test generates invalid xpath:

$x("(.//*[(local-name(.) = 'a')][(./@href = '/with_simple_html')])[]")
VM99:1 Uncaught DOMException: Failed to execute '$x' on 'CommandLineAPI': The string '(.//*[(local-name(.) = 'a')][(./@href = '/with_simple_html')])[]' is not a valid XPath expression.
    at <anonymous>:1:1

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants