wojtekmach / req

Req is a batteries-included HTTP client for Elixir.
https://hexdocs.pm/req
Apache License 2.0
1.09k stars 117 forks source link

Stubbing a Req.Request in a Phoenix.LiveView.assign_async #429

Open davydog187 opened 3 days ago

davydog187 commented 3 days ago

Disclaimer: I will note that this is an "advanced" use case which may not be solvable, but I wanted to have a discussion first.

Hello @wojtekmach, thank you for your amazing library, I love Req!

This issue is to determine if there's a possible solution for testing a Req.Request inside of async LiveView assign. Imagine the following live view (btw I can totally appreciate that this might need a change in LiveView but I'll start here if thats ok)

defmodule MyApp.Live do
  use Phoenix.LiveView

  def mount(_, _, _) do
    {:ok, assign_async(socket, :foo, fn -> {:ok, Req.get!("http://something.com").body} end)}
  end

  def render(assigns) do
    ~H"""
    <.async_result :let={foo} assign={@foo}>
      <%= inspect(foo) %>
    </.async_result>
    """
  end
end

The problem here is that in order to test this LiveView and stub out the call to Req.get!, there is a race condition, as the request to the LiveView will have already been made before you get the pid back that you can callReq.Test.allow/3` against.

e.g.

test "assync_assign with a Req.Request", %{conn: conn} do
  {:ok, live, _html} = live(conn, ~p"/my-route")

  # Race condition as `Req.get!` may have already run
  Req.Test.allow(MyApp.Req, self(), live.pid)
  Req.Test.stub(MyApp.Req, fn conn -> Req.Test.json(conn, %{}) end)
end

Of course, this could be solved with some sort of conditional that uses environment config, or a mocking library like Mox or Mimic.

However, I'd like to imagine there could be another way to propagate ownership so that the allow could be called prior to the LiveView creation. This may require changes to LiveView, it might be impossible, or it might not be something you want to support.

Thanks for the consideration and feel free to close if this is something you are not interested in supporting

davydog187 commented 3 days ago

I suppose this generalizes to GenServers that call Req in init/1 or a handle_continue/2 from init, so the mention of LV is maybe not necessary

zachallaun commented 3 days ago

Mox allows you to pass a function to allow in case you need to delay evaluation of the allowed pid. Could something like this work?

parent = self()

Req.Test.allow(MyApp.Req, self(), fn ->
  send(parent, {:allow, self()})
  receive(do: (pid -> pid))
end)

{:ok, live, _html} = live(conn, ...)

assert_receive {:allow, pid}
send(pid, live.pid)

This is a little clunky. It means that, if a function is passed, it needs to be run in its own process to prevent deadlocks, but perhaps a pattern like this could work? It could be wrapped in a helper to make it a bit more ergonomic.

davydog187 commented 3 days ago

Another solution would use telemetry events to hook into the process

wojtekmach commented 3 days ago

Yeah speaking of telemetry events, we have this Broadway guide: https://hexdocs.pm/req/Req.Test.html#module-broadway

I think it is fine when people do that in their own GenServers and Broadway pipelines which I’d considered intermediate level but this is too much for people using LV so we should definitely improve this. Since Ecto sandbox works in LV we should have infrastructure in place to make that happen. Or it does in LV but not in async assigns? In any case, yes, this is definitely a priority and any help is appreciated.

wojtekmach commented 3 days ago

Wait, this seems to work already? https://gist.github.com/wojtekmach/2ee023315dd6ea209d67b6d3ef8cd985

Could you modify the snippet to show the bug?