allow to configure if detected errors raise or warn (#3653)

* allow to configure if detected errors raise or warn when testing LiveViews

Adds a check for duplicate LiveComponents (Fixes #3650).

* test duplicate id / component errors

lib/phoenix_live_view/test/client_proxy.ex

@@ -22,7 +22,8 @@ defmodule Phoenix.LiveViewTest.ClientProxy do
             id: nil,
             uri: nil,
             connect_params: %{},
-            connect_info: %{}
+            connect_info: %{},
+            on_error: :raise
 
   alias Plug.Conn.Query
   alias Phoenix.LiveViewTest.{ClientProxy, DOM, Element, View, Upload}
@@ -82,12 +83,13 @@ defmodule Phoenix.LiveViewTest.ClientProxy do
       router: router,
       session: session,
       url: url,
-      test_supervisor: test_supervisor
+      test_supervisor: test_supervisor,
+      on_error: on_error
     } = opts
 
     # We can assume there is at least one LiveView
     # because the live_module assign was set.
-    root_html = DOM.parse(response_html)
+    root_html = DOM.parse(response_html, fn msg -> send(self(), {:test_error, msg}) end)
 
     {id, session_token, static_token, redirect_url} =
       case Map.fetch(opts, :live_redirect) do
@@ -111,7 +113,10 @@ defmodule Phoenix.LiveViewTest.ClientProxy do
       router: router,
       uri: URI.parse(url),
       child_statics: Map.delete(DOM.find_static_views(root_html), id),
-      topic: "lv:#{id}"
+      topic: "lv:#{id}",
+      # we store on_error in the view ClientProxy struct as well
+      # to pass it when live_redirecting
+      on_error: on_error
     }
 
     # We build an absolute path to any relative
@@ -143,7 +148,8 @@ defmodule Phoenix.LiveViewTest.ClientProxy do
       session: session,
       test_supervisor: test_supervisor,
       url: url,
-      page_title: :unset
+      page_title: :unset,
+      on_error: on_error
     }
 
     try do
@@ -474,6 +480,23 @@ defmodule Phoenix.LiveViewTest.ClientProxy do
     end
   end
 
+  def handle_info({:test_error, error}, state) do
+    case state.on_error do
+      :raise ->
+        raise """
+        #{String.trim(error)}
+
+        You can prevent this from raising by passing `on_error: :warn` to
+        `Phoenix.LiveViewTest.live/3` or `Phoenix.LiveViewTest.live_isolated/3`.
+        """
+
+      :warn ->
+        IO.warn(error, [])
+    end
+
+    {:noreply, state}
+  end
+
   def handle_call({:upload_progress, from, %Element{} = el, entry_ref, progress, cid}, _, state) do
     payload = maybe_put_cid(%{"entry_ref" => entry_ref, "progress" => progress}, cid)
     topic = proxy_topic(el)
@@ -653,7 +676,12 @@ defmodule Phoenix.LiveViewTest.ClientProxy do
   end
 
   defp patch_view(state, view, child_html, streams) do
-    case DOM.patch_id(view.id, state.html, child_html, streams) do
+    result =
+      DOM.patch_id(view.id, state.html, child_html, streams, fn msg ->
+        send(self(), {:test_error, msg})
+      end)
+
+    case result do
       {new_html, [_ | _] = will_destroy_cids} ->
         topic = view.topic
         state = %{state | html: new_html}

lib/phoenix_live_view/test/dom.ex

@@ -23,46 +23,68 @@ defmodule Phoenix.LiveViewTest.DOM do
           | {:pi | binary, binary | list, list}
           | {:doctype, binary, binary, binary}
         ]
-  def parse(html) do
+  def parse(html, error_reporter \\ nil) do
     {:ok, parsed} = Floki.parse_document(html)
-    detect_duplicate_ids(parsed)
+
+    if is_function(error_reporter, 1) do
+      detect_duplicate_ids(parsed, error_reporter)
+    end
 
     parsed
   end
 
-  defp detect_duplicate_ids(tree), do: detect_duplicate_ids(tree, MapSet.new())
+  defp detect_duplicate_ids(tree, error_reporter),
+    do: detect_duplicate_ids(tree, tree, MapSet.new(), error_reporter)
 
-  defp detect_duplicate_ids([node | rest], ids) do
-    ids = detect_duplicate_ids(node, ids)
-    detect_duplicate_ids(rest, ids)
+  defp detect_duplicate_ids(tree, [node | rest], ids, error_reporter) do
+    ids = detect_duplicate_ids(tree, node, ids, error_reporter)
+    detect_duplicate_ids(tree, rest, ids, error_reporter)
   end
 
   # ignore declarations
-  defp detect_duplicate_ids({:pi, _type, _attrs}, seen_ids), do: seen_ids
+  defp detect_duplicate_ids(_tree, {:pi, _type, _attrs}, seen_ids, _error_reporter), do: seen_ids
 
-  defp detect_duplicate_ids({_tag_name, _attrs, children} = node, ids) do
+  defp detect_duplicate_ids(tree, {_tag_name, _attrs, children} = node, ids, error_reporter) do
     case Floki.attribute(node, "id") do
       [id] ->
         if MapSet.member?(ids, id) do
-          IO.warn("""
+          error_reporter.("""
           Duplicate id found while testing LiveView: #{id}
 
-          #{inspect_html(node)}
+          #{inspect_html(all(tree, "[id=#{id}]"))}
 
           LiveView requires that all elements have unique ids, duplicate IDs will cause
           undefined behavior at runtime, as DOM patching will not be able to target the correct
           elements.
           """)
         end
 
-        detect_duplicate_ids(children, MapSet.put(ids, id))
+        detect_duplicate_ids(tree, children, MapSet.put(ids, id), error_reporter)
 
       _ ->
-        detect_duplicate_ids(children, ids)
+        detect_duplicate_ids(tree, children, ids, error_reporter)
     end
   end
 
-  defp detect_duplicate_ids(_non_tag, seen_ids), do: seen_ids
+  defp detect_duplicate_ids(_tree, _non_tag, seen_ids, _error_reporter), do: seen_ids
+
+  defp detect_duplicate_components(tree, cids, error_reporter) do
+    cids
+    |> Enum.frequencies()
+    |> Enum.each(fn {cid, count} ->
+      if count > 1 do
+        error_reporter.("""
+        Duplicate live component found while testing LiveView:
+
+        #{inspect_html(all(tree, "[#{@phx_component}=#{cid}]"))}
+
+        This most likely means that you are conditionally rendering the same
+        LiveComponent multiple times with the same ID in the same LiveView.
+        This is not supported and will lead to broken behavior on the client.
+        """)
+      end
+    end)
+  end
 
   def all(html_tree, selector), do: Floki.find(html_tree, selector)
 
@@ -336,7 +358,7 @@ defmodule Phoenix.LiveViewTest.DOM do
 
   # Patching
 
-  def patch_id(id, html_tree, inner_html, streams) do
+  def patch_id(id, html_tree, inner_html, streams, error_reporter \\ nil) do
     cids_before = component_ids(id, html_tree)
 
     phx_update_tree =
@@ -354,6 +376,12 @@ defmodule Phoenix.LiveViewTest.DOM do
       end)
 
     cids_after = component_ids(id, new_html)
+
+    if is_function(error_reporter, 1) do
+      detect_duplicate_ids(new_html, error_reporter)
+      detect_duplicate_components(new_html, cids_after, error_reporter)
+    end
+
     {new_html, cids_before -- cids_after}
   end
 

lib/phoenix_live_view/test/live_view_test.ex

@@ -203,6 +203,12 @@ defmodule Phoenix.LiveViewTest do
   `%Plug.Conn{}` is given, which will be converted to
   a LiveView immediately.
 
+  ## Options
+
+    * `:on_error` - Can be either `:raise` or `:warn` to control whether
+       detected errors like duplicate IDs or live components fail the test or just log
+       a warning. Defaults to `:raise`.
+
   ## Examples
 
       {:ok, view, html} = live(conn, "/path")
@@ -212,14 +218,14 @@ defmodule Phoenix.LiveViewTest do
       assert {:error, {:redirect, %{to: "/somewhere"}}} = live(conn, "/path")
 
   """
-  defmacro live(conn, path \\ nil) do
+  defmacro live(conn, path \\ nil, opts \\ []) do
     quote bind_quoted: binding(), generated: true do
       cond do
         is_binary(path) ->
-          Phoenix.LiveViewTest.__live__(get(conn, path), path)
+          Phoenix.LiveViewTest.__live__(get(conn, path), path, opts)
 
         is_nil(path) ->
-          Phoenix.LiveViewTest.__live__(conn)
+          Phoenix.LiveViewTest.__live__(conn, nil, opts)
 
         true ->
           raise RuntimeError, "path must be nil or a binary, got: #{inspect(path)}"
@@ -238,6 +244,9 @@ defmodule Phoenix.LiveViewTest do
   ## Options
 
     * `:session` - the session to be given to the LiveView
+    * `:on_error` - Can be either `:raise` or `:warn` to control whether
+       detected errors like duplicate IDs or live components fail the test or just log
+       a warning. Defaults to `:raise`.
 
   All other options are forwarded to the LiveView for rendering. Refer to
   `Phoenix.Component.live_render/3` for a list of supported render
@@ -272,16 +281,16 @@ defmodule Phoenix.LiveViewTest do
     |> Plug.Test.init_test_session(%{})
     |> Phoenix.LiveView.Router.fetch_live_flash([])
     |> Phoenix.LiveView.Controller.live_render(live_view, opts)
-    |> connect_from_static_token(nil)
+    |> connect_from_static_token(nil, opts)
   end
 
   @doc false
-  def __live__(%Plug.Conn{state: state, status: status} = conn) do
+  def __live__(%Plug.Conn{state: state, status: status} = conn, _path = nil, opts) do
     path = rebuild_path(conn)
 
     case {state, status} do
       {:sent, 200} ->
-        connect_from_static_token(conn, path)
+        connect_from_static_token(conn, path, opts)
 
       {:sent, 302} ->
         error_redirect_conn(conn)
@@ -311,13 +320,14 @@ defmodule Phoenix.LiveViewTest do
   end
 
   @doc false
-  def __live__(conn, path) do
-    connect_from_static_token(conn, path)
+  def __live__(conn, path, opts) do
+    connect_from_static_token(conn, path, opts)
   end
 
   defp connect_from_static_token(
          %Plug.Conn{status: 200, assigns: %{live_module: live_module}} = conn,
-         path
+         path,
+         opts
        ) do
     DOM.ensure_loaded!()
 
@@ -336,15 +346,16 @@ defmodule Phoenix.LiveViewTest do
       router: router,
       endpoint: Phoenix.Controller.endpoint_module(conn),
       session: maybe_get_session(conn),
-      url: Plug.Conn.request_url(conn)
+      url: Plug.Conn.request_url(conn),
+      on_error: opts[:on_error] || :raise
     })
   end
 
-  defp connect_from_static_token(%Plug.Conn{status: 200}, _path) do
+  defp connect_from_static_token(%Plug.Conn{status: 200}, _path, _opts) do
     {:error, :nosession}
   end
 
-  defp connect_from_static_token(%Plug.Conn{status: redir} = conn, _path)
+  defp connect_from_static_token(%Plug.Conn{status: redir} = conn, _path, _opts)
        when redir in [301, 302] do
     error_redirect_conn(conn)
   end
@@ -382,11 +393,12 @@ defmodule Phoenix.LiveViewTest do
         endpoint: opts.endpoint,
         session: opts.session,
         url: opts.url,
-        test_supervisor: fetch_test_supervisor!()
+        test_supervisor: fetch_test_supervisor!(),
+        on_error: opts.on_error
       })
 
     case ClientProxy.start_link(opts) do
-      {:ok, _} ->
+      {:ok, _pid} ->
         receive do
           {^ref, {:ok, view, html}} -> {:ok, view, html}
         end
@@ -1802,7 +1814,8 @@ defmodule Phoenix.LiveViewTest do
       endpoint: root.endpoint,
       router: root.router,
       session: session,
-      url: url
+      url: url,
+      on_error: root.on_error
     })
   end