trevato/forge

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

More
Forks1g fMapg aRuns13g rEventsFlows

A green deploy says its tests passed: #962's testsClause lands, with its deploy tests in their own file #1162

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/987-a-green-deploy-says-its-tests-passed-962 → main 6 commits fork 4 threads · 4 unresolved

  • Merged. scout/987-a-green-deploy-says-its-tests-passed-962 was merged into main as e8643b4.

Review

Sign in to review this change.
10 files changed +329 −14
docs/STUDIO.md +9
@@ -527,3 +527,12 @@ It never says _shipped_ before production redeployed, never hides that a
527527 run cost money, never shows a preview that is not the pull's real head, and
528528 never invents an answer for a card. The complexity is removed from the
529529 surface, not from the system.
530
531 **What a green deploy says about its tests.** One clause, `testsClause`
532 (`src/web/studio/words.ts`): "its tests passed" when the app declares a test
533 and it ran, "it answered, no tests were run" when it declares none. It is read
534 from the finished deploy's summary (see `finishRun`) and drawn on the pull's
535 note, the look card in the home's needs-you (`decisionsFor`), and the previews
536 panel (only when the check is of the commit being served). The terminal draws
537 none of it: it shows the app's shell, not the studio's words, so nothing there
538 changed.
scripts/gate.ts +1
@@ -155,6 +155,7 @@ export const GATE: readonly string[] = [
155155 "test/apps-identity.test.ts",
156156 "test/apps-new.test.ts",
157157 "test/apps-slow-test.test.ts",
158 "test/apps-tests-clause.test.ts",
158159 "test/apps-adversarial.test.ts",
159160 "test/apps-busy.test.ts",
160161 "test/apps-token.test.ts",
src/apps/runtime.ts +6 −1
@@ -156,6 +156,11 @@ const BUSY_GRACE_MS = 45 * 60_000;
156156 * the studio reads the same constant to word its badge. */
157157 export const VERIFYING_NOTE = "serving — still verifying its tests";
158158
159 /** What a finished deploy's summary ends with: the studio words its green
160 * from these, so a pull says whether its tests ran (`words.ts`). */
161 export const TESTS_PASSED_TAIL = " · its tests passed";
162 export const NO_TESTS_TAIL = " · no tests declared, none run";
163
159164 export interface EnvStatus {
160165 readonly env: EnvName;
161166 readonly state: EnvState;
@@ -1112,7 +1117,7 @@ export class AppsRuntime {
11121117 if (tested.ok) {
11131118 const done = finishRun(this.app.db, run.id, {
11141119 state: "ok",
1115 summary: `running at ${url}`,
1120 summary: `running at ${url}${live.manifest.test === null ? NO_TESTS_TAIL : TESTS_PASSED_TAIL}`,
11161121 });
11171122 write(`done: ${url}`);
11181123 if (done && state.env === "production") await this.noteCarried(repo, done);
src/core/runs.ts +6
@@ -252,6 +252,12 @@ export function markServing(db: Database, runId: string): Run | undefined {
252252 * Finish a run. Only a running run finishes; a cancelled one stays
253253 * cancelled however late its worker reports in. Returns the final row, or
254254 * undefined when nothing changed.
255 *
256 * A deploy's `summary` ends with `TESTS_PASSED_TAIL` or `NO_TESTS_TAIL`
257 * (`apps/runtime.ts`); `testsClause` (`web/studio/words.ts`) reads it back by
258 * suffix. That is a contract kept in free text, chosen over a new column to
259 * avoid a migration; both sides use the exported constants, never literals,
260 * and a run without either tail (older runs) simply says nothing.
255261 */
256262 export function finishRun(
257263 db: Database,
src/web/studio/asks.ts +10 −4
@@ -42,6 +42,7 @@ import {
4242 activeRunFor,
4343 automaticRunsSince,
4444 latestRunsForIssues,
45 type Run,
4546 } from "../../core/runs.ts";
4647 import { getUserById, type User } from "../../core/users.ts";
4748 import {
@@ -62,6 +63,7 @@ import { btn, card, icon, row } from "../ui.ts";
6263 import { lookUrl } from "../../core/looks.ts";
6364 import { lookThumb, thumbLook } from "../pages/looks.ts";
6465 import { changeHref } from "./nouns.ts";
66 import { testsClause } from "./words.ts";
6567
6668 /** Repos narrowed to those with an app record, with the records — the one
6769 * join `appsOf` and `writableAppsOf` share. */
@@ -225,6 +227,8 @@ interface AskCandidate {
225227 * differently for a person's own change than for the crew's (below).
226228 * Absent for "paused", which names no author at all. */
227229 readonly authorIsAgent?: boolean;
230 /** For "look": the green check, whose summary says what its tests did. */
231 readonly check?: Run;
228232 /** For "review": why the agent's change waits for a person, and since when. */
229233 readonly reviewWait?: ReviewWait;
230234 /** For "stopped": how many automatic tries, and whether the decider itself stopped it. */
@@ -348,6 +352,7 @@ function askCandidatesFor(
348352 pull,
349353 who,
350354 authorIsAgent: issue.authorIsAgent,
355 check,
351356 });
352357 } else if (
353358 check?.state === "ok" &&
@@ -910,6 +915,7 @@ export function decisionsFor(
910915 };
911916 if (c.kind === "look" && c.pull) {
912917 const picture = pictureOf(c.repo, c.pull);
918 const clause = testsClause(c.check ?? null);
913919 // The crew's own words say who finished it; a person's own change
914920 // gets the wording that is true of it — nobody "finished" it for
915921 // them, it is simply green and waiting.
@@ -920,11 +926,11 @@ export function decisionsFor(
920926 pull: c.pull,
921927 who: c.who,
922928 says: c.authorIsAgent
923 ? `${c.who} finished "${c.pull.title}" and the check is green. Look at the preview, then ship it or say what is off.`
924 : `"${c.pull.title}" is green — ready for a look. Ship it, or say what is off.`,
929 ? `${c.who} finished "${c.pull.title}" and the check is green${clause === "" ? "" : ` (${clause})`}. Look at the preview, then ship it or say what is off.`
junipercrew

New wording for says and short with no test. Add one in the asks suite that fails without the clause.
930 : `"${c.pull.title}" is green — ${clause === "" ? "" : `${clause}, `}ready for a look. Ship it, or say what is off.`,
925931 short: c.authorIsAgent
926 ? `${c.who} finished it; the check is green`
927 : `green — ready for a look`,
932 ? `${c.who} finished it; the check is green${clause === "" ? "" : ` (${clause})`}`
933 : `green — ${clause === "" ? "" : `${clause}, `}ready for a look`,
928934 ...(picture === undefined ? {} : { picture }),
929935 };
930936 }
src/web/studio/previews.ts +21 −4
@@ -17,10 +17,11 @@ import {
1717 import { getAppByRepo, type AppRecord } from "../../core/apps.ts";
1818 import { listPulls, type Pull } from "../../core/pulls.ts";
1919 import { canSeeOwnerFacts, type Repo } from "../../core/repos.ts";
20 import { listRuns } from "../../core/runs.ts";
20 import { latestRunsForIssues, listRuns, type Run } from "../../core/runs.ts";
2121 import type { Ctx } from "../../http/context.ts";
2222 import { readRunLogTail } from "../../runs/log.ts";
2323 import { loadRepo, repoBase } from "../chrome.ts";
24 import { testsClause } from "./words.ts";
2425 import { html, when, type Html } from "../html.ts";
2526 import { fragment } from "../layout.ts";
2627 import {
@@ -81,6 +82,17 @@ function previewBadge(env: EnvStatus | undefined): Html {
8182 return badge(word, { tone, icon: ic });
8283 }
8384
85 /** The tests clause for a serving preview, only when the green check is of
86 * the very commit being served — a stale check says nothing. */
87 export function previewVerdict(
88 env: EnvStatus | undefined,
89 check: Run | undefined,
90 ): string | undefined {
91 if (env?.state !== "running" || check?.state !== "ok") return undefined;
92 if (check.commitOid !== env.commitOid) return undefined;
93 return testsClause(check) || undefined;
94 }
95
8496 /**
8597 * One open pull's preview, as one row: its host is the click, its state is
8698 * the badge, and its meta line says what the change page's Preview card
@@ -102,6 +114,7 @@ function previewRow(
102114 pull: Pull,
103115 env: EnvStatus | undefined,
104116 writer: boolean,
117 check: Run | undefined,
105118 ): Html {
106119 const href =
107120 runtime === undefined
junipercrew outdated

The commitOid match and the fallback order (error, failedDeploy, note, verdict) are untested. Cover a stale-commit check showing no clause.
@@ -113,7 +126,9 @@ function previewRow(
113126 : runtime.urlFor(record, previewEnv(pull.number)).replace(/^https?:\/\//, "");
114127 const bits: Html[] = [html`#${pull.number} ${pull.title}`];
115128 if (env !== undefined) bits.push(...envLine(base, env));
116 const say = env?.error ?? env?.failedDeploy?.error ?? env?.note;
129 // A serving preview whose deploy finished green says what its tests did.
130 const say =
131 env?.error ?? env?.failedDeploy?.error ?? env?.note ?? previewVerdict(env, check);
117132 if (say) bits.push(html`${say}`);
118133 const asleep = env === undefined ? null : dormantNote(env);
119134 if (asleep !== null) bits.push(asleep);
@@ -160,6 +175,7 @@ function previewsPanel(
160175 openPulls: readonly Pull[],
161176 envs: readonly EnvStatus[],
162177 writer: boolean,
178 checks: ReadonlyMap<string, Run>,
163179 ): Html | null {
164180 if (openPulls.length === 0) return null;
165181 const byPull = new Map(
@@ -169,7 +185,7 @@ function previewsPanel(
169185 );
170186 return listCard(
171187 openPulls.map((p) =>
172 previewRow(base, runtime, record, p, byPull.get(p.number), writer),
188 previewRow(base, runtime, record, p, byPull.get(p.number), writer, checks.get(p.id)),
173189 ),
174190 {
175191 head: html`${icon("rocket")}
@@ -274,7 +290,8 @@ export function appPanels(
274290 const runtime = appsRuntime(ctx.app);
275291 const envs: EnvStatus[] = envsFor(ctx.app, ctx.auth.user, record);
276292 const writer = canSeeOwnerFacts(db, repo, ctx.auth.user);
277 const previews = previewsPanel(base, runtime, record, openPulls, envs, writer);
293 const checks = latestRunsForIssues(db, openPulls.map((p) => p.id));
294 const previews = previewsPanel(base, runtime, record, openPulls, envs, writer, checks);
278295 const production = envs.find((e) => e.env === "production");
279296 const url = runtime?.urlFor(record, "production");
280297 const tail = productionTail(ctx, repo, production, writer);
src/web/studio/words.ts +25 −3
@@ -10,7 +10,12 @@
1010
1111 import type { Database } from "bun:sqlite";
1212 import { ancestorsOf } from "../../git/merge.ts";
13 import type { AppsRuntime, EnvStatus } from "../../apps/runtime.ts";
13 import {
14 NO_TESTS_TAIL,
15 TESTS_PASSED_TAIL,
16 type AppsRuntime,
17 type EnvStatus,
18 } from "../../apps/runtime.ts";
1419 import type { AppRecord } from "../../core/apps.ts";
1520 import { canEnterApp, getRepoById } from "../../core/repos.ts";
1621 import type { User } from "../../core/users.ts";
@@ -151,11 +156,28 @@ function detailNote(detail: CheckDetail): string {
151156 return `${detail.passed} passed, ${detail.failed} failed: ${names}`;
152157 }
153158
159 /**
160 * What a green deploy says about its tests, from the summary the runtime
161 * wrote when it finished: the one clause every surface that draws a green
162 * change uses. Empty for a run that predates the clause.
163 */
164 export function testsClause(run: Run | null): string {
165 const summary = run?.summary ?? "";
166 if (summary.endsWith(TESTS_PASSED_TAIL)) return "its tests passed";
167 if (summary.endsWith(NO_TESTS_TAIL)) return "it answered, no tests were run";
168 return "";
169 }
170
154171 /** A single open pull's check, in the studio's own words — never dev's
155172 * "Checks passed/failed": the same five words as the rest of the page. */
156173 function checkRunWord(check: CheckSummary): { word: Word; note: string } {
157 if (check.state === "ok")
158 return { word: "look", note: "green — ready for a look" };
174 if (check.state === "ok") {
175 const clause = testsClause(check.run);
176 return {
177 word: "look",
178 note: `green — ${clause === "" ? "" : `${clause}, `}ready for a look`,
179 };
180 }
159181 if (check.state === "failed") {
160182 const base =
161183 check.run?.servingAt != null
test/apps-tests-clause.test.ts added +171
@@ -0,0 +1,171 @@
1 /**
2 * A green deploy says what its tests did (words.ts `testsClause`): the
3 * pull's note reads "its tests passed" when the app's suite ran, and says
4 * nothing was run when the app declares no test. Real deploys, so this file
5 * owns its forge and data directory: `test/apps.test.ts` shares one forge
6 * across its tests and counts previews, which these would leak into.
7 */
8
9 import { afterAll, beforeAll, describe, expect, test } from "bun:test";
10 import { mkdtemp, rm } from "node:fs/promises";
11 import { tmpdir } from "node:os";
12 import { join } from "node:path";
13 import { createApp, type App } from "../src/app.ts";
14 import { AppsRuntime } from "../src/apps/runtime.ts";
15 import { newApp } from "../src/actions/apps.ts";
16 import { mergePull, openPull } from "../src/actions/pulls.ts";
17 import { commitToBranch } from "../src/actions/repos.ts";
18 import type { AppRecord } from "../src/core/apps.ts";
19 import { getPull } from "../src/core/pulls.ts";
20 import type { Repo } from "../src/core/repos.ts";
21 import { listRuns, type Run } from "../src/core/runs.ts";
22 import { createUser, type User } from "../src/core/users.ts";
23 import { repoDir } from "../src/git/repos.ts";
24 import { serve, type Servers } from "../src/http/server.ts";
25 import { pullWord, servingHistory } from "../src/web/studio/words.ts";
26
27 describe("a green deploy says what its tests did", () => {
28 let dataDir: string;
29 let app: App;
30 let servers: Servers;
31 let runtime: AppsRuntime;
32 let alice: User;
33 let repo: Repo;
34 let record: AppRecord;
35
36 async function settled(where: (run: Run) => boolean): Promise<Run> {
37 const deadline = Date.now() + 60_000;
38 while (Date.now() < deadline) {
39 const run = listRuns(app.db, repo.id, { kind: "deploy", limit: 50 }).find(
40 where,
41 );
42 if (run && ["ok", "failed", "cancelled"].includes(run.state)) return run;
43 await Bun.sleep(100);
44 }
45 throw new Error("run did not settle in time");
46 }
47
48 const word = (number: number) =>
49 pullWord(
50 {
51 db: app.db,
52 dir: repoDir(app.reposRoot, repo.ownerName, repo.name),
53 runtime,
54 record,
55 history: servingHistory(),
56 viewer: alice,
57 },
58 getPull(app.db, repo.id, number)!,
59 );
60
61 beforeAll(async () => {
62 dataDir = await mkdtemp(join(tmpdir(), "forge-apps-clause-"));
63 app = createApp({ dataDir, port: 0 });
64 servers = await serve(app);
65 runtime = new AppsRuntime(app, {
66 domain: "apps.test",
67 scheme: "http",
68 publicPort: servers.port,
69 forgeUrl: servers.url,
70 concurrency: 2,
71 });
72 await runtime.start();
73 const created = await createUser(app.db, {
74 trusted: true,
75 username: "alice",
76 email: "[email protected]",
77 password: "a-good-passphrase",
78 adminIfFirst: true,
79 });
80 if (!created.ok) throw new Error(created.error);
81 alice = created.value;
82 const made = await newApp(app, alice, {
83 name: "clause",
84 description: "Tests clause",
85 visibility: "public",
86 });
87 if (!made.ok) throw new Error(made.error);
88 repo = made.value.repo;
89 record = made.value.app;
90 await settled((r) => r.env === "production");
91 }, 90_000);
92
93 afterAll(async () => {
94 await runtime.stop();
95 servers.stop();
96 app.close();
97 await rm(dataDir, { recursive: true, force: true });
98 });
99
100 test("the suite passed: the pull says its tests passed", async () => {
101 const committed = await commitToBranch(app, alice, repo, {
102 branch: "passing",
103 base: repo.defaultBranch,
104 message: "A passing test",
105 files: [
106 {
107 path: "src/server.test.ts",
108 content:
109 'import { expect, test } from "bun:test";\ntest("passes", () => { expect(1).toBe(1); });\n',
110 },
111 ],
112 });
113 expect(committed.ok).toBe(true);
114 if (!committed.ok) return;
115 const opened = await openPull(app, alice, repo, {
116 title: "Passing",
117 baseBranch: repo.defaultBranch,
118 headBranch: "passing",
119 });
120 expect(opened.ok).toBe(true);
121 if (!opened.ok) return;
122 const green = await settled(
123 (r) =>
124 r.env === `pull-${opened.value.number}` &&
125 r.commitOid === committed.value.commitOid,
126 );
127 expect(green.state).toBe("ok");
128 expect((await word(opened.value.number)).note).toBe(
129 "green — its tests passed, ready for a look",
130 );
131 const merged = await mergePull(app, alice, repo, opened.value, "merge");
132 expect(merged.ok).toBe(true);
133 }, 120_000);
134
135 test("an app that declares no test: the pull says nothing was run", async () => {
136 const committed = await commitToBranch(app, alice, repo, {
137 branch: "untested",
138 base: repo.defaultBranch,
139 message: "No tests declared",
140 files: [
141 {
142 path: "forge.json",
143 content: `${JSON.stringify(
144 { app: { run: "bun run src/server.ts", tty: "bun run src/tty.ts" } },
145 null,
146 2,
147 )}\n`,
148 },
149 ],
150 });
151 expect(committed.ok).toBe(true);
152 if (!committed.ok) return;
153 const opened = await openPull(app, alice, repo, {
154 title: "Untested",
155 baseBranch: repo.defaultBranch,
156 headBranch: "untested",
157 });
158 expect(opened.ok).toBe(true);
159 if (!opened.ok) return;
160 const green = await settled(
161 (r) =>
162 r.env === `pull-${opened.value.number}` &&
163 r.commitOid === committed.value.commitOid,
164 );
165 expect(green.state).toBe("ok");
166 expect(await word(opened.value.number)).toEqual({
167 word: "look",
168 note: "green — it answered, no tests were run, ready for a look",
169 });
170 }, 120_000);
171 });
test/asks.test.ts +37
@@ -12,11 +12,14 @@ import { createApp, type App } from "../src/app.ts";
1212 import { addToCrew, createAgent } from "../src/actions/crew.ts";
1313 import { openPull } from "../src/actions/pulls.ts";
1414 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
15 import { TESTS_PASSED_TAIL } from "../src/apps/runtime.ts";
1516 import { recordDecision, resolveOpen } from "../src/core/decisions.ts";
1617 import type { Repo } from "../src/core/repos.ts";
1718 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
1819 import { getUserByName } from "../src/core/users.ts";
20 import type { Ctx } from "../src/http/context.ts";
1921 import { serve, type Servers } from "../src/http/server.ts";
22 import { decisionsFor } from "../src/web/studio/asks.ts";
2023 import { quietLogs } from "./helpers/globals.ts";
2124
2225 quietLogs();
@@ -225,4 +228,38 @@ describe("the asks page", () => {
225228 check("failed");
226229 expect(await get(alice, "/")).not.toContain(card);
227230 });
231
232 test("a green change's card says what its tests did", async () => {
233 const bot = getUserByName(app.db, "bot")!;
234 const branch = await commitToBranch(app, getUserByName(app.db, "alice")!, repo, {
235 branch: "bot/3-clause",
236 base: "main",
237 message: "Bot work",
238 files: [{ path: "CLAUSE.md", content: "x\n" }],
239 });
240 expect(branch.ok).toBe(true);
241 const opened = await openPull(app, bot, repo, {
242 title: "Bot clause",
243 body: "",
244 baseBranch: "main",
245 headBranch: "bot/3-clause",
246 });
247 if (!opened.ok) throw new Error(String(opened.error));
248 const pull = opened.value;
249 const run = createRun(app.db, {
250 repoId: repo.id,
251 kind: "check",
252 issueId: pull.id,
253 env: `pull-${pull.number}`,
254 commitOid: pull.headOid,
255 });
256 claimRun(app.db, run.id);
257 finishRun(app.db, run.id, { state: "ok", summary: `running at x${TESTS_PASSED_TAIL}` });
258 const card = decisionsFor(
259 { app, auth: { user: getUserByName(app.db, "alice")! } } as unknown as Ctx,
260 [repo],
261 ).find((d) => d.id === `look:${pull.id}`);
262 expect(card?.says).toContain("its tests passed");
263 expect(card?.short).toContain("its tests passed");
264 });
228265 });
test/studio-words.test.ts +43 −2
@@ -30,15 +30,22 @@ import { join } from "node:path";
3030 import { createApp, type App } from "../src/app.ts";
3131 import { enableApp } from "../src/actions/apps.ts";
3232 import { previewEnv, type EnvName } from "../src/apps/data.ts";
33 import { AppsRuntime } from "../src/apps/runtime.ts";
33 import {
34 AppsRuntime,
35 NO_TESTS_TAIL,
36 TESTS_PASSED_TAIL,
37 type EnvStatus,
38 } from "../src/apps/runtime.ts";
3439 import { getAppByRepo, type AppRecord } from "../src/core/apps.ts";
3540 import { addToCrew, createAgent } from "../src/actions/crew.ts";
3641 import { assignIssue, openIssue } from "../src/actions/issues.ts";
3742 import { mergePull, openPull } from "../src/actions/pulls.ts";
3843 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
39 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
44 import { claimRun, createRun, finishRun, type Run } from "../src/core/runs.ts";
4045 import { createUser, getUserByName } from "../src/core/users.ts";
4146 import { serve, type Servers } from "../src/http/server.ts";
47 import { previewVerdict } from "../src/web/studio/previews.ts";
48 import { testsClause } from "../src/web/studio/words.ts";
4249
4350 /**
4451 * Dev's own words, each with the reason it must never reach a studio page.
@@ -883,3 +890,37 @@ describe("dev's vocabulary never reaches a studio page", () => {
883890 );
884891 });
885892 });
893
894 describe("testsClause", () => {
895 const run = (summary: string) => ({ summary }) as unknown as Run;
896
897 test("a deploy from before the clause existed says nothing about tests; the two tails say their own", () => {
898 expect(testsClause(run("running at http://x"))).toBe("");
899 expect(testsClause(null)).toBe("");
900 expect(testsClause(run(`running at http://x${TESTS_PASSED_TAIL}`))).toBe(
901 "its tests passed",
902 );
903 expect(testsClause(run(`running at http://x${NO_TESTS_TAIL}`))).toBe(
904 "it answered, no tests were run",
905 );
906 });
907 });
908
909 describe("a preview row's tests verdict", () => {
910 const run = (summary: string, commitOid: string, state = "ok") =>
911 ({ state, summary, commitOid }) as unknown as Run;
912 const env = (state: string, commitOid: string) =>
913 ({ state, commitOid }) as unknown as EnvStatus;
914
915 test("speaks only when the green check is of the commit being served", () => {
916 const green = run(`running at x${TESTS_PASSED_TAIL}`, "abc");
917 expect(previewVerdict(env("running", "abc"), green)).toBe("its tests passed");
918 expect(previewVerdict(env("running", "def"), green)).toBeUndefined();
919 expect(previewVerdict(env("asleep", "abc"), green)).toBeUndefined();
920 expect(previewVerdict(undefined, green)).toBeUndefined();
921 expect(
922 previewVerdict(env("running", "abc"), run(`x${TESTS_PASSED_TAIL}`, "abc", "failed")),
923 ).toBeUndefined();
924 expect(previewVerdict(env("running", "abc"), undefined)).toBeUndefined();
925 });
926 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs