From 92f82e050a2f34422ada92f14c4fe99142a2593a Mon Sep 17 00:00:00 2001 From: GenericJam Date: Sun, 6 Sep 2026 13:36:13 -0600 Subject: [PATCH] Make the test suite tell the truth (MOB-154, MOB-119, MOB-123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 1-in-20 flake corrupted a mutation-testing verdict and, a day later, sent a bisect down the wrong path when it surfaced in the same run as a real failure. The cost of a noisy suite is not the red build, it is the hours spent trusting it. Three mechanisms, all fixed by construction rather than by retry: Mob.ComponentRegistry is a globally-named singleton owning a named ETS table, and two async modules each started it with start_supervised/1. Whichever test won owned it, and ExUnit tore the table down while the other module was still reading from it. It now starts in test_helper.exs, owned by the run, so no test can take it down mid-flight. Nineteen `if Process.alive?(pid), do: GenServer.stop(pid)` sites were check-then-act across a process boundary; thirteen further modules had each privately written the same correct workaround, byte for byte, which is a fair signal it belonged somewhere shared. Mob.Test.ProcessHelpers gains stop_pid/2, await_exit/2 and eventually/2. All 35 Process.sleep calls were classified rather than swept. Twelve are Process.sleep(:infinity) — a parked stub, not a wait. Of the 23 finite ones, 15 went: five had nothing to wait for at all (a GenServer.call from the process that sent the messages is already an ordering barrier), and ten became the actual barrier — a ready-message, Logger.flush/0, a monitor, or a bounded poll. The 8 that remain are named individually in the decision record, including the one that is still a genuine bet. Adds `mix mob.flake`, which runs the suite repeatedly and reports which tests are non-deterministic. Its green output states outright that 20 green runs miss a 1-in-17 flake about 30% of the time, because "I ran it 20 times" is the reasoning that let this survive. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 37 +++++ ...-06-tests-wait-for-events-not-durations.md | 125 ++++++++++++++ lib/mix/tasks/mob.flake.ex | 155 ++++++++++++++++++ test/mob/component_server_test.exs | 10 +- test/mob/component_test.exs | 7 +- test/mob/device_test.exs | 47 +++--- test/mob/event/integration_test.exs | 7 +- test/mob/event/target_test.exs | 2 +- test/mob/event/trace_test.exs | 14 +- test/mob/listener_test.exs | 17 +- test/mob/native_component_examples_test.exs | 14 +- test/mob/native_logger_test.exs | 7 +- test/mob/nav/multi_stack_test.exs | 13 +- test/mob/nav/registry_test.exs | 18 +- test/mob/nav/reset_all_test.exs | 12 +- test/mob/nav/reset_transition_test.exs | 16 +- test/mob/nav/screen_nav_test.exs | 16 +- test/mob/nav/tab_transition_test.exs | 20 +-- test/mob/process_helpers_test.exs | 86 ++++++++++ test/mob/registry_test.exs | 2 +- test/mob/render_stats_test.exs | 19 +-- test/mob/router_hot_path_test.exs | 19 +-- test/mob/screen/isolation_test.exs | 13 +- test/mob/screen/migration_test.exs | 16 +- test/mob/screen/restart_test.exs | 10 +- test/mob/screen_repaint_test.exs | 12 +- test/mob/screen_sender_wiring_test.exs | 13 +- test/mob/sender_test.exs | 6 +- test/mob/state_test.exs | 2 +- test/mob/theme_host_test.exs | 4 +- test/support/process_helpers.ex | 139 +++++++++++++++- test/test_helper.exs | 25 +++ 32 files changed, 693 insertions(+), 210 deletions(-) create mode 100644 decisions/2026-09-06-tests-wait-for-events-not-durations.md create mode 100644 lib/mix/tasks/mob.flake.ex create mode 100644 test/mob/process_helpers_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 876ee9dc..2064af80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,43 @@ Full module documentation: [hexdocs.pm/mob](https://hexdocs.pm/mob). ## [Unreleased] +### Added +- **`mix mob.flake`** — run the suite repeatedly and report which tests are not + deterministic. `--runs N`, `--until-failure`, `--keep-going`, `--seed`, and a + path to narrow the target. A flake does not announce itself; it fails once on + someone else's branch and the natural response is to re-run and move on. This + makes looking cheap and deliberate. + +### Fixed +- **Test-suite races that made every automated verdict unreliable** (MOB-154, + MOB-119, MOB-123). A 1-in-20 flake corrupted a mutation-testing result and, + a day later, sent a bisect down the wrong path when it appeared in the same + run as a real failure. + + `Mob.ComponentRegistry` is a globally-named singleton owning a named ETS + table, and two `async: true` modules each started it with + `start_supervised/1` — so whichever test won the race owned it, and ExUnit + tore it down while the other module was still using it. It now starts in + `test_helper.exs`, owned by the run. + + Nineteen `on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end)` + sites across eleven modules were check-then-act across a process boundary; + thirteen further modules had each written the same correct workaround + privately, byte for byte. All now use `Mob.Test.ProcessHelpers`, which gains + `stop_pid/2`, `await_exit/2` and `eventually/2`. + + All 35 `Process.sleep` calls were classified rather than swept. Twelve are + `Process.sleep(:infinity)` — a parked stub, not a wait. Of the 23 finite + ones, 8 remain: three are the subject under test in `render_stats_test.exs`, + three are the backoff inside a poll loop that has its own deadline, one is a + `@doc` example, and one is a genuine bet that is recorded rather than + disguised. The other 15 either had nothing to wait for (a `GenServer.call` + from the same process is already an ordering barrier) or were replaced with + the actual barrier — a ready-message, `Logger.flush/0`, a monitor, or a + bounded poll. See + `decisions/2026-09-06-tests-wait-for-events-not-durations.md`. + + ### Changed - **Input NIFs now run on a dirty IO scheduler.** `tap`, `tap_xy`, `long_press_xy`, `swipe_xy`, `type_text`, `delete_backward` and `clear_text` diff --git a/decisions/2026-09-06-tests-wait-for-events-not-durations.md b/decisions/2026-09-06-tests-wait-for-events-not-durations.md new file mode 100644 index 00000000..0df8046a --- /dev/null +++ b/decisions/2026-09-06-tests-wait-for-events-not-durations.md @@ -0,0 +1,125 @@ +# Tests wait for events, not durations + +Date: 2026-09-06 +Status: accepted +Ticket: MOB-154 (with MOB-119, MOB-123) + +## Context + +A 1-in-20 flake corrupted a mutation-testing verdict, and a day later sent a +bisect down the wrong path: a real failure and a flake appeared in the same +run, and the flake was investigated first. That is the actual cost of a noisy +suite — not the red build, but the hours spent trusting it. + +Three mechanisms were behind it. + +**A globally-named singleton owned by whichever test started it.** +`Mob.ComponentRegistry` is named and owns a named ETS table. Two `async: true` +modules each called `start_supervised({Mob.ComponentRegistry, []})` and +tolerated `{:error, {:already_started, _}}`. Whichever test won the race owned +it, and ExUnit tore the process — and its table — down when that test ended, +while a concurrent test in the other module was still using it. The loser died +on `:ets.lookup` against a table that no longer existed. + +**Check-then-act across a process boundary.** `on_exit(fn -> if +Process.alive?(pid), do: GenServer.stop(pid) end)` appeared 19 times across 11 +modules. +Since MOB-112 the screen owner is linked to the test process, which ExUnit +exits with `:shutdown` at test end — so the owner is dying concurrently with +the callback trying to stop it. Thirteen more modules had each independently +written a correct private `stop_safely/1`, byte for byte identically, which is +a fair signal it belonged somewhere shared. + +**Fixed durations standing in for synchronisation.** `Process.exit(pid, :kill)` +followed by `Process.sleep(10)` followed by an assertion that requires the +process to be gone. + +## Decision + +**A test waits for the event it depends on, never for a duration it hopes is +long enough.** `Process.monitor` plus a `:DOWN`, or `assert_receive`. A monitor +is not a tighter bet than a sleep — the `:DOWN` cannot arrive before the +process is gone, so there is nothing left to race. + +Shared state that outlives a single test is owned by the **run**, not by +whichever test got there first. `Mob.ComponentRegistry` now starts in +`test_helper.exs`, so both setups take the `:already_started` branch, nobody +owns it, and nobody can tear it down mid-flight. Sharing is safe because every +entry is keyed by a per-test `screen_pid`. + +`Mob.Test.ProcessHelpers` is where these live: `stop_if_running/2` (named), +`stop_pid/2` (pid), `await_exit/2` (block until actually gone, raising rather +than continuing on timeout — a helper that returns quietly on timeout leaves +the caller asserting against a live process, which is the situation being +avoided). + +### Not every sleep is a bug + +All 35 `Process.sleep` calls were classified rather than swept. Twelve were +`Process.sleep(:infinity)` — a stub process parked until something kills it, +not a wait at all. That left 23 finite sleeps, now 8: + +**Removed because there was never anything to wait for (5).** A `GenServer.call` +from the same process that sent the earlier messages is already a barrier: +Erlang orders messages pairwise between two processes, so every `send` is ahead +of the `call` in the mailbox and has been handled before the reply comes back +(`event/integration_test.exs` ×3, `nav/screen_nav_test.exs`). And +`Trace.broadcast/3` folds over the table *in the calling process*, so its +cleanup is done by the time `dispatch/4` returns `:ok` (`event/trace_test.exs`). +These sleeps were guarding against nothing. + +**Replaced with the real barrier (8).** A ready-message where the test was +waiting on another process to reach a known point (`trace_test.exs`, +`device_test.exs`); `Logger.flush/0` where it was waiting on handlers to drain +(`native_logger_test.exs` ×2, `theme_host_test.exs` ×2); a monitor where it was +waiting for an exit. + +**Replaced with a bounded poll (3).** `device_test.exs` (×2) and +`native_component_examples_test.exs` wait for a GenServer to +process a `:DOWN` sent by a *monitor*, not by the test. Pairwise ordering does +not help here — the test never sent that message, so a `call` orders nothing +against it. `ProcessHelpers.eventually/2` polls with a deadline: it returns as +soon as the state is right rather than always paying the worst case, and it +fails with "condition still false after Nms" instead of letting the next +assertion report something confusing. + +**Kept, deliberately (8).** Three in `render_stats_test.exs` are the subject +under test — it measures elapsed time, so time must actually elapse. Three are +the backoff *inside* a poll loop that has its own deadline (`eventually/2` +itself, `migration_test.exs`, `reset_transition_test.exs`). One is a `@doc` +example, quoting the bad pattern in order to name it. One remains a genuine +bet: `router_hot_path_test.exs:206` waits for a message the *screen* sent to +the router, and the test is neither party, so it has no ordering guarantee to +lean on. It is left as a sleep and recorded here rather than disguised. + +One honest caveat about the `Logger.flush/0` swaps: with the barrier deleted +outright those tests still passed 8 times out of 8 on this machine, so the wait +is not demonstrably load-bearing here. `flush/0` is kept because it is the +correct primitive and costs nothing when there is nothing pending, whereas the +sleep it replaced was a guess that cost 300ms per run. That is a reason, not a +measurement, and is stated as such. + +## Consequences + +- `mix mob.flake` runs the suite repeatedly and reports which tests are + non-deterministic. Its green output says explicitly that a 1-in-17 flake + survives 20 green runs about 30% of the time, because "I ran it 20 times" is + the reasoning that let this persist. +- **The registry race is fixed by construction, not by demonstration.** It did + not reproduce here in 25 full-suite runs or 40 concentrated ones, so there is + no before-and-after to show. The mechanism is provable by reading the code + and it was observed once on this machine; the fix removes the ownership that + makes it possible. That is weaker evidence than a reproduction and is stated + as such. + +- **Tightening `stop_pid/2` broke three tests, and only under a full run.** + Making the timeout raise meant replacing a blanket `:exit, _ -> :ok` with + enumerated clauses. The enumeration was wrong: a linked owner dying while + ExUnit tears the test down exits with `{{:shutdown, {:sys, :terminate, _}}, + {GenServer, :stop, _}}`, which matched none of them. It passed every file run + on its own and failed 3 tests in one full run and 4 in the next. The fix is + to invert the logic — special-case only the timeout, treat every other exit + as "already gone" — because "did it stop" has one interesting answer and an + open-ended set of uninteresting ones. Enumerating the uninteresting set is a + bet on having seen every shutdown shape, which is the same class of mistake + as betting on a duration. diff --git a/lib/mix/tasks/mob.flake.ex b/lib/mix/tasks/mob.flake.ex new file mode 100644 index 00000000..66199bcd --- /dev/null +++ b/lib/mix/tasks/mob.flake.ex @@ -0,0 +1,155 @@ +defmodule Mix.Tasks.Mob.Flake do + @shortdoc "Run the suite repeatedly to surface flaky tests" + + @moduledoc """ + Run the test suite until it fails, or a set number of times, and report + which tests were not deterministic. + + A flake does not announce itself. It fails once, on someone else's branch, + and the natural response is to re-run and move on — which is how a 1-in-17 + failure survived long enough to corrupt a mutation-testing verdict and send + a bisect down the wrong path. This makes looking cheap and deliberate. + + mix mob.flake # 20 runs, stop at the first failure + mix mob.flake --runs 50 # more attempts + mix mob.flake --until-failure # keep going until one fails + mix mob.flake --keep-going # run them all, report every failure + mix mob.flake test/mob/nav # narrow the target + mix mob.flake --seed 0 # fix the seed, so ordering is constant + + ## Reading the result + + A test that fails under `--seed 0` on every run is not flaky, it is broken. + This looks for the other kind: tests that pass and fail with everything else + held still. Those are almost always a race — a `Process.sleep` standing in + for synchronisation, a check-then-act across a process boundary, or shared + global state (a named process, an ETS table) whose lifetime is tied to + whichever test happened to start it. + + Narrowing helps more than volume. If a failure names one module, run that + module 200 times rather than the suite 20 more times. + """ + + use Mix.Task + + @switches [runs: :integer, until_failure: :boolean, keep_going: :boolean, seed: :integer] + + @impl Mix.Task + def run(argv) do + {opts, paths} = OptionParser.parse!(argv, strict: @switches) + + runs = Keyword.get(opts, :runs, 20) + until_failure? = Keyword.get(opts, :until_failure, false) + keep_going? = Keyword.get(opts, :keep_going, false) + limit = if until_failure?, do: :infinity, else: runs + + Mix.shell().info([ + :cyan, + "Running the suite #{describe(limit)}, ", + if(keep_going?, do: "reporting every failure.", else: "stopping at the first failure."), + :reset + ]) + + loop(1, limit, keep_going?, opts, paths, []) + end + + defp describe(:infinity), do: "until it fails" + defp describe(n), do: "#{n} time(s)" + + defp loop(attempt, limit, keep_going?, opts, paths, failures) do + if limit != :infinity and attempt > limit do + report(attempt - 1, failures) + else + case run_once(opts, paths) do + :ok -> + IO.write(".") + loop(attempt + 1, limit, keep_going?, opts, paths, failures) + + {:failed, output} -> + IO.write("F") + failures = [{attempt, output} | failures] + + if keep_going? and limit != :infinity do + loop(attempt + 1, limit, keep_going?, opts, paths, failures) + else + report(attempt, failures) + end + end + end + end + + defp run_once(opts, paths) do + args = + ["test"] ++ + paths ++ + case Keyword.fetch(opts, :seed) do + {:ok, seed} -> ["--seed", to_string(seed)] + :error -> [] + end + + # Fresh OS process per run, deliberately: an in-process re-run would share + # the ETS tables and named processes that cause most of these failures, so + # it could not reproduce them. + {output, status} = System.cmd("mix", args, stderr_to_stdout: true, env: [{"MIX_ENV", "test"}]) + + if status == 0, do: :ok, else: {:failed, output} + end + + defp report(runs, []) do + Mix.shell().info([:green, "\n\n#{runs} run(s), no failures.", :reset]) + + Mix.shell().info([ + "\nThat is evidence, not proof. A 1-in-17 flake survives 20 green runs ", + "about 30% of the time — narrow the target and raise --runs before ", + "concluding a suspected flake is gone." + ]) + end + + defp report(runs, failures) do + ordered = Enum.reverse(failures) + + Mix.shell().error("\n\n#{length(ordered)} failure(s) in #{runs} run(s):\n") + + for {attempt, output} <- ordered do + Mix.shell().error("── run #{attempt} " <> String.duplicate("─", 50)) + Mix.shell().error(failing_tests(output)) + end + + Mix.shell().info([ + "\nRe-run the named module alone, many times, before changing anything. ", + "A failure that only appears in the full suite is usually shared global ", + "state, not the test's own logic." + ]) + + exit({:shutdown, 1}) + end + + # The whole log is noise; what matters is which tests failed and why. + # + # Split on failure headers rather than sliding a window over the lines. A + # window silently loses any failure near the end of the output — and because + # an earlier match makes the fallback unreachable, it loses it without saying + # so. For a tool that exists to surface rare failures, that is the worst + # possible bug, and it was in the first version of this function. + @header ~r/^\s+\d+\) (test|doctest|property) / + + @doc false + @spec failing_tests(String.t()) :: [String.t()] + def failing_tests(output) do + output + |> String.split("\n") + |> Enum.reduce([], fn line, acc -> + cond do + Regex.match?(@header, line) -> [[line] | acc] + acc == [] -> acc + true -> [[line | hd(acc)] | tl(acc)] + end + end) + |> Enum.reverse() + |> Enum.map(fn block -> block |> Enum.reverse() |> Enum.take(8) |> Enum.join("\n") end) + |> case do + [] -> output |> String.split("\n") |> Enum.take(-15) |> Enum.join("\n") + blocks -> Enum.join(blocks, "\n\n") + end + end +end diff --git a/test/mob/component_server_test.exs b/test/mob/component_server_test.exs index 8316651d..c49feb70 100644 --- a/test/mob/component_server_test.exs +++ b/test/mob/component_server_test.exs @@ -75,10 +75,7 @@ defmodule Mob.ComponentServerTest do # async test file (component_test.exs) may have already started it — # start_supervised! would raise on {:already_started, _}, so tolerate # that instead of racing to be first. - case start_supervised({Mob.ComponentRegistry, []}) do - {:ok, _pid} -> :ok - {:error, {:already_started, _pid}} -> :ok - end + Mob.Test.ProcessHelpers.ensure_component_registry() {:ok, pid} = Mob.ComponentServer.start( @@ -253,10 +250,7 @@ defmodule Mob.ComponentServerTest do end setup do - case start_supervised({Mob.ComponentRegistry, []}) do - {:ok, _pid} -> :ok - {:error, {:already_started, _pid}} -> :ok - end + Mob.Test.ProcessHelpers.ensure_component_registry() # Unlinked, fixed-name Agent (mirrors test/mob/renderer_test.exs's # MockNIF) — reset rather than restarted, since a prior test in this diff --git a/test/mob/component_test.exs b/test/mob/component_test.exs index 78d23d73..83fe6ba7 100644 --- a/test/mob/component_test.exs +++ b/test/mob/component_test.exs @@ -130,11 +130,8 @@ defmodule Mob.ComponentTest do # racing to be first (MOB-98: this is the shared-name race that fix # already covers on that file's side; this file needed the same # tolerance). - reg = - case start_supervised({Mob.ComponentRegistry, []}) do - {:ok, pid} -> pid - {:error, {:already_started, pid}} -> pid - end + :ok = Mob.Test.ProcessHelpers.ensure_component_registry() + reg = Process.whereis(Mob.ComponentRegistry) {:ok, reg: reg} end diff --git a/test/mob/device_test.exs b/test/mob/device_test.exs index 5d290acc..f574441e 100644 --- a/test/mob/device_test.exs +++ b/test/mob/device_test.exs @@ -17,7 +17,7 @@ defmodule Mob.DeviceTest do GenServer.start_link(Device, [], name: :"device_#{System.unique_integer([:positive])}") on_exit(fn -> - if Process.alive?(pid), do: GenServer.stop(pid) + Mob.Test.ProcessHelpers.stop_pid(pid) end) {:ok, dispatcher: pid} @@ -197,22 +197,22 @@ defmodule Mob.DeviceTest do end test "multiple subscribers all receive matching events", %{dispatcher: d} do - task1 = - Task.async(fn -> - :ok = GenServer.call(d, {:subscribe, self(), [:app]}) - assert_receive {:mob_device, :did_become_active}, 200 - :got_it - end) + parent = self() - task2 = - Task.async(fn -> - :ok = GenServer.call(d, {:subscribe, self(), [:app]}) - assert_receive {:mob_device, :did_become_active}, 200 - :got_it - end) + subscriber = fn -> + :ok = GenServer.call(d, {:subscribe, self(), [:app]}) + send(parent, :subscribed) + assert_receive {:mob_device, :did_become_active}, 200 + :got_it + end + + task1 = Task.async(subscriber) + task2 = Task.async(subscriber) - # Give both tasks time to subscribe. - Process.sleep(20) + # Both subscriptions must be registered before the event is sent. The + # tasks say when that is true; sleeping only guessed at it. + assert_receive :subscribed + assert_receive :subscribed send(d, {:mob_device, :did_become_active}) assert Task.await(task1) == :got_it @@ -234,11 +234,13 @@ defmodule Mob.DeviceTest do end) assert Task.await(task) == :done - # Wait for the :DOWN to be processed. - Process.sleep(50) - subs = GenServer.call(d, :__test_subscribers__) - refute Map.has_key?(subs, task.pid) + # The :DOWN comes from the monitor, not from this process, so a call here + # orders nothing. Poll until the server has actually pruned the entry. + Mob.Test.ProcessHelpers.eventually(fn -> + subs = GenServer.call(d, :__test_subscribers__) + not Map.has_key?(subs, task.pid) + end) end test "double-subscribe replaces categories rather than duplicating", %{dispatcher: d} do @@ -292,10 +294,11 @@ defmodule Mob.DeviceTest do end) assert Task.await(task) == :done - Process.sleep(50) - subs = GenServer.call(Mob.Device.IOS, :__test_subscribers__) - refute Map.has_key?(subs, task.pid) + Mob.Test.ProcessHelpers.eventually(fn -> + subs = GenServer.call(Mob.Device.IOS, :__test_subscribers__) + not Map.has_key?(subs, task.pid) + end) end end diff --git a/test/mob/event/integration_test.exs b/test/mob/event/integration_test.exs index c4cfd073..0385287b 100644 --- a/test/mob/event/integration_test.exs +++ b/test/mob/event/integration_test.exs @@ -123,7 +123,10 @@ defmodule Mob.Event.IntegrationTest do send(screen, {:tap, {:list, :items, :select, 0}}) send(screen, {:tap, :stop}) - Process.sleep(20) + # No sleep: `get_log/1` is a GenServer.call from the same process that + # sent the events above. Erlang guarantees message order between a pair + # of processes, so every send is already in the mailbox ahead of the + # call, and the screen handles them in order before replying. log = TestScreen.get_log(screen) assert log == [ @@ -141,7 +144,6 @@ defmodule Mob.Event.IntegrationTest do send(screen, {:not_an_event, :ignored}) send(screen, {:tap, :b}) - Process.sleep(20) log = TestScreen.get_log(screen) # Only the two recognised taps: @@ -204,7 +206,6 @@ defmodule Mob.Event.IntegrationTest do ) end - Process.sleep(20) log = TestScreen.get_log(screen) seqs = diff --git a/test/mob/event/target_test.exs b/test/mob/event/target_test.exs index 6cb86e03..94ddf92d 100644 --- a/test/mob/event/target_test.exs +++ b/test/mob/event/target_test.exs @@ -66,7 +66,7 @@ defmodule Mob.Event.TargetTest do test "dead pid errors" do pid = spawn(fn -> :ok end) - Process.sleep(10) + Mob.Test.ProcessHelpers.await_exit(pid) refute Process.alive?(pid) s = scope() assert Target.resolve(pid, s) == {:error, :dead_pid} diff --git a/test/mob/event/trace_test.exs b/test/mob/event/trace_test.exs index ae92c81d..c2b3eee5 100644 --- a/test/mob/event/trace_test.exs +++ b/test/mob/event/trace_test.exs @@ -26,15 +26,21 @@ defmodule Mob.Event.TraceTest do end test "multiple subscribers all see the event" do + parent = self() + task = Task.async(fn -> Trace.subscribe() + # The dispatch below must not run until this subscription is in the + # table. Sleeping guessed at how long that takes across processes; + # the ready-message is the actual ordering constraint. + send(parent, :subscribed) assert_receive {:mob_trace, _, :tap, nil}, 200 :got_it end) Trace.subscribe() - Process.sleep(20) + assert_receive :subscribed :ok = Event.dispatch(self(), addr(), :tap, nil) @@ -95,12 +101,14 @@ defmodule Mob.Event.TraceTest do # Exit immediately end) - Process.sleep(10) + Mob.Test.ProcessHelpers.await_exit(pid) refute Process.alive?(pid) # Now dispatch — broadcast should silently skip the dead pid and clean up. + # `broadcast/3` folds over the table in the *calling* process, so the + # delete has already happened by the time dispatch returns :ok. There is + # nothing to wait for. :ok = Event.dispatch(self(), addr(), :tap, nil) - Process.sleep(10) # Verify the dead pid was removed. assert :ets.lookup(:mob_event_trace, pid) == [] diff --git a/test/mob/listener_test.exs b/test/mob/listener_test.exs index 4b757f44..3c70094e 100644 --- a/test/mob/listener_test.exs +++ b/test/mob/listener_test.exs @@ -13,19 +13,10 @@ defmodule Mob.ListenerTest do defp start_listener do {:ok, pid} = Listener.start_link([]) - on_exit(fn -> stop_safely(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) pid end - # `if Process.alive?, do: GenServer.stop` races: the process can exit between - # the check and the stop, and the :noproc exit then fails the test from inside - # the on_exit runner. - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - describe "handler/1 without a listener" do test "returns a tagged target unchanged" do target = {self(), :save} @@ -284,7 +275,9 @@ defmodule Mob.ListenerTest do assert :ok = Listener.ensure_started() # Registered before the assertions below, so a failure cannot leak a # listener into unrelated test files. - on_exit(fn -> if pid = Process.whereis(Listener), do: stop_safely(pid) end) + on_exit(fn -> + if pid = Process.whereis(Listener), do: Mob.Test.ProcessHelpers.stop_pid(pid) + end) assert Listener.running?() pid = Process.whereis(Listener) @@ -304,7 +297,7 @@ defmodule Mob.ListenerTest do assert_receive :started listener = Process.whereis(Listener) - on_exit(fn -> stop_safely(listener) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(listener) end) ref = Process.monitor(caller) send(caller, :die) diff --git a/test/mob/native_component_examples_test.exs b/test/mob/native_component_examples_test.exs index d31c6dbd..04323341 100644 --- a/test/mob/native_component_examples_test.exs +++ b/test/mob/native_component_examples_test.exs @@ -748,7 +748,19 @@ defmodule Mob.NativeComponentExamplesTest do ]) :rpc.call(@node, Process, :exit, [pid, :shutdown]) - Process.sleep(50) + + # The registry prunes on a :DOWN from its own monitor, which this test + # never sent — so nothing here orders against it. Poll until it lands. + Mob.Test.ProcessHelpers.eventually(fn -> + match?( + {:error, :not_found}, + :rpc.call(@node, Mob.ComponentRegistry, :lookup, [ + screen_pid, + :od_pdf_term, + PDFComponent + ]) + ) + end) assert {:error, :not_found} = :rpc.call(@node, Mob.ComponentRegistry, :lookup, [ diff --git a/test/mob/native_logger_test.exs b/test/mob/native_logger_test.exs index 7aada2c4..71e0ed77 100644 --- a/test/mob/native_logger_test.exs +++ b/test/mob/native_logger_test.exs @@ -107,8 +107,9 @@ defmodule Mob.NativeLoggerTest do test "Logger.info/1 reaches the handler end-to-end", %{nif_pid: pid} do Logger.info("end-to-end test") - # Give the async logger handler a moment to flush - Process.sleep(50) + # `Logger.flush/0` blocks until the handlers have drained — the actual + # barrier the sleep was approximating. + Logger.flush() calls = MockNIF.calls(pid) assert Enum.any?(calls, fn {level, msg} -> @@ -118,7 +119,7 @@ defmodule Mob.NativeLoggerTest do test "Logger.error/1 reaches the handler with :error level", %{nif_pid: pid} do Logger.error("something broke") - Process.sleep(50) + Logger.flush() calls = MockNIF.calls(pid) assert Enum.any?(calls, fn {level, msg} -> diff --git a/test/mob/nav/multi_stack_test.exs b/test/mob/nav/multi_stack_test.exs index 7216aaa0..c57016c8 100644 --- a/test/mob/nav/multi_stack_test.exs +++ b/test/mob/nav/multi_stack_test.exs @@ -129,23 +129,14 @@ defmodule Mob.Nav.MultiStackTest do Mob.Test.ProcessHelpers.stop_if_running(Mob.Nav.Registry) {:ok, pid} = Mob.Nav.Registry.start_link(TabApp) - on_exit(fn -> stop_safely(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) {:ok, screen} = Mob.Screen.start_link(HomeScreen, %{}) - on_exit(fn -> stop_safely(screen) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(screen) end) %{screen: screen} end - # `if Process.alive?, do: GenServer.stop` races: the process can exit between - # the check and the stop, and the :noproc exit then fails the test from inside - # the on_exit runner. Screens and their owner die with the test process. - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - describe "switching stacks" do test "first switch mounts the target stack's declared root", %{screen: screen} do Mob.Screen.dispatch(screen, "to_settings", %{}) diff --git a/test/mob/nav/registry_test.exs b/test/mob/nav/registry_test.exs index 2a15075e..6b29492f 100644 --- a/test/mob/nav/registry_test.exs +++ b/test/mob/nav/registry_test.exs @@ -50,26 +50,26 @@ defmodule Mob.Nav.RegistryTest do test "starts the registry and seeds it from the app module" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) assert is_pid(pid) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) end end describe "lookup/1" do test "finds a registered screen" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) assert {:ok, HomeScreen} = Mob.Nav.Registry.lookup(:home) end test "returns not_found for unknown atom" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) assert {:error, :not_found} = Mob.Nav.Registry.lookup(:nonexistent) end test "seeds both platforms from tab_bar app" do {:ok, pid} = Mob.Nav.Registry.start_link(TabApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) assert {:ok, HomeScreen} = Mob.Nav.Registry.lookup(:home) assert {:ok, ProfileScreen} = Mob.Nav.Registry.lookup(:profile) assert {:ok, SettingsScreen} = Mob.Nav.Registry.lookup(:settings) @@ -79,14 +79,14 @@ defmodule Mob.Nav.RegistryTest do describe "register/2" do test "registers a name→module mapping at runtime" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) :ok = Mob.Nav.Registry.register(:detail, ProfileScreen) assert {:ok, ProfileScreen} = Mob.Nav.Registry.lookup(:detail) end test "overwrites an existing mapping" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) :ok = Mob.Nav.Registry.register(:home, ProfileScreen) assert {:ok, ProfileScreen} = Mob.Nav.Registry.lookup(:home) end @@ -95,7 +95,7 @@ defmodule Mob.Nav.RegistryTest do describe "register/3 + lookup_route/1 (route-bound params)" do test "params registered with the route come back via lookup_route" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) :ok = Mob.Nav.Registry.register(:"/ash/post/list", ProfileScreen, %{resource: Post}) @@ -108,7 +108,7 @@ defmodule Mob.Nav.RegistryTest do test "register/2 entries resolve with empty route params" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) :ok = Mob.Nav.Registry.register(:detail, ProfileScreen) assert Mob.Nav.Registry.lookup_route(:detail) == {:ok, ProfileScreen, %{}} @@ -116,7 +116,7 @@ defmodule Mob.Nav.RegistryTest do test "app-navigation seeded routes resolve with empty route params" do {:ok, pid} = Mob.Nav.Registry.start_link(SimpleApp) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) assert {:ok, _module, %{}} = Mob.Nav.Registry.lookup_route(:home) end end diff --git a/test/mob/nav/reset_all_test.exs b/test/mob/nav/reset_all_test.exs index be2d8795..1e7a755f 100644 --- a/test/mob/nav/reset_all_test.exs +++ b/test/mob/nav/reset_all_test.exs @@ -155,14 +155,14 @@ defmodule Mob.Nav.ResetAllTest do setup do case Process.whereis(Mob.Nav.Registry) do nil -> :ok - pid -> stop_safely(pid) + pid -> Mob.Test.ProcessHelpers.stop_pid(pid) end {:ok, registry} = Mob.Nav.Registry.start_link(TabApp) - on_exit(fn -> stop_safely(registry) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(registry) end) {:ok, router} = Mob.Screen.start_link(LoginScreen, %{source: :initial}) - on_exit(fn -> stop_safely(router) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(router) end) %{router: router} end @@ -303,10 +303,4 @@ defmodule Mob.Nav.ResetAllTest do assert Mob.Router.reset_all_supported?(Mob.Screen.Server, Mob.ScreenState) refute :code.is_loaded(Mob.ScreenState) == false end - - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end end diff --git a/test/mob/nav/reset_transition_test.exs b/test/mob/nav/reset_transition_test.exs index 5d82ad50..4df3838d 100644 --- a/test/mob/nav/reset_transition_test.exs +++ b/test/mob/nav/reset_transition_test.exs @@ -103,12 +103,6 @@ defmodule Mob.Nav.ResetTransitionTest do end end - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - # The transition for the frame the reset painted, ignoring the initial mount. defp last_transition do Mob.Sender.sync(:infinity) @@ -153,7 +147,7 @@ defmodule Mob.Nav.ResetTransitionTest do setup do for name <- [Mob.Nav.Registry, Mob.Sender, Mob.Listener, Mob.ComponentRegistry], pid = Process.whereis(name) do - stop_safely(pid) + Mob.Test.ProcessHelpers.stop_pid(pid) end # The render path reconciles components, which needs the registry's table. @@ -163,10 +157,10 @@ defmodule Mob.Nav.ResetTransitionTest do # directly. Mob.Router brings up the Sender and Listener under their global # names; leaving them behind is what produces cross-file ordering flakes. on_exit(fn -> - stop_safely(components) + Mob.Test.ProcessHelpers.stop_pid(components) for name <- [Mob.Sender, Mob.Listener], pid = Process.whereis(name) do - stop_safely(pid) + Mob.Test.ProcessHelpers.stop_pid(pid) end case Process.whereis(RecordingNif) do @@ -181,10 +175,10 @@ defmodule Mob.Nav.ResetTransitionTest do end {:ok, registry} = Mob.Nav.Registry.start_link(DemoApp) - on_exit(fn -> stop_safely(registry) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(registry) end) {:ok, router} = Mob.Router.start_root(HomeScreen, %{}, nif: RecordingNif) - on_exit(fn -> stop_safely(router) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(router) end) RecordingNif.reset() %{router: router} diff --git a/test/mob/nav/screen_nav_test.exs b/test/mob/nav/screen_nav_test.exs index c8c17e00..9f44443a 100644 --- a/test/mob/nav/screen_nav_test.exs +++ b/test/mob/nav/screen_nav_test.exs @@ -3,15 +3,6 @@ defmodule Mob.Nav.ScreenNavTest do import ExUnit.CaptureLog - # `if Process.alive?, do: GenServer.stop` races: the router dies with the test - # process, so it can exit between the check and the stop and fail the test - # from inside the on_exit runner. - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - # ── Screen fixtures ──────────────────────────────────────────────────────── # Bare module names inside nested defmodule blocks don't auto-alias to siblings. # Use module attributes with fully qualified names for cross-screen references. @@ -93,7 +84,7 @@ defmodule Mob.Nav.ScreenNavTest do Mob.Test.ProcessHelpers.stop_if_running(Mob.Nav.Registry) {:ok, pid} = Mob.Nav.Registry.start_link(DemoApp) - on_exit(fn -> stop_safely(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) :ok end @@ -194,7 +185,8 @@ defmodule Mob.Nav.ScreenNavTest do {:ok, pid} = Mob.Screen.start_link(HomeScreen, %{}) # Send an info message that would trigger pop — default handle_info is noop send(pid, :pop_test) - Process.sleep(10) + # `get_current_module/1` is a call from this same process, so :pop_test is + # already handled by the time it replies — no sleep needed. assert Mob.Screen.get_current_module(pid) == HomeScreen GenServer.stop(pid) end @@ -297,7 +289,7 @@ defmodule Mob.Nav.ScreenNavTest do # would take down every screen — over a typo in push_screen/2. It is # caught: navigation is left untouched and the app carries on. {:ok, pid} = Mob.Screen.start_link(UnknownNavScreen, %{}) - on_exit(fn -> stop_safely(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) log = capture_log(fn -> assert :ok = Mob.Screen.dispatch(pid, "bad_nav", %{}) end) diff --git a/test/mob/nav/tab_transition_test.exs b/test/mob/nav/tab_transition_test.exs index e9d3e480..6b72a8c9 100644 --- a/test/mob/nav/tab_transition_test.exs +++ b/test/mob/nav/tab_transition_test.exs @@ -105,7 +105,7 @@ defmodule Mob.Nav.TabTransitionTest do setup do for name <- [Mob.Nav.Registry, Mob.Sender, Mob.Listener, Mob.ComponentRegistry], pid = Process.whereis(name) do - stop_safely(pid) + Mob.Test.ProcessHelpers.stop_pid(pid) end {:ok, components} = Mob.ComponentRegistry.start_link() @@ -115,16 +115,16 @@ defmodule Mob.Nav.TabTransitionTest do {:ok, router} = Mob.Router.start_root(HomeScreen, %{}, nif: RecordingNif) on_exit(fn -> - stop_safely(router) - stop_safely(registry) - stop_safely(components) - stop_safely(crash_control) + Mob.Test.ProcessHelpers.stop_pid(router) + Mob.Test.ProcessHelpers.stop_pid(registry) + Mob.Test.ProcessHelpers.stop_pid(components) + Mob.Test.ProcessHelpers.stop_pid(crash_control) for name <- [Mob.Sender, Mob.Listener], pid = Process.whereis(name) do - stop_safely(pid) + Mob.Test.ProcessHelpers.stop_pid(pid) end - stop_safely(recording) + Mob.Test.ProcessHelpers.stop_pid(recording) end) # The router queues initial paint from its process. Dispatching through the @@ -135,12 +135,6 @@ defmodule Mob.Nav.TabTransitionTest do %{router: router} end - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - defp transitions do Mob.Sender.sync(:infinity) RecordingNif.transitions() diff --git a/test/mob/process_helpers_test.exs b/test/mob/process_helpers_test.exs new file mode 100644 index 00000000..d5e2301e --- /dev/null +++ b/test/mob/process_helpers_test.exs @@ -0,0 +1,86 @@ +defmodule Mob.Test.ProcessHelpersTest do + @moduledoc """ + The helpers exist to remove races from test setup and teardown, so they are + worth testing: a helper that silently does nothing would hide the very + failures it was written to prevent. + """ + use ExUnit.Case, async: true + + alias Mob.Test.ProcessHelpers + + describe "await_exit/2" do + test "returns once the process is gone" do + pid = spawn(fn -> :ok end) + + assert ProcessHelpers.await_exit(pid) == :ok + refute Process.alive?(pid) + end + + test "raises rather than continuing when the process outlives the timeout" do + # The failure mode that matters. Returning quietly here would let the + # caller assert against a process that is still running — exactly the + # situation a fixed Process.sleep leaves you in, just with a nicer name. + pid = spawn(fn -> Process.sleep(:infinity) end) + + assert_raise RuntimeError, ~r/still alive after/, fn -> + ProcessHelpers.await_exit(pid, 20) + end + + Process.exit(pid, :kill) + end + + test "an already-dead process returns immediately" do + pid = spawn(fn -> :ok end) + :ok = ProcessHelpers.await_exit(pid) + + # Monitoring a dead pid delivers :DOWN straight away rather than hanging. + assert ProcessHelpers.await_exit(pid) == :ok + end + end + + describe "stop_pid/2" do + test "stops a live process" do + {:ok, pid} = Agent.start(fn -> :state end) + + assert ProcessHelpers.stop_pid(pid) == :ok + refute Process.alive?(pid) + end + + test "tolerates a process that is already gone — the race it exists for" do + {:ok, pid} = Agent.start(fn -> :state end) + :ok = ProcessHelpers.stop_pid(pid) + + assert ProcessHelpers.stop_pid(pid) == :ok + end + + test "raises when the process ignores a :normal stop" do + # :timeout is the opposite of "already gone" — the process is alive and + # about to leak into the next test. Returning :ok here would hide the + # exact failure this module exists to prevent. + pid = + spawn(fn -> + Process.flag(:trap_exit, true) + Process.sleep(:infinity) + end) + + assert_raise RuntimeError, ~r/still alive/, fn -> + ProcessHelpers.stop_pid(pid, 50) + end + + Process.exit(pid, :kill) + end + end + + describe "stop_if_running/2" do + test "stops a named process and tolerates its absence" do + # Unique per run, not a fixed atom. A test that documents the danger of + # shared global names should not register one: if this failed before its + # cleanup, the agent would outlive the run and break the next repeat. + name = :"helpers_probe_#{System.unique_integer([:positive])}" + {:ok, _} = Agent.start(fn -> :state end, name: name) + + assert ProcessHelpers.stop_if_running(name) == :ok + assert ProcessHelpers.stop_if_running(name) == :ok + end + end +end diff --git a/test/mob/registry_test.exs b/test/mob/registry_test.exs index a4e6d4fb..f9edd1ef 100644 --- a/test/mob/registry_test.exs +++ b/test/mob/registry_test.exs @@ -47,7 +47,7 @@ defmodule Mob.RegistryTest do # Use a unique name per test to avoid async collisions name = :"Mob.Registry.#{System.unique_integer([:positive])}" {:ok, pid} = Registry.start_link(name: name) - on_exit(fn -> if Process.alive?(pid), do: Agent.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) %{default_reg: name} end diff --git a/test/mob/render_stats_test.exs b/test/mob/render_stats_test.exs index 2bbb8855..33341d6d 100644 --- a/test/mob/render_stats_test.exs +++ b/test/mob/render_stats_test.exs @@ -250,9 +250,9 @@ defmodule Mob.RenderStatsTest do end setup do - for name <- [Mob.Sender], pid = Process.whereis(name), do: GenServer.stop(pid) + Mob.Test.ProcessHelpers.stop_if_running(Mob.Sender) {:ok, sender} = Mob.Sender.start_link(active: :the_screen) - on_exit(fn -> if Process.alive?(sender), do: GenServer.stop(sender) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(sender) end) RenderStats.enable() RenderStats.reset() @@ -338,7 +338,7 @@ defmodule Mob.RenderStatsTest do setup do services = [Mob.Sender, Mob.Listener, Mob.ComponentRegistry, Mob.Nav.Registry] - for name <- services, pid = Process.whereis(name), do: safe_stop(pid) + for name <- services, pid = Process.whereis(name), do: Mob.Test.ProcessHelpers.stop_pid(pid) {:ok, _} = Mob.ComponentRegistry.start_link() {:ok, _} = Mob.Nav.Registry.start_link(DemoApp) @@ -349,19 +349,16 @@ defmodule Mob.RenderStatsTest do {:ok, router} = Mob.Router.start_root(CounterScreen, %{}, nif: RealNif) on_exit(fn -> - safe_stop(router) - for name <- services, pid = Process.whereis(name), do: safe_stop(pid) + Mob.Test.ProcessHelpers.stop_pid(router) + + for name <- services, + pid = Process.whereis(name), + do: Mob.Test.ProcessHelpers.stop_pid(pid) end) %{router: router} end - defp safe_stop(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - test "a real render records a complete frame", %{router: router} do RenderStats.reset() Mob.Screen.dispatch(router, "bump", %{}) diff --git a/test/mob/router_hot_path_test.exs b/test/mob/router_hot_path_test.exs index cdc53b4a..a7648d37 100644 --- a/test/mob/router_hot_path_test.exs +++ b/test/mob/router_hot_path_test.exs @@ -64,20 +64,14 @@ defmodule Mob.RouterHotPathTest do def set_root(_json), do: :ok end - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - setup do Mob.Test.ProcessHelpers.stop_if_running(Mob.Nav.Registry) {:ok, registry} = Mob.Nav.Registry.start_link(DemoApp) - on_exit(fn -> stop_safely(registry) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(registry) end) {:ok, router} = Mob.Screen.start_link(HomeScreen, %{}) - on_exit(fn -> stop_safely(router) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(router) end) screen = Mob.Screen.get_screen_pid(router) %{router: router, screen: screen} @@ -152,15 +146,18 @@ defmodule Mob.RouterHotPathTest do describe "the render path does not reach it either" do setup do services = [Mob.Sender, Mob.Listener, Mob.ComponentRegistry] - for name <- services, pid = Process.whereis(name), do: stop_safely(pid) + for name <- services, pid = Process.whereis(name), do: Mob.Test.ProcessHelpers.stop_pid(pid) # The render path reconciles components, which needs the registry's table. {:ok, _} = Mob.ComponentRegistry.start_link() {:ok, router} = Mob.Router.start_root(HomeScreen, %{}, nif: StubNif) on_exit(fn -> - stop_safely(router) - for name <- services, pid = Process.whereis(name), do: stop_safely(pid) + Mob.Test.ProcessHelpers.stop_pid(router) + + for name <- services, + pid = Process.whereis(name), + do: Mob.Test.ProcessHelpers.stop_pid(pid) end) %{rendering_router: router, rendering_screen: Mob.Screen.get_screen_pid(router)} diff --git a/test/mob/screen/isolation_test.exs b/test/mob/screen/isolation_test.exs index ae443684..d16d7b59 100644 --- a/test/mob/screen/isolation_test.exs +++ b/test/mob/screen/isolation_test.exs @@ -57,23 +57,14 @@ defmodule Mob.Screen.IsolationTest do Mob.Test.ProcessHelpers.stop_if_running(Mob.Nav.Registry) {:ok, registry} = Mob.Nav.Registry.start_link(DemoApp) - on_exit(fn -> stop_safely(registry) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(registry) end) {:ok, owner} = Mob.Screen.start_link(HomeScreen, %{}) - on_exit(fn -> stop_safely(owner) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(owner) end) %{owner: owner} end - # `if Process.alive?, do: GenServer.stop` races: the process can exit between - # the check and the stop, and the :noproc exit then fails the test from inside - # the on_exit runner. Screens and their owner die with the test process. - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - describe "one process per screen" do test "the owner and the screen are different processes", %{owner: owner} do assert Mob.Screen.get_screen_pid(owner) != owner diff --git a/test/mob/screen/migration_test.exs b/test/mob/screen/migration_test.exs index 6a03087b..688cf355 100644 --- a/test/mob/screen/migration_test.exs +++ b/test/mob/screen/migration_test.exs @@ -82,17 +82,11 @@ defmodule Mob.Screen.MigrationTest do def navigation(_), do: tab_bar([stack(:home, root: @home), stack(:settings, root: @settings)]) end - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - # Screens dump in their own terminate/2, which runs after the router exits — # so the router being down does not mean the writes have landed. defp stop_and_await_screens(router) do refs = for pid <- live_screen_pids(router), do: {pid, Process.monitor(pid)} - stop_safely(router) + Mob.Test.ProcessHelpers.stop_pid(router) for {pid, ref} <- refs do receive do @@ -137,24 +131,24 @@ defmodule Mob.Screen.MigrationTest do defp reset_services do for name <- [Mob.Sender, Mob.Listener, Mob.ComponentRegistry, Mob.Nav.Registry], pid = Process.whereis(name), - do: stop_safely(pid) + do: Mob.Test.ProcessHelpers.stop_pid(pid) end describe "hot reload reaches every live screen" do setup do reset_services() - if pid = Process.whereis(Renders), do: Agent.stop(pid) + Mob.Test.ProcessHelpers.stop_if_running(Renders) {:ok, renders} = Renders.start() # Globally named and unlinked, so it outlives the suite unless stopped. - on_exit(fn -> if Process.alive?(renders), do: Agent.stop(renders) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(renders) end) {:ok, _} = Mob.ComponentRegistry.start_link() {:ok, _} = Mob.Nav.Registry.start_link(TabApp) {:ok, router} = Mob.Router.start_root(HomeScreen, %{}, nif: StubNif) on_exit(fn -> - stop_safely(router) + Mob.Test.ProcessHelpers.stop_pid(router) reset_services() end) diff --git a/test/mob/screen/restart_test.exs b/test/mob/screen/restart_test.exs index 98ac12b3..5a8af6e0 100644 --- a/test/mob/screen/restart_test.exs +++ b/test/mob/screen/restart_test.exs @@ -63,12 +63,6 @@ defmodule Mob.Screen.RestartTest do end end - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - defp owner_state(owner), do: :sys.get_state(owner) defp history(owner), do: owner |> owner_state() |> Map.fetch!(:nav) |> Mob.Nav.history() defp parked(owner), do: owner |> owner_state() |> Map.fetch!(:nav) |> Map.fetch!(:parked) @@ -93,10 +87,10 @@ defmodule Mob.Screen.RestartTest do Mob.Test.ProcessHelpers.stop_if_running(Mob.Nav.Registry) {:ok, registry} = Mob.Nav.Registry.start_link(TabApp) - on_exit(fn -> stop_safely(registry) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(registry) end) {:ok, owner} = Mob.Screen.start_link(HomeScreen, %{}) - on_exit(fn -> stop_safely(owner) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(owner) end) %{owner: owner} end diff --git a/test/mob/screen_repaint_test.exs b/test/mob/screen_repaint_test.exs index df3e5ba7..4e8472f1 100644 --- a/test/mob/screen_repaint_test.exs +++ b/test/mob/screen_repaint_test.exs @@ -88,14 +88,14 @@ defmodule Mob.ScreenRepaintTest do screen = Mob.Screen.get_screen_pid(router) on_exit(fn -> - stop_safely(router) - for pid <- started, do: stop_safely(pid) + Mob.Test.ProcessHelpers.stop_pid(router) + for pid <- started, do: Mob.Test.ProcessHelpers.stop_pid(pid) # Mob.Router.start_root starts Mob.Listener when render_mode is :render, # globally named and unlinked. Leaving it running makes Mob.Renderer route # every later tap through it, so other files see {listener_pid, {:mob_route, # ...}} where they assert {pid, tag}. Two renderer_test cases failed that # way, and only when the files happened to run in the wrong order. - if pid = Process.whereis(Mob.Listener), do: stop_safely(pid) + if pid = Process.whereis(Mob.Listener), do: Mob.Test.ProcessHelpers.stop_pid(pid) Mob.Theme.set(theme_before) end) @@ -103,12 +103,6 @@ defmodule Mob.ScreenRepaintTest do %{screen: screen} end - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - # Deterministic, not timing-based. # # A poll that waits for the counter to stop moving returns immediately on the diff --git a/test/mob/screen_sender_wiring_test.exs b/test/mob/screen_sender_wiring_test.exs index 493e916d..ad1a2cd4 100644 --- a/test/mob/screen_sender_wiring_test.exs +++ b/test/mob/screen_sender_wiring_test.exs @@ -86,24 +86,15 @@ defmodule Mob.ScreenSenderWiringTest do {:ok, registry} = Mob.Nav.Registry.start_link(TabApp) on_exit(fn -> - for pid <- [sender, registry], do: stop_safely(pid) + for pid <- [sender, registry], do: Mob.Test.ProcessHelpers.stop_pid(pid) end) {:ok, screen} = Mob.Screen.start_link(HomeScreen, %{}) - on_exit(fn -> stop_safely(screen) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(screen) end) %{screen: screen} end - # `if Process.alive?, do: GenServer.stop` races: the process can exit between - # the check and the stop, and the :noproc exit then fails the test from inside - # the on_exit runner. Screens and their owner die with the test process. - defp stop_safely(pid) do - GenServer.stop(pid) - catch - :exit, _ -> :ok - end - test "mounting a screen makes that screen active", %{screen: screen} do Sender.sync() assert active() == current_ref(screen) diff --git a/test/mob/sender_test.exs b/test/mob/sender_test.exs index 6c671199..39402fe5 100644 --- a/test/mob/sender_test.exs +++ b/test/mob/sender_test.exs @@ -52,7 +52,7 @@ defmodule Mob.SenderTest do defp start_sender(active) do {:ok, pid} = Sender.start_link(active: active) - on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_pid(pid) end) pid end @@ -469,7 +469,7 @@ defmodule Mob.SenderTest do refute Sender.running?() assert :ok = Sender.ensure_started() assert Sender.running?() - on_exit(fn -> if Sender.running?(), do: GenServer.stop(Sender) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_if_running(Sender) end) end test "is a no-op when one is already running" do @@ -495,7 +495,7 @@ defmodule Mob.SenderTest do assert_receive {:DOWN, ^ref, :process, ^caller, _} assert Process.alive?(sender) - on_exit(fn -> if Sender.running?(), do: GenServer.stop(Sender) end) + on_exit(fn -> Mob.Test.ProcessHelpers.stop_if_running(Sender) end) end end diff --git a/test/mob/state_test.exs b/test/mob/state_test.exs index 1adf40e0..3d54c1c5 100644 --- a/test/mob/state_test.exs +++ b/test/mob/state_test.exs @@ -65,7 +65,7 @@ defmodule Mob.StateTest do Process.unlink(pid) # bypasses terminate/2, no dets.close Process.exit(pid, :kill) - Process.sleep(10) + Mob.Test.ProcessHelpers.await_exit(pid) {:ok, _} = Mob.State.start_link() assert Mob.State.get(:kill_survived) == :yes end diff --git a/test/mob/theme_host_test.exs b/test/mob/theme_host_test.exs index 02faea88..b3d62131 100644 --- a/test/mob/theme_host_test.exs +++ b/test/mob/theme_host_test.exs @@ -20,7 +20,7 @@ defmodule Mob.ThemeHostTest do Enum.each(tasks, &send(&1.pid, :go)) results = Enum.map(tasks, &Task.await/1) true = Enum.all?(results, &(&1 in [:light, :ok])) - Process.sleep(100) + Logger.flush() end) 1 = length(:binary.matches(log, "The on_load function for module mob_nif returned")) @@ -69,7 +69,7 @@ defmodule Mob.ThemeHostTest do Code.compile_string(fake_nif) :dark = Mob.Theme.color_scheme() :available = :persistent_term.get(key) - Process.sleep(100) + Logger.flush() end) :persistent_term.erase(key) diff --git a/test/support/process_helpers.ex b/test/support/process_helpers.ex index d8b3bd49..80403e5a 100644 --- a/test/support/process_helpers.ex +++ b/test/support/process_helpers.ex @@ -2,7 +2,7 @@ defmodule Mob.Test.ProcessHelpers do @moduledoc """ Stopping a named process from test setup, without the race. - The idiom this replaces appeared in nine test files: + The idiom this replaces appeared six times across five test modules: case Process.whereis(Name) do nil -> :ok @@ -33,15 +33,138 @@ defmodule Mob.Test.ProcessHelpers do :ok pid -> - try do - GenServer.stop(pid, :normal, timeout) - catch - # :noproc — it exited between the whereis and here, which is the race. - # :normal / :shutdown — it was already on its way down. - :exit, _ -> :ok - end + stop_pid(pid, timeout) + end + end + + @doc """ + Stop `pid` if it is running, tolerating it having already stopped. + + The pid-shaped version of the same race, which appeared 19 times across + 11 modules as: + on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) + + `Process.alive?/1` is a check-then-act just as `whereis` is. Since MOB-112 + the screen owner is *linked to the test process*, and ExUnit exits that + process with `:shutdown` when the test ends — so the owner is dying + concurrently with the very callback trying to stop it. The window is + microseconds wide and CI is where it lands (MOB-123). + + Thirteen test modules had each written this correctly in private, byte for byte + identically, which is a fair signal it belongs here instead. + """ + @spec stop_pid(pid(), timeout()) :: :ok + def stop_pid(pid, timeout \\ 5_000) when is_pid(pid) do + GenServer.stop(pid, :normal, timeout) + :ok + catch + # Only one exit reason means "did not work": the process ignored a :normal + # stop and is still alive, about to leak into the next test. That is the + # failure this module exists to prevent, so it is the one thing that must + # not be swallowed. `GenServer.stop/3` reports it as + # `{:timeout, {GenServer, :stop, _}}`, not a bare atom. + :exit, {:timeout, _} -> + raise "#{inspect(pid)} ignored a :normal stop for #{timeout}ms and is still alive" + + # Everything else means it was already on its way down, which is the state + # the caller wanted. Do not enumerate those shapes: they nest to varying + # depths depending on how far into shutdown the process got — a linked + # owner dying as ExUnit tears the test down arrives as + # `{{:shutdown, {:sys, :terminate, _}}, {GenServer, :stop, _}}`. An earlier + # version of this clause listed `{reason, _} when reason in [:normal, + # :shutdown]` and failed on exactly that, in three tests, only under a full + # concurrent run. + :exit, _ -> + :ok + end + + @doc """ + Block until `pid` has actually exited, or fail loudly. + + Replaces the shape MOB-154 was opened for: + + Process.exit(pid, :kill) + Process.sleep(10) + # ... assert something that requires the process to be gone + + A fixed sleep is a bet on the scheduler. It wins on an idle laptop and loses + on a loaded CI box, so the failure lands on whoever pushed next and looks + unrelated to their change. A monitor is not a tighter bet — the `:DOWN` + cannot arrive before the process is gone, so there is nothing left to race. + + Raises rather than returning on timeout: a test that continues after this + fails is asserting against a process that may still be alive, which is the + situation being avoided. + """ + @spec await_exit(pid(), timeout()) :: :ok + def await_exit(pid, timeout \\ 1_000) when is_pid(pid) do + ref = Process.monitor(pid) + + receive do + {:DOWN, ^ref, :process, ^pid, _reason} -> :ok + after + timeout -> + Process.demonitor(ref, [:flush]) + raise "#{inspect(pid)} was still alive after #{timeout}ms" + end + end + + @doc """ + Poll `fun` until it returns a truthy value, or fail after `timeout` ms. + + For the narrow case where a process mutates its own state in response to a + message from *somewhere else* — a `:DOWN` from a monitor, say. A `call` from + the test process is an ordering barrier only for messages the test itself + sent; it says nothing about a `:DOWN` that arrived from a third party. + + Prefer a ready-message or a monitor when the thing you are waiting for is + observable. Reach for this only when it genuinely is not: unlike a fixed + sleep it returns as soon as the condition holds, and it reports the failure + instead of letting the next assertion produce a confusing one. + """ + @spec eventually((-> any()), non_neg_integer()) :: :ok + def eventually(fun, timeout \\ 1_000) when is_function(fun, 0) do + deadline = System.monotonic_time(:millisecond) + timeout + do_eventually(fun, deadline, timeout) + end + + defp do_eventually(fun, deadline, timeout) do + cond do + fun.() -> :ok + + System.monotonic_time(:millisecond) >= deadline -> + raise "condition still false after #{timeout}ms" + + true -> + Process.sleep(5) + do_eventually(fun, deadline, timeout) + end + end + + @doc """ + Make sure `Mob.ComponentRegistry` is running, without any test owning it. + + The registry is globally named and owns a named ETS table, and two + `async: true` modules use it. Under `start_supervised/1` whichever test won + the race OWNED it, and ExUnit tore it — and its table — down at that test's + end while the other module was still running (MOB-119). + + `test_helper.exs` starts it for the run, which fixes that for one `mix test`. + It is not enough on its own: several sync modules deliberately stop it in + teardown and do not restart it, and `--repeat-until-failure` loops inside + `ExUnit.run/0` without re-running `test_helper.exs` — so on the second + iteration the async setups would find it absent and own it again, restoring + the exact race. + + Starting it here, unlinked and unsupervised, means no test can ever own it. + """ + @spec ensure_component_registry() :: :ok + def ensure_component_registry do + case GenServer.start(Mob.ComponentRegistry, [], name: Mob.ComponentRegistry) do + {:ok, _pid} -> :ok + {:error, {:already_started, _pid}} -> :ok end end end diff --git a/test/test_helper.exs b/test/test_helper.exs index e4561b1b..950b22cb 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -1 +1,26 @@ +# `Mob.ComponentRegistry` is a globally-named singleton that owns a named ETS +# table, and two `async: true` modules use it — `component_test.exs` and +# `component_server_test.exs`. Both setups do: +# +# case start_supervised({Mob.ComponentRegistry, []}) do +# {:ok, _pid} -> :ok +# {:error, {:already_started, _pid}} -> :ok +# end +# +# `start_supervised/1` ties the process to the *individual test*. So whichever +# test wins the race OWNS the registry, and ExUnit tears it — and its ETS table +# — down when that test ends, while a concurrent test in the other module is +# still using it. The loser dies on `:ets.lookup` against a table that no +# longer exists (MOB-119, and the mechanism behind MOB-154). +# +# Starting it here makes it owned by the RUN. Both setups then take the +# `:already_started` branch, nobody owns it, and nobody can tear it down +# mid-flight. Sharing is safe because every registry entry is keyed by a +# per-test `screen_pid`, so no two tests can collide on a key. +# +# `router_hot_path_test.exs` deliberately stops and restarts it to prove the +# hot path does not touch it. That is `async: false`, and ExUnit runs every +# async module before any sync one, so it cannot race the two above. +{:ok, _} = Mob.ComponentRegistry.start_link() + ExUnit.start(exclude: [:onboarding, :on_device])