From 94f0f4b14e5e69375cf536376a15a8d8e67e792b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 17 Jul 2026 03:05:59 -0700 Subject: [PATCH] fix(test): stop rebuilding env/keys per iteration in repo-name validation tests test/unit/github-labels.test.ts's "rejects invalid repository names before making GitHub calls" and test/unit/github-pr-actions.test.ts's "validates the repo name before any GitHub call" each called createTestEnv() up to 9 times and generated a fresh RSA-2048 key (generateRsaPrivateKeyPem(), real synchronous CPU work) up to 4 times -- once per malformed-name case in a loop. ensurePullRequestLabel/ closePullRequest reject on parseRepoFullName()/splitRepo() before ever touching `env` (src/github/labels.ts, src/github/pr-actions.ts), so none of that setup was needed more than once per env-shape. Reported as a timeout flake alongside three already-fixed sibling tests (test/unit/ai-summaries.test.ts, test/unit/agent-sdk-driver.test.ts, the #5132 miner clone/worktree suite), but this one didn't reproduce under realistic single-suite-equivalent load (3x clean runs under simulated CPU contention) -- the earlier failure was traced to two full test suites running concurrently on one machine, not a normal-load risk. Fixing the confirmed redundant setup anyway since it's the same safe, zero-behavior-change pattern already proven on the sibling PRs: measured ~20% faster on both files together under sustained load (44.7s/45.4s/40.9s vs a 56.0s/56.4s/54.8s baseline, 3 runs each), all 58 tests still pass, and a full local npm run test:ci pass shows both files fully clean. --- test/unit/github-labels.test.ts | 16 ++++++++++++---- test/unit/github-pr-actions.test.ts | 21 ++++++++++++++------- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/test/unit/github-labels.test.ts b/test/unit/github-labels.test.ts index 4a0a0b5fd3..7f361c440e 100644 --- a/test/unit/github-labels.test.ts +++ b/test/unit/github-labels.test.ts @@ -11,12 +11,19 @@ describe("GitHub PR labels", () => { }); it("rejects invalid repository names before making GitHub calls", async () => { - await expect(ensurePullRequestLabel(createTestEnv(), 123, "invalid", 4, "gittensor", { createMissingLabel: true })).rejects.toThrow(/Invalid repository full name/); - await expect(ensurePullRequestLabel(createTestEnv(), 123, "owner/repo/extra", 4, "gittensor", { createMissingLabel: true })).rejects.toThrow( + // Every malformed name below is rejected by parseRepoFullName() (src/github/labels.ts) before + // ensurePullRequestLabel ever touches `env`, so one shared env per env-shape covers every case + // without rebuilding a fresh in-memory SQLite database (166-migration replay, see + // test/helpers/d1.ts) or generating a fresh RSA-2048 key (real synchronous CPU work) on every + // iteration -- that redundant setup, not the code under test, is what timed this test out under + // concurrent full-suite load. + const env = createTestEnv(); + await expect(ensurePullRequestLabel(env, 123, "invalid", 4, "gittensor", { createMissingLabel: true })).rejects.toThrow(/Invalid repository full name/); + await expect(ensurePullRequestLabel(env, 123, "owner/repo/extra", 4, "gittensor", { createMissingLabel: true })).rejects.toThrow( /Invalid repository full name/, ); for (const padded of [" owner/repo ", "owner/ repo", "owner /repo", "own er/repo"]) { - await expect(ensurePullRequestLabel(createTestEnv(), 123, padded, 4, "gittensor", { createMissingLabel: true })).rejects.toThrow( + await expect(ensurePullRequestLabel(env, 123, padded, 4, "gittensor", { createMissingLabel: true })).rejects.toThrow( /Invalid repository full name/, ); } @@ -25,10 +32,11 @@ describe("GitHub PR labels", () => { called = true; return Response.json({ token: "t" }); }); + const keyedEnv = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }); for (const malformed of ["owner/repo/extra", "owner/ repo", "owner /repo"]) { await expect( ensurePullRequestLabel( - createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), + keyedEnv, 123, malformed, 4, diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 389a632ecf..673b8298ec 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -14,16 +14,22 @@ describe("GitHub PR action primitives (#778)", () => { }); it("validates the repo name before any GitHub call", async () => { - await expect(closePullRequest(createTestEnv(), 1, "invalid", 4)).rejects.toThrow(/Invalid repository full name/); - await expect(closePullRequest(createTestEnv(), 1, "owner/repo/extra", 4)).rejects.toThrow( + // Every malformed name below is rejected by splitRepo() (src/github/pr-actions.ts) before + // closePullRequest ever touches `env`, so one shared env per env-shape covers every case without + // rebuilding a fresh in-memory SQLite database (166-migration replay, see test/helpers/d1.ts) or + // generating a fresh RSA-2048 key (real synchronous CPU work) on every call -- that redundant + // setup, not the code under test, is what timed this test out under concurrent full-suite load. + const env = createTestEnv(); + await expect(closePullRequest(env, 1, "invalid", 4)).rejects.toThrow(/Invalid repository full name/); + await expect(closePullRequest(env, 1, "owner/repo/extra", 4)).rejects.toThrow( /Invalid repository full name/, ); - await expect(closePullRequest(createTestEnv(), 1, " owner/repo ", 4)).rejects.toThrow( + await expect(closePullRequest(env, 1, " owner/repo ", 4)).rejects.toThrow( /Invalid repository full name/, ); // Per-segment padding (#6613) — mirrors assignees.ts parseRepoFullName coverage. for (const padded of ["owner/ repo", "owner /repo"]) { - await expect(closePullRequest(createTestEnv(), 1, padded, 4)).rejects.toThrow( + await expect(closePullRequest(env, 1, padded, 4)).rejects.toThrow( /Invalid repository full name/, ); } @@ -32,14 +38,15 @@ describe("GitHub PR action primitives (#778)", () => { called = true; return Response.json({ token: "t" }); }); - await expect(closePullRequest(envWithKey(), 1, "owner/repo/extra", 4)).rejects.toThrow( + const keyedEnv = envWithKey(); + await expect(closePullRequest(keyedEnv, 1, "owner/repo/extra", 4)).rejects.toThrow( /Invalid repository full name/, ); - await expect(closePullRequest(envWithKey(), 1, " owner/repo ", 4)).rejects.toThrow( + await expect(closePullRequest(keyedEnv, 1, " owner/repo ", 4)).rejects.toThrow( /Invalid repository full name/, ); for (const padded of ["owner/ repo", "owner /repo"]) { - await expect(closePullRequest(envWithKey(), 1, padded, 4)).rejects.toThrow( + await expect(closePullRequest(keyedEnv, 1, padded, 4)).rejects.toThrow( /Invalid repository full name/, ); }