Extend set_value for use on check boxes, radio buttons and drop-down options #237
Conversation
|
Sorry for the clumsy gitting. (I tend to use BZR). I will try to do better in future. ;-) |
|
@graeme-defty don't worrty about the git-ing too much, we usually squash commits which means that all of this will end up as a single commit on master 👍 |
|
:-) Thanks
…On 13 May 2017 at 19:52, Tobias Pfeiffer ***@***.***> wrote:
@graeme-defty <https://github.com/graeme-defty> don't worrty about the
git-ing too much, we usually squash commits which means that all of this
will end up as a single commit on master 👍
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#237 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAK80JecrgEx7jdt3-uUhIJIeTeGiiZrks5r5adwgaJpZM4NZ9zI>
.
|
There was a problem hiding this comment.
Looks good to me overall, I like the test coverage! I had some style comments, which is mostly due to us not having setup credo or some sort of linter yet. Sorry
Thank you very much for your contribution. I'd also wait for a review by @keathley as API vision et. al. still sit better with him, I'm rather new around this block her =D
I asked Nala though, and she likes your PR.
.gitignore
Outdated
| @@ -3,6 +3,7 @@ | |||
| /deps | |||
| erl_crash.dump | |||
| *.ez | |||
| mix.lock | |||
There was a problem hiding this comment.
We don't wanna ignore mix.lock :)
In the Ruby world it's common and I used to do it, but I argued with José once and he had (of course!) very valid points, e.g. a common experience for maintainers etc. - so we'd wanna keep this around :)
There was a problem hiding this comment.
I just read some of the discussions on this. Thanks for the pointer. (I should have guessed that it was needed by the fact that nobody ignored it yet - duh!)
|
|
||
| assert page | ||
| |> find(Query.text_field("email")) | ||
| |> has_value?("c@keathley.io") |
There was a problem hiding this comment.
this test confuses me a bit, it doesn't seem to call set_value at all?
There was a problem hiding this comment.
I addressed this in response to your other point. Should I just remove this test or try to make it fit set_value?
There was a problem hiding this comment.
I think just removing it should be fien
| describe "set_value/3" do | ||
| test "accepts a query", %{page: page} do | ||
| page | ||
| |> send_keys(Query.text_field("Name"), ["Chris", :tab, "c@keathley.io"]) |
There was a problem hiding this comment.
Is there something specific here that we'd wanna use send_keys over fill_in for filling in the form fields?
There was a problem hiding this comment.
I copied the test code from sends_keys and seem to have left this in as a hangover from that. To be honest, I would not have a test for "accepts a query", as I could not see what it was doing over and above the other tests. I kept it to stay consistent, but did not tailor it to set_value. Mea culpa.
|
|
||
| assert page | ||
| |> find(Query.text_field("Name")) | ||
| |> has_value?("Chris") |
There was a problem hiding this comment.
sorry we don't have a code linter setup yet, but I think indentation wise we'd like this rather to be:
assert page
|> call
|> call
(goes for all stuff here) :)
There was a problem hiding this comment.
OK - thanks. Noted for the future.
| |> set_value(Query.option("Select Option 2"), :selected) | ||
| |> find(Query.option("Select Option 1")) | ||
| |> selected?() | ||
| end |
There was a problem hiding this comment.
Good amount of test coverage imo, thanks! 👍
|
|
||
| assert page | ||
| |> find(Query.text_field("Name")) | ||
| |> has_value?("Chris") |
There was a problem hiding this comment.
This also makes me think that maybe we should have a value option for queries (if we don't have that already) so that this could just be: assert_has(parent, Query.text_field("Name", value: "Chris")) but that's for another time :)
|
Thanks for the comments. Most importantly, if Nala likes it them I am happy! What is the procedure now - do I make changes addressing your comments and re-submit the PR? |
|
@graeme-defty you make the changes addressing the comments, then you make a new commit with them and just push them to the branch again. We'll get them here and everything should be fine :) |
|
OK - done! |
lib/wallaby/browser.ex
Outdated
| # IO.warn "set_value/2 has been deprecated. Please use Element.set_value/2" | ||
| # | ||
| # Element.set_value(element, value) | ||
| # end |
There was a problem hiding this comment.
I think we want to keep this in the Browser module since it accepts queries. We will want to remove the commented out code.
lib/wallaby/browser.ex
Outdated
| @@ -392,12 +392,37 @@ defmodule Wallaby.Browser do | |||
| @doc """ | |||
| Sets the value of an element. | |||
There was a problem hiding this comment.
Since we're explicitly looking for :selected and :unselected we'll want to call that out in the documentation.
|
Ouch! Yes, I should have done this.
…On 15 May 2017 at 20:10, Aaron Renner ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/wallaby/browser.ex
<#237 (comment)>:
> @@ -392,12 +392,37 @@ defmodule Wallaby.Browser do
@doc """
Sets the value of an element.
"""
- @SPEC set_value(element, any()) :: element
+ @SPEC set_value(parent, Query.t, Element.value) :: parent
+# @SPEC set_value(element, any()) :: element
+
+# def set_value(element, value) do
+# IO.warn "set_value/2 has been deprecated. Please use Element.set_value/2"
+#
+# Element.set_value(element, value)
+# end
Could the commented out code be removed now?
@keathley <https://github.com/keathley>, @PragTob
<https://github.com/pragtob> Should this change be on Element.set_value
instead Browser.set_value?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#237 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAK80KzkguzFYaR83I1l-lw-i0FTWaxAks5r6E7ZgaJpZM4NZ9zI>
.
|
|
Documentation updated as requested |
|
I think this clashed with a merge to master now where Sorry for the trouble @graeme-defty if you can't get to it one of us probably will :) |
|
Let me try - my gitting skills could use the workout :-s |
If there are duplicate buttons then Wallaby will incorrectly return an error saying that the button has a bad type. However, the real error is that the specified count is incorrect. This commit adds a test for this case and fixes the underlying bug.
…lixir-wallaby#238) * Follow up to elixir-wallaby#229 * @aaronrenner mentioned that it doesn't work on selenium either so not much sense in keeping it around sadly
* Remove select and selected?/1 from Browser module. * Add negative tests for selected?/2 and selected?/1
|
Done! I still officially loathe git though. How on earth has it won over BZR? (Though I have to admit --rebase is pretty cool. Once I eventually got the incantation right, it just ... well ... worked! Maybe I will get used to it. ;-) |
|
@graeme-defty No worries on the git stuff. I cleaned up your branch and merged everything here: e9f4e1a Thanks 🎉 |
No description provided.