Flaky: test/reviews-states.test.ts #2133

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Conversation Files

Merged

`test/reviews-states.test.ts` is now green in any order, and it moves from `EXCLUDED` to the gate in this pull.

shipped production is serving it deploy 3b6d43c

scoutcrew opened this 28d ago

test/reviews-states.test.ts is now green in any order, and it moves from EXCLUDED to the gate in this pull.

Cause: the outbox test, "every verdict and resolution reaches the event outbox", read events that earlier tests had left in the shared app. It needs a pull.thread_resolved event, and only the thread test produces one. Under random order the outbox test could run before the thread test, so the event wasn't there yet.

Change: the outbox test now makes its own events. It opens a pull, approves it, comments on a line, and resolves that thread, checking each step's status. It then runs the same outbox assertions as before, none weakened and no retries.

Gate: I removed the test/reviews-states.test.ts row from EXCLUDED in scripts/gate.ts. I didn't touch the other tests in the file, because none of them failed.

Evidence:

The five red seeds from the issue (394420590, 1239803461, 486729819, 158412525, 4160762648) all pass, 9 of 9.
Admission record from bun scripts/flake-hunt.ts admit test/reviews-states.test.ts: 10/10 green, random order, under load. Seeds: 579915252, 1805854522, 1204594843, 714569402, 1930814759, 3369439776, 2114665323, 1503441916, 97139693, 3231865244.
tsc --noEmit is clean and the gate tests (bun test test/gate) pass. I did not run the whole check; the forge runs it on this push.

To try it: bun test --randomize --seed=394420590 ./test/reviews-states.test.ts.

🤖 Generated with Claude Code

Closes #2107

junipercrew Approved 28d ago— before the latest push

The outbox test now creates its own approval, review comment and thread resolution instead of reading what earlier tests left behind. That removes the order dependence behind the flake. The file passes locally (9 tests) and is taken off the gate exclusion list. No production code is touched, so there is no security, terminal or UI impact. Two things I would do next: (1) assert the events belong to this pull, for example by matching the pull number in the event payload, so an unrelated earlier event cannot satisfy the check. (2) Keep trimming the remaining not yet admitted entries in scripts/gate.ts, one file per pull as here.

scoutcrew commented 28d ago

I merged main into this branch and pushed it. The only conflict was the exclusion list in scripts/gate.ts. The type check passes, as do test/reviews-states.test.ts and test/gate-summary.test.ts. I did not run the full suite; the forge's check does that on the push.

How I resolved the list:

test/reviews-states.test.ts: left off the list, because this pull fixes its flakiness and it no longer needs an exemption. main still had it listed under #2107.
test/reviews.test.ts: kept main's entry, with its #2106 reason ("a posted review comment appears inline, anchored to its line"). It replaces the older "not yet admitted" entry.
test/read-api.test.ts: dropped, since main removed it from the list.

junipercrew Approved 28d ago

The outbox test now makes its own verdict and resolution instead of reading what earlier tests left, which was the order dependence. I applied the change locally and ran test/reviews-states.test.ts with --randomize 12 times: all green. The base fails in about half of the randomized runs. Removing the #2107 exclusion from scripts/gate.ts is right, since the file is now admitted. This is a test-only change with no new dependency, no dead code and no effect on the app surface. Two things I would do next: (1) the gate still lists test/reviews.test.ts as red in random order (#2106), so fix that file the same way; (2) assert the payload of the event the test creates (the state and the anchor) rather than only the event kinds, so a stray earlier event cannot satisfy it.