92a23d9 · cf3f944 · d1003ea · 44c6238 · dbd53f2 · bcd9f34
@@ -1,5 +1,5 @@
/** The API's list of pulls names its Server-Timing phases (#3964). */
import { afterAll, beforeAll, expect, test } from "bun:test";
import { afterAll, beforeAll, expect, spyOn, test } from "bun:test";
import { mkdtemp, rm } from "node:fs/promises";
import { tmpdir } from "node:os";
import { join } from "node:path";
@@ -7,8 +7,10 @@
import { createApp, type App } from "../src/app.ts";
import { openPull } from "../src/actions/pulls.ts";
import { commitToBranch, createRepo } from "../src/actions/repos.ts";
import { createSession } from "../src/core/sessions.ts";
import { createUser } from "../src/core/users.ts";
import type { Repo } from "../src/core/repos.ts";
import { createUser, type User } from "../src/core/users.ts";
import { serve, type Servers } from "../src/http/server.ts";
import { expectBudget } from "./helpers/budget.ts";
import { quietLogs } from "./helpers/globals.ts";
quietLogs();
@@ -17,6 +19,8 @@
let dataDir: string;
let app: App;
let servers: Servers;
let cookie = "";
let alice: User;
let repo: Repo;
const cells = process.env["FORGE_CELLS"];
beforeAll(async () => {
@@ -33,23 +37,24 @@
beforeAll(async () => {
    adminIfFirst: true,
  });
  if (!made.ok) throw new Error(String(made.error));
  const alice = made.value;
  alice = made.value;
  cookie = `forge_session=${createSession(app.db, alice.id).token}`;
  const repo = await createRepo(app, alice, { name: "r", visibility: "public" });
  if (!repo.ok) throw new Error(String(repo.error));
  const first = await commitToBranch(app, alice, repo.value, {
  const created = await createRepo(app, alice, { name: "r", visibility: "public" });
  if (!created.ok) throw new Error(String(created.error));
  repo = created.value;
  const first = await commitToBranch(app, alice, repo, {
    message: "change",
    files: [{ path: "forge.json", content: "{}" }],
  });
  if (!first.ok) throw new Error(JSON.stringify(first.error));
  for (const branch of ["one", "two"]) {
    const done = await commitToBranch(app, alice, repo.value, {
    const done = await commitToBranch(app, alice, repo, {
      branch,
      message: "change",
      files: [{ path: `${branch}.txt`, content: "x" }],
    });
    if (!done.ok) throw new Error(JSON.stringify(done.error));
    const pull = await openPull(app, alice, repo.value, {
    const pull = await openPull(app, alice, repo, {
      title: branch,
      body: "",
      baseBranch: "main",
@@ -86,3 +91,39 @@
test("an unknown repository still names session", async () => {
  expect(got.status).toBe(404);
  expect(got.names).toEqual(["session"]);
});
test("130 open pulls read their mergeability from the cache: the merge phase is cheap and spawns no git", async () => {
  for (let i = 0; i < 128; i++) {
    const branch = `b${i}`;
    const done = await commitToBranch(app, alice, repo, {
      branch,
      message: "change",
      files: [{ path: `${branch}.txt`, content: "x" }],
    });
    if (!done.ok) throw new Error(JSON.stringify(done.error));
    const pull = await openPull(app, alice, repo, {
      title: branch,
      body: "",
      baseBranch: "main",
      headBranch: branch,
    });
    if (!pull.ok) throw new Error(JSON.stringify(pull.error));
  }
  const mergeMs = async (): Promise<number> => {
    const res = await fetch(`${servers.url}/api/v1/repos/alice/r/pulls?limit=200`, { headers: { cookie } });
    const items = (JSON.parse(await res.text()) as { items: unknown[] }).items;
    expect(items.length).toBeGreaterThanOrEqual(130);
    const m = /merge;dur=([\d.]+)/.exec(res.headers.get("server-timing") ?? "");
    return Number(m?.[1] ?? NaN);
  };
  const uncached = await mergeMs();
  const spawn = spyOn(Bun, "spawn");
  try {
    let best = Infinity;
    for (let k = 0; k < 5; k++) best = Math.min(best, await mergeMs());
    expect(spawn.mock.calls.filter((c) => (c[0] as string[]).includes("merge-tree")).length).toBe(0);
    expectBudget(best, { ms: 20, against: uncached / 100 });
  } finally {
    spawn.mockRestore();
  }
      
        junipercrew
        17d ago
        
      
      If the Server-Timing header ever lacks merge, best is NaN and the failure reads oddly. Assert the header matched first so the failure names the cause.
    
}, 120_000);
@@ -4,10 +4,13 @@
import { tmpdir } from "node:os";
import { join } from "node:path";
import { readTips } from "../src/git/tips.ts";
import { pullMergeStates, type Pull } from "../src/core/pulls.ts";
import { expectBudget } from "./helpers/budget.ts";
import { gitClientEnv } from "./helpers/git-client.ts";
const env = gitClientEnv();
      
        junipercrew
        18d ago
        outdated
      
      The issue names 130 open pulls. Use N = 130 so the test proves the stated number, not a nearby one.
    
const N = 50;
const N = 130;
/** The uncached read spawns git per pull; a cached one should cost a small fraction of it. */
const BASELINE_DIVISOR = 100;
let root: string;
let dir: string;
@@ -54,16 +57,26 @@
beforeEach(async () => {
afterEach(() => rm(root, { recursive: true, force: true }));
test("a second read of 50 pulls does no git work; a moved head recomputes only its pull", async () => {
test("a second read of 130 pulls does no git work and is cheap beside the first; a moved head recomputes only its pull", async () => {
  const items = Array.from({ length: N }, (_, i) => ({ dir, pull: pullOf(i) }));
  const spawn = spyOn(Bun, "spawn");
  try {
    const t1 = performance.now();
    const first = await pullMergeStates(items);
    const uncached = performance.now() - t1;
      
        junipercrew
        18d ago
        outdated
      
      A single wall-clock measurement against 20 ms for 100+ ref-file reads will flake when the check machine is busy. The direction asks for tests that never flake. Time several cached reads and assert on the minimum, or drop the timing assertion and rely on the zero-spawn assertion below it.
    
    expect(first.every((m) => m?.mergeable === true)).toBe(true);
    expect(spawn.mock.calls.length).toBeGreaterThan(0);
    spawn.mockClear();
    const second = await pullMergeStates(items);
    // Best of a few cached reads: a busy machine slows one, not all.
    let best = Infinity;
      
        junipercrew
        17d ago
        outdated
      
      Best-of-5 is a sound way to keep a wall-clock bound from flaking. If it still flakes, keep the zero-spawn assertion and drop this bound.
    
    let second = first;
    for (let k = 0; k < 5; k++) {
      const t0 = performance.now();
      second = await pullMergeStates(items);
      best = Math.min(best, performance.now() - t0);
    }
      
        junipercrew
        17d ago
        
      
      Good: relative to the same-run uncached time, best of five. If it flakes, widen the loop rather than the ms.
    
    expectBudget(best, { ms: 20, against: uncached / BASELINE_DIVISOR });
      
        junipercrew
        17d ago
        
      
      Good use of expectBudget. The ratio of 100 is a magic constant; it is named and commented, which is enough. If it flakes on a loaded cell, widen the divisor rather than the ms.
    
    expect(spawn.mock.calls.length).toBe(0);
    expect(second).toEqual(first);
		@@ -1,5 +1,5 @@
1	1	/** The API's list of pulls names its Server-Timing phases (#3964). */
2		import { afterAll, beforeAll, expect, test } from "bun:test";
	2	import { afterAll, beforeAll, expect, spyOn, test } from "bun:test";
3	3	import { mkdtemp, rm } from "node:fs/promises";
4	4	import { tmpdir } from "node:os";
5	5	import { join } from "node:path";
		@@ -7,8 +7,10 @@ import { createApp, type App } from "../src/app.ts";
7	7	import { openPull } from "../src/actions/pulls.ts";
8	8	import { commitToBranch, createRepo } from "../src/actions/repos.ts";
9	9	import { createSession } from "../src/core/sessions.ts";
10		import { createUser } from "../src/core/users.ts";
	10	import type { Repo } from "../src/core/repos.ts";
	11	import { createUser, type User } from "../src/core/users.ts";
11	12	import { serve, type Servers } from "../src/http/server.ts";
	13	import { expectBudget } from "./helpers/budget.ts";
12	14	import { quietLogs } from "./helpers/globals.ts";
13	15
14	16	quietLogs();
		@@ -17,6 +19,8 @@ let dataDir: string;
17	19	let app: App;
18	20	let servers: Servers;
19	21	let cookie = "";
	22	let alice: User;
	23	let repo: Repo;
20	24	const cells = process.env["FORGE_CELLS"];
21	25
22	26	beforeAll(async () => {
		@@ -33,23 +37,24 @@ beforeAll(async () => {
33	37	adminIfFirst: true,
34	38	});
35	39	if (!made.ok) throw new Error(String(made.error));
36		const alice = made.value;
	40	alice = made.value;
37	41	cookie = `forge_session=${createSession(app.db, alice.id).token}`;
38		const repo = await createRepo(app, alice, { name: "r", visibility: "public" });
39		if (!repo.ok) throw new Error(String(repo.error));
40		const first = await commitToBranch(app, alice, repo.value, {
	42	const created = await createRepo(app, alice, { name: "r", visibility: "public" });
	43	if (!created.ok) throw new Error(String(created.error));
	44	repo = created.value;
	45	const first = await commitToBranch(app, alice, repo, {
41	46	message: "change",
42	47	files: [{ path: "forge.json", content: "{}" }],
43	48	});
44	49	if (!first.ok) throw new Error(JSON.stringify(first.error));
45	50	for (const branch of ["one", "two"]) {
46		const done = await commitToBranch(app, alice, repo.value, {
	51	const done = await commitToBranch(app, alice, repo, {
47	52	branch,
48	53	message: "change",
49	54	files: [{ path: `${branch}.txt`, content: "x" }],
50	55	});
51	56	if (!done.ok) throw new Error(JSON.stringify(done.error));
52		const pull = await openPull(app, alice, repo.value, {
	57	const pull = await openPull(app, alice, repo, {
53	58	title: branch,
54	59	body: "",
55	60	baseBranch: "main",
		@@ -86,3 +91,39 @@ test("an unknown repository still names session", async () => {
86	91	expect(got.status).toBe(404);
87	92	expect(got.names).toEqual(["session"]);
88	93	});
	94
	95	test("130 open pulls read their mergeability from the cache: the merge phase is cheap and spawns no git", async () => {
	96	for (let i = 0; i < 128; i++) {
	97	const branch = `b${i}`;
	98	const done = await commitToBranch(app, alice, repo, {
	99	branch,
	100	message: "change",
	101	files: [{ path: `${branch}.txt`, content: "x" }],
	102	});
	103	if (!done.ok) throw new Error(JSON.stringify(done.error));
	104	const pull = await openPull(app, alice, repo, {
	105	title: branch,
	106	body: "",
	107	baseBranch: "main",
	108	headBranch: branch,
	109	});
	110	if (!pull.ok) throw new Error(JSON.stringify(pull.error));
	111	}
	112	const mergeMs = async (): Promise<number> => {
	113	const res = await fetch(`${servers.url}/api/v1/repos/alice/r/pulls?limit=200`, { headers: { cookie } });
	114	const items = (JSON.parse(await res.text()) as { items: unknown[] }).items;
	115	expect(items.length).toBeGreaterThanOrEqual(130);
	116	const m = /merge;dur=([\d.]+)/.exec(res.headers.get("server-timing") ?? "");
	117	return Number(m?.[1] ?? NaN);
	118	};
	119	const uncached = await mergeMs();
	120	const spawn = spyOn(Bun, "spawn");
	121	try {
	122	let best = Infinity;
	123	for (let k = 0; k < 5; k++) best = Math.min(best, await mergeMs());
	124	expect(spawn.mock.calls.filter((c) => (c[0] as string[]).includes("merge-tree")).length).toBe(0);
	125	expectBudget(best, { ms: 20, against: uncached / 100 });
	126	} finally {
	127	spawn.mockRestore();
	128	}
		junipercrew 17d ago If the Server-Timing header ever lacks merge, best is NaN and the failure reads oddly. Assert the header matched first so the failure names the cause.
	129	}, 120_000);
		@@ -4,10 +4,13 @@ import { tmpdir } from "node:os";
4	4	import { join } from "node:path";
5	5	import { readTips } from "../src/git/tips.ts";
6	6	import { pullMergeStates, type Pull } from "../src/core/pulls.ts";
	7	import { expectBudget } from "./helpers/budget.ts";
7	8	import { gitClientEnv } from "./helpers/git-client.ts";
8	9
9	10	const env = gitClientEnv();
		junipercrew 18d ago outdated The issue names 130 open pulls. Use N = 130 so the test proves the stated number, not a nearby one.
10		const N = 50;
	11	const N = 130;
	12	/** The uncached read spawns git per pull; a cached one should cost a small fraction of it. */
	13	const BASELINE_DIVISOR = 100;
11	14	let root: string;
12	15	let dir: string;
13	16
		@@ -54,16 +57,26 @@ beforeEach(async () => {
54	57
55	58	afterEach(() => rm(root, { recursive: true, force: true }));
56	59
57		test("a second read of 50 pulls does no git work; a moved head recomputes only its pull", async () => {
	60	test("a second read of 130 pulls does no git work and is cheap beside the first; a moved head recomputes only its pull", async () => {
58	61	const items = Array.from({ length: N }, (_, i) => ({ dir, pull: pullOf(i) }));
59	62	const spawn = spyOn(Bun, "spawn");
60	63	try {
	64	const t1 = performance.now();
61	65	const first = await pullMergeStates(items);
	66	const uncached = performance.now() - t1;
		junipercrew 18d ago outdated A single wall-clock measurement against 20 ms for 100+ ref-file reads will flake when the check machine is busy. The direction asks for tests that never flake. Time several cached reads and assert on the minimum, or drop the timing assertion and rely on the zero-spawn assertion below it.
62	67	expect(first.every((m) => m?.mergeable === true)).toBe(true);
63	68	expect(spawn.mock.calls.length).toBeGreaterThan(0);
64	69
65	70	spawn.mockClear();
66		const second = await pullMergeStates(items);
	71	// Best of a few cached reads: a busy machine slows one, not all.
	72	let best = Infinity;
		junipercrew 17d ago outdated Best-of-5 is a sound way to keep a wall-clock bound from flaking. If it still flakes, keep the zero-spawn assertion and drop this bound.
	73	let second = first;
	74	for (let k = 0; k < 5; k++) {
	75	const t0 = performance.now();
	76	second = await pullMergeStates(items);
	77	best = Math.min(best, performance.now() - t0);
	78	}
		junipercrew 17d ago Good: relative to the same-run uncached time, best of five. If it flakes, widen the loop rather than the ms.
	79	expectBudget(best, { ms: 20, against: uncached / BASELINE_DIVISOR });
		junipercrew 17d ago Good use of expectBudget. The ratio of 100 is a magic constant; it is named and commented, which is enough. If it flakes on a loaded cell, widen the divisor rather than the ms.
67	80	expect(spawn.mock.calls.length).toBe(0);
68	81	expect(second).toEqual(first);
69	82