Make the test suite tell the truth (MOB-154, MOB-119, MOB-123) - #140
Open
GenericJam wants to merge 1 commit into
Open
Make the test suite tell the truth (MOB-154, MOB-119, MOB-123)#140GenericJam wants to merge 1 commit into
GenericJam wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MOB-154, MOB-119, MOB-123.
A 1-in-20 flake corrupted a mutation-testing verdict and, a day later, sent a bisect down the wrong path when it appeared 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
A globally-named singleton owned by whichever test started it.
Mob.ComponentRegistryis named and owns a named ETS table. Twoasync: truemodules each calledstart_supervised({Mob.ComponentRegistry, []})and tolerated{:error, {:already_started, _}}. The winner 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. It now starts intest_helper.exs, owned by the run.Check-then-act across a process boundary. 19
on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end)sites across 11 modules. Since MOB-112 the screen owner is linked to the test process, which ExUnit exits with:shutdownat test end, so the owner is dying concurrently with the callback trying to stop it. 13 further modules had each independently written the correct private helper, byte for byte identically.Fixed durations standing in for synchronisation.
Not every sleep is a bug
All 35 were classified. 12 are
Process.sleep(:infinity)— a parked stub, not a wait. Of the 23 finite ones, 15 went and 8 remain:GenServer.callfrom the same process that sent the earlier messages is already a barrier: Erlang orders messages pairwise, so everysendis ahead of thecall. AndTrace.broadcast/3folds over the table in the calling process, so its cleanup is done beforedispatch/4returns.Logger.flush/0, a monitor.eventually/2), for the one shape where pairwise ordering genuinely does not help: a GenServer processing a:DOWNsent by a monitor, not by the test.render_stats_test.exs(it measures elapsed time), 3 are the backoff inside a poll loop with its own deadline, 1 is a@docexample quoting the bad pattern, and 1 is a genuine bet (router_hot_path_test.exs:206) recorded rather than disguised.mix mob.flakeRuns the suite repeatedly and reports which tests are non-deterministic. Its green output says 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 persist.
Honesty notes
stop_pid/2broke 3 tests, and only under a full run. Making the timeout raise meant replacing a blanket:exit, _ -> :okwith enumerated clauses, and the enumeration was wrong: a linked owner dying during teardown exits with{{:shutdown, {:sys, :terminate, _}}, {GenServer, :stop, _}}. Every file passed alone; the full run failed 3 tests, then 4. Fixed by inverting — special-case only the timeout, treat every other exit as "already gone". Enumerating the uninteresting set is a bet on having seen every shutdown shape, the same class of mistake as betting on a duration.Logger.flush/0swaps are not demonstrably load-bearing — with the barrier deleted outright those tests still passed 8/8 here.flush/0is kept because it is the correct primitive and free when nothing is pending, whereas the sleep cost 300ms per run. That is a reason, not a measurement.Verification
mix credo --strictclean,mix format --check-formattedclean.mix mob.flakeverified against a deliberate 2-failure test: reports both, exits 1. Its first version silently dropped failures near the end of the output viachunk_every(12, 1, :discard); now it splits on the failure-header regex.process_helpers_test.exs(7 tests) pins the helper's own contract — including thatstop_pid/2raises on timeout. That test caught a real bug in the first version of the catch clause, which matched a bare:timeoutthatGenServer.stop/3never emits.Decision record:
decisions/2026-09-06-tests-wait-for-events-not-durations.md🤖 Generated with Claude Code