diff --git a/changelog/.gitkeep b/changelog/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/comparison/elixir-vs-phoenix.md b/comparison/elixir-vs-phoenix.md index 8393f59..e2d437c 100644 --- a/comparison/elixir-vs-phoenix.md +++ b/comparison/elixir-vs-phoenix.md @@ -11,8 +11,8 @@ How the same concepts are approached differently (or similarly) between Elixir c | **Process identity** | Registry `:via` tuples | Topic-based (channels identified by topic) | | **Supervision** | Direct supervisor reference | Endpoint supervisor manages all | -**Source (Elixir):** `lib/elixir/lib/gen_server.ex:843-848` (child_spec without restart = defaults to :permanent) -**Source (Phoenix):** `lib/phoenix/channel.ex:470-475` (explicit restart: :temporary) +**Source (Elixir):** `lib/elixir/lib/gen_server.ex:911-919` (child_spec defaults to :permanent via Supervisor.child_spec) +**Source (Phoenix):** `lib/phoenix/channel.ex:464-472` (explicit restart: :temporary) --- @@ -25,8 +25,8 @@ How the same concepts are approached differently (or similarly) between Elixir c | **Failure response** | `{:error, reason}` tuple | `{:error, reason}` + HTTP status | | **Recovery** | Supervisor restart | Client reconnection | -**Source (Elixir):** Standard tagged tuples throughout (`lib/elixir/lib/agent.ex:210`) -**Source (Phoenix):** `lib/phoenix/router.ex:7-8` (NoRouteError with plug_status: 404) +**Source (Elixir):** `lib/elixir/lib/agent.ex:187` (standard on_start type: `{:ok, pid} | {:error, ...}`) +**Source (Phoenix):** `lib/phoenix/router.ex:2-6` (NoRouteError with plug_status: 404) --- @@ -39,8 +39,8 @@ How the same concepts are approached differently (or similarly) between Elixir c | **Configuration** | Via `use Module, opts` | Via `use Module, opts` + module attributes | | **Before-compile** | Rarely used | Heavily used (routes, intercepts) | -**Source (Elixir):** `lib/elixir/lib/gen_server.ex:834-851` -**Source (Phoenix):** `lib/phoenix/channel.ex:450-500` +**Source (Elixir):** `lib/elixir/lib/gen_server.ex:899-919` (__using__ generates child_spec + @behaviour) +**Source (Phoenix):** `lib/phoenix/channel.ex:450-485` (__using__ generates child_spec + behaviour + DSL setup) --- @@ -53,8 +53,8 @@ How the same concepts are approached differently (or similarly) between Elixir c | **DSL creation** | Avoided (except Kernel/SpecialForms) | Embraced (Router DSL) | | **Attribute accumulation** | Rare | Central pattern (routes, sockets) | -**Source (Elixir):** `lib/elixir/lib/gen_server.ex:834` — simple `__using__` -**Source (Phoenix):** `lib/phoenix/router.ex:263-290` — complex DSL setup with attribute accumulation +**Source (Elixir):** `lib/elixir/lib/gen_server.ex:899` — simple `__using__` (behaviour + child_spec + defaults) +**Source (Phoenix):** `lib/phoenix/router.ex:288-312` — complex DSL setup with attribute accumulation, imports, and @before_compile --- @@ -80,8 +80,8 @@ Both follow the same convention: public API on the parent module, implementation | **State shape** | Any term (developer's choice) | `%Socket{}` struct (framework-defined) | | **State access** | Direct in callbacks | Via `socket.assigns` | -**Source (Elixir):** `lib/elixir/lib/agent.ex:62-82` (compute in server vs client) -**Source (Phoenix):** `lib/phoenix/channel.ex:463-464` (`import Phoenix.Socket, only: [assign: 3, assign: 2]`) +**Source (Elixir):** `lib/elixir/lib/agent.ex:62-82` (compute in server vs client pattern) +**Source (Phoenix):** `lib/phoenix/channel.ex:463` (`import Phoenix.Socket, only: [assign: 3, assign: 2]`) --- @@ -115,3 +115,35 @@ var!(code_reloading?) = ``` This pattern — reading config at compile time and validating it against runtime — is Phoenix-specific. Elixir core reads config only at runtime. + +--- + +## Telemetry + +| Aspect | Elixir Core | Phoenix | +|--------|-------------|---------| +| **Built-in events** | None (telemetry is a separate library) | Extensive event catalog | +| **Instrumentation** | Manual by library authors | Baked into router, endpoint, socket | +| **Event naming** | Varies by library | `[:phoenix, :component, :phase]` convention | +| **Logging** | `Logger` calls | Telemetry → Logger adapter (`Phoenix.Logger`) | + +**Source (Phoenix):** `lib/phoenix/logger.ex:7-50` (telemetry event catalog) +**Source (Phoenix):** `lib/phoenix/router.ex:400-438` (telemetry in router dispatch) + +Phoenix wraps every request dispatch in telemetry start/stop/exception events. This provides distributed tracing, monitoring, and logging without any application code changes. + +--- + +## Testing + +| Aspect | Elixir Core | Phoenix | +|--------|-------------|---------| +| **Test helper** | `ExUnit.Case` | `Phoenix.ConnTest`, `Phoenix.ChannelTest` | +| **Test subject** | Module functions | Endpoint (full plug pipeline) | +| **Communication** | Direct function calls | HTTP verbs (ConnTest), messages (ChannelTest) | +| **Isolation** | Process per test | Process per test + sandbox (Ecto) | + +**Source (Phoenix):** `lib/phoenix/test/conn_test.ex:1-30` (endpoint-based integration testing) +**Source (Phoenix):** `lib/phoenix/test/channel_test.ex:1-30` (process-based channel testing) + +Phoenix test helpers test at the integration level by default — `ConnTest` dispatches through the full plug pipeline, `ChannelTest` exercises the full channel lifecycle via message passing. This catches middleware bugs that unit tests miss. diff --git a/phoenix/deviations.md b/phoenix/deviations.md index 3663d26..98a7b32 100644 --- a/phoenix/deviations.md +++ b/phoenix/deviations.md @@ -10,7 +10,7 @@ Where Phoenix deliberately differs from Elixir core patterns and why. **Phoenix deviation:** The Router uses macros extensively. -**Source:** `lib/phoenix/router.ex:109-123` +**Source:** `lib/phoenix/router.ex:106-128` > We use `get`, `post`, `put`, and `delete` to define your routes. We use macros > for two purposes: @@ -31,10 +31,12 @@ Where Phoenix deliberately differs from Elixir core patterns and why. **Phoenix deviation:** The Router imports entire modules: -**Source:** `lib/phoenix/router.ex:274-276` +**Source:** `lib/phoenix/router.ex:303-306` ```elixir import Phoenix.Router + +# TODO v2: No longer automatically import dependencies import Plug.Conn import Phoenix.Controller ``` @@ -49,13 +51,24 @@ import Phoenix.Controller **Phoenix deviation:** Aggressive use of module attribute accumulation. -**Source:** `lib/phoenix/router.ex:271-280` +**Source:** `lib/phoenix/router.ex:297-312` ```elixir -Module.register_attribute(__MODULE__, :phoenix_routes, accumulate: true) -@phoenix_pipeline nil -Phoenix.Router.Scope.init(__MODULE__) -@before_compile unquote(__MODULE__) +defp prelude(opts) do + quote do + Module.register_attribute(__MODULE__, :phoenix_routes, accumulate: true) + @phoenix_helpers Keyword.get(unquote(opts), :helpers, true) + + import Phoenix.Router + import Plug.Conn + import Phoenix.Controller + + # Set up initial scope + @phoenix_pipeline nil + Phoenix.Router.Scope.init(__MODULE__) + @before_compile unquote(__MODULE__) + end +end ``` **Why the deviation:** The Router needs to collect ALL routes, then compile them into a single dispatch function. This requires building up state during module compilation, then consuming it all at `@before_compile`. @@ -68,7 +81,7 @@ Phoenix.Router.Scope.init(__MODULE__) **Phoenix Channel default:** `:temporary` (never restart). -**Source:** `lib/phoenix/channel.ex:470-475` +**Source:** `lib/phoenix/channel.ex:464-472` ```elixir def child_spec(init_arg) do @@ -91,7 +104,7 @@ end **Phoenix Channel:** Defaults to hibernate after 15 seconds of inactivity. -**Source:** `lib/phoenix/channel.ex:460` +**Source:** `lib/phoenix/channel.ex:459` ```elixir @phoenix_hibernate_after Keyword.get(opts, :hibernate_after, 15_000) @@ -115,7 +128,7 @@ end **Phoenix Endpoint:** Uses `Plug.Builder` — a macro that generates the `call/2` pipeline by chaining plugs at compile time. -**Source:** `lib/phoenix/endpoint.ex:481-483` +**Source:** `lib/phoenix/endpoint.ex:478-480` ```elixir defp plug() do @@ -136,14 +149,20 @@ end **Phoenix exceptions:** Include `plug_status` for HTTP response mapping. -**Source:** `lib/phoenix/router.ex:7-8` +**Source:** `lib/phoenix/router.ex:2-26` ```elixir defmodule NoRouteError do + @moduledoc """ + Exception raised when no route is found. + """ defexception plug_status: 404, message: "no route found", conn: nil, router: nil end defmodule MalformedURIError do + @moduledoc """ + Exception raised when the URI is malformed on matching. + """ defexception [:message, plug_status: 400] end ``` diff --git a/phoenix/patterns.md b/phoenix/patterns.md index efc5791..3a5c9e0 100644 --- a/phoenix/patterns.md +++ b/phoenix/patterns.md @@ -42,7 +42,7 @@ The endpoint is four things composed together: ## 2. Router: Compile-Time Route Optimization -**Source:** `lib/phoenix/router.ex:109-123` (Why the macros? info block) +**Source:** `lib/phoenix/router.ex:106-128` (Why the macros? info block) > We use macros for two purposes: > @@ -53,7 +53,7 @@ The endpoint is four things composed together: > > * For each route you define, we also define metadata to implement `Phoenix.VerifiedRoutes`. -**Source:** `lib/phoenix/router.ex:280` (route accumulation) +**Source:** `lib/phoenix/router.ex:299` (route accumulation) ```elixir Module.register_attribute(__MODULE__, :phoenix_routes, accumulate: true) @@ -67,7 +67,7 @@ Module.register_attribute(__MODULE__, :phoenix_routes, accumulate: true) ## 3. Pipeline and `pipe_through` for Request Processing -**Source:** `lib/phoenix/router.ex:230-260` (Pipelines and plugs section) +**Source:** `lib/phoenix/router.ex:243-270` (Pipelines and plugs section) ```elixir pipeline :browser do @@ -109,7 +109,7 @@ Controllers: - Call domain logic (the Repo/context layer) - Render the result -**Source:** `lib/phoenix/controller.ex:1-3` (imports) +**Source:** `lib/phoenix/controller.ex:1-5` (imports) ```elixir defmodule Phoenix.Controller do @@ -126,7 +126,7 @@ defmodule Phoenix.Controller do ## 5. Channel as GenServer with Topic-Based Routing -**Source:** `lib/phoenix/channel.ex:1-20` (topic pattern) +**Source:** `lib/phoenix/channel.ex:1-25` (topic pattern) ```elixir channel "room:*", MyAppWeb.RoomChannel @@ -143,7 +143,7 @@ def join("room:" <> room_id, _payload, socket) do end ``` -**Source:** `lib/phoenix/channel.ex:476-479` (channels are GenServers) +**Source:** `lib/phoenix/channel.ex:474-478` (channels are GenServers) ```elixir def start_link(triplet) do @@ -161,7 +161,7 @@ end ## 6. PubSub Integration via Endpoint -**Source:** `lib/phoenix/endpoint.ex:440-475` (pubsub macro) +**Source:** `lib/phoenix/endpoint.ex:437-475` (pubsub macro) ```elixir def subscribe(topic, opts \\ []) when is_binary(topic) do @@ -186,7 +186,7 @@ end ## 7. Socket as Authentication Boundary -**Source:** `lib/phoenix/socket.ex` (connect callback pattern) +**Source:** `lib/phoenix/socket.ex:1-30` (moduledoc: connect callback pattern) ```elixir defmodule MyAppWeb.UserSocket do @@ -210,7 +210,7 @@ end ## 8. Plug Pattern: `init/1` + `call/2` -**Source:** `lib/phoenix/router/route.ex:51-58` +**Source:** `lib/phoenix/router/route.ex:47-57` ```elixir @doc "Used as a plug on forwarding" @@ -233,3 +233,102 @@ end This is Phoenix's fundamental composition pattern. Everything is a plug. **Anti-pattern:** Doing expensive setup work in `call/2` instead of `init/1` — it runs on every request. + +--- + +## 9. Telemetry Integration in Router Dispatch + +**Source:** `lib/phoenix/router.ex:400-438` (telemetry instrumentation) + +```elixir +def __call__(conn, metadata, prepare, pipeline, {plug, opts}) do + conn = prepare.(conn, metadata) + start = System.monotonic_time() + measurements = %{system_time: System.system_time()} + metadata = %{metadata | conn: conn} + :telemetry.execute([:phoenix, :router_dispatch, :start], measurements, metadata) + + case pipeline.(conn) do + %Plug.Conn{halted: true} = halted_conn -> + measurements = %{duration: System.monotonic_time() - start} + metadata = %{metadata | conn: halted_conn} + :telemetry.execute([:phoenix, :router_dispatch, :stop], measurements, metadata) + halted_conn + + %Plug.Conn{} = piped_conn -> + try do + plug.call(piped_conn, plug.init(opts)) + else + conn -> + measurements = %{duration: System.monotonic_time() - start} + metadata = %{metadata | conn: conn} + :telemetry.execute([:phoenix, :router_dispatch, :stop], measurements, metadata) + conn + rescue + e in Plug.Conn.WrapperError -> + measurements = %{duration: System.monotonic_time() - start} + :telemetry.execute([:phoenix, :router_dispatch, :exception], measurements, metadata) + Plug.Conn.WrapperError.reraise(e) + end + end +end +``` + +**Source:** `lib/phoenix/logger.ex:7-50` (telemetry event catalog) + +Phoenix emits these telemetry events: +- `[:phoenix, :endpoint, :init]` — endpoint supervision tree started +- `[:phoenix, :endpoint, :start]` — request begins (via `Plug.Telemetry`) +- `[:phoenix, :endpoint, :stop]` — response sent +- `[:phoenix, :router_dispatch, :start]` — route dispatch begins +- `[:phoenix, :router_dispatch, :stop]` — route dispatch succeeds +- `[:phoenix, :router_dispatch, :exception]` — route dispatch raises +- `[:phoenix, :socket_connected]` — socket connection established + +**Why:** Telemetry is baked into every request path. The router wraps ALL dispatches in start/stop/exception telemetry, enabling monitoring, tracing (OpenTelemetry), and logging without modifying application code. + +**Anti-pattern:** Manual timing/logging in controllers — telemetry provides this automatically at the infrastructure level. + +--- + +## 10. ConnTest Pattern: Endpoint-Based Integration Testing + +**Source:** `lib/phoenix/test/conn_test.ex:1-30` (moduledoc) + +```elixir +@endpoint MyAppWeb.Endpoint + +test "says welcome on the home page" do + conn = get(build_conn(), "/") + assert conn.resp_body =~ "Welcome!" +end + +test "logs in" do + conn = post(build_conn(), "/login", [username: "john", password: "doe"]) + assert conn.resp_body =~ "Logged in!" +end +``` + +**Why:** `Phoenix.ConnTest` tests against the full endpoint stack (plugs, router, controller) without starting an HTTP server. `build_conn()` creates a test connection, HTTP verb functions dispatch through the endpoint. This gives integration-level confidence with unit-test speed. + +**Anti-pattern:** Testing controllers in isolation without the plug pipeline — you miss middleware bugs (auth, CSRF, sessions). + +--- + +## 11. ChannelTest Pattern: Process-Based Channel Testing + +**Source:** `lib/phoenix/test/channel_test.ex:1-30` (moduledoc) + +```elixir +{:ok, _, socket} = + socket(UserSocket, "user:id", %{some_assigns: 1}) + |> subscribe_and_join(RoomChannel, "room:lobby", %{"id" => 3}) + +# Or using connect/3 to call your UserSocket.connect callback: +{:ok, socket} = connect(UserSocket, %{"some" => "params"}, %{}) +{:ok, _, socket} = subscribe_and_join(socket, "room:lobby", %{"id" => 3}) +``` + +**Why:** Channel tests communicate via messages (not HTTP). `subscribe_and_join/4` connects a test process to the channel, and you can assert on broadcasts (`assert_broadcast`), pushes (`assert_push`), and replies (`assert_reply`). The test process subscribes to the same PubSub topic, so it sees everything the channel broadcasts. + +**Anti-pattern:** Testing channels by connecting real WebSocket clients — too slow, too brittle, tests the transport layer unnecessarily. diff --git a/smells/anti-patterns.md b/smells/anti-patterns.md index 6d04c7a..3d27185 100644 --- a/smells/anti-patterns.md +++ b/smells/anti-patterns.md @@ -1,181 +1,326 @@ -# Anti-Patterns +# Anti-Patterns (Code Smells) in Elixir -Things the Elixir and Phoenix source deliberately avoids — and why you should too. - -## 1. GenServer for Pure Functions - -**Source:** `lib/elixir/lib/gen_server.ex:533-575` ("When (not) to use a GenServer") - -The GenServer docs explicitly say: - -> If you don't need a process, then you don't need a process. - -**What they avoid:** Creating a GenServer to wrap stateless computation. - -**Why:** A process adds message passing overhead, serialization (one request at a time), and supervision complexity — all unnecessary for pure functions. - -**Do this instead:** A plain module with functions: -```elixir -# Good: pure function, no process needed -defmodule MyApp.Calculator do - def add(a, b), do: a + b -end - -# Bad: unnecessary process -defmodule MyApp.Calculator do - use GenServer - def add(a, b), do: GenServer.call(__MODULE__, {:add, a, b}) - def handle_call({:add, a, b}, _from, state), do: {:reply, a + b, state} -end -``` +Patterns the Elixir core team actively avoids, extracted from studying what they DON'T do in their source code. --- -## 2. Dynamic Atoms for Process Names +## 1. Process.sleep for Synchronization (Timing-Based Tests) -**Source:** `lib/elixir/lib/registry.ex:28-60` (Registry as alternative to atoms) +**What they avoid:** Using `Process.sleep(N)` to wait for async operations to complete. -The Registry module exists specifically because dynamic atom creation is dangerous: +**Source evidence:** `lib/elixir/test/elixir/task_test.exs` — 13 uses of `Process.sleep`, ALL are `Process.sleep(:infinity)` for process parking. Zero uses of `Process.sleep(100)` or similar for timing. -> atoms are never garbage collected - -**What they avoid:** `String.to_atom("worker_#{id}")` +**Why it's bad:** Race conditions. A sleep might be too short on a loaded CI server, causing flaky tests. Too long wastes time. **What they do instead:** ```elixir -{:via, Registry, {MyApp.Registry, "worker-#{id}"}} +# BAD — timing-based synchronization +spawn(fn -> send(parent, :done) end) +Process.sleep(50) +assert_received :done + +# GOOD — event-based synchronization +spawn(fn -> send(parent, :done) end) +assert_receive :done # waits up to 100ms with proper timeout ``` ---- - -## 3. Broad `import` Without `:only` - -**Source:** `lib/elixir/lib/enum.ex:250` - ```elixir -import Kernel, except: [max: 2, min: 2] +# BAD — waiting for process to die +Process.exit(pid, :kill) +Process.sleep(10) +refute Process.alive?(pid) + +# GOOD — monitor-based confirmation +ref = Process.monitor(pid) +Process.exit(pid, :kill) +assert_receive {:DOWN, ^ref, _, _, _} ``` -Even within the standard library, imports are scoped. Enum explicitly excludes the specific Kernel functions it replaces. - -**What they avoid:** `import Kernel` without qualification when they define conflicting names. - -**Exception:** Phoenix Router (`lib/phoenix/router.ex:274-276`) imports broadly — but it's a DSL where usability trumps explicitness. - --- -## 4. Exceptions for Control Flow +## 2. Mutable Global State in Tests -**Source:** `lib/elixir/lib/task.ex:455-460` (async documentation on error handling) +**What they avoid:** Tests that depend on or modify global state without cleanup. -> an asynchronous task should be thought of as an extension of the -> caller process rather than a mechanism to isolate it from all errors. +**Source evidence:** `lib/mix/test/test_helper.exs:98-113` — MixTest.Case restores ALL global state in `on_exit`: +- `Mix.env(:dev)`, `Mix.target(:host)`, `Mix.Task.clear()`, `Mix.Shell.Process.flush()` +- Unloads all applications that were loaded during the test -The Task documentation advises returning `{:ok, val} | :error` for normal flow, NOT using try/rescue: - -> For example, to either return `{:ok, val} | :error` results or, -> in more extreme cases, by using `try/rescue` - -**What they avoid:** Using `try/rescue` around expected failure cases. - -**Why:** Pattern matching on tagged tuples is more explicit, composable (works with `with`), and doesn't hide the error path. - ---- - -## 5. Trapping Exits in Normal Code - -**Source:** `lib/elixir/lib/task.ex:469-477` (explicit warning) - -> Setting `:trap_exit` to `true` - trapping exits should be used only in special -> circumstances as it would make your process immune to not only exits from the -> task but from any other processes. -> -> Moreover, even when trapping exits, calling `await` will still exit if the -> task has terminated without sending its result back. - -**What they avoid:** `Process.flag(:trap_exit, true)` in normal application code. - -**Why:** Trapping exits breaks the supervision contract. A supervisor expects to be able to kill its children — if they trap exits, shutdown semantics change unpredictably. - ---- - -## 6. Expensive Work in `init/1` - -**Source:** `lib/elixir/lib/gen_server.ex:127-145` (handle_continue pattern) +**Why it's bad:** Tests pass in isolation but fail when run together. Order-dependent failures are the hardest bugs to debug. +**What they do instead:** ```elixir -# What NOT to do — blocks the supervisor -def init(url) do - data = HTTP.get!(url) # BAD: blocks here - {:ok, data} -end +# Every test using Mix gets automatic cleanup +setup do + on_exit(fn -> + Mix.env(:dev) + Mix.target(:host) + Mix.Task.clear() + Mix.Shell.Process.flush() + Mix.State.clear_cache() + Mix.ProjectStack.clear_stack() -# What TO do — return immediately, do work later -def init(url) do - {:ok, :unset, {:continue, {:fetch, url}}} -end - -def handle_continue({:fetch, url}, _state) do - {:noreply, HTTP.get!(url)} + for {app, _, _} <- Application.loaded_applications(), app not in @apps do + Application.stop(app) + Application.unload(app) + end + end) end ``` -**What they avoid:** Network calls, disk I/O, or any slow operation in `init/1`. - -**Why:** `init/1` blocks `start_link`, which blocks the supervisor. If your init takes 5 seconds, the entire supervision tree startup stalls. - --- -## 7. Unlinking Task Processes +## 3. try/rescue for Control Flow -**Source:** `lib/elixir/lib/task.ex:478-482` +**What they avoid:** Using exceptions for expected conditions. -> Unlinking the task process started with `async`/`await`. If you unlink the -> processes and the task does not belong to any supervisor, you may leave -> dangling tasks in case the caller process dies. +**Source evidence:** `lib/elixir/lib/enum.ex` — 5,242 lines with only 2 `try do` blocks. `lib/elixir/lib/kernel.ex` — 7,102 lines with only 5 `try do` blocks. The ratio is vanishingly small. -**What they avoid:** `Process.unlink/1` on task processes. - -**Why:** The link is a safety mechanism. If the caller dies, the task should die too (since nobody will read the result). Unlinking creates orphan processes. - ---- - -## 8. Blocking the Agent with Expensive Computation - -**Source:** `lib/elixir/lib/agent.ex:62-82` (client vs server computation) +**Why it's bad:** Exceptions are for exceptional conditions. Using them for control flow obscures intent, defeats pattern matching, and is slower. +**What they do instead:** ```elixir -# BAD: blocks the agent, other callers queue up -def get_something(agent) do - Agent.get(agent, fn state -> do_something_expensive(state) end) +# BAD — exception as control flow +def find_user(id) do + try do + fetch_user!(id) + rescue + NotFoundError -> nil + end end -# GOOD: copies state to caller, work happens in caller's process -def get_something(agent) do - Agent.get(agent, & &1) |> do_something_expensive() +# GOOD — tagged tuples for expected outcomes +def find_user(id) do + case fetch_user(id) do + {:ok, user} -> user + {:error, :not_found} -> nil + end end ``` -**What they avoid:** Running expensive operations inside the Agent's process. - -**Why:** The Agent is a single process. While it's computing, ALL other get/update/cast operations queue up. Move computation to the caller unless atomicity is required. +The standard library uses `with` statements and tagged tuples (`{:ok, result}` / `{:error, reason}`) for all fallible operations. --- -## 9. Raw `spawn` Instead of Supervised Processes +## 4. God Modules (Unbounded Module Growth) -**Source:** `lib/elixir/lib/task.ex:24-26` (why Task over spawn) +**What they avoid:** Stuffing everything into one module. -> Compared to plain processes, started with `spawn/1`, tasks include monitoring -> metadata and logging in case of errors. +**Source evidence:** The Elixir standard library splits functionality aggressively: +- `ExUnit.Case` — test case registration +- `ExUnit.Callbacks` — setup/teardown lifecycle +- `ExUnit.Assertions` — assertion macros +- `ExUnit.CaptureIO` — IO capture +- `ExUnit.CaptureLog` — log capture +- `ExUnit.DocTest` — doctest extraction -**Source:** `lib/elixir/lib/task.ex:100-115` +Each module has a single, clear responsibility. -> We encourage developers to rely on supervised tasks as much as possible. -> Supervised tasks improve the visibility of how many tasks are running -> at a given moment and enable a variety of patterns that give you -> explicit control on how to handle the results, errors, and timeouts. +**Why it's bad:** Large modules are hard to navigate, hard to test in isolation, and create implicit coupling between unrelated features. -**What they avoid:** `spawn/1` and `spawn_link/1` in application code. +**What they do instead:** Single-responsibility modules that compose. `use ExUnit.Case` imports from multiple focused modules. -**Why:** Unsupervised processes are invisible to the system. They don't appear in observer, don't get logged on crash, and can't be gracefully shut down. +--- + +## 5. Stringly-Typed APIs + +**What they avoid:** Using strings where atoms, structs, or tagged tuples would be clearer. + +**Source evidence:** Throughout the codebase, atoms and structs are preferred: +- `lib/elixir/lib/task.ex` — `defstruct @enforce_keys` (Task is a struct, not a map) +- `lib/elixir/lib/uri.ex` — URI is a struct +- `lib/elixir/lib/version.ex` — Version is a struct +- `lib/elixir/lib/range.ex` — Range is a struct +- All ExUnit tags use atoms: `:async`, `:capture_log`, `:tmp_dir` + +**Why it's bad:** Strings lack compile-time checking, are prone to typos, and don't pattern match cleanly. + +**What they do instead:** +```elixir +# BAD — string keys, no structure +%{"type" => "user", "name" => "Alice", "role" => "admin"} + +# GOOD — struct with enforced keys +defmodule User do + @enforce_keys [:name, :role] + defstruct [:name, :role, :email] +end +%User{name: "Alice", role: :admin} +``` + +--- + +## 6. Bare Maps Where Structs Belong + +**What they avoid:** Using plain maps for data with a known, fixed shape. + +**Source evidence:** 69 `defstruct` declarations in `lib/elixir/lib/` alone. Every domain concept with a stable shape gets a struct: Task, URI, Version, Range, Regex, MapSet, Stream. + +**Why it's bad:** +- No compile-time key checking +- No default values +- No protocol implementations +- No `@enforce_keys` for required fields +- Pattern matching is fragile (extra keys silently ignored) + +**What they do instead:** +```elixir +# The Task struct enforces its shape: +defmodule Task do + @enforce_keys [:ref, :pid, :owner, :mfa] + defstruct @enforce_keys +end + +# Pattern matching on struct is type-safe: +def await(%Task{ref: ref, owner: owner}) when owner == self() do + # ... +end +``` + +--- + +## 7. Deep Nesting + +**What they avoid:** Deeply nested `case`/`if`/`cond` blocks. + +**Source evidence:** The standard library heavily uses: +- Early returns via pattern matching in function heads +- `with` statements for sequential fallible operations +- Pipeline operator for transformations +- Multi-clause functions instead of nested conditionals + +**Why it's bad:** Deep nesting increases cognitive load and makes the "happy path" hard to identify. + +**What they do instead:** +```elixir +# BAD — nested conditionals +def process(input) do + case validate(input) do + {:ok, valid} -> + case transform(valid) do + {:ok, result} -> + case save(result) do + {:ok, saved} -> {:ok, saved} + {:error, reason} -> {:error, reason} + end + {:error, reason} -> {:error, reason} + end + {:error, reason} -> {:error, reason} + end +end + +# GOOD — with statement flattens the happy path +def process(input) do + with {:ok, valid} <- validate(input), + {:ok, result} <- transform(valid), + {:ok, saved} <- save(result) do + {:ok, saved} + end +end +``` + +--- + +## 8. Shared Mutable State Between Tests + +**What they avoid:** ETS tables, registered names, or application env used across tests without isolation. + +**Source evidence:** `lib/elixir/test/elixir/registry_test.exs:28-31` — Each test gets a uniquely-named Registry: +```elixir +name = :"#{config.test}_#{partitions}_#{inspect(keys)}" +``` + +`lib/elixir/test/elixir/gen_server_test.exs:166` — Uses `%{test: name}` for unique process registration. + +**Why it's bad:** Tests that share state can't run concurrently. They're order-dependent and fragile. + +**What they do instead:** Every shared resource gets a unique name derived from `config.test` (the test name atom, guaranteed unique within a module). + +--- + +## 9. Testing Internal State Directly + +**What they avoid:** Reaching into process state, private functions, or internal data structures for assertions. + +**Source evidence:** `lib/elixir/test/elixir/gen_server_test.exs` — The Stack GenServer is tested entirely through its public call/cast interface. No `:sys.get_state/1` anywhere in the test. + +**Why it's bad:** Tests become coupled to implementation. Refactoring internals breaks tests even when behavior is unchanged. + +**What they do instead:** Test the behavior (what goes in, what comes out) through the public API. + +--- + +## 10. Overly Complex Test Setup + +**What they avoid:** Setup blocks that are longer than the tests themselves. + +**Source evidence:** The Elixir test suite keeps setup minimal: +- `lib/elixir/test/elixir/registry_test.exs` — 5-line setup +- `lib/elixir/test/elixir/gen_server_test.exs` — no setup at all, each test is self-contained +- `lib/elixir/test/elixir/task_test.exs` — helper functions instead of complex setup + +**Why it's bad:** Complex setup obscures what's actually being tested. When a test fails, you have to understand the setup to understand the failure. + +**What they do instead:** +- Self-contained tests that set up exactly what they need +- Small helper functions for common patterns +- `start_supervised!` for process-heavy tests (one line) + +--- + +## 11. Unsupervised Processes + +**What they avoid:** Using raw `spawn/1` or `spawn_link/1` in application code. + +**Source evidence:** The standard library provides `Task`, `GenServer`, `Agent`, and `DynamicSupervisor` for every process use case. Raw spawns appear only in test helpers. + +**Why it's bad:** Unsupervised processes are invisible to the system: +- Don't appear in Observer +- Don't get logged on crash +- Can't be gracefully shut down +- Create orphan processes on restart + +**What they do instead:** +```elixir +# BAD — orphan process +spawn(fn -> do_work() end) + +# GOOD — supervised, observable, restartable +Task.Supervisor.start_child(MyApp.TaskSupervisor, fn -> do_work() end) +``` + +--- + +## 12. Atom Creation from User Input + +**What they avoid:** Converting untrusted strings to atoms. + +**Source evidence:** `lib/elixir/lib/option_parser.ex:855` — Uses `String.to_existing_atom/1` with the `:switches` allowlist pattern. The only `String.to_atom/1` calls in library code are in compiler/macro contexts where the set is bounded. + +**Why it's bad:** Atoms are never garbage collected. User-controlled atom creation is a denial-of-service vector (1,048,576 atom limit by default). + +**What they do instead:** +```elixir +# BAD — unbounded atom creation +key = String.to_atom(user_input) + +# GOOD — bounded, pre-existing atoms only +key = String.to_existing_atom(user_input) + +# BETTER — don't convert at all, use string keys +config = Map.get(settings, user_input) +``` + +--- + +## 13. Callback-Heavy Abstractions (When Protocols Suffice) + +**What they avoid:** Behaviour callbacks for polymorphism when protocols are more appropriate. + +**Source evidence:** The standard library uses protocols for data-type polymorphism (`Enumerable`, `Collectable`, `Inspect`, `String.Chars`, `List.Chars`) and behaviours only for process contracts (`GenServer`, `Supervisor`, `Application`). + +**Why it's bad:** Behaviours require the implementing module to know about the behaviour at compile time. Protocols allow retroactive implementation for types you don't control. + +**The heuristic:** +- **Protocol:** "Different data types need different treatment" (e.g., printing, iterating) +- **Behaviour:** "Different modules implement the same process contract" (e.g., GenServer callbacks) diff --git a/smells/common-mistakes.md b/smells/common-mistakes.md index 739db32..7d4ecb0 100644 --- a/smells/common-mistakes.md +++ b/smells/common-mistakes.md @@ -1,265 +1,409 @@ -# Common Mistakes +# Common Mistakes in Elixir (What the Core Team Avoids) -What "bad Elixir" looks like, based on what the source code explicitly warns against or demonstrates the correct way to avoid. +Patterns that suggest "if you're doing X, you're doing it wrong" — extracted from studying the Elixir standard library and ExUnit source. -## 1. Using `++` in a Loop (O(n²) List Building) +--- -**Source:** `lib/elixir/lib/enum.ex:306-320` (internal macros all use prepend) - -```elixir -# What the source does: prepend then reverse -defmacrop next(_, entry, acc) do - quote(do: [unquote(entry) | unquote(acc)]) -end -``` +## 1. Using `Process.sleep` Instead of Message-Based Synchronization **The mistake:** ```elixir -# O(n²) — copies the entire left list for every element -Enum.reduce(items, [], fn item, acc -> acc ++ [transform(item)] end) +test "process sends response" do + pid = spawn(fn -> send(test_pid, :done) end) + Process.sleep(50) # Hope 50ms is enough... + assert_received :done +end ``` +**Why it's wrong:** The test is a race condition. It might pass locally but fail on CI. The sleep is arbitrary. + **The fix:** ```elixir -# O(n) — prepend is O(1), reverse once at the end -items |> Enum.map(&transform/1) -# or -Enum.reduce(items, [], fn item, acc -> [transform(item) | acc] end) |> Enum.reverse() -``` - ---- - -## 2. Forgetting `@impl true` - -**Source:** `lib/elixir/lib/gen_server.ex:44-56` (every callback uses @impl) - -**The mistake:** -```elixir -defmodule MyServer do - use GenServer - - # Typo! This will never be called — no warning without @impl - def handle_cll(msg, _from, state) do - {:reply, msg, state} - end +test "process sends response" do + pid = spawn(fn -> send(test_pid, :done) end) + assert_receive :done, 1000 # Explicit timeout, proper wait end ``` -**The fix:** +**Source:** Elixir's test suite has 39 `assert_receive`/`refute_receive` calls in `task_test.exs` alone, vs 0 `Process.sleep(N)` for synchronization. + +--- + +## 2. Not Using `start_supervised` in Tests + +**The mistake:** ```elixir -@impl true -def handle_call(msg, _from, state) do - {:reply, msg, state} +setup do + {:ok, pid} = MyGenServer.start_link(name: :my_server) + on_exit(fn -> GenServer.stop(pid) end) + %{pid: pid} end ``` -With `@impl true`, the compiler catches the typo at compile time. - ---- - -## 3. Not Handling All `with` Failure Cases - -**Source:** `lib/elixir/lib/kernel/special_forms.ex:1680-1715` (with Beware! section) - -**The mistake:** -```elixir -with {:ok, width} <- Map.fetch(opts, "width"), - {:ok, height} <- Map.fetch(opts, "height") do - {:ok, width * height} -else - # Only handles one case — what if Map.fetch returns something else? - :error -> {:error, :missing_field} -end -``` - -If an `else` block is used and no clause matches, a `WithClauseError` is raised. - -**The fix:** Either handle all possible non-match values in `else`, or better yet, normalize return values in helper functions so you don't need `else` at all. - ---- - -## 4. async Without await - -**Source:** `lib/elixir/lib/task.ex:38-40` - -> If you are using async tasks, you **must await** a reply as they are *always* sent. - -**The mistake:** -```elixir -# Leaked reference — message sits in mailbox forever -Task.async(fn -> send_email(user) end) -# Never awaited! -``` - -**The fix:** -```elixir -# Fire-and-forget: use start_child -Task.Supervisor.start_child(MyApp.TaskSupervisor, fn -> send_email(user) end) - -# OR if you need the result: -task = Task.async(fn -> send_email(user) end) -Task.await(task) -``` - ---- - -## 5. Anonymous Functions in Distributed Agents - -**Source:** `lib/elixir/lib/agent.ex:141-160` ("A word on distributed agents") - -> In a distributed setup with multiple nodes, the API that accepts anonymous -> functions only works if the caller (client) and the agent have the same -> version of the caller module. - -**The mistake:** -```elixir -# Fails if nodes have different code versions -Agent.get({MyAgent, :remote@node}, fn state -> state.count end) -``` - -**The fix:** -```elixir -# Use MFA (module, function, args) for distributed calls -Agent.get({MyAgent, :remote@node}, MyModule, :get_count, []) -``` - ---- - -## 6. Starting Processes Outside Supervision Trees - -**Source:** `lib/elixir/lib/task.ex:100-115` - -> We encourage developers to rely on supervised tasks as much as possible. - -**The mistake:** -```elixir -# No supervision, no monitoring, no logging -spawn(fn -> do_important_work() end) - -# Or: -Task.async(fn -> do_important_work() end) |> Task.await() -# Linked to caller but not supervised -``` - -**The fix:** -```elixir -# Add to your supervision tree: -{Task.Supervisor, name: MyApp.TaskSupervisor} - -# Then use it: -Task.Supervisor.start_child(MyApp.TaskSupervisor, fn -> do_important_work() end) -``` - ---- - -## 7. Putting State Logic in Controllers - -**Source:** `lib/phoenix/controller.ex:28-45` - -Controllers are shown as thin dispatch layers: -```elixir -def show(conn, %{"id" => id}) do - user = Repo.get(User, id) - render(conn, :show, user: user) -end -``` - -**The mistake:** -```elixir -def create(conn, params) do - # Business logic in the controller - changeset = User.changeset(%User{}, params) - if changeset.valid? do - user = Repo.insert!(changeset) - send_welcome_email(user) - update_analytics(user) - notify_admin(user) - render(conn, :show, user: user) - else - render(conn, :error, errors: changeset.errors) - end -end -``` - -**The fix:** Move business logic to a context module. The controller just dispatches: -```elixir -def create(conn, params) do - case Accounts.register_user(params) do - {:ok, user} -> render(conn, :show, user: user) - {:error, changeset} -> render(conn, :error, errors: changeset.errors) - end -end -``` - ---- - -## 8. Using `:permanent` Restart for One-Shot Tasks - -**Source:** `lib/elixir/lib/task.ex:178-186` - -> a Task has a default `:restart` of `:temporary`. This means the task will -> not be restarted even if it crashes. - -**The mistake:** -```elixir -# Will restart infinitely if the HTTP call keeps failing -use Task, restart: :permanent - -def start_link(url) do - Task.start_link(fn -> HTTP.get!(url) end) -end -``` - -**The fix:** Use `:temporary` (default) for one-shot work. Use `:transient` if you want restart only on abnormal exit: -```elixir -use Task, restart: :transient -``` - ---- - -## 9. Pattern Matching the Internals of Opaque Types - -**Source:** `lib/elixir/lib/task.ex:298-300` - -```elixir -@opaque ref :: reference() -``` - -**The mistake:** -```elixir -# Accessing internal structure of an opaque type -%Task{ref: ref} = task -send(ref, :custom_message) # This breaks if internals change -``` - -**The fix:** Use the public API. If a type is `@opaque`, its structure is not guaranteed between versions. Use functions like `Task.await/2` that work with the type properly. - ---- - -## 10. Not Using `on_exit` for Test Cleanup - -**Source:** `lib/ex_unit/lib/ex_unit/case.ex:86-94` - -**The mistake:** -```elixir -test "writes to file" do - File.write!("/tmp/test_file", "data") - assert File.read!("/tmp/test_file") == "data" - File.rm!("/tmp/test_file") # Never runs if assert above fails! -end -``` +**Why it's wrong:** If the process crashes during the test, `on_exit` will try to stop an already-dead process. If `start_link` links to the test process, a crash kills the test before cleanup. **The fix:** ```elixir setup do - path = "/tmp/test_file_#{System.unique_integer()}" - on_exit(fn -> File.rm(path) end) - {:ok, path: path} -end - -test "writes to file", %{path: path} do - File.write!(path, "data") - assert File.read!(path) == "data" - # Cleanup happens automatically, even on failure + pid = start_supervised!(MyGenServer) + %{pid: pid} end ``` + +**Source:** `lib/ex_unit/lib/ex_unit/callbacks.ex:277-340` — `start_supervised` is designed specifically for this: guaranteed shutdown in reverse order, no leaked processes, no race conditions. + +--- + +## 3. Asserting Exact Equality on Concurrent/Shared Output + +**The mistake:** +```elixir +# In an async test +test "logs error message" do + assert capture_io(:stderr, fn -> Logger.error("oops") end) == "[error] oops\n" +end +``` + +**Why it's wrong:** With `async: true`, other tests may write to `:stderr` simultaneously. The captured output may include their messages. + +**The fix:** +```elixir +test "logs error message" do + assert capture_io(:stderr, fn -> Logger.error("oops") end) =~ "oops" +end +``` + +**Source:** `lib/ex_unit/lib/ex_unit/capture_io.ex` docs explicitly warn: "use `=~` instead of `==` for assertions on `:stderr` if your tests are async" + +--- + +## 4. Registering Global Names in Async Tests + +**The mistake:** +```elixir +defmodule MyTest do + use ExUnit.Case, async: true + + test "starts a server" do + {:ok, _} = GenServer.start_link(MyServer, [], name: :my_server) + # Another test instance might try to register the same name! + end +end +``` + +**Why it's wrong:** Registered names are global. Two concurrent test runs will collide on `:my_server`. + +**The fix:** +```elixir +test "starts a server", %{test: test_name} do + {:ok, _} = GenServer.start_link(MyServer, [], name: test_name) + # test_name is unique per test +end +``` + +**Source:** `lib/elixir/test/elixir/registry_test.exs:28` — `name = :"#{config.test}_#{partitions}_#{inspect(keys)}"` — always derives unique names from test context. + +--- + +## 5. Testing Private Functions Directly + +**The mistake:** +```elixir +# Exposing private implementation for testing +@doc false +def __internal_transform__(data), do: ... + +# In test: +test "internal transform works" do + assert MyModule.__internal_transform__(%{}) == %{transformed: true} +end +``` + +**Why it's wrong:** Tests become coupled to implementation. You can't refactor without breaking tests. The public API is the contract. + +**The fix:** Test through the public interface. If a private function is complex enough to need its own tests, it should probably be its own module. + +**Source:** The Elixir test suite tests public APIs exclusively. `gen_server_test.exs` tests `GenServer.call/cast/stop` — never the internal `handle_*` callbacks directly. + +--- + +## 6. Not Cleaning Up After Global State Changes + +**The mistake:** +```elixir +test "changes log level" do + Logger.configure(level: :error) + # ... test stuff ... + # Oops, forgot to restore! All subsequent tests have wrong log level. +end +``` + +**Why it's wrong:** Contaminates the test environment for all subsequent tests. Causes mysterious failures in unrelated tests. + +**The fix:** +```elixir +test "changes log level" do + Logger.configure(level: :error) + # ... test stuff ... +after + Logger.configure(level: :debug) +end + +# Or better, use on_exit in setup: +setup do + on_exit(fn -> Logger.configure(level: :debug) end) +end +``` + +**Source:** `lib/logger/test/logger_test.exs:12-17` — Every Logger config change has a corresponding `on_exit` restoration. `lib/logger/test/test_helper.exs:57-62` — `capture_log` uses `after` to always restore level. + +--- + +## 7. Nested `describe` Blocks + +**The mistake:** +```elixir +describe "users" do + describe "admin users" do # This won't compile! + test "can delete" do + end + end +end +``` + +**Why it's wrong:** ExUnit explicitly prevents nested describe blocks. The framework raises: "cannot call describe inside another describe." + +**The fix:** Use flat describe blocks with descriptive names, or prefix test names: +```elixir +describe "admin users - deletion" do + test "can delete other users" do + end +end +``` + +**Source:** `lib/ex_unit/lib/ex_unit/callbacks.ex:423-425` — `no_describe!` check prevents nesting. + +--- + +## 8. Using `assert` Where `assert_receive` Belongs + +**The mistake:** +```elixir +test "message is sent" do + send(self(), {:result, 42}) + assert {:result, 42} in Process.info(self(), :messages) |> elem(1) +end +``` + +**Why it's wrong:** Reinvents the wheel poorly. Doesn't wait for async messages. No pattern matching. Bad failure messages. + +**The fix:** +```elixir +test "message is sent" do + send(self(), {:result, 42}) + assert_received {:result, 42} +end +``` + +**Source:** ExUnit provides specialized assertion macros for messages precisely because generic `assert` is inadequate for mailbox testing. + +--- + +## 9. Forgetting `Process.flag(:trap_exit, true)` When Testing Linked Processes + +**The mistake:** +```elixir +test "handles process crash" do + task = Task.async(fn -> raise "boom" end) + # Test process crashes because it's linked to the task! + assert catch_exit(Task.await(task)) +end +``` + +**Why it's wrong:** Without trapping exits, the linked process crash kills the test process before `catch_exit` can catch anything. + +**The fix:** +```elixir +test "handles process crash" do + Process.flag(:trap_exit, true) + task = Task.async(fn -> raise "boom" end) + assert {{%RuntimeError{}, _}, _} = catch_exit(Task.await(task)) +end +``` + +**Source:** `lib/elixir/test/elixir/task_test.exs:297,305,315,330` — Every test that expects a linked process to crash sets `:trap_exit` first. + +--- + +## 10. Writing Flaky Tests with Timing Assumptions + +**The mistake:** +```elixir +test "debounce fires after 100ms" do + start_debounce(:my_action, 100) + Process.sleep(110) # "should be enough" + assert_received :my_action +end +``` + +**Why it's wrong:** System load, GC pauses, or CI resource contention can make 110ms insufficient. The test will flake. + +**The fix:** Use generous timeouts with `assert_receive`: +```elixir +test "debounce fires after delay" do + start_debounce(:my_action, 100) + assert_receive :my_action, 1000 # generous timeout, still fast on success +end +``` + +Or better, make the system under test notify completion: +```elixir +test "debounce fires callback" do + start_debounce(fn -> send(self(), :fired) end, 100) + assert_receive :fired, 1000 +end +``` + +**Source:** Elixir's own `ExUnit.configure` allows setting `assert_receive_timeout` globally (default 100ms, CI uses 300ms via env var `ELIXIR_ASSERT_TIMEOUT`). + +--- + +## 11. Not Using `=~` for Regex/Partial Matching + +**The mistake:** +```elixir +test "error message" do + {:error, msg} = do_thing() + assert msg == "failed to connect to localhost:5432 (connection refused)" +end +``` + +**Why it's wrong:** The message might include timestamps, PIDs, or other dynamic content. Any format change breaks the test. + +**The fix:** +```elixir +test "error message" do + {:error, msg} = do_thing() + assert msg =~ "connection refused" + # Or with regex: + assert msg =~ ~r/failed to connect to .+:\d+/ +end +``` + +**Source:** `lib/elixir/test/elixir/gen_server_test.exs:70-82` uses `~r"expected :name option to be one of the following:"` in `assert_raise` — testing the stable part, ignoring the dynamic rest. + +--- + +## 12. Relying on Process.alive? Without Synchronization + +**The mistake:** +```elixir +test "process stops" do + GenServer.stop(pid) + refute Process.alive?(pid) # Race condition! +end +``` + +**Why it's wrong:** `GenServer.stop` is synchronous for the server's exit, but the `Process.alive?` check and :DOWN signal delivery have a timing gap. + +**The fix:** +```elixir +test "process stops" do + ref = Process.monitor(pid) + GenServer.stop(pid) + assert_receive {:DOWN, ^ref, :process, ^pid, :normal} +end +``` + +**Source:** `lib/elixir/test/elixir/supervisor_test.exs:278-285` — `assert_kill` helper always uses monitor + assert_receive, never `Process.alive?` polling. + +--- + +## 13. Using `== ""` for Empty Capture Assertions in Async Tests + +**The mistake:** +```elixir +# In an async test +test "no output on success" do + assert capture_io(:stderr, fn -> do_quiet_thing() end) == "" +end +``` + +**Why it's wrong:** Another async test might write to stderr during your capture window, making your capture non-empty. + +**The fix:** Either make the test synchronous, or don't assert emptiness on shared devices: +```elixir +# Use :stdio (group leader) which is per-process and safe +test "no output on success" do + assert capture_io(fn -> do_quiet_thing() end) == "" +end +``` + +**Source:** `lib/ex_unit/lib/ex_unit/capture_io.ex` docs: "avoid empty captures on `:stderr` with async tests" + +--- + +## 14. Overriding ExUnit Reserved Tags + +**The mistake:** +```elixir +@tag test: "my_custom_value" # Overwrites ExUnit's :test tag! +``` + +**Why it's wrong:** ExUnit reserves certain context keys (`:case`, `:file`, `:line`, `:test`, `:async`, `:registered`, `:describe`). Overriding them breaks ExUnit internals. + +**The fix:** Use your own tag names that don't conflict: +```elixir +@tag test_type: "integration" +``` + +**Source:** `lib/ex_unit/lib/ex_unit/callbacks.ex` — ExUnit raises if you try to set reserved keys to different values in setup. + +--- + +## 15. Complex Conditional Logic in Tests + +**The mistake:** +```elixir +test "handles all cases" do + for input <- [:a, :b, :c] do + result = process(input) + if input == :a do + assert result == 1 + else + if input == :b do + assert result == 2 + else + assert result == 3 + end + end + end +end +``` + +**Why it's wrong:** When this test fails, you don't know which case failed. The logic is harder to read than separate tests. Conditionals in tests suggest you're testing multiple behaviors in one test. + +**The fix:** Separate tests for separate behaviors, or use parameterize: +```elixir +# Option 1: Separate tests +test "processes :a" do + assert process(:a) == 1 +end + +test "processes :b" do + assert process(:b) == 2 +end + +# Option 2: Parameterize (Elixir 1.18+) +use ExUnit.Case, parameterize: [ + %{input: :a, expected: 1}, + %{input: :b, expected: 2}, + %{input: :c, expected: 3} +] + +test "processes input", %{input: input, expected: expected} do + assert process(input) == expected +end +``` + +**Source:** ExUnit case.ex docs: "If you use parameterized tests and then find yourself adding conditionals in your tests to deal with different parameters, then parameterized tests may be the wrong solution."