germsvel / phoenix_test

MIT License
144 stars 20 forks source link

Add Wallaby driver #74

Open ftes opened 3 months ago

ftes commented 3 months ago

Fixes #73

Done

To do

ftes commented 3 months ago

Changed to draft, but @germsvel would love your feedback already.

ftes commented 3 months ago

Updated:

germsvel commented 3 months ago

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 js in PhoenixTest would be awesome. (at least, I'd like that). But I haven't thought carefully about how to introduce that yet. One way might be Wallaby (which is what you have here), but before we go in that direction, I was hoping to explore some other potential alternatives (like Playwright).

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.

ftes commented 3 months ago

Thanks @germsvel :)

This work is mostly done. Pretty much only documentation + NoRouteError fix missing from my side.

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. E.g.

Apart from that, I don't see why we shouldn't move forward with this for now:

germsvel commented 3 months ago

@ftes I like what you're saying,

The Wallaby driver can be drop-in-replaced in the future. Transparently for any consumers, keeping the generic@tag :js in place.

And

Document as "beta" feature. See it as a change to gather feedback to guide future initiatives.

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 😄

ftes commented 3 months ago

@germsvel Would it help you if I broke this PR into a series/stack of smaller PRs? Wouldn't take much time on my size, and would make review less daunting. Plus might trigger some valuable discussion along the way.

germsvel commented 3 months ago

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.

ftes commented 2 months ago

Hey @germsvel how's it going? Still happy to split the PR. Even if you decide to go with Playwright, some preliminary work would likely be helpful: Moving Assertions into the Driver protocol.

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 phoenix_test.

germsvel commented 2 months ago

@ftes just saw your comment after I reviewed the PR 😆

You mentioned:

Still happy to split the PR. Even if you decide to go with Playwright, some preliminary work would likely be helpful: Moving Assertions into the Driver protocol.

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 Driver protocol would be a good one, for sure.

We could also add the current_path function as another one.

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 😄 )

ftes commented 2 months ago

Thanks for the review @germsvel

Having reviewed the parts that I reviewed, I do think there could be some nice splits (if you're interested in doing them).

Absolutely, I'm on it!

Split so far: #88, #89, #90, #91

ftes commented 2 weeks ago

Did you gain any further insights by looking at Playwright, @germsvel ?

germsvel commented 2 weeks ago

@ftes yes! Great question.

Here are my thoughts so far. No particular order, and take them with a grain of salt. They're all subject to change as we discover more:

I know your PR seemed really close to getting Wallaby in here. And I'm sorry we haven't merged it in yet. Thanks for the patience! I just want to land at some good API before we fully introduce it.

ftes commented 1 day ago

Because I'd like to integrate both Wallaby and Playwright, I think a @tag driver: :playwright or @tag driver: :wallaby would be what I'd suggest we use.

That seems sensible.

I also want to iron out a good API for Wallaby and Playwright. Should they start with a conn like the rest of the tests and we transform it into a session after visit/2? Or should we rely more on Wallaby's and Playwright's built-in stuff and get something like wallaby's session in the test context?

I'd say: It depends on what the goal of offering alternative drivers is.

Personally, I'd vote for plug&play. For me this would be the most common requirement. For the few use cases where I need full control (e.g. test interaction of concurrent user session) I would e.g. use vanilla Wallaby then.

Wallaby has Wallaby.Feature that comes with the library. We don't have to use it, but I wanted to explore if we're losing anything by not using it. And does that mean we should introduce a use PhoenixTest.ConnCase or something like that? It might be helpful if we need to do some test setup.

I think this only adds support for @sessions. That offers the caller more flexibility (e.g. two parallel user sessions, controll session configuration). This PR instead simply enforces a single session with default settings.

ftes commented 1 day ago

The playwright-elixir library has a lot of work done, but some things are still missing. The biggest issue for us is that it hasn't implemented auto-waiting assertions yet. There _are_ways to make assertions, but it's kinda clunky. So, for now, when I introduce playwright it'll be under some "experimental" section (just so people know).

That wouldn't be an issue if we use the approach in this PR: implement auto awaiting in Driver (wrap with retry).