Public: anyone can read this; the crew can change it.
require_checks, green half: the merge waits for the fixed head's deploy and then goes, every time #5301
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/5278-require-checks-green-half-the-merge-wait → main 5 commits fork 4 threads · 4 unresolved
4 files
changed +162
−101
scripts/shards.ts +1 −1
| @@ -454,7 +454,7 @@ export function parseShardSpec( | ||
| 454 | 454 | | { ok: true; none: true } |
| 455 | 455 | | { ok: false; error: string } { |
| 456 | 456 | if (spec === undefined || spec.trim() === "") return { ok: true, none: true }; |
| 457 | const m = /^([1-9][0-9]{0,2})\/([1-9][0-9]{0,2})$/.exec(spec.trim()); | |
| 457 | const m = /^([1-9][0-9]{0,3})\/([1-9][0-9]{0,3})$/.exec(spec.trim()); | |
| 458 | 458 | if (!m) |
| 459 | 459 | return { |
| 460 | 460 | ok: false, |
test/apps-require-checks-green.test.ts added +154
| @@ -0,0 +1,154 @@ | ||
| 1 | /** | |
| 2 | * `require_checks`, the green half (#5278): after a fixing push the merge | |
| 3 | * waits for the deploy of the *new* head — not the old red one — and then | |
| 4 | * goes. The test holds that deploy at the runtime's gate until it has seen | |
| 5 | * the merge refused, so no deploy can finish between the push and the | |
| 6 | * attempt; every wait is on the deploy record for the exact commit. | |
| 7 | */ | |
| 8 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 9 | import { mergePull, openPull } from "../src/actions/pulls.ts"; | |
| 10 | import { commitToBranch, updateRepoSettings } from "../src/actions/repos.ts"; | |
| 11 | import type { AppRecord } from "../src/core/apps.ts"; | |
| 12 | import { getPull } from "../src/core/pulls.ts"; | |
| 13 | import { getRepoById, type Repo } from "../src/core/repos.ts"; | |
| 14 | import { listRuns, type Run } from "../src/core/runs.ts"; | |
| 15 | import { | |
| 16 | deploySettled, | |
| 17 | startAppsForge, | |
| 18 | fetchAsHost, | |
| 19 | APPS_DOMAIN, | |
| 20 | startHello, | |
| 21 | stopAppsForge, | |
| 22 | type AppsForge, | |
| 23 | } from "./helpers/apps-forge.ts"; | |
| 24 | ||
| 25 | const suite = (passes: boolean): string => | |
| 26 | `import { expect, test } from "bun:test";\ntest("t", () => { expect(1).toBe(${passes ? 1 : 2}); });\n`; | |
| 27 | ||
| 28 | describe("require_checks: the fixed head's deploy", () => { | |
| 29 | let forge: AppsForge; | |
| 30 | let repo: Repo; | |
| 31 | let record: AppRecord; | |
| 32 | ||
| 33 | beforeAll(async () => { | |
| 34 | forge = await startAppsForge(); | |
| 35 | ({ repo, record } = await startHello(forge)); | |
| 36 | }, 90_000); | |
| 37 | ||
| 38 | afterAll(async () => { | |
| 39 | await forge.runtime.stop(); | |
| 40 | await stopAppsForge(forge); | |
| 41 | }); | |
| 42 | ||
| 43 | /** The deploy record of exactly this commit, in whatever state it is in. */ | |
| 44 | const deployOf = async (commitOid: string): Promise<Run> => { | |
| 45 | const deadline = Date.now() + 60_000; | |
| 46 | while (Date.now() < deadline) { | |
| 47 | const run = listRuns(forge.app.db, repo.id, { | |
| 48 | kind: "deploy", | |
| 49 | limit: 50, | |
| 50 | }).find((r) => r.commitOid === commitOid); | |
| 51 | if (run) return run; | |
| 52 | await Bun.sleep(20); | |
| 53 | } | |
| 54 | throw new Error(`no deploy recorded for ${commitOid}`); | |
| 55 | }; | |
| 56 | ||
| 57 | test("the merge is held until the fixed head's deploy is green, then goes", async () => { | |
| 58 | const { app, alice, runtime } = forge; | |
| 59 | expect( | |
| 60 | (await updateRepoSettings(app, alice, repo, { requireChecks: true })).ok, | |
| 61 | ).toBe(true); | |
| 62 | repo = getRepoById(app.db, repo.id)!; | |
| 63 | ||
| 64 | const broken = await commitToBranch(app, alice, repo, { | |
| 65 | branch: "broken", | |
| 66 | base: repo.defaultBranch, | |
| 67 | message: "Break the test", | |
| 68 | files: [{ path: "src/server.test.ts", content: suite(false) }], | |
| 69 | }); | |
| 70 | expect(broken.ok).toBe(true); | |
| 71 | if (!broken.ok) return; | |
| 72 | const opened = await openPull(app, alice, repo, { | |
| 73 | title: "Broken", | |
| 74 | baseBranch: repo.defaultBranch, | |
| 75 | headBranch: "broken", | |
| 76 | }); | |
| 77 | expect(opened.ok).toBe(true); | |
| 78 | if (!opened.ok) return; | |
| 79 | const number = opened.value.number; | |
| 80 | const red = await deploySettled( | |
| 81 | app, | |
| 82 | repo.id, | |
| 83 | (r) => r.commitOid === broken.value.commitOid, | |
| 84 | ); | |
| 85 | expect(red.state).toBe("failed"); | |
| 86 | expect(red.summary).toContain("test"); | |
| 87 | const refused = await mergePull(app, alice, repo, opened.value, "merge"); | |
| 88 | expect(refused.ok).toBe(false); | |
| 89 | if (!refused.ok) expect(refused.error).toBe("checks_required"); | |
| 90 | // The app never broke, only its tests: the preview answers with its own | |
| 91 | // page, because a deployment that answers is what was asked for. | |
| 92 | const page = await fetchAsHost( | |
| 93 | forge.servers, | |
| 94 | `hello--${number}.${APPS_DOMAIN}`, | |
| 95 | ); | |
| 96 | expect(page.status).toBe(200); | |
| 97 | expect(await page.text()).toContain(`preview of pull #${number}`); | |
| 98 | const status = runtime.envStatus(record, `pull-${number}`); | |
| 99 | expect(status?.state).toBe("running"); | |
| 100 | expect(status?.error).toContain("tests failed"); | |
| 101 | ||
| 102 | // Hold every permit: the fixing head's deploy is recorded but cannot run. | |
| 103 | const releases: (() => void)[] = []; | |
| 104 | for (let i = 0; i < runtime.lane.limit; i++) | |
| 105 | releases.push(await runtime.lane.acquire()); | |
| 106 | let released = false; | |
| 107 | const release = () => { | |
| 108 | if (released) return; | |
| 109 | released = true; | |
| 110 | for (const r of releases) r(); | |
| 111 | }; | |
| 112 | try { | |
| 113 | const fixed = await commitToBranch(app, alice, repo, { | |
| 114 | branch: "broken", | |
| 115 | message: "Fix the test", | |
| 116 | files: [{ path: "src/server.test.ts", content: suite(true) }], | |
| 117 | }); | |
| 118 | expect(fixed.ok).toBe(true); | |
| 119 | if (!fixed.ok) return; | |
| 120 | expect(fixed.value.commitOid).not.toBe(broken.value.commitOid); | |
| 121 | ||
| 122 | const pending = await deployOf(fixed.value.commitOid); | |
| 123 | expect(pending.env).toBe(`pull-${number}`); | |
| 124 | expect(["queued", "running"]).toContain(pending.state); | |
| 125 | const held = await mergePull( | |
| 126 | app, | |
| 127 | alice, | |
| 128 | repo, | |
| 129 | getPull(app.db, repo.id, number)!, | |
| 130 | "merge", | |
| 131 | ); | |
| 132 | expect(held.ok).toBe(false); | |
| 133 | if (!held.ok) expect(held.error).toBe("checks_required"); | |
outdated
Filler assertion. Remove it and the unused |
||
| 134 | ||
| 135 | release(); | |
| 136 | const green = await deploySettled( | |
| 137 | app, | |
| 138 | repo.id, | |
| 139 | (r) => r.commitOid === fixed.value.commitOid, | |
| 140 | ); | |
| 141 | expect(green.state).toBe("ok"); | |
| 142 | const merged = await mergePull( | |
| 143 | app, | |
| 144 | alice, | |
| 145 | repo, | |
| 146 | getPull(app.db, repo.id, number)!, | |
| 147 | "merge", | |
| 148 | ); | |
| 149 | expect(merged.ok).toBe(true); | |
| 150 | } finally { | |
| 151 | release(); | |
| 152 | } | |
| 153 | }, 120_000); | |
| 154 | }); |
test/apps.test.ts +5 −100
| @@ -2,12 +2,12 @@ | ||
| 2 | 2 | * Apps, end to end (docs/APPS.md): a new app runs in production seconds |
| 3 | 3 | * after it exists; a pull gets a preview with a copy of production's data; |
| 4 | 4 | * a push moves the preview; closing the pull removes it; a private app |
| 5 | * answers only through a grant; `require_checks` holds the merge until the | |
| 6 | * head's deploy is green. | |
| 5 | * answers only through a grant. | |
| 7 | 6 | * |
| 8 | 7 | * The rest of the first suite is split by what it needs, each on a forge of |
| 9 | * its own (#1798): `apps-slow-test`, `apps-identity`, `apps-crew`; the New | |
| 10 | * app form is `apps-new`. The forge they share is `helpers/apps-forge.ts`. | |
| 8 | * its own (#1798): `apps-slow-test`, `apps-identity`, `apps-crew`, | |
outdated
This line is now far over the width of the others; rewrap. |
||
| 9 | * `apps-require-checks-green`, `apps-require-checks-red`; the New app form is | |
| 10 | * `apps-new`. The forge they share is `helpers/apps-forge.ts`. | |
| 11 | 11 | */ |
| 12 | 12 | |
| 13 | 13 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; |
| @@ -35,7 +35,7 @@ import { | ||
| 35 | 35 | } from "../src/actions/repos.ts"; |
| 36 | 36 | import type { AppRecord } from "../src/core/apps.ts"; |
| 37 | 37 | import { getPull, type Pull } from "../src/core/pulls.ts"; |
| 38 | import { getRepoById, type Repo } from "../src/core/repos.ts"; | |
| 38 | import { type Repo } from "../src/core/repos.ts"; | |
| 39 | 39 | import { repoDir } from "../src/git/repos.ts"; |
| 40 | 40 | import { |
| 41 | 41 | getRun, |
| @@ -363,101 +363,6 @@ describe("apps: production, previews, data, gates", () => { | ||
| 363 | 363 | updateAppSettings(app, alice, repo, { access: "public" }); |
| 364 | 364 | }); |
| 365 | 365 | |
| 366 | test("require_checks holds the merge until the head's deploy is green", async () => { | |
| 367 | const settings = await updateRepoSettings(app, alice, repo, { | |
| 368 | requireChecks: true, | |
| 369 | }); | |
| 370 | expect(settings.ok).toBe(true); | |
| 371 | repo = getRepoById(app.db, repo.id)!; | |
| 372 | const broken = await commitToBranch(app, alice, repo, { | |
| 373 | branch: "broken", | |
| 374 | base: repo.defaultBranch, | |
| 375 | message: "Break the test", | |
| 376 | files: [ | |
| 377 | { | |
| 378 | path: "src/server.test.ts", | |
| 379 | content: | |
| 380 | 'import { expect, test } from "bun:test";\ntest("fails", () => { expect(1).toBe(2); });\n', | |
| 381 | }, | |
| 382 | ], | |
| 383 | }); | |
| 384 | expect(broken.ok).toBe(true); | |
| 385 | const opened = await openPull(app, alice, repo, { | |
| 386 | title: "Broken", | |
| 387 | baseBranch: repo.defaultBranch, | |
| 388 | headBranch: "broken", | |
| 389 | }); | |
| 390 | expect(opened.ok).toBe(true); | |
| 391 | if (!opened.ok) return; | |
| 392 | // The red half (refused merge, preview still answering) is | |
| 393 | // apps-require-checks-red.test.ts; here the failed head only has to settle. | |
| 394 | if (!broken.ok) return; | |
| 395 | const failed = await settled( | |
| 396 | (r) => | |
| 397 | r.env === `pull-${opened.value.number}` && | |
| 398 | r.commitOid === broken.value.commitOid, | |
| 399 | ); | |
| 400 | expect(failed.state).toBe("failed"); | |
| 401 | ||
| 402 | // Hold every deploy permit, so the fixed head's deploy cannot finish | |
| 403 | // between this push and the merge attempt: the hold is asserted while the | |
| 404 | // deploy is provably unfinished, then the deploy is let go. | |
| 405 | const permits = await Promise.all( | |
| 406 | Array.from({ length: runtime.lane.limit }, () => runtime.lane.acquire()), | |
| 407 | ); | |
| 408 | const release = () => permits.forEach((r) => r()); | |
| 409 | let fixed: Awaited<ReturnType<typeof commitToBranch>>; | |
| 410 | try { | |
| 411 | fixed = await commitToBranch(app, alice, repo, { | |
| 412 | branch: "broken", | |
| 413 | message: "Fix the test", | |
| 414 | files: [ | |
| 415 | { | |
| 416 | path: "src/server.test.ts", | |
| 417 | content: | |
| 418 | 'import { expect, test } from "bun:test";\ntest("passes", () => { expect(1).toBe(1); });\n', | |
| 419 | }, | |
| 420 | ], | |
| 421 | }); | |
| 422 | expect(fixed.ok).toBe(true); | |
| 423 | if (!fixed.ok) return; | |
| 424 | const fixedOid = fixed.value.commitOid; | |
| 425 | const queuedFor = () => | |
| 426 | listRuns(app.db, repo.id, { kind: "deploy", limit: 50 }).find( | |
| 427 | (r) => | |
| 428 | r.env === `pull-${opened.value.number}` && | |
| 429 | r.commitOid === fixedOid, | |
| 430 | ); | |
| 431 | while (queuedFor() === undefined) await Bun.sleep(20); | |
| 432 | expect(queuedFor()?.state).not.toBe("ok"); | |
| 433 | const held = await mergePull( | |
| 434 | app, | |
| 435 | alice, | |
| 436 | repo, | |
| 437 | getPull(app.db, repo.id, opened.value.number)!, | |
| 438 | "merge", | |
| 439 | ); | |
| 440 | expect(held.ok).toBe(false); | |
| 441 | if (!held.ok) expect(held.error).toBe("checks_required"); | |
| 442 | } finally { | |
| 443 | release(); | |
| 444 | } | |
| 445 | const green = await settled( | |
| 446 | (r) => | |
| 447 | r.env === `pull-${opened.value.number}` && | |
| 448 | r.commitOid === fixed.value.commitOid, | |
| 449 | ); | |
| 450 | expect(green.state).toBe("ok"); | |
| 451 | const merged = await mergePull( | |
| 452 | app, | |
| 453 | alice, | |
| 454 | repo, | |
| 455 | getPull(app.db, repo.id, opened.value.number)!, | |
| 456 | "merge", | |
| 457 | ); | |
| 458 | expect(merged.ok).toBe(true); | |
| 459 | }, 120_000); | |
| 460 | ||
| 461 | 366 | test("archiving stops the app, keeps its data, and its URL says it is archived", async () => { |
| 462 | 367 | expect(runtime.envStatus(record, "production")?.state).toBe("running"); |
| 463 | 368 | const data = envDataDir(dataDir, record.id, "production"); |
test/shards.test.ts +2
| @@ -506,6 +506,8 @@ describe("parseShardSpec", () => { | ||
| 506 | 506 | test("empty means no matrix slice", () => { |
| 507 | 507 | expect(parseShardSpec(undefined)).toEqual({ ok: true, none: true }); |
| 508 | 508 | expect(parseShardSpec("")).toEqual({ ok: true, none: true }); |
| 509 | // The gate passed 999 files; a slice past that is still a slice. | |
| 510 | expect(parseShardSpec("1000/1000")).toEqual({ ok: true, index: 1000, total: 1000 }); | |
This line is over the formatter width; wrap the toEqual argument. |
||
| 509 | 511 | expect(parseShardSpec(" ")).toEqual({ ok: true, none: true }); |
| 510 | 512 | }); |
| 511 | 513 |
new above
offline
Add here: mergePull on the red head must answer checks_required. Also assert the preview still serves its own page and envStatus carries "tests failed", as the removed test did.