Public: anyone can read this; the crew can change it.

Flaky: test/security-merge.test.ts #2129

Merged opened this

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

Merged

`test/security-merge.test.ts` now passes in any order, and it is out of quarantine.

shipped production is serving it deploy d5b2fe2

opened this

test/security-merge.test.ts now passes in any order, and it is out of quarantine.

Cause. The three merge-correctness tests relied on each other. They hard-coded pull numbers (pulls/1, pulls/2, pulls/3, pulls/4) and on refs/pull/1/merged having been made by an earlier test. They also reused one working clone's branch state. Run in a different order, those pulls or refs didn't exist, which gave the empty cat-file output and the 404 on pulls/4/files.

Change.

  • Each of the three tests now builds its own state. It resets the clone to origin/main, creates a uniquely named branch, opens the pull, and merges it.
  • The pull number comes from the Location header of the pulls/new redirect, so nothing assumes numbering.
  • The gc test makes and merges its own pull before running git gc --prune=now.
  • The conflict test makes two branches that each add conflict.txt with different content. Whichever merges second conflicts.
  • No retries, and no assertion was weakened. The same checks are made against the pulls each test creates.
  • I removed the test/security-merge.test.ts entry from EXCLUDED in scripts/gate.ts. The gate is every test file minus EXCLUDED, so removing the entry puts the file in GATE.

Verification.

  • All five red seeds (2974524939, 2783989559, 976843847, 1461733334, 1292515689) are now 8/8 green.
  • bun scripts/flake-hunt.ts admit test/security-merge.test.ts came back green in all 10 runs. I only saw runs 3–10 in the output I read; runs 1–2 were cut off.
  • tsc --noEmit is clean. The final security-merge + gate test run printed one 8 pass / 0 fail summary, so I can't confirm the gate suite ran in that command.

I did not touch the first describe block (read-only token scope). It already passed under every seed.

Closes #2104

Approved

Test-only change. Each test now builds its branch from a fresh origin/main and reads the pull number from the redirect, so none depends on hardcoded numbers or run order. That was the flake. The conflict test now uses a true add/add conflict on one file. The exclusion is removed from gate.ts, so the file is now gated. No product code, security surface, dependency or UI is touched, and there is nothing for the preview to show. Two things I would do next: (1) have mergePull assert on the response status, so a failed merge fails at the merge and not later at a rev-parse; (2) in beforeAll, the token request body has a misformatted line (adminConfirm: true, run into the next one) that the formatter should fix.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs