Add `open_browser/1`

[sodapopcan]

Hey German!

Me again.

First off I'd like to say that I'll be totally cool if you don't think this belongs in PhoenixTest, but I did a quick implementation and wanted to create the PR to get the discussion going.

So this introduces a preview/1 function which is essentially a wrapper around Phoenix.LiveViewTest.open_browser/1.

The problem with using open_browser/1 directly

PhoenixTest's functions are all pipeable taking in a "session" as the first arg. With open_browser essentially being IO.inspect for LiveViewTest, we want to be able to stick one in the middle of a pipeline. We can't use open_browser for this since it takes a Phoenix.LiveViewTest.View. Instead we have to change:

conn
|> click_link("foo")
|> fill_form("form", foo: %{bar: "baz"})
|> click_button("Submit")
|> assert_has("Profit!")

into:

sess =
  conn
  |> click_link("foo")

open_browser(sess.view)

sess
|> fill_form("form", foo: %{bar: "baz"})
|> click_button("Submit")
|> assert_has("Profit!")

and back just to "quickly" look at something in the browser.

I'm advocating for this to be a part of PhoenixTest because even though I just started using it yesterday, I'm already pretty in love with it and would prefer not to carry around my own preview function to all my projects (but I will if I have to 🙂).

In a nutshell, preview simply does the following:

def preview(session) do
  Phoenix.LiveViewTest.open_browser(session.view)

  session
end

Why call it preview/1?

Since this function can only work on LiveView tests, I wanted to come up with a more generic name in case some very rough equivalent could be implemented for controller tests. As it stands, this implementation simply raises if given in a controller test.

DECISION: We're gonna call it open_browser.

Problems with this PR

The tests I have now basically just ensure it raises in controller tests but otherwise takes a dummy code path to test the LiveView functionality. In couldn't come up with a good regression test that was simple and un-intrusive. I took inspiration from how open_browser is tested so that's why preview/2 exists, but I realized it's really not the same thing. I think something like Mox would be the proper way to go but I didn't want to introduce that without asking.

Anyway, I'm open to any and all feedback, this is a bit of a draft. Thanks again for this project!

[sodapopcan]

I thought I had was that made preview could pretty print the resp_body in controller test, though it feels odd choosing something when Phoenix doesn't have anything itself.

[germsvel]

Hi @sodapopcan, thanks for this PR! 🎉

I love the idea of introducing something like this preview helper. And I hope to do it soon!

But before we do that, I think I'd want to make two changes (one small and one big).

1. The small one would be to rename it open_browser. As far as it makes sense, I think it's okay to keep with LiveView's convention. I like that because it'll be familiar to people and easy to guess. I like not violating the principle of least surprise -- i.e. if I'm using PhoenixTest for the first time, and I'm acquainted with LiveView, I might reach for open_browser without even checking docs. But I would never guess preview is a helper that's availble.
2. The larger change is to find a better implementation for the Static side of things. I don't want to include features that only support one side or the other, or we might get out of sync quickly and the library will lose value. So, my hope is to go into the LiveView code and see how they're doing the opening of the browser and try to replicate that. Of course, if you wanted to do that, I'd love it if someone else went and tried to figure that out. 😄

[sodapopcan]

The small one would be to rename it open_browser.

This touches on something I brought up in another issue which is: is the overall goal of this project to be able to used in place of LiveViewTest? I'm hoping it is 🙂 but if not, I appreciate the different naming conventions since we can just import LiveViewTest without conflict. Generally I'm not a fan of blanket imports like that (I prefer to avoid import and alias whenever I can) but there are certain cases I prefer it and tests are one of them. On a less significant note, I always like to avoid making my own helpers and open_browser sucks to type where as preview doesn't bother me, but I'm totally find with calling it open_browser 😄

I don't want to include features that only support one side or the other, or we might get out of sync quickly and the library will lose value.

I very much appreciate that!

The larger change is to find a better implementation for the Static side of things.

So I took a stab at this yesterday and got fairly close to a working version. The main problem is the assets but I think I can sort it out. I mostly stopped as I spent a lot of time thinking about this lib yesterday to the point where I started procrastinating on the things I needed to get done. Your response excites me though so I'm going to see if I can push it through. I do keep wondering why such a thing isn't available in Phoenix, though 🤔

[sodapopcan]

Ok, so I actually have a working prototype now haha. I've pushed it but there some things to fix and clean up and hopefully there aren't any total blockers (again, still thinking about why Phoenix doesn't have this). If you give me the go-ahead I'll push through and try and finish it today.

[germsvel]

Ok, so I actually have a working prototype now haha. I've pushed it but there some things to fix and clean up and hopefully there aren't any total blockers (again, still thinking about why Phoenix doesn't have this). If you give me the go-ahead I'll push through and try and finish it today.

🤩 that's awesome! Yeah, by all means, don't stop on my account. It would be great to have that helper.

---

I mostly stopped as I spent a lot of time thinking about this lib yesterday to the point where I started procrastinating on the things I needed to get done

😂 sorry about that. I'm actually on the same boat. I'm having to actively not spend time on the library because I'm excited about it but need to do other things too.

---

This touches on something I brought up in another issue which is: is the overall goal of this project to be able to used in place of LiveViewTest?

To a large extent, the answer is yes. I'd love that. But I don't know that we'll fully be able to replace LiveView testing for two reasons:

1. There are LiveView-specific things I don't think make sense to replace. The first that comes to mind is assert_patch. I don't think we should be asserting that. I just want to assert that the user sees something on the page -- whether it's done through a live patch or something else seems like an implementation detail.
2. I envision some people wanting LiveView-specific tests as more of a "unit-test" level tests. By that I mean they may want to test specifics about how their LiveView behaves that are more LliveView-esque and less browser related. Or perhaps they need to reach out for an escape hatch, like sending messages to the LiveView process. I don't expect to replace that.
3. And then there's a gut feeling. I think LiveView might have some interactive stuff that just doesn't translate well or that we won't be able to cover perfectly and have a good counterpart in the non-liveview world. I'd like to minimize this as much as possible, but there may be some things.

[sodapopcan]

😂 sorry about that. I'm actually on the same boat. I'm having to actively not spend time on the library because I'm excited about it but need to do other things too.

😅 Not your fault! This is one of the reasons I put off trying this out despite my initial enthusiasm on Elixir Forum.

To a large extent, the answer is yes. I'd love that.

Ok good, me too, lol. The immediate value I saw in this lib was the streamlined experience writing LiveView tests more so than the unification aspect (though don't get me wrong, that part is really cool too). I've been wanting something like this for a while and seen a few attempts and have made a few of my own. But this is the first time I saw something and was like, "Ya, this is it!" I'm a fully top-down TDD'r (I guess it's really BDD) and while LiveViewTest is super awesome, I've always seen it as more of a lower-level toolkit to enable something like this, so yay! Thanks for really getting it going :)

---

So back to the PR—I've got it working!

The bad news is that it involved copying over a non-insignificant amount of private functions from LiveView. Not too many, and I made sure I grokked anything I brought over. In essence the following was needed:

• The actual function that opens the browser in a system agnostic fashion.
• The code that prefixes the hrefs and srcs to static assets.
• Code to generate a random filename to save the page in the system's tmp dir.

On that last point, I'm not sure what the problem with using System.unique_integer is but I figured I'd play it safe. I can ask on Elixir Forum.

You can find all of this code in PhoenixTest.Utils (I'm happy to rename this or put it elsewhere).

There is the still issue that the test for the Live version isn't really doing anything. We could write a better test if we brought in more LiveView but I don't think that's a good idea as it's significantly more code and I think it's better to just use Phoenix.LiveViewTest.open_browser directly.

[sodapopcan]

Discussion around random_id function instead of System.unique_integer is found here.

[sodapopcan]

Ha, I just did my last force push! I made the test for the Live version actually test something :) I DI's Phoenix.LiveViewTest.open_browser itself and then just test that open_browser receives a function that is called with a Phoenix.LiveViewTest.View. I'm much happier with this. In fact I'm just plain happy with it though if you know something even better, I'm all ears!

I have to leave for work soon but will address your comments later today.

Woo!

[sodapopcan]

Ok I lied, feedback has been addressed.

I hit Resolve Conversation on one comment so not sure if you'll see my answer (oh boy it's been a bit too long since I've worked on a team) but I'm leaving phx-test- prefix (technically the - comes from System.random_integer) since I believe it has value. If for some reason someone wants to hunt down a test output, it'll help them identify them. Unless you were suggesting to just call it phx-? I can check now since I resolved the convo 😆

[sodapopcan]

I've made some final nit-picky clean ups as well as some light docs and comments before committing to really starting my workday. That's it from me for now!

[germsvel]

Thanks so much for all the great work here @sodapopcan! 🥳

I'm going to put this in the next release. 🎉

[sodapopcan]

🥳 My pleasure! Thank you for your kind words and once again for this lib!

[alexslade]

@sodapopcan @germsvel Thanks for your work on this excellent library, and the work to include open_browser.

I'm looking to add a small change, and that's to stop the removal of all <script> tags when creating the html file to show. Primarily because this prevents the Swoosh email previewer from looking good when opened like this; because it contains a <script src="https://cdn.tailwindcss.com"></script>

I can see where this is happening, but before I dive in and issue a PR, do you have any guidance on how you would prefer this exclusion to exist?

I was curious as to why you are excluding script files, and if you had any objections to keeping script files from external sources in place.

[sodapopcan]

Hey @alexslade!

This implementation was taken from LiveView and tweaked to work with controllers. While I did take care at the time to grok what was going on, and I specifically remember script tags being filtered out,H,H,H, that knowledge has completely vacated my brain 😅 Script tags have been filtered out since the initial introduction of the feature. I had a poke around old issues though couldn't find anything conclusive. This is the most discussion I could find (also here). Maybe the answer is in there? I'm not sure, I only skimmed it.

The best I can theorize is that since JS can't run in LiveView tests, having it run while debugging in an open browser could be potentially be very confusing. The use-case for open_browser isn't meant to debug design, just to get a visual on the functionality. CSS even often looks messed up for me in some apps depending on how I do my layouts.

I suggest bringing this up in LiveView or even just asking on the forums for a better answer than I can provide.

[alexslade]

Thanks for the context ❤️ I'll continue to think about if / where this should be addressed.

[germsvel]

Another thought off the top of my head (not sure if there's backing to it) is that allowing arbitrary JS to run could be a security concern? But... it's in tests, so not sure why that would be. Again, no backing for it. I just know that's why we'd take out script tags in other contexts -- say, when parsing markdown that allows some HTML too.