Support assert_has value finder for select - #309
Conversation
| case opts.label do | ||
| :no_label -> | ||
| &Query.find(&1, selector, Opts.to_list(opts)) | ||
| &Query.find_by_value(&1, selector, value, Opts.to_list(opts)) |
There was a problem hiding this comment.
Find by value implemented differently for select than input.
This makes we wonder: Should we make this explicit in the API: assert_has("select", label: "Race", selected_option_text: "Elf")?
There was a problem hiding this comment.
I think using one of the following would be good for clarity:
assert_has("select", label: "Race", selected: "Elf")
assert_has("select", label: "Race", selected_value: "Elf")
assert_has("select", label: "Race", selected_option: "Elf")What do you think?
There was a problem hiding this comment.
selected_value is my least favourite, because it might imply you're asserting on the selected option's value rather than the option's label.
How about selected. It's not precise (as in selected_option_label), but given PhoenixTest preference for operating on visible labels, I think this is consistent and concise.
There was a problem hiding this comment.
Yeah, I'm with you. I think selected is the best of the three.
| selected_options | ||
| |> Stream.concat(all(select, "option")) | ||
| |> Enum.take(1) |
There was a problem hiding this comment.
Too cryptic?
"Take first selected option, otherwise fall back to first option (selected by default)"
There was a problem hiding this comment.
That is a bit cryptic. I didn't realize that's what it was doing. 😄
Can we add tests at this level that document that? I think I'd be okay with cryptic so long as the html_test.exs tests capture that clearly.
There was a problem hiding this comment.
Made the code more explicit and added tests.
germsvel
left a comment
germsvel
left a comment
There was a problem hiding this comment.
Love the work! 🙌
Thanks for doing this. I left a few comments. The biggest one (I think) is what should we call the option? Otherwise, this is looking great!
| case opts.label do | ||
| :no_label -> | ||
| &Query.find(&1, selector, Opts.to_list(opts)) | ||
| &Query.find_by_value(&1, selector, value, Opts.to_list(opts)) |
There was a problem hiding this comment.
I think using one of the following would be good for clarity:
assert_has("select", label: "Race", selected: "Elf")
assert_has("select", label: "Race", selected_value: "Elf")
assert_has("select", label: "Race", selected_option: "Elf")What do you think?
| selected_options | ||
| |> Stream.concat(all(select, "option")) | ||
| |> Enum.take(1) |
There was a problem hiding this comment.
That is a bit cryptic. I didn't realize that's what it was doing. 😄
Can we add tests at this level that document that? I think I'd be okay with cryptic so long as the html_test.exs tests capture that clearly.
7e6c218 to
0871355
Compare
8283fa5 to
af39450
Compare
| when is_binary(label) do | ||
| &Query.find_by_label(&1, selector, label, Opts.to_list(opts)) | ||
| end | ||
| case {content, opts, operation} do |
There was a problem hiding this comment.
I tried to make the conflicting content opts more explicit.
That led me to discover that text + label opts are unsupported (label silently ignored). I made that explicit by raising an error instead.
| defp finder_fun(selector, %Opts{text: :no_text, value: value} = opts, _operation) do | ||
| value_finder_fun(ensure_binary(value), selector, opts) | ||
| end | ||
| {[], %Opts{label: label}, _} when is_binary(label) -> |
There was a problem hiding this comment.
We keep repeating the is_binary(label) guard and ensure_binary(value) type coercions.
That could be hoisted into Opts.parse - since we don't support different Opts types elsewhere.
Didn't want to change too much here though.
1d57cc0 to
eee4c7b
Compare
2 participants
Support
assert_has/3andrefute_has/3withvalue:forselectelements by matching selected option text.Implements #262.