From dd294224c2b183571bea06e3e01b79f6357757e3 Mon Sep 17 00:00:00 2001 From: Bryandero98 Date: Thu, 3 Sep 2026 23:56:23 -0500 Subject: [PATCH] test(sprites): hoist lockAllAnchors fixture onto a real-locked prototype (#6180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 136 call sites across walk.test.js/atlas.test.js/animationTrackWorkflow.test.js locked a character from byte-identical cached candidate PNGs, so each one re-ran the real Sharp normalize + chroma-key-selection pipeline from scratch. `lockAllAnchors` now locks ONE real prototype per TEST_ROOT (at the full anchor set, since every caller's `directions` is a subset of it) and materializes every other character by recursive-copying only the artifacts a caller actually asked for, id-normalizing the manifest, and resetting any anchor outside the requested set to the exact seeded-pending shape a fresh partial lock would leave (mirroring unlockReferenceAnchorImpl's own reset). No call site changed except the three that build the record + pass `records` through (`characterWithLockedAnchors`, atlas.test.js's `lockAllAnchors(id)`, animationTrackWorkflow.test.js's `characterWithEastAnchor`) — the fast path lives entirely inside spriteTestFixtures.js. Each suite gets an equivalence test (the acceptance gate, not an extra, per the issue): it builds a character both ways — through `lockAllAnchorsReal` (exported unchanged) and through the materialized path — and asserts the result is byte-identical after id normalization: file names, PNG bytes, the full manifest (including every embedded sha256), and the record's chromaKey/status. The partial-set case is the one this test actually earns its keep on: an early version of the reset logic left the `south` anchor entry wrongly reset to pending (it's synced from mainReference, never locked through its own `target`) and left a partial manifest's `status` at 'complete' instead of 'in-progress' — both caught by this test, not by eye. Measured locally (Windows, before/after, `npx vitest run `): walk.test.js: ~29.4s -> ~19.7-21.4s tests phase (~30% faster) atlas.test.js: ~24.4s -> ~20.2-21.3s tests phase (~15% faster) atlas.test.js's own numbers are lower than walk.test.js's because it only has 19 lockAllAnchors calls (all at the full 9-anchor set) vs walk.test.js's 117 (mostly 1-3 anchors) -- fewer calls to save Sharp work on, even though each one touches more anchors. buildFinalizedWalkSet (atlas.test.js's other named fixture cost per the issue, ~32% of its suite time) is untouched here: it never calls Sharp (writeWalkFramePng already caches the encoded buffer), so a copy-based materialization of it doesn't help -- see the companion PR for what does. Full sprites/ suite: 716 passed, 1 skipped, 0 failed (unchanged pass count). Co-Authored-By: Claude Sonnet 5 --- .../sprites/animationTrackWorkflow.test.js | 2 +- server/services/sprites/atlas.test.js | 46 ++++- server/services/sprites/spriteTestFixtures.js | 171 +++++++++++++++++- server/services/sprites/walk.test.js | 69 ++++++- 4 files changed, 280 insertions(+), 8 deletions(-) diff --git a/server/services/sprites/animationTrackWorkflow.test.js b/server/services/sprites/animationTrackWorkflow.test.js index cdcbe225db..9aebdc95a2 100644 --- a/server/services/sprites/animationTrackWorkflow.test.js +++ b/server/services/sprites/animationTrackWorkflow.test.js @@ -136,7 +136,7 @@ const newId = () => `track-${++sequence}`; // the record's single locked main. That is the registry difference under test. async function characterWithEastAnchor(id) { await records.createRecord({ kind: 'character', name: 'Placeholder Hero' }, id); - await lockAllAnchors(TEST_ROOT, id, { lockReference, directions: ['east'] }); + await lockAllAnchors(TEST_ROOT, id, { lockReference, directions: ['east'], records }); return id; } diff --git a/server/services/sprites/atlas.test.js b/server/services/sprites/atlas.test.js index 2219cce0a2..ac1d6b680e 100644 --- a/server/services/sprites/atlas.test.js +++ b/server/services/sprites/atlas.test.js @@ -20,9 +20,11 @@ import { mkdir, writeFile, readFile, rm } from 'fs/promises'; import { createHash } from 'crypto'; import { lockAllAnchors as lockAllAnchorsFixture, + lockAllAnchorsReal, placeCandidate, trackSpan as fullSpan, writeWalkFramePng, + normalizeManifestForComparison, } from './spriteTestFixtures.js'; const TEST_ROOT = mkdtempSync(join(tmpdir(), 'sprite-atlas-test-')); @@ -69,7 +71,7 @@ const sha256 = (buf) => createHash('sha256').update(buf).digest('hex'); async function lockAllAnchors(id) { await records.createRecord({ kind: 'character', name: 'Atlas Walker' }, id); - await lockAllAnchorsFixture(TEST_ROOT, id, { lockReference, directions: SPRITE_DIRECTIONS }); + await lockAllAnchorsFixture(TEST_ROOT, id, { lockReference, directions: SPRITE_DIRECTIONS, records }); } const walkFramePng = writeWalkFramePng; @@ -300,6 +302,48 @@ async function replaceFinalizedFrame(recordId, direction, index, bytes) { return frame.phase; } +// #6180 — this suite's own `lockAllAnchors(id)` wrapper always requests the +// full SPRITE_DIRECTIONS set, so its 19 call sites are all fully covered by +// spriteTestFixtures.js's materialized fast path. This is the acceptance +// gate for that: it proves a materialized character is byte-for-byte +// indistinguishable (after id normalization) from one the real per-anchor +// Sharp lock pipeline built. +describe('lockAllAnchors fixture materialization (#6180)', () => { + it('matches the real lock pipeline byte-for-byte (file names, PNG bytes, manifest, record state) after id normalization', async () => { + const realId = newId(); + await records.createRecord({ kind: 'character', name: 'Atlas Walker' }, realId); + await lockAllAnchorsReal(TEST_ROOT, realId, { lockReference, directions: SPRITE_DIRECTIONS }); + const fastId = newId(); + await lockAllAnchors(fastId); + + const { listSpriteAssets } = await import('./paths.js'); + const [realAssets, fastAssets] = await Promise.all([ + listSpriteAssets(realId, { subdir: 'reference', metadata: false }), + listSpriteAssets(fastId, { subdir: 'reference', metadata: false }), + ]); + const idNormalizedNames = (id, assets) => assets.map((a) => a.path.split(id).join('')).sort(); + expect(idNormalizedNames(fastId, fastAssets)).toEqual(idNormalizedNames(realId, realAssets)); + + for (const asset of realAssets.filter((a) => a.path.endsWith('.png'))) { + const fastRel = asset.path.split(realId).join(fastId); + const [realBytes, fastBytes] = await Promise.all([ + readFile(join(TEST_ROOT, 'sprites', realId, asset.path)), + readFile(join(TEST_ROOT, 'sprites', fastId, fastRel)), + ]); + expect(fastBytes.equals(realBytes)).toBe(true); + } + + const [realManifest, fastManifest] = await Promise.all([loadManifest(realId), loadManifest(fastId)]); + expect(normalizeManifestForComparison(fastManifest, fastId)) + .toEqual(normalizeManifestForComparison(realManifest, realId)); + + const [realRecord, fastRecord] = await Promise.all([records.getRecord(realId), records.getRecord(fastId)]); + expect(fastRecord.chromaKey).toBe(realRecord.chromaKey); + expect(fastRecord.status).toBe(realRecord.status); + expect(fastRecord.status).toBe('reference-complete'); + }); +}); + beforeEach(() => { rmSync(join(TEST_ROOT, 'sprite-records.json'), { force: true }); }); diff --git a/server/services/sprites/spriteTestFixtures.js b/server/services/sprites/spriteTestFixtures.js index 8544a11d4a..93b7d7b834 100644 --- a/server/services/sprites/spriteTestFixtures.js +++ b/server/services/sprites/spriteTestFixtures.js @@ -17,8 +17,10 @@ */ import { join } from 'path'; import sharp from 'sharp'; -import { mkdir, writeFile } from 'fs/promises'; -import { SPRITE_DIRECTIONS } from './prompts.js'; +import { + mkdir, writeFile, readFile, copyFile, +} from 'fs/promises'; +import { SPRITE_DIRECTIONS, ANCHOR_DIRECTIONS } from './prompts.js'; // Sharp PNG encoding is the expensive part; the same raw pixels show up in // dozens of tests. Cache the encoded buffer per unique pixel config and @@ -116,7 +118,12 @@ export async function placeCandidate(testRoot, recordId, target, name, opts = {} // and atlas.test.js's lockAllAnchors (which only differ in which directions // they pass and how they name/create the character beforehand). The turnaround // comes first because anchors are gated on it (#2979). -export async function lockAllAnchors(testRoot, recordId, { lockReference, directions }) { +// +// This is the REAL pipeline (real Sharp normalize + chroma-key selection per +// anchor) — exported so callers that need a genuine from-scratch lock (the +// equivalence tests, and `getOrBuildLockPrototype` below) can still reach it. +// `lockAllAnchors` below is the fast path everything else uses. +export async function lockAllAnchorsReal(testRoot, recordId, { lockReference, directions }) { await lockReference(recordId, { target: 'turnaround', candidate: await placeCandidate(testRoot, recordId, 'turnaround', 'turnaround-candidate-01.png'), @@ -133,6 +140,164 @@ export async function lockAllAnchors(testRoot, recordId, { lockReference, direct } } +// #6180 — every `lockAllAnchors` caller in walk.test.js/atlas.test.js locks a +// character from BYTE-IDENTICAL cached candidate PNGs (the same fixture +// buffers every other candidate reuses), so 136 call sites were re-deriving +// the same normalize + chroma-key-selection tree over and over. One real lock +// per TEST_ROOT — always at the FULL anchor set, since every caller's +// `directions` is a subset of it — then every subsequent call recursive-copies +// that prototype's `reference/` tree and trims it to the requested subset. +// +// Keyed on testRoot (not a bare module-level singleton) so two suites sharing +// this module in one Vitest worker can't leak a prototype across each other's +// isolated tmpdir. +const lockPrototypes = new Map(); +const LOCK_PROTOTYPE_ID = 'lock-fixture-prototype'; + +function getOrBuildLockPrototype(testRoot, { lockReference, records }) { + let pending = lockPrototypes.get(testRoot); + if (!pending) { + pending = (async () => { + await records.createRecord({ kind: 'character', name: 'Lock Fixture Prototype' }, LOCK_PROTOTYPE_ID); + await lockAllAnchorsReal(testRoot, LOCK_PROTOTYPE_ID, { lockReference, directions: ANCHOR_DIRECTIONS }); + return LOCK_PROTOTYPE_ID; + })(); + lockPrototypes.set(testRoot, pending); + } + return pending; +} + +// Copy one locked artifact PNG from the prototype's id-stamped filename to the +// target's — driven by the ACTUAL filename the prototype's own lock wrote +// (read off the just-loaded manifest), not by re-deriving the naming +// convention, so this can't drift out of sync with reference.js's own scheme. +async function copyLockedArtifact(protoDir, targetDir, prototypeId, recordId, rel) { + const slash = rel.lastIndexOf('/'); + const dir = rel.slice(0, slash + 1); + const filename = rel.slice(slash + 1); + const newRel = filename.startsWith(`${prototypeId}-`) + ? `${dir}${recordId}-${filename.slice(prototypeId.length + 1)}` + : rel; // defensive; shouldn't happen + await mkdir(join(targetDir, dir), { recursive: true }); + await copyFile(join(protoDir, rel), join(targetDir, newRel)); + return newRel; +} + +// The candidate a locked artifact's `lockedFrom` names never carries the id +// (placeCandidate's filenames are direction-only), so it copies straight +// across — but its `.generation.json` sidecar rides along on the same path. +async function copyCandidate(protoDir, targetDir, rel) { + await mkdir(join(targetDir, rel, '..'), { recursive: true }); + await copyFile(join(protoDir, rel), join(targetDir, rel)); + const sidecarRel = rel.replace(/\.png$/, '.generation.json'); + await copyFile(join(protoDir, sidecarRel), join(targetDir, sidecarRel)); +} + +// Materialize `recordId` from the prototype WITHOUT paying for anchors nobody +// asked for: copy only the turnaround, the main, and each requested anchor +// (artifact + the candidate it was locked from), id-normalize the manifest, +// and reset every anchor NOT in `directions` to the exact seeded-pending shape +// `unlockReferenceAnchorImpl` (reference.js) produces — so a caller asking for +// one direction pays for one direction's files, not the prototype's full nine, +// and a caller asking for a partial set gets a manifest indistinguishable from +// one where only those anchors were ever locked. PNG bytes are copied +// verbatim, so every embedded `sha256` for a copied artifact stays valid with +// no recompute. +async function materializeLockedAnchors(testRoot, prototypeId, recordId, { directions, records }) { + const protoDir = join(testRoot, 'sprites', prototypeId); + const targetDir = join(testRoot, 'sprites', recordId); + const protoManifest = JSON.parse( + await readFile(join(protoDir, `reference/${prototypeId}-reference-set-v1.json`), 'utf8'), + ); + const copyArtifact = (rel) => copyLockedArtifact(protoDir, targetDir, prototypeId, recordId, rel); + + const manifest = { ...protoManifest, manifestId: `${recordId}-reference-set-v1`, characterFamily: recordId }; + + manifest.turnaround = { ...protoManifest.turnaround, path: await copyArtifact(protoManifest.turnaround.path) }; + await copyCandidate(protoDir, targetDir, protoManifest.turnaround.lockedFrom); + + manifest.mainReference = { + ...protoManifest.mainReference, path: await copyArtifact(protoManifest.mainReference.path), + }; + await copyCandidate(protoDir, targetDir, protoManifest.mainReference.lockedFrom); + + const requested = new Set(directions.filter((d) => d !== 'south')); + manifest.anchors = await Promise.all(protoManifest.anchors.map(async (protoAnchor) => { + if (protoAnchor.direction === 'south') { + // South is never locked through its own `target` — the main lock syncs + // this entry directly from the SAME rel/candidate/sha256 it just wrote + // to mainReference (reference.js's lockReferenceImpl), so it mirrors + // the just-copied mainReference 1:1 regardless of what `directions` asked for. + return { + ...protoAnchor, + status: 'locked', + path: manifest.mainReference.path, + lockedFrom: manifest.mainReference.lockedFrom, + sha256: manifest.mainReference.sha256, + }; + } + if (!requested.has(protoAnchor.direction)) { + // The exact shape seedManifest() gives a never-generated anchor. + return { + id: protoAnchor.id, kind: protoAnchor.kind, direction: protoAnchor.direction, + status: 'pending', source: 'derive-from-turnaround', + }; + } + const path = await copyArtifact(protoAnchor.path); + await copyCandidate(protoDir, targetDir, protoAnchor.lockedFrom); + return { ...protoAnchor, path }; + })); + + // The prototype is always fully locked, so its manifest.status is 'complete' + // — a real PARTIAL lock never reaches that (lockReferenceImpl only sets it + // inside the all-anchors-locked branch; otherwise it stays at the value the + // main lock left, 'in-progress'). Re-derive it from what actually got + // materialized above, or a partial character would misreport as finished. + const allLocked = manifest.anchors.every((a) => a.status === 'locked'); + manifest.status = allLocked ? 'complete' : 'in-progress'; + + await mkdir(join(targetDir, 'reference'), { recursive: true }); + await writeFile(join(targetDir, `reference/${recordId}-reference-set-v1.json`), JSON.stringify(manifest)); + + // Mirrors lockReferenceImpl's own record side effects: the turnaround lock + // sets {chromaKey, status:'reference'}; the record only reaches + // 'reference-complete' once every anchor (the full ANCHOR_DIRECTIONS set) is + // locked — never on a partial `directions` subset. + await records.updateRecord(recordId, { + chromaKey: manifest.chromaKey, + status: allLocked ? 'reference-complete' : 'reference', + }); +} + +export async function lockAllAnchors(testRoot, recordId, { lockReference, directions, records }) { + const prototypeId = await getOrBuildLockPrototype(testRoot, { lockReference, records }); + await materializeLockedAnchors(testRoot, prototypeId, recordId, { directions, records }); +} + +/** + * Deep-clone `manifest`, strip every `lockedAt` timestamp (real and + * materialized locks never share a clock tick), and replace every occurrence + * of `id` inside a string value with `` — the normalization the #6180 + * equivalence tests in walk.test.js/atlas.test.js both need to compare a + * real-locked manifest against a materialized one. + */ +export function normalizeManifestForComparison(manifest, id) { + const clone = JSON.parse(JSON.stringify(manifest)); + const strip = (node) => { + if (Array.isArray(node)) { + node.forEach(strip); + } else if (node && typeof node === 'object') { + delete node.lockedAt; + for (const key of Object.keys(node)) { + if (typeof node[key] === 'string') node[key] = node[key].split(id).join(''); + else strip(node[key]); + } + } + }; + strip(clone); + return clone; +} + /** * Assert that `prompt` carries the re-roll correction for `note` (#3216). * diff --git a/server/services/sprites/walk.test.js b/server/services/sprites/walk.test.js index aca886011d..9ededb4e73 100644 --- a/server/services/sprites/walk.test.js +++ b/server/services/sprites/walk.test.js @@ -15,7 +15,9 @@ import { tmpdir } from 'os'; import { join } from 'path'; import { mkdir, writeFile, readFile, rm } from 'fs/promises'; import { createHash } from 'crypto'; -import { lockAllAnchors, expectCarriesCorrection } from './spriteTestFixtures.js'; +import { + lockAllAnchors, lockAllAnchorsReal, expectCarriesCorrection, normalizeManifestForComparison, +} from './spriteTestFixtures.js'; const TEST_ROOT = mkdtempSync(join(tmpdir(), 'sprite-walk-test-')); @@ -181,7 +183,7 @@ vi.mock('../mediaJobQueue/index.js', async (importOriginal) => ({ const records = await import('./records.js'); const { listSpriteAssets } = await import('./paths.js'); -const { lockReference } = await import('./reference.js'); +const { lockReference, loadManifest } = await import('./reference.js'); const { getWalkState, startWalkGeneration, attachTuiWalkResult, approveWalkDirection, rerunWalkPostprocess, unlockWalkSet, reopenWalkDirection, invalidateWalkDirectionForAnchorRevision, @@ -196,7 +198,7 @@ const sha256 = (buf) => createHash('sha256').update(buf).digest('hex'); async function characterWithLockedAnchors(id, directions = ['east']) { await records.createRecord({ kind: 'character', name: 'Walker' }, id); - await lockAllAnchors(TEST_ROOT, id, { lockReference, directions }); + await lockAllAnchors(TEST_ROOT, id, { lockReference, directions, records }); return id; } @@ -336,6 +338,67 @@ async function importedCharacter(id, perDirection) { return runIds; } +// #6180 — characterWithLockedAnchors materializes every locked character from +// a single real-locked prototype (spriteTestFixtures.js's lockAllAnchors) +// instead of re-running the real lock pipeline per test. This is the +// acceptance gate for that refactor: it proves a materialized character is +// indistinguishable from one the real pipeline built, for both the full +// anchor set and a partial one — the partial case is the one where the +// reset-to-pending logic could silently drift from a real partial lock. +describe('lockAllAnchors fixture materialization (#6180)', () => { + it('matches the real lock pipeline byte-for-byte (file names, PNG bytes, manifest, record state) after id normalization', async () => { + const realId = newId(); + await records.createRecord({ kind: 'character', name: 'Walker' }, realId); + await lockAllAnchorsReal(TEST_ROOT, realId, { lockReference, directions: ANCHOR_DIRECTIONS }); + const fastId = await characterWithLockedAnchors(newId(), ANCHOR_DIRECTIONS); + + const [realAssets, fastAssets] = await Promise.all([ + listSpriteAssets(realId, { subdir: 'reference', metadata: false }), + listSpriteAssets(fastId, { subdir: 'reference', metadata: false }), + ]); + const idNormalizedNames = (id, assets) => assets.map((a) => a.path.split(id).join('')).sort(); + expect(idNormalizedNames(fastId, fastAssets)).toEqual(idNormalizedNames(realId, realAssets)); + + for (const asset of realAssets.filter((a) => a.path.endsWith('.png'))) { + const fastRel = asset.path.split(realId).join(fastId); + const [realBytes, fastBytes] = await Promise.all([ + readFile(join(TEST_ROOT, 'sprites', realId, asset.path)), + readFile(join(TEST_ROOT, 'sprites', fastId, fastRel)), + ]); + expect(fastBytes.equals(realBytes)).toBe(true); + } + + const [realManifest, fastManifest] = await Promise.all([loadManifest(realId), loadManifest(fastId)]); + expect(normalizeManifestForComparison(fastManifest, fastId)) + .toEqual(normalizeManifestForComparison(realManifest, realId)); + + const [realRecord, fastRecord] = await Promise.all([records.getRecord(realId), records.getRecord(fastId)]); + expect(fastRecord.chromaKey).toBe(realRecord.chromaKey); + expect(fastRecord.status).toBe(realRecord.status); + }); + + it('resets an anchor outside the requested set to the exact seeded-pending shape a fresh partial lock would leave', async () => { + const realId = newId(); + await records.createRecord({ kind: 'character', name: 'Walker' }, realId); + await lockAllAnchorsReal(TEST_ROOT, realId, { lockReference, directions: ['east'] }); + const fastId = await characterWithLockedAnchors(newId(), ['east']); + + const [realManifest, fastManifest] = await Promise.all([loadManifest(realId), loadManifest(fastId)]); + expect(normalizeManifestForComparison(fastManifest, fastId)) + .toEqual(normalizeManifestForComparison(realManifest, realId)); + + const west = fastManifest.anchors.find((a) => a.direction === 'west'); + expect(west.status).toBe('pending'); + expect(west.path).toBeUndefined(); + expect(west.sha256).toBeUndefined(); + expect(fastManifest.status).toBe('in-progress'); + + const [realRecord, fastRecord] = await Promise.all([records.getRecord(realId), records.getRecord(fastId)]); + expect(fastRecord.status).toBe(realRecord.status); + expect(fastRecord.status).toBe('reference'); + }); +}); + describe('named-track isolation', () => { it('does not surface or reprocess a scanner run through the walk workflow', async () => { const id = await characterWithLockedAnchors(newId(), []);