rook/val Public Public: anyone can read this; nobody can change it.
forked from benjivers/val
workbenchthis machine
Stop the frame test measuring a view nobody chose
The client test failed about one full-suite run in twenty-five, always the same way: `expect(fp.fills).toBeGreaterThan(40)` receiving 31. fable hit it before touching anything, flagged it as seed-dependent, and left it alone; this is what it actually is. The loop above the measurement advances the SHARED match until somebody dies — anywhere between five and two hundred seconds of play — and then the test counts how many fills the resulting view costs. So the thing being measured was wherever that loop happened to leave the seat. Run on its own the file draws 154 to 236 fills across twelve runs; the run that failed drew 31. The bound was a floor nobody chose over a view nobody fixed. Two changes, and the second is the one that matters: The seat is stood in a known place looking a known way before the frame is counted, so the walls in shot are the same every run. The few fills that still move are the bodies standing among them. And the bound now says what run-batching MEANS: fewer fills than half the columns the frame actually used, read off the client's own offscreen buffer, which the fake document now keeps a handle on. A constant cannot express this — the frame budget moves the world between 960 and 448 columns, so a per-column frame at the coarsest rung is 448 fills, which sits comfortably under any constant loose enough to survive the finest rung. My own first attempt at this used `< 450` and would have let exactly that through. Proved both directions rather than assuming: - forced the renderer to emit one run per column behind a flag, and the new bound fails as it should; the old constant would not have. - 30 full-suite runs, 0 failures (was ~1 in 25). 304 tests, unchanged. Also added: the same state drawn twice must cost the same. A frame whose cost wanders while nothing moves is a renderer bug, and it is the shape of thing the old bound was mistaking for flakiness.
1 file
changed +60
−10
src/client.test.ts +60 −10
| @@ -9,6 +9,8 @@ | ||
| 9 | 9 | */ |
| 10 | 10 | |
| 11 | 11 | import { expect, test } from "bun:test"; |
| 12 | import { TICK } from "./arena.ts"; | |
| 13 | import { HEIGHT, WIDTH } from "./map.ts"; | |
| 12 | 14 | import { mkdtempSync } from "node:fs"; |
| 13 | 15 | import { tmpdir } from "node:os"; |
| 14 | 16 | import { join } from "node:path"; |
| @@ -21,6 +23,15 @@ const base = `http://${server.hostname}:${server.port}`; | ||
| 21 | 23 | // ------------------------------------------------------------ the stub DOM |
| 22 | 24 | |
| 23 | 25 | const drawn: string[] = []; |
| 26 | /** | |
| 27 | * Every canvas the client asked the document for. The widest is its offscreen | |
| 28 | * world buffer, whose width is however many columns the frame budget settled | |
| 29 | * on; the other is the minimap's tile layer. Keeping them is what lets a test | |
| 30 | * bound the frame's cost as a fraction of the columns actually used, instead | |
| 31 | * of against a number somebody picked once. | |
| 32 | */ | |
| 33 | const canvases: Record<string, unknown>[] = []; | |
| 34 | const worldColumns = (): number => Math.max(0, ...canvases.map((c) => Number(c.width) || 0)); | |
| 24 | 35 | // Where the canvas was moved to: on the top-down view your own body is an |
| 25 | 36 | // arrow drawn about the origin, so the last translate of a frame is where |
| 26 | 37 | // this client thinks you are. |
| @@ -131,7 +142,11 @@ Object.assign(globalThis, { | ||
| 131 | 142 | getElementById: (id: string) => nodes.get(id) ?? null, |
| 132 | 143 | querySelector: (sel: string) => (sel.includes("red") ? scoreRed : scoreBlue), |
| 133 | 144 | querySelectorAll: (sel: string) => (sel.includes("data-buy") ? buys : agentButtons), |
| 134 | createElement: (tag: string) => element(tag), | |
| 145 | createElement: (tag: string) => { | |
| 146 | const node = element(tag); | |
| 147 | if (tag === "canvas") canvases.push(node); | |
| 148 | return node; | |
| 149 | }, | |
| 135 | 150 | addEventListener: noop, |
| 136 | 151 | pointerLockElement: null, |
| 137 | 152 | exitPointerLock: noop, |
| @@ -248,13 +263,34 @@ test("the bundled client connects, paints the HUD and draws a frame", async () = | ||
| 248 | 263 | expect(row).not.toMatch(/\d/); |
| 249 | 264 | |
| 250 | 265 | // A frame of the first-person view draws the world into its own buffer and |
| 251 | // blits it, with the minimap's tiles blitted too. It is a picture, but it | |
| 252 | // is not one fill a screen column any more, nor one a map tile: the map has | |
| 253 | // 570 tiles and the canvas 900 columns, and the frame used to pay for both. | |
| 254 | // The minimap's tiles are the blit; the walls are a run a fill, and how | |
| 255 | // many runs a view has depends on where the seat happens to be standing — | |
| 256 | // a long corridor is many depths — so the bound is the columns, which no | |
| 257 | // view can reach, rather than the tile count, which a corridor can. | |
| 266 | // blits it, with the minimap's tiles blitted too. It is a picture, but it is | |
| 267 | // not one fill a screen column, nor one a map tile: the map has 570 tiles | |
| 268 | // and the buffer up to 960 columns, and the frame used to pay for both. | |
| 269 | // The minimap's tiles are the blit; the walls are a run a fill. | |
| 270 | // | |
| 271 | // What is asserted is that run-batching, and NOT a particular count. This | |
| 272 | // used to demand more than 40 fills, which failed about one full-suite run | |
| 273 | // in twenty-five while passing forty runs out of forty on its own. The loop | |
| 274 | // above advances the shared match until somebody dies — anywhere between | |
| 275 | // five and two hundred seconds of play — so the view being measured was | |
| 276 | // wherever that happened to leave the seat: 154 to 236 fills when run | |
| 277 | // alone, and 31 on the run that failed. A floor on a number nobody chose, | |
| 278 | // over a view nobody fixed, is not a test. | |
| 279 | // | |
| 280 | // So the seat is stood somewhere known first. The walls in shot are then | |
| 281 | // the same every run; the few fills that still move are the bodies standing | |
| 282 | // among them, which is why the bound below is a fraction rather than an | |
| 283 | // equality. | |
| 284 | const arenaNow = await import("./arena.ts"); | |
| 285 | const camera = arenaNow.match().players.find((p) => !p.bot)!; | |
| 286 | camera.x = WIDTH / 2; | |
| 287 | camera.y = HEIGHT / 2; | |
| 288 | camera.angle = 0; | |
| 289 | camera.alive = true; | |
| 290 | camera.blind = 0; | |
| 291 | arenaNow.advance(TICK * 2); // a snapshot carrying that position reaches the client | |
| 292 | await until(() => true); | |
| 293 | ||
| 258 | 294 | const tally = () => ({ |
| 259 | 295 | fills: drawn.filter((op) => op.startsWith("fillRect")).length, |
| 260 | 296 | blits: drawn.filter((op) => op.startsWith("drawImage")).length, |
| @@ -263,9 +299,23 @@ test("the bundled client connects, paints the HUD and draws a frame", async () = | ||
| 263 | 299 | drawn.length = 0; |
| 264 | 300 | runFrame(); |
| 265 | 301 | const fp = tally(); |
| 302 | drawn.length = 0; | |
| 303 | runFrame(); | |
| 304 | const again = tally(); | |
| 305 | ||
| 266 | 306 | expect(fp.blits).toBeGreaterThanOrEqual(2); // the world, and the map's tiles |
| 267 | expect(fp.fills).toBeGreaterThan(40); | |
| 268 | expect(fp.fills).toBeLessThan(900); | |
| 307 | // It drew a world, not an empty buffer. | |
| 308 | expect(fp.fills).toBeGreaterThan(0); | |
| 309 | // Run-batched, said against the columns the frame ACTUALLY used. The budget | |
| 310 | // moves the world between 960 and 448 columns depending on what frames cost, | |
| 311 | // so no constant can separate "a run a wall" from "a fill a column": at the | |
| 312 | // coarsest rung a per-column frame is 448, which sits under any constant | |
| 313 | // loose enough to survive the finest one. | |
| 314 | expect(fp.fills).toBeLessThan(worldColumns() / 2); | |
| 315 | // The same state drawn twice costs the same. A frame whose cost wanders | |
| 316 | // while nothing moves is a renderer bug, and it is the shape of thing the | |
| 317 | // old bound was mistaking for flakiness. | |
| 318 | expect(again.fills).toBe(fp.fills); | |
| 269 | 319 | |
| 270 | 320 | // Tab swaps that frame for the whole map. Its tiles are drawn once into a |
| 271 | 321 | // canvas of their own and blitted from then on: the first such frame pays |