Fix: make Locator.wait_for timeout safe

Previously, failures due to the timeout being reached were treated as
unknown, unexpected errors. In such cases, `Channel.recv/2` would `raise`.

With this change, we can now match on `%Error{type: "TimeoutError"}`,
which avoids the `raise` and improves the API.

Note that the change also includes a small "hack": an attempt to ensure
that the related `GenServer` timeout does not occur before the
Playwright server timeout.

Fixes [issue #55](#55).

lib/playwright/frame.ex

@@ -896,9 +896,9 @@ defmodule Playwright.Frame do
   @doc """
   Returns when element specified by selector satisfies state option.
 
-  Returns `nil` if waiting for a hidden or detached element.
+  FIXME: the following is NOT TRUE... Returns `nil` if waiting for a hidden or detached element.
   """
-  @spec wait_for_selector(t(), binary(), map()) :: ElementHandle.t() | nil
+  @spec wait_for_selector(t(), binary(), map()) :: ElementHandle.t() | {:error, Channel.Error.t()}
   def wait_for_selector(%Frame{session: session} = frame, selector, options \\ %{}) do
     Channel.post(session, {:guid, frame.guid}, :wait_for_selector, Map.merge(%{selector: selector}, options))
   end

lib/playwright/locator.ex

@@ -986,6 +986,7 @@ defmodule Playwright.Locator do
   Triggers a change and input event once all the provided options have been selected.
 
   ## Example
+
       alias Playwright.Locator
       locator = Locator.new(page, "select#colors")
 
@@ -1245,26 +1246,44 @@ defmodule Playwright.Locator do
   immediately. Otherwise, waits for up to `option: timeout` milliseconds until
   the condition is met.
 
-  ## Options
+  ## Returns
+
+    - `Locator.t()`
+
+  ## Arguments
 
   | key/name   | type   |              | description |
   | ---------- | ------ | ------------ | ----------- |
-  | `:state`   | option | state option | Defaults to `visible`. See "state options" below" |
+  | `:state`   | option | state option | Defaults to `visible`. See "Options for `:state`" below". |
   | `:timeout` | option | float        | Maximum time in milliseconds, defaults to 30 seconds, pass 0 to disable timeout. The default value can be changed by using the browser_context.set_default_timeout(timeout) or page.set_default_timeout(timeout) methods. |
 
-  ## State options
+  ## Options for `:state`
 
   | value      | description |
   | ---------- | ----------- |
-  | 'attached' | wait for element to be present in DOM. |
+  | 'attached' | wait for element to be present in DOM. (default) |
   | 'detached' | wait for element to not be present in DOM. |
   | 'visible'  | wait for element to have non-empty bounding box and no visibility:hidden. Note that element without any content or with display:none has an empty bounding box and is not considered visible. |
   | 'hidden'   | wait for element to be either detached from DOM, or have an empty bounding box or visibility:hidden. This is opposite to the 'visible' option. |
+
+  ## Example
+
+
+
   """
-  @spec wait_for(t(), options()) :: :ok
+
+  # const orderSent = page.locator('#order-sent');
+  # await orderSent.waitFor();
+
+  @spec wait_for(t(), options()) :: t() | {:error, Channel.Error.t()}
   def wait_for(%Locator{} = locator, options \\ %{}) do
-    Frame.wait_for_selector(locator.frame, locator.selector, options)
-    :ok
+    case Frame.wait_for_selector(locator.frame, locator.selector, options) do
+      %ElementHandle{} ->
+        locator
+
+      {:error, _} = error ->
+        error
+    end
   end
 
   # private

lib/playwright/page.ex

@@ -181,7 +181,7 @@ defmodule Playwright.Page do
 
   ## Arguments
 
-  | key/name          | type   |             | description |
+  | key/name            | type   |             | description |
   | ------------------- | ------ | ----------- | ----------- |
   | `run_before_unload` | option | `boolean()` | Whether to run the before unload page handlers. `(default: false)` |
 

lib/playwright/sdk/channel.ex

@@ -27,9 +27,9 @@ defmodule Playwright.SDK.Channel do
     Catalog.put(catalog, Map.merge(owner, data))
   end
 
-  def post(session, {:guid, guid}, message, params \\ %{}) when is_binary(guid) when is_pid(session) do
+  def post(session, {:guid, guid}, action, params \\ %{}) when is_binary(guid) when is_pid(session) do
     connection = Session.connection(session)
-    message = Message.new(guid, message, params)
+    message = Message.new(guid, action, params)
 
     # IO.inspect(message, label: "---> Channel.post/4")
 
@@ -54,44 +54,8 @@ defmodule Playwright.SDK.Channel do
 
   def recv(session, {from, message}) when is_map(message) do
     # IO.inspect(message, label: "<--- Channel.recv/2 B")
-
-    case Response.recv(session, message) do
-      # NOTE: The following errors are known and expected (from tests).
-      # TODO: Translated these to various, distinct `API.Errors.<ErrorType>`
-
-      # require Logger
-
-      %Error{message: "Target page, context or browser has been closed"} = error ->
-        # Logger.warning(message)
-        reply(error, from)
-
-      %Error{message: "net::ERR_INTERNET_DISCONNECTED at http://localhost:4002/assets/empty.html"} = error ->
-        # Logger.warning(message)
-        reply(error, from)
-
-      %Error{message: "Unknown permission: foo"} = error ->
-        # Logger.warning(message)
-        reply(error, from)
-
-      %Error{message: "Protocol error (Page.navigate): Cannot navigate to invalid URL"} = error ->
-        # Logger.warning(message)
-        reply(error, from)
-
-      %Error{message: "Timeout 500ms exceeded."} = error ->
-        # Logger.warning(message)
-        reply(error, from)
-
-      # NOTE: Any other errors are not yet exected, so we raise:
-
-      %Error{} = error ->
-        raise RuntimeError, message: "#{inspect(error)}"
-
-      %Event{} = event ->
-        reply(event, from)
-
-      %Response{} = response ->
-        reply(response, from)
-    end
+    Response.recv(session, message)
+    |> reply(from)
   end
 
   # or, "expect"?

lib/playwright/sdk/channel/connection.ex

@@ -96,14 +96,16 @@ defmodule Playwright.SDK.Channel.Connection do
     update =
       case response do
         %{id: message_id} ->
-          key = {:message, message_id}
-          {from, callbacks} = Map.pop!(callbacks, key)
+          source = {:message, message_id}
+          # must have a match
+          {from, callbacks} = Map.pop!(callbacks, source)
           Channel.recv(session, {from, response})
           %{state | callbacks: callbacks}
 
         %{guid: guid, method: method} ->
-          key = {as_atom(method), guid}
-          {from, callbacks} = Map.pop(callbacks, key)
+          source = {as_atom(method), guid}
+          # might have a match
+          {from, callbacks} = Map.pop(callbacks, source)
           Channel.recv(session, {from, response})
           %{state | callbacks: callbacks}
 

lib/playwright/sdk/channel/error.ex

@@ -1,17 +1,30 @@
 defmodule Playwright.SDK.Channel.Error do
   @moduledoc false
-  # `Error` represents an error message received from the Playwright server that is
-  # in response to a `Message` previously sent.
+  # `Error` represents an error message received from the Playwright server
+  # that is in response to a `Message` previously sent.
   alias Playwright.SDK.Channel
 
-  @enforce_keys [:message]
-  defstruct [:message]
+  @enforce_keys [:type, :message]
+  defstruct [:type, :message]
 
-  @type t() :: %__MODULE__{message: String.t()}
+  @type t() :: %__MODULE__{
+          type: String.t(),
+          message: String.t()
+        }
 
-  def new(%{error: error}, _catalog) do
+  def new(%{error: %{name: name, message: message} = _error}, _catalog) do
     %Channel.Error{
-      message: String.split(error.message, "\n") |> List.first()
+      type: name,
+      message: String.split(message, "\n") |> List.first()
+    }
+  end
+
+  # TODO: determine why we get here...
+  # DONE: see comment at error_handling.ex:9.
+  def new(%{error: %{message: message} = _error}, _catalog) do
+    %Channel.Error{
+      type: "NotImplementedError",
+      message: String.split(message, "\n") |> List.first()
     }
   end
 end

lib/playwright/sdk/channel_owner.ex

@@ -97,9 +97,9 @@ defmodule Playwright.SDK.ChannelOwner do
         {:ok, event.target}
       end
 
-      defp post!(owner, action, data) do
-        case Channel.post(owner.session, {:guid, owner.guid}, action, data) do
-          # simple "succes": send "self"
+      defp post!(owner, action, params) do
+        case Channel.post(owner.session, {:guid, owner.guid}, action, params) do
+          # simple "success": send "self"
           {:ok, %{id: _}} ->
             Channel.find(owner.session, {:guid, owner.guid})
         end

lib/playwright/sdk/helpers/error_handling.ex

@@ -6,7 +6,12 @@ defmodule Playwright.SDK.Helpers.ErrorHandling do
     timeout = options |> Map.get(:timeout, 30_000)
 
     try do
-      action.(timeout)
+      # NOTE the HACK!
+      # In most cases (as of 20240802), the timeout value provided here is also
+      # used as a timeout option passed to the Playwright server. As such, there
+      # is/was a race condition in which the `action` provided here would often
+      # time out before a response from the server indicated it's own timeout.
+      action.(timeout + 5)
     catch
       :exit, {:timeout, _} = _reason ->
         {:error, Error.new(%{error: %{message: "Timeout #{inspect(timeout)}ms exceeded."}}, nil)}

test/api/locator_test.exs

@@ -256,7 +256,7 @@ defmodule Playwright.LocatorTest do
     test "accepts `option: timeout` for expression evaluation", %{page: page} do
       locator = Page.locator(page, ".missing")
       options = %{timeout: 500}
-      errored = {:error, %Error{message: "Timeout 500ms exceeded."}}
+      errored = {:error, %Error{type: "TimeoutError", message: "Timeout 500ms exceeded."}}
 
       page
       |> Page.set_content("""
@@ -273,7 +273,7 @@ defmodule Playwright.LocatorTest do
     test "accepts `option: timeout` without a `param: arg`", %{page: page} do
       locator = Page.locator(page, ".missing")
       options = %{timeout: 500}
-      errored = {:error, %Error{message: "Timeout 500ms exceeded."}}
+      errored = {:error, %Error{type: "TimeoutError", message: "Timeout 500ms exceeded."}}
 
       page
       |> Page.set_content("""
@@ -723,19 +723,53 @@ defmodule Playwright.LocatorTest do
       [options: options]
     end
 
-    test "waiting for 'attached'", %{options: options, page: page} do
-      frame = Page.main_frame(page)
+    test "on success (i.e., a match is found in time) returns the `Locator` instance", %{page: page} do
+      locator = Locator.new(page, "div > span")
 
-      locator = Locator.new(frame, "a#link")
+      setup =
+        Task.async(fn ->
+          Page.set_content(page, "<div><span>target</span></div>")
+        end)
 
-      task =
+      check =
         Task.async(fn ->
-          assert :ok = Locator.wait_for(locator, Map.put(options, :state, "attached"))
+          Locator.wait_for(locator, %{timeout: 100})
         end)
 
-      page |> Page.set_content("<a id='link' target=_blank rel=noopener href='/one-style.html'>yo</a>")
+      assert [:ok, %Locator{}] = Task.await_many([setup, check])
+    end
+
+    test "on failure (i.e., the timeout is reached) returns an `{:error, error}` tuple", %{page: page} do
+      locator = Locator.new(page, "div > span")
+
+      setup =
+        Task.async(fn ->
+          :timer.sleep(200)
+          Page.set_content(page, "<div><span>target</span></div>")
+        end)
+
+      check =
+        Task.async(fn ->
+          Locator.wait_for(locator, %{timeout: 100})
+        end)
+
+      assert [:ok, {:error, %Error{type: "TimeoutError"}}] = Task.await_many([setup, check])
+    end
+
+    test "waiting for 'attached'", %{options: options, page: page} do
+      locator = Locator.new(page, "a#link")
+
+      setup =
+        Task.async(fn ->
+          Page.set_content(page, "<a id='link' target=_blank rel=noopener href='/one-style.html'>link</a>")
+        end)
+
+      check =
+        Task.async(fn ->
+          Locator.wait_for(locator, Map.put(options, :state, "attached"))
+        end)
 
-      Task.await(task)
+      assert [:ok, %Locator{}] = Task.await_many([setup, check])
     end
   end
 end

test/api/navigation_test.exs

@@ -53,10 +53,11 @@ defmodule Playwright.NavigationTest do
 
     test "fails when navigating to bad URL", %{page: page} do
       error = %Error{
+        type: "Error",
         message: "Protocol error (Page.navigate): Cannot navigate to invalid URL"
       }
 
-      assert Page.goto(page, "asdfasdf") == {:error, error}
+      assert {:error, ^error} = Page.goto(page, "asdfasdf")
     end
 
     test "works when navigating to valid URL", %{assets: assets, page: page} do