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

Locators should be optional when the selector is specified #783

Closed
adammck opened this Issue Aug 9, 2012 · 23 comments

Comments

Projects
None yet
4 participants
@adammck
Contributor

adammck commented Aug 9, 2012

I was surprised to find that all(:checkbox) does not return all checkboxes. It returns those with an id of "checkbox", because it simply assumes that the argument is a locator. The same applies to most other selectors, too. I can't imagine a situation in which this would be the desired behavior.

For example, given this node:

node = Capybara.string <<-HTML
  <input type="checkbox" id="a">
  <input type="checkbox" name="b">
  <input type="checkbox">
HTML

The current behavior is:

>> node.all(:checkbox).length
=> 0

>> node.find(:checkbox)
Capybara::ElementNotFound: Unable to find id :checkbox

But I think it should be changed to:

>> node.all(:checkbox).length
=> 3

>> node.find(:checkbox)
Capybara::Ambiguous: Ambiguous match, found 3 elements matching [...]

If this is considered a bug, I'd be happy to fix it and submit a pull request.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 9, 2012

Contributor

I just noticed that this is closely related to #775. That issue suggests changing the docs, but I think it'd be better to change the implementation

Contributor

adammck commented Aug 9, 2012

I just noticed that this is closely related to #775. That issue suggests changing the docs, but I think it'd be better to change the implementation

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Aug 9, 2012

Collaborator

Sounds reasonable to me. Jonas?

Collaborator

joliss commented Aug 9, 2012

Sounds reasonable to me. Jonas?

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 10, 2012

Contributor

Hm. Unfortunately, it doesn't appear that this is possible without breaking backwards compatibility. Finders called with just a symbol currently assume that it's an element id. This seems pretty unintuitive to me, especially since find_by_id is already provided. Would you be interested in changing this behavior?

Contributor

adammck commented Aug 10, 2012

Hm. Unfortunately, it doesn't appear that this is possible without breaking backwards compatibility. Finders called with just a symbol currently assume that it's an element id. This seems pretty unintuitive to me, especially since find_by_id is already provided. Would you be interested in changing this behavior?

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Aug 10, 2012

Collaborator

I'd really like this behaviour. I agree that the current behaviour with symbols isn't particularly intuitive, and I doubt that many people use it (though I have on occasion). I'd be okay with breaking this behaviour. A couple of problems though:

API

What would the API for the selectors look like?

Capybara.add_selector :checkbox do
  all_xpath { XPath::HTML.checkbox }
  xpath { |value| XPath::HTML.checkbox(value) }
end

Or maybe we should default to a nil value and not specify a separate XPath for it, but then how do we know that the selector supports finding all elements?

Effort

We would have to write XPath expressions for all selectors built into Capybara to find all elements of the relevant type. Then we'd have to write tests to see that they actually work. And that's aside from the engineering effort to get this behaviour in the first place. That's mostly why I've been hesitant about doing this. It's a lot of work.

Collaborator

jnicklas commented Aug 10, 2012

I'd really like this behaviour. I agree that the current behaviour with symbols isn't particularly intuitive, and I doubt that many people use it (though I have on occasion). I'd be okay with breaking this behaviour. A couple of problems though:

API

What would the API for the selectors look like?

Capybara.add_selector :checkbox do
  all_xpath { XPath::HTML.checkbox }
  xpath { |value| XPath::HTML.checkbox(value) }
end

Or maybe we should default to a nil value and not specify a separate XPath for it, but then how do we know that the selector supports finding all elements?

Effort

We would have to write XPath expressions for all selectors built into Capybara to find all elements of the relevant type. Then we'd have to write tests to see that they actually work. And that's aside from the engineering effort to get this behaviour in the first place. That's mostly why I've been hesitant about doing this. It's a lot of work.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 10, 2012

Contributor

To make selectors as powerful as possible, I'd probably forward the call made to Query#initialize to XPath::HTML pretty much as-is. So defining simple selectors would look something like:

Capybara.add_selector :checkbox do
  xpath { |*args| XPath::HTML.checkbox *args }
end

We'd need to modify XPath::HTML to make those locators optional, but that seems useful anyway. And a (totally contrived, not tested) more complex example might look like:

Capybara.add_selector :comment do
  xpath { |*args| XPath.descendant[XPath.attr("data-role") == "comment"] }
  filter(:by) { |node, author| node.find(:css, ".author").text.match author }
end

I don't think I'd worry about declaring support for finding all elements. If it doesn't work, it'll can/should raise an error. And I don't foresee any of the built-in selectors having trouble with that.

Regarding effort, yeah, this is pretty big. If you're good with the API, I'll take a stab at it in my fork and see what happens. I think that the majority of the work will be updating the test suites and docs. The actual implementation might even be simpler than we have now, since it'll be delegating more to XPath.

Contributor

adammck commented Aug 10, 2012

To make selectors as powerful as possible, I'd probably forward the call made to Query#initialize to XPath::HTML pretty much as-is. So defining simple selectors would look something like:

Capybara.add_selector :checkbox do
  xpath { |*args| XPath::HTML.checkbox *args }
end

We'd need to modify XPath::HTML to make those locators optional, but that seems useful anyway. And a (totally contrived, not tested) more complex example might look like:

Capybara.add_selector :comment do
  xpath { |*args| XPath.descendant[XPath.attr("data-role") == "comment"] }
  filter(:by) { |node, author| node.find(:css, ".author").text.match author }
end

I don't think I'd worry about declaring support for finding all elements. If it doesn't work, it'll can/should raise an error. And I don't foresee any of the built-in selectors having trouble with that.

Regarding effort, yeah, this is pretty big. If you're good with the API, I'll take a stab at it in my fork and see what happens. I think that the majority of the work will be updating the test suites and docs. The actual implementation might even be simpler than we have now, since it'll be delegating more to XPath.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Aug 11, 2012

Collaborator

The advantage to being explicit about whether a selector supports a query for all elements is that we could use that information in generating better error messages when a find fails. e.g.:

expected to find link "Hello", but found no matches. Maybe you meant one of "Hllo", "Goodbye".

That would be very cool!

Collaborator

jnicklas commented Aug 11, 2012

The advantage to being explicit about whether a selector supports a query for all elements is that we could use that information in generating better error messages when a find fails. e.g.:

expected to find link "Hello", but found no matches. Maybe you meant one of "Hllo", "Goodbye".

That would be very cool!

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 13, 2012

Contributor

Hm, that would indeed be very cool. But it's possible to achieve that implicitly, just by retrying without the query. If the selector doesn't support that (e.g. the :id selector), then it can raise an error, and not be included in the error message. This could be encapsulated by Capybara::Query, and would simplify the selectors.

Contributor

adammck commented Aug 13, 2012

Hm, that would indeed be very cool. But it's possible to achieve that implicitly, just by retrying without the query. If the selector doesn't support that (e.g. the :id selector), then it can raise an error, and not be included in the error message. This could be encapsulated by Capybara::Query, and would simplify the selectors.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 13, 2012

Contributor

Here's an example. Please disregard the fruity implementation and check out the specs.

Contributor

adammck commented Aug 13, 2012

Here's an example. Please disregard the fruity implementation and check out the specs.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Aug 13, 2012

Collaborator

I don't think this is going to work that well implicitly. Imagine a selector which works just fine without an argument given, but returns a completely crazy result. That wouldn't be great obviously. I really like this idea, but we should be able to come up with some kind of explicit api for making this work. If we can get this in before 2.0 we can even break the selector API if we need to (we already have, with the removal of failure_message).

Collaborator

jnicklas commented Aug 13, 2012

I don't think this is going to work that well implicitly. Imagine a selector which works just fine without an argument given, but returns a completely crazy result. That wouldn't be great obviously. I really like this idea, but we should be able to come up with some kind of explicit api for making this work. If we can get this in before 2.0 we can even break the selector API if we need to (we already have, with the removal of failure_message).

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Aug 13, 2012

Collaborator

Like this, maybe:

Capybara.add_selector :checkbox do
  all_xpath { XPath::HTML.checkbox }
  find_xpath { |value| XPath::HTML.checkbox(value) }
end

Capybara.add_selector :checkbox do
  all_css { "input.checkbox" }
  find_css { |value| "input.checkbox[name=#{value}]" }
end

I know it's not super pretty, but it does the job, and it allows us to continue to support both XPath and CSS.

Collaborator

jnicklas commented Aug 13, 2012

Like this, maybe:

Capybara.add_selector :checkbox do
  all_xpath { XPath::HTML.checkbox }
  find_xpath { |value| XPath::HTML.checkbox(value) }
end

Capybara.add_selector :checkbox do
  all_css { "input.checkbox" }
  find_css { |value| "input.checkbox[name=#{value}]" }
end

I know it's not super pretty, but it does the job, and it allows us to continue to support both XPath and CSS.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 14, 2012

Contributor

Okay, here's a rough implementation of that. Only XPath and checkboxes are supported right now, but it's (surprisingly) 100% backwards compatible, just by aliasing the old Selector#xpath stuff to #find_xpath. Obviously the whole thing needs refactoring and better tests, but your feedback on the general direction would be appreciated.

Contributor

adammck commented Aug 14, 2012

Okay, here's a rough implementation of that. Only XPath and checkboxes are supported right now, but it's (surprisingly) 100% backwards compatible, just by aliasing the old Selector#xpath stuff to #find_xpath. Obviously the whole thing needs refactoring and better tests, but your feedback on the general direction would be appreciated.

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Aug 22, 2012

Collaborator

Regarding compatibility, we'll have to remove find(:my_id), right? It seems to me that there's two ways to do this:

(1) Deprecate it in 2.0, remove it in 2.1, and add find(:checkbox) and friends in 2.1.
(2) Remove it in 2.0, and add find(:checkbox) and friends in 2.0.

Both are compatible with semver (the way I interpret it).

Collaborator

joliss commented Aug 22, 2012

Regarding compatibility, we'll have to remove find(:my_id), right? It seems to me that there's two ways to do this:

(1) Deprecate it in 2.0, remove it in 2.1, and add find(:checkbox) and friends in 2.1.
(2) Remove it in 2.0, and add find(:checkbox) and friends in 2.0.

Both are compatible with semver (the way I interpret it).

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Aug 22, 2012

Collaborator

Regarding whether we should have this feature: It seems to be a rather rare use case, for expressions like all(:checkbox). For find, something like find(:checkbox) seems too brittle to be useful.

Because of that, I'm not convinced that it's worth the added complexity in the selector definitions (and other code).

The only real usability issue I see is node.all(:checkbox).length being 0 (confusingly), because it searches by ID. If we deprecated the symbols-as-ID feature, this problem would go away. But even that seems rare enough that I wouldn't worry about it.

So my vote is, leave it as it is. It's @jnicklas's call though, as always.

Collaborator

joliss commented Aug 22, 2012

Regarding whether we should have this feature: It seems to be a rather rare use case, for expressions like all(:checkbox). For find, something like find(:checkbox) seems too brittle to be useful.

Because of that, I'm not convinced that it's worth the added complexity in the selector definitions (and other code).

The only real usability issue I see is node.all(:checkbox).length being 0 (confusingly), because it searches by ID. If we deprecated the symbols-as-ID feature, this problem would go away. But even that seems rare enough that I wouldn't worry about it.

So my vote is, leave it as it is. It's @jnicklas's call though, as always.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 22, 2012

Contributor

My rough implementation is backwards-compatible with find(:my_id), because it simply falls back to the old behavior if the first parameter isn't a valid selector type. I recognize that this is more complex than absolutely necessary, but backwards compatibility seems more important than purity here.

I think that we should remove find(:my_id) anyway, since it's so easy to update to find(:id, "my_id") or find_by_id("my_id"), but this would be a backwards-incompatible change, so it would be a major version bump. I'd deprecate the old behavior in 2.0 and remove it in 3.0, but it's obviously @jnicklas' call.

Contributor

adammck commented Aug 22, 2012

My rough implementation is backwards-compatible with find(:my_id), because it simply falls back to the old behavior if the first parameter isn't a valid selector type. I recognize that this is more complex than absolutely necessary, but backwards compatibility seems more important than purity here.

I think that we should remove find(:my_id) anyway, since it's so easy to update to find(:id, "my_id") or find_by_id("my_id"), but this would be a backwards-incompatible change, so it would be a major version bump. I'd deprecate the old behavior in 2.0 and remove it in 3.0, but it's obviously @jnicklas' call.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Aug 22, 2012

Contributor

Regarding @joliss' second point, I agree that find(:checkbox) is not very useful. But since find(:x, locator) is used so frequently, it seems reasonable to expect that find(:x) and all(:x) would work in the same way. I don't think that it should be required that all selectors expect a locator, especially not user-defined selectors.

Contributor

adammck commented Aug 22, 2012

Regarding @joliss' second point, I agree that find(:checkbox) is not very useful. But since find(:x, locator) is used so frequently, it seems reasonable to expect that find(:x) and all(:x) would work in the same way. I don't think that it should be required that all selectors expect a locator, especially not user-defined selectors.

jnicklas added a commit that referenced this issue Sep 9, 2012

Change selector normalization to check for symbol
With this change, any call to `find`, `all`, etc where the first argument is a Symbol, will assume that that Symbol is the name of the selector. This makes the `match` option on the `id` selector impossible, so `find(:foo)` will no longer find elements by id.

This change is in preparation for allowing selectors without locators, as discussed in #783, which we will probably hold off with until Capybara 2.1, because it is too much work.

This commit ensures we won't have to break the API to make implementing #783 possible.
@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Sep 9, 2012

Collaborator

@joliss and @adammck check out the commit I just pushed. I don't think we'll have time to get this into Capybara 2.0, so I preemptively broke the API. I think it's better to be unambiguous. Now find(:foo) will always be a custom selector with the name :foo, you don't have to know whether or not that is defined, which seems better to me. That leaves us the option of creating a nice consistent API for this in the future.

Collaborator

jnicklas commented Sep 9, 2012

@joliss and @adammck check out the commit I just pushed. I don't think we'll have time to get this into Capybara 2.0, so I preemptively broke the API. I think it's better to be unambiguous. Now find(:foo) will always be a custom selector with the name :foo, you don't have to know whether or not that is defined, which seems better to me. That leaves us the option of creating a nice consistent API for this in the future.

@adammck

This comment has been minimized.

Show comment
Hide comment
@adammck

adammck Sep 9, 2012

Contributor

Great. I totally agree that it's better to be unambiguous, so long as you're happy with the compatibility breakage. I'll rebase my fork against this, which will drastically simplify things. Thanks!

Contributor

adammck commented Sep 9, 2012

Great. I totally agree that it's better to be unambiguous, so long as you're happy with the compatibility breakage. I'll rebase my fork against this, which will drastically simplify things. Thanks!

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Sep 9, 2012

Collaborator

I think that's a good move.

Collaborator

joliss commented Sep 9, 2012

I think that's a good move.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Sep 9, 2012

Collaborator

@adammck do send a pull request once you're ready!

Collaborator

jnicklas commented Sep 9, 2012

@adammck do send a pull request once you're ready!

@abotalov

This comment has been minimized.

Show comment
Hide comment
@abotalov

abotalov Feb 20, 2014

Collaborator

I don't see the use case for this too. Consider the following:

Capybara.ignore_hidden_elements = false
find(:option, 'Option text') # will make 1 driver method invocation
find(:option, text: 'Option text') # will make N+1 driver method invocation where N is a number of options
Capybara.ignore_hidden_elements = true
find(:option, 'Option text') # will make 1 driver methods invocation
find(:option, text: 'Option text') # will make 2*N+1 driver method invocation where N is a number of options

IMO we shouldn't add API that encourages people to write slower tests.

Collaborator

abotalov commented Feb 20, 2014

I don't see the use case for this too. Consider the following:

Capybara.ignore_hidden_elements = false
find(:option, 'Option text') # will make 1 driver method invocation
find(:option, text: 'Option text') # will make N+1 driver method invocation where N is a number of options
Capybara.ignore_hidden_elements = true
find(:option, 'Option text') # will make 1 driver methods invocation
find(:option, text: 'Option text') # will make 2*N+1 driver method invocation where N is a number of options

IMO we shouldn't add API that encourages people to write slower tests.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Feb 20, 2014

Collaborator

@abotalov did you post this to the right issue? I agree, but I'm not sure what this has to do with this issue?

Collaborator

jnicklas commented Feb 20, 2014

@abotalov did you post this to the right issue? I agree, but I'm not sure what this has to do with this issue?

@abotalov

This comment has been minimized.

Show comment
Hide comment
@abotalov

abotalov Feb 20, 2014

Collaborator

@jnicklas I think I posted to the right issue. I think adding this API will encourage people to write something like:

find(:option, text: 'Option text')

which would be bad. I don't see a way how we can write it so it won't make N+1 driver method invocation.

Collaborator

abotalov commented Feb 20, 2014

@jnicklas I think I posted to the right issue. I think adding this API will encourage people to write something like:

find(:option, text: 'Option text')

which would be bad. I don't see a way how we can write it so it won't make N+1 driver method invocation.

@jnicklas

This comment has been minimized.

Show comment
Hide comment
@jnicklas

jnicklas Feb 20, 2014

Collaborator

Ahh gotcha! Hadn't considered that. Yes that's obviously not awesome. I don't think that was the original intention with the idea, but I see how it would encourage people to do that. This is a pretty old issue with no movement on it, the only reason it's still open is because it's kind of a darling of mine, I'll close this now. There's really no point in keeping this open, no one seems to be stepping up to fix this anyway.

Collaborator

jnicklas commented Feb 20, 2014

Ahh gotcha! Hadn't considered that. Yes that's obviously not awesome. I don't think that was the original intention with the idea, but I see how it would encourage people to do that. This is a pretty old issue with no movement on it, the only reason it's still open is because it's kind of a darling of mine, I'll close this now. There's really no point in keeping this open, no one seems to be stepping up to fix this anyway.

@jnicklas jnicklas closed this Feb 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment