Fixing Flaky Failures

Fixing flaky failures

Covers diagnosing the real mechanism, attributing a flake to a change, this repo's flaky: retry harness, and the known false-flake causes — a missing Tailwind build, the shared Redis autocomplete cache, Turbo frame timing, probe-run interference.

The rule that overrides everything else here

You may not reduce test coverage to make a flaky test pass. Not as a last resort, not "temporarily", not when the coverage looks redundant, not when you have already decided the failing step is a harness artifact rather than a real bug.

All of these are reducing coverage, and none of them is a fix:

• Deleting an assertion, a step, a helper call, or a whole example.
• Loosening a matcher so it can't fail (have_css(count: 2) → have_css, an exact string → a regex, have_no_content → nothing).
• skip/pending/xit, or excluding the spec from a CI shard.

A flaky test is a reporting problem — the suite is telling you something real and telling you unreliably. Every item above changes the reporting and leaves the underlying behaviour untested.

If the only fix you can see requires giving up coverage, that is a decision for the user — describe what you'd have to give up and ask. Do not make that trade yourself and mention it in the summary afterwards; announcing a scope reduction is not the same as getting agreement for it.

The one exception that isn't an exception: if the assertion is wrong — it asserts behaviour the app never promised — then fixing it is correcting a bad test, not reducing coverage. Say explicitly why it was wrong.

Retries and longer waits: allowed, but earn them first

:flaky (and raising an existing flaky: N) and a bigger wait: are a different category from the list above, because they keep every assertion and still require it to pass. Nothing goes untested. They're a legitimate last resort — reach for them when the diagnosis below has genuinely run out, not as the first thing you try.

What "earn them" means in practice:

• Diagnose first. Instrument, read the failure literally, check the known causes below. Most flakes here have a mechanism you can find in one focused pass, and a retry over a findable cause just makes it intermittent for longer.
• Leave a comment saying what you found and why the retry stands in for a fix — the harness artifact, the contention, the thing you ruled out. The existing flaky: 4 on search_registrations_spec is the pattern: it names the click landing on about:blank, lists what it ruled out, and says it went unreproduced under local CPU throttling. A bare flaky: true with no comment tells the next person nothing and will outlive the problem.
• Say plainly in your summary that you papered over it rather than fixed it, so the user can decide whether that's good enough.

Two signals that a retry is the wrong answer even as a last resort: an example that fails through all its retries (the cause is structural, and a bigger N won't help), and a wait you can't say what it's waiting for. A bump is honest when the assertion starts before the work does — a held route released, a job enqueued, an expensive response — and the comment names that.

Diagnose before you touch anything

1. Get the real failure, not the summary

gh run view <run-id> --repo bikeindex/bike_index --json jobs \
  --jq '.jobs[] | "\(.databaseId) \(.name) \(.conclusion)"'
gh run view --repo bikeindex/bike_index --job <job-id> --log-failed

The log is large and ANSI-coloured; pipe through sed 's/\x1b\[[0-9;]*m//g' and grep for Failure/Error, expected, and rspec ./spec/... to get the failing example ids and the actual message.

Then download the Capybara screenshot. Every :js failure writes one, and ci.yml uploads tmp/capybara/ to the shard's test-results-<node-index> artifact for exactly this — but the log names the file without saying it's fetchable, so it usually goes unread. It shows the page the failure saw, which routinely settles a mechanism the log can only hint at: what a click actually landed on, a frame still loading, a control in a state no step in the example set.

gh api repos/bikeindex/bike_index/actions/runs/<run-id>/artifacts \
  --jq '.artifacts[] | "\(.id) \(.name)"'
gh api repos/bikeindex/bike_index/actions/artifacts/<id>/zip > tmp/a.zip && unzip -o tmp/a.zip -d tmp/ci_artifact

2. Read the message literally

The exact failure text usually names the mechanism, and it is easy to skim past into a wrong assumption. Worked example from this repo: expected nil to match /\/bikes\/\d+/ was long assumed to mean "the click was lost". It doesn't — a nil current_path means the URL had no path for Capybara to return, which is three different browser states, not one (capybara/session.rb:207: nil for an about: scheme, then path unless path&.empty?):

All three screenshot blank, so the picture can't tell them apart — tmp/capybara/browser_events.log (written by spec/support/capybara.rb, uploaded with the screenshots) can. Check the matcher's source when a message is surprising, and don't read one of these three as another: the fix differs, and "about:blank" has been the standing wrong guess.

3. Instrument rather than theorise

When you can't reproduce, you can still make the browser tell you what happened. Copy the spec to a scratch file, record the events the app actually emits, and run it — a log beats an argument, and this routinely overturns the theory you were about to ship:

page.execute_script(<<~JS)
  window.__events = []
  const stamp = (name, extra) => window.__events.push(Object.assign({name, t: Math.round(performance.now())}, extra))
  document.addEventListener("turbo:before-fetch-response", (e) => {
    stamp("response", {target: e.target?.id, url: e.detail?.fetchResponse?.response?.url, prevented: e.defaultPrevented})
  })
JS
# ...drive the spec...
# A file, not puts - rtk's rspec wrapper reports a summary and drops the run's stdout
File.open("tmp/probe.log", "a") { |f| page.evaluate_script("window.__events").each { |e| f.puts e.inspect } }

Parameterise the scratch spec over the variable you suspect ([0, 8].each do |delay| around an injected sleep) so one run compares the fast and slow paths. Delete the scratch file when you're done. Two things this buys that reasoning doesn't: it distinguishes "the event never arrived" from "the event arrived and the assertion misread it", and it tells you when things happened, which is usually the answer.

4. Try to reproduce, and treat failure-to-reproduce as information

for i in 1 2 3; do bundle exec rspec <the spec file> 2>&1 | grep -E "examples, " | tail -1; done

Keep that loop on the one spec file — escalating it to bin/ci costs minutes of parallel workers and browsers per iteration, and answers the same question no better.

Green locally three times doesn't mean "not reproducible, add a retry". It narrows the cause to something CI has and you don't: contention (browser, Rails and Postgres sharing one runner) or ordering (a different seed, knapsack handing this shard a different set of files, or state left by another example). Reason about which, then look for the mechanism.

For contention, slow the renderer rather than the machine — CPU hogs slow the Ruby side too, so a loop of runs takes minutes and the extra load is spent where the race isn't, and one backgrounded from a non-interactive shell survives kill $(jobs -p) — pgrep -f it. CDP throttles the browser alone, and the driver hands you a session:

page.driver.with_playwright_page do |playwright_page|
  session = playwright_page.context.new_cdp_session(playwright_page)
  session.send_message("Emulation.setCPUThrottlingRate", params: {rate: 6})
end

A rate that leaves the spec green over ~20 runs is evidence, not proof: it stretches main-thread work, not the network or a parallel shard's I/O.

Which is why it can't reach a race about when a response arrives. The common one here is a lazily loaded Stimulus controller, since the module is a fetch — hold it on the route and the late connect is deterministic, no loop:

playwright_page.route(%r{serial_controller}, ->(route, request) { held << request.url; sleep 1; route.continue })

Registering a route disables the http cache, so the reload asks again. Assert on what the handler held: a route that stops matching (a moved asset path) otherwise leaves the example green on a page that held nothing back. Measured against register--serial's connect-time reconcile, throttling at 6, 12 and 25 left it green over 30+ runs; the route hold failed it every time.

A sleep in the handler is still a race — it has to outlast whatever else the page is doing, and the duration that wins locally is not the one that wins on a loaded CI runner. When the spec can observe the state the module must arrive after, block the handler on a Queue and release it from the example instead:

playwright_page.context.route(%r{parking_notification_form_controller}, proc { |route, request|
  held << request.url
  release.pop
  route.continue
})
# ...only the accordion can reveal the panel, so this is it having already opened
expect(page).to have_content("Set on map", wait: 10)
release << :continue

Blocking the handler doesn't stall the driver, so Capybara still polls while it waits.

Caveat when measuring locally: after a heavy record-creating run (seeding, probe scripts, a big suite), :js specs fail spuriously for a while. Re-measure in a quiet environment before concluding a spec is flaky.

5. Blaming your own change needs both arms measured together

"Is this spec flaky?" and "did my change make it flaky?" are different questions. The second one is where sequential sampling lies to you: local load drifts — a browser left open, another suite, the machine waking up — so a sample taken now and one taken an hour ago aren't comparable, and whichever arm ran while things were busy looks guilty.

Revert only the suspect change and run both arms the same number of times, back to back, then compare. Measured that way here, a patch blamed for :js failures on sequential samples (0 failures in 9 clean runs against 5 in 13 patched ones) came out at 2/6 versus the baseline's 1/6 — indistinguishable, and the spec was flaky on its own. Sample sizes this small can't separate a 17% failure rate from a 7% one, so treat a handful of green runs as weak evidence in either direction.

Ruling ordering out is cheap and worth doing first: RSpec prints Randomized with seed N, and --seed N replays that order. A failing seed that passes on replay leaves timing, not ordering or leaked state.

Measure the "without the fix" arm against the base ref by name — git checkout origin/main -- <paths>. Once the fix is committed, git checkout -- <paths> restores it, so the arm you think is unpatched is the patched one, and a regression test that does fail without the fix reads as passing.