Tests Guard Intentional Workarounds

Tests Guard Intentional Workarounds

Problem

You observe a production log line that looks clearly wrong — a function being called when its precondition already appears satisfied, a retry firing when the first attempt seemed to work, state being set to the value it already holds. Instinct says "this is a redundant no-op, I'll remove it." You remove it, run local tests (which may even pass if coverage is thin), push to CI, and a test fails with a title that describes exactly the behavior you just removed. The comment on that test explains a subtle platform bug or race condition that required the "redundant" call as a workaround.

The log line you reacted to wasn't evidence of a bug. It was evidence of the workaround executing correctly for the exact scenario it was designed for.

Context / Trigger Conditions

Apply this skill's caution when ANY of these hold:

1. Redundant-looking call: You're about to wrap a call in if (!already_X) or delete it outright because logs show it firing when X is already true.
2. Impossible state in logs: Logs show a state combination that seems logically impossible or contradictory (playing=true + positionMs=0 for seconds, isLoading=false + data still empty, etc.).
3. "Why would anyone write this?": Reading code and you can't imagine a reason for a defensive check, extra await, or secondary call.
4. Simplifying a conditional: A branch looks dead because you believe the condition can't be reached, or both branches look identical to you.
5. Receiving a failing test: CI returns a failure with a test title that names exactly the behavior your change removed or modified.

Solution

Before touching the "redundant" code, run this five-step check:

Step 1: Grep for tests that assert the exact behavior

Search the test suite for the function name, the state you think is redundant, and any synonyms:

# Example: considering removing player.play() on rebuffer complete
grep -rn "player.play" test/ | grep -iE "already|even when|still|twice|redundant"
grep -rn "rebuffer.*play\|playing.*true.*play" test/

Look specifically for test names containing phrases like:

• "even when X" / "even if X"
• "still calls Y when Z"
• "twice on Y" / "idempotent"
• "after Y" (especially "after seek", "after error", "after reconnect")
• "recovers from" / "nudges" / "resumes"
• The specific state word from your log (stalled, frozen, orphan, race)

Step 2: Read the test comment, not just the assertion

If you find a matching test, read the comment above the assertion, not just the expect() call. The comment almost always explains why the otherwise-redundant behavior is required. Look for phrases like:

• "mpv/iOS/Chrome/Safari can X even when Y"
• "nudge", "kick", "wake up", "unstick"
• "workaround for", "due to", "race with"
• Bug tracker references, commit SHAs, PR numbers

Step 3: Correlate with your production log

Re-read the log line that prompted the "fix" through the lens of the test comment. Does the log state match the bug scenario the test describes?

• If the test says "mpv reports playing=true while stalled at positionMs=0" and your log shows playing=true, positionMs=0, you are looking at the bug the workaround exists to handle. The workaround is working.
• If the log state doesn't match the test's stated scenario, you may have found a new bug OR a genuinely redundant branch. Proceed carefully.

Step 4: Preserve behavior, improve observability

If the workaround is load-bearing, don't remove it. Instead:

• Add or improve the log message so future-you understands the intent (nudge_stalled_decoder is clearer than redundant_play).
• Add a code comment referencing the test that guards this behavior (// See test: "rebuffer recovery calls play() even when playing=true").
• If the volume is noisy, consider downgrading the log level or sampling it, but keep the behavior.

Step 5: If CI already caught you, revert and learn

If you've already pushed and CI surfaced the guarding test:

1. Don't "fix" the test to match the new behavior. The test is the spec.
2. Revert just the behavior change — keep any unrelated improvements (logging, comments, etc.).
3. Read the test comment carefully and update your mental model of the system.
4. Commit the revert with a message that captures the lesson so the next engineer reading git blame understands why the "redundant" call is there.