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

Sentinel: red:01m23vygyffx5hg2f5vm8r74sr was degraded 142 minutes today: trevato/forge: main is red at ea64226 — test-shards: over 45 s: test/crew.test.ts (52.5s); 8 pull checks wait for it to… #2378

Merged opened this

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

Merged

I split `test/crew.test.ts` into four files so the 45 s test-shard limit stops tripping on it. I ran all four files together here and they pass (12 tests, about 27 s), and `tsc` is clean. The shard timing itself is untested; the forge's check on this push is the real test.

shipped production is serving it deploy 28f939d

opened this

I split test/crew.test.ts into four files so the 45 s test-shard limit stops tripping on it. I ran all four files together here and they pass (12 tests, about 27 s), and tsc is clean. The shard timing itself is untested; the forge's check on this push is the real test.

Alone, the old file took about 15 s here; the 52.5 s was likely load on the shard. Its tests share one forge and run one after another, and three of them each spend 4–5 s on the stand-in crew's git work.

  • test/crew.test.ts keeps the stand-in check and the assignment test (clone, branch, brief, pull), about 6 s.
  • test/crew-feedback.test.ts assigns the issue in its own setup, then runs the feedback-resumes-the-session test.
  • test/crew-closed.test.ts assigns in its own setup, then runs the close-removes-the-clone test and the closed-without-merge follow-up test.
  • test/crew-paths.test.ts holds the pure stripForgeDirs and symlink tests, which never needed the forge.

No assertions changed. Each of the new feedback and closed files repeats the assignment as setup, which is about 5 s of duplicated work spread across parallel shards. Splitting this way follows the convention in the file's own header.

I also set estimated durations for the new files in scripts/test-times.json; they are my estimates, not measured shard times. I left some imports in the new files unused, and tsc doesn't flag them.

To try it: bun test test/crew.test.ts test/crew-feedback.test.ts test/crew-closed.test.ts test/crew-paths.test.ts.

Closes #2375

commented

I split test/crew.test.ts into four files so the 45 s test-shard limit stops tripping on it. I ran all four files together here and they pass (12 tests, about 27 s), and tsc is clean. The shard timing itself is untested; the forge's check on this push is the real test.

Alone, the old file took about 15 s here; the 52.5 s was likely load on the shard. Its tests share one forge and run one after another, and three of them each spend 4–5 s on the stand-in crew's git work.

  • test/crew.test.ts keeps the stand-in check and the assignment test (clone, branch, brief, pull), about 6 s.
  • test/crew-feedback.test.ts assigns the issue in its own setup, then runs the feedback-resumes-the-session test.
  • test/crew-closed.test.ts assigns in its own setup, then runs the close-removes-the-clone test and the closed-without-merge follow-up test.
  • test/crew-paths.test.ts holds the pure stripForgeDirs and symlink tests, which never needed the forge.

No assertions changed. Each of the new feedback and closed files repeats the assignment as setup, which is about 5 s of duplicated work spread across parallel shards. Splitting this way follows the convention in the file's own header.

I also set estimated durations for the new files in scripts/test-times.json; they are my estimates, not measured shard times. I left some imports in the new files unused, and tsc doesn't flag them.

To try it: bun test test/crew.test.ts test/crew-feedback.test.ts test/crew-closed.test.ts test/crew-paths.test.ts.

Changes requested — before the latest push

The split is sound and follows the file header convention, and the assertions are unchanged. But the new files ship dead code, which the list counts as a miss: crew-closed and crew-feedback each copy the whole import block of crew.test.ts, and most of it is unused (the author said so). Fix first: delete every unused import and unused destructured variable (servers, crew, argsFile/promptFile where not read, etc.) in crew-closed.test.ts, crew-feedback.test.ts and crew-paths.test.ts, and in crew.test.ts now that stripForgeDirs, the feedback and the closing tests have left it (mkdtemp, rm, tmpdir, symlinkSync, spellings and others). Also tidy the stray blank lines and the unspaced destructuring { dataDir,app,... }. Also: afterAll and stopStandInCrew are imported but the new describes never stop the stand-in forge, so each file leaks a server and temp dir until process exit; add the afterAll teardown as crew.test.ts does. The test-times.json values are guesses (crew-paths 0.1, others rounded); acceptable only if the pull says so, ideally replace with measured times after the first shard run. Tests-only change, so no page or door is affected.

commented

test/crew.test.ts is now split into four files so the 45 s test-shard limit stops tripping on it. The four files pass together (12 tests), tsc --noUnusedLocals is clean on all four, and the forge's check on this push will run the shard timing.

  • test/crew.test.ts: the stand-in check and the assignment test, about 6 s.
  • test/crew-feedback.test.ts: assigns in its own setup, then the feedback-resumes-the-session test.
  • test/crew-closed.test.ts: assigns in its own setup, then the clone-removed-on-close test and the follow-up on a pull closed without a merge.
  • test/crew-paths.test.ts: the pure stripForgeDirs and symlink tests.

Assertions are unchanged. Each of the feedback and closed files repeats the assignment as setup, about 5 s of duplicated work spread across parallel shards.

In review round 1 (@juniper):

  • Dead code: I removed every unused import and unused destructured variable from all four files. I also dropped the issueNumber variable from the feedback file, since nothing read it.
  • Teardown: the feedback and closed files now call afterAll(() => stopStandInCrew(standIn)), so each stops its forge and temp dir.
  • Style: the unspaced destructuring in crew.test.ts is now spaced; the stray blank lines are gone.

The scripts/test-times.json entries for the new files are my estimates, not measured times: closed 10, feedback 11, paths 0.1, crew.test.ts 7. I haven't replaced them because I can't see shard timings from here; the first shard run on the forge will give measured ones.

I did not run Prettier. It flags 21 existing test files, so it isn't enforced in this repository.

Approved

Test-only split of crew.test.ts into four files; assertions moved unchanged, the pure path tests no longer pay for a forge, and each new file tears down its own stand-in crew. No product surface changes, so no security, terminal or README impact. Next: (1) replace the estimated entries in scripts/test-times.json with measured shard times after the first run (closed 10, feedback 11, paths 0.1, crew 7 are guesses). (2) crew.test.ts header still says it covers the pull being fed back on and closed; reword it to say the assignment only, since the later steps now live in the sibling files.

Approved

Test-only split of crew.test.ts into four files, following the file header convention. The assertions move unchanged. I ran tsc --noUnusedLocals: no unused imports in the four files. Each new forge stops in afterAll. The preview has no UI change, so there is nothing to look at. Two things I would do next: (1) replace the estimated entries in scripts/test-times.json with measured shard times after the first run. (2) Consider sharing the assignment setup, which feedback and closed now duplicate (about 5 s each), through a helper in helpers/crew-standin.ts.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs