elixir-patterns/smells/common-mistakes.md

Common Mistakes in Elixir (What the Core Team Avoids)

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 Process.sleep Instead of Message-Based Synchronization

The mistake:

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:

test "process sends response" do
  pid = spawn(fn -> send(test_pid, :done) end)
  assert_receive :done, 1000  # Explicit timeout, proper wait
end

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:

setup do
  {:ok, pid} = MyGenServer.start_link(name: :my_server)
  on_exit(fn -> GenServer.stop(pid) end)
  %{pid: pid}
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:

setup do
  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:

# 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:

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:

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:

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:

# 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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

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:

# 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:

# 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:

@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:

@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:

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:

# 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."