Add Wallaby driver #74
Conversation
|
Changed to |
ftes
force-pushed
the
feature/wallaby
branch
3 times, most recently
from
f2163a9
to
a5f6d7a
Compare
| defp map_errors(fun) do | ||
| fun.() | ||
| rescue | ||
| e -> |
There was a problem hiding this comment.
I would prefer:
e in Wallaby.QueryError ->
raise ArgumentError, e.messageBut that fails to match. Why? 🤔
There was a problem hiding this comment.
I don't know. I would expect that to work. Is it that a different error was being raised when you tested?
| conn | ||
| |> visit("/live/redirect_on_mount/push_navigate") | ||
| |> assert_has("h1", text: "LiveView main page") | ||
| end | ||
|
|
||
| test "raises error if route doesn't exist", %{conn: conn} do | ||
| @tag :skip |
|
Updated:
|
|
Hi @ftes, thanks so much for this work! I really appreciate it. I know you've already put a lot of time and effort into this. Sorry for not responding sooner. I was hoping to get a good chunk of time to start reviewing this, since it's such a large change. But it doesn't look like I'll have the time anytime soon. Given that, I would suggest we put this on pause. I know having something that can handle Another potential idea I had was to expose the driver protocol so that other libraries could implement their own handlers -- and that would open the possibility for a Wallaby or Playwright or whatever else. So, just to share where my mind is:
I just need to carve out time to review this PR and have an idea in my head of what I'd like the JS handling to be like. Does that make sense? Of course, I'm open to other suggestions -- maybe I'm missing something here, and we should just go forward with this path. |
|
Thanks @germsvel :) This work is mostly done. Pretty much only documentation + I appreciate that it is a sizeable PR which demands significant time for review though. The only potential blocker I see is if you found any changes to existing code problematic.
Apart from that, I don't see why we shouldn't move forward with this for now:
|
|
@ftes I like what you're saying,
And
As for your two potential blockers (assertions and endpoint changes), I'll have to give those a look. Since this is so close to being done, I'll try to carve out some time to review this before going on my tour of alternatives 😄 |
|
@germsvel Would it help you if I broke this PR into a series/stack of smaller PRs? |
|
Thanks for the offer @ftes, but don't spend your time on it just yet! I've been doing a little bit of research on playwright to see if that's a realistic alternative. I know you said we could introduce this and then swap it out if we need to, but once something is in the codebase, it'll be hard to remove it. Don't want to ship breaking changes too often (even in pre 1.0 version). Just hang in there for me if you can. I would really like to handle JS somehow -- just want to do due diligence before we fully commit to Wallaby. |
|
Hey @germsvel how's it going? And getting the preliminary changes in would also allow us to implement an external Wallaby Driver in the current form until playwright is usable. Without having to fork |
There was a problem hiding this comment.
@ftes thanks again for the PR!
I haven't finished reviewing the full thing, but I figured I should at least do it in chunks when I can. There's a lot of work done here. Really appreciate it. 👍
I left a few comments and questions that I hope are helpful.
I'll continue reviewing the PR when I have more time. Let me know if you have questions in the meantime.
| defdelegate assert_has(session, selector), to: Assertions | ||
| defdelegate assert_has(session, selector, opts), to: Assertions | ||
| defdelegate refute_has(session, selector), to: Assertions | ||
| defdelegate refute_has(session, selector, opts), to: Assertions | ||
| defdelegate assert_path(session, path), to: Assertions | ||
| defdelegate assert_path(session, path, opts), to: Assertions | ||
| defdelegate refute_path(session, path), to: Assertions | ||
| defdelegate refute_path(session, path, opts), to: Assertions |
There was a problem hiding this comment.
This is smart. I was hesitant about moving the assertions to the drivers themselves, but so long as we can depend on the shared logic (even better if we're just delegating) then I think this makes sense. 👍
| @@ -20,6 +20,8 @@ defmodule PhoenixTest.Live do | |||
| %__MODULE__{view: view, conn: conn, current_path: current_path} | |||
| end | |||
|
|
|||
| def current_path(session), do: session.current_path | |||
There was a problem hiding this comment.
👍 makes sense that we'd introduce this to get Wallaby's current path.
|
|
||
| session | ||
| |> click(selector, text) | ||
| |> Map.update!(:last_field_query, &if(belongs_to_form?, do: :none, else: &1)) |
There was a problem hiding this comment.
Can you fill me in on why we're keeping track of last_field_query? (I think I get it, but just want to make sure I'm understanding correctly)
There was a problem hiding this comment.
Sure!
To support submit/1, which submits the last touched form.
We don't maintain an active_form state, since that state is in the browser itself.
But this way we at least track which input was last touched, so we can submit the wrapping form.
lib/phoenix_test/wallaby.ex
Outdated
| |> Wallaby.Browser.find(Wallaby.Query.css(selector, text: text)) | ||
| |> Wallaby.Element.click() |
There was a problem hiding this comment.
I think we can combine this to use Wallaby.Browser.click(Query.css(selector, text: text)), right? It basically does what we're doing here if I'm reading the implementation right.
There was a problem hiding this comment.
Absolutely, good catch.
| # Set via JS to avoid locale format issues | ||
| type when type in ~w(date datetime-local time week) -> | ||
| js = """ | ||
| el = document.querySelector('#{field.selector}'); | ||
| el.value = '#{value}'; | ||
| """ | ||
|
|
||
| session.session | ||
| |> Wallaby.Browser.execute_script(js) |
There was a problem hiding this comment.
I'm not sure I'm following here. Why do we do this via JS?
There was a problem hiding this comment.
<input type="date" has different formats depending on the user locale.
E.g. DD.MM.YYYY (german), MM.DD.YYYY (US).
While the browser is usually smart enough to cope with different separates (-, .) it will get mixed up if the month and day are swapped.
Basically giving an incorrect input.
I've had to work around this issue before, this is the best we came up with back then.
Maybe there's a better solution.
E.g. forcing the Chrome locale to a known good value and applying a correctly formatted input string via the usual Wallaby fill_in. But we got stuck on that route (CI was still behaving differently than local machines).
|
|
||
| def uncheck(session, label) do | ||
| query = query(session, label, &Field.find_checkbox!/2) | ||
| Wallaby.Browser.set_value(session.session, query, :unselected) |
There was a problem hiding this comment.
👍 nice. I didn't know about Wallaby's set_value function taking :unselected.
lib/phoenix_test/wallaby.ex
Outdated
| |> Wallaby.Browser.find(query) | ||
| |> Wallaby.Element.click() |
There was a problem hiding this comment.
Maybe simplify to Browser.click(query)?
|
|
||
| defp trigger_phx_change_validations(session, query) do | ||
| if has?(session.session, query) do | ||
| Wallaby.Browser.send_keys(session.session, query, [:tab]) |
There was a problem hiding this comment.
This is a clever way to handle this! Do you know if it works reliably? I haven't tested this myself.
There was a problem hiding this comment.
I've been using it for a while in tests.
"Works for me", but I can't speak to how stable this is.
| defp retry(fun, timeout_ms \\ 300, interval_ms \\ 10) do | ||
| now = DateTime.to_unix(DateTime.utc_now(), :millisecond) | ||
| timeout_at = DateTime.utc_now() |> DateTime.add(timeout_ms, :millisecond) |> DateTime.to_unix(:millisecond) | ||
| retry(fun, now, timeout_at, interval_ms) | ||
| end | ||
|
|
||
| defp retry(fun, now, timeout_at, _interval_ms) when now >= timeout_at do | ||
| fun.() | ||
| end | ||
|
|
||
| defp retry(fun, _now, timeout_at, interval_ms) do | ||
| fun.() | ||
| rescue | ||
| AssertionError -> | ||
| Process.sleep(interval_ms) | ||
| now = DateTime.to_unix(DateTime.utc_now(), :millisecond) | ||
| retry(fun, now, timeout_at, interval_ms) | ||
| end |
There was a problem hiding this comment.
I'm not necessarily against this, but I wonder if this is something we need to introduce now. I think we should rely as much as possible on Wallaby's built-in waiting mechanisms that come from find. Is there a reason why we're including this here and wrapping our assertions with retry?
There was a problem hiding this comment.
There are two routes we can go down to implement phoenix_test assertions for Wallaby:
- Wrap Wallaby's built-in assertions
a. (+) built-in retry/waiting - Use
PhoenixTest.Assertions
a. (+) behaviour automatically the same as other Drivers
I opted for (2.) to ensure the Wallaby Driver behaves as similarly as possible as the other Drivers.
But if we warp PhoenixTest.Assertions we have to add our own retry around them.
Similar case:
For the mutations (fill_in) I also used PhoenixTest.Query instead of Wallaby.Query to find the correct field.
Wallaby doesn't support exact-match label semantics (e.g. field label Race vs Race 2).
Does that make sense to you?
| defp map_errors(fun) do | ||
| fun.() | ||
| rescue | ||
| e -> |
There was a problem hiding this comment.
I don't know. I would expect that to work. Is it that a different error was being raised when you tested?
|
@ftes just saw your comment after I reviewed the PR 😆 You mentioned:
Having reviewed the parts that I reviewed, I do think there could be some nice splits (if you're interested in doing them). Moving the assertions into the We could also add the I don't want you to do more work if you don't feel like it, but if you want to do that, that'd be a great way to introduce changes in pieces. (always easier to review too 😄 ) |
```
** (FunctionClauseError) no function clause matching in Floki.Selector.Parser.do_parse/2
The following arguments were given to Floki.Selector.Parser.do_parse/2:
# 1
[{~c"]", 1}]
# 2
%Floki.Selector{id: nil, type: "input", classes: [], attributes: [%Floki.Selector.AttributeSelector{match_type: :equal, attribute: "name", value: "payer", flag: nil}, %Floki.Selector.AttributeSelector{match_type: :equal, attribute: "type", value: "hidden", flag: nil}], namespace: nil, pseudo_classes: [], combinator: nil}
Attempted function clauses (showing 10 out of 21):
defp do_parse([], selector)
defp do_parse([{:close_parentesis, _} | t], selector)
defp do_parse([{:comma, _} | t], selector)
defp do_parse([{:identifier, _, namespace}, {:namespace_pipe, _} | t], selector)
defp do_parse([{:identifier, _, type} | t], selector)
defp do_parse([{~c"*", _} | t], selector)
defp do_parse([{:hash, _, id} | t], selector)
defp do_parse([{:class, _, class} | t], selector)
defp do_parse([{~c"[", _} | t], selector)
defp do_parse([{:pseudo_not, _} | t], selector)
...
(11 clauses not shown)
```
This commit escapes brackets in the hidden input name by wrapping it in single quotes.
Fixes #73
Done
Endpointto proper/regular endpoint)AssertionsintoDriverprotocol (wrap with retry for Wallaby)test_also_with_jsmacroTo do
Wallaby, use via@tag :jsconnpassed tovisit/2is mostly ignored (not supported:conn.assigns, supported:conn.req_cookies)NoRouteErrortests (broken due to changes toEndpoint)open_browservalue/selected/checkedattributes)conn.req_cookiesviaWallaby.Browser.set_cookie/4