From 971e1373bdb8dc31180772d328e7f14bc7d853bf Mon Sep 17 00:00:00 2001 From: "Abdullah M. Nazif" Date: Thu, 8 Oct 2026 20:00:27 +0000 Subject: [PATCH] fix(engine): restore discarded warmup timing samples (#5204) --- .../frameCapture-discardWarmup.test.ts | 63 ++++++++++++++----- packages/engine/src/services/frameCapture.ts | 6 +- 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/packages/engine/src/services/frameCapture-discardWarmup.test.ts b/packages/engine/src/services/frameCapture-discardWarmup.test.ts index 8e43fd501e..677da7d91f 100644 --- a/packages/engine/src/services/frameCapture-discardWarmup.test.ts +++ b/packages/engine/src/services/frameCapture-discardWarmup.test.ts @@ -1,6 +1,5 @@ /** - * Tests for `discardWarmupCapture` — the helper distributed chunk workers - * run before their first real capture to prime `lastFrameCache`. + * Tests for `discardWarmupCapture` — the exported helper for discarded captures. * * The helper is a thin wrapper around the inner `captureFrameCore` * machinery, so its testable contract is post-conditional rather than @@ -23,12 +22,14 @@ import { describe, expect, it } from "vitest"; import { existsSync, readdirSync, mkdtempSync, rmSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { discardWarmupCapture, type CaptureSession } from "./frameCapture.js"; +import { + discardWarmupCapture, + getCapturePerfSummary, + type CaptureSession, +} from "./frameCapture.js"; function makeFakeSession(): CaptureSession { - // The discardWarmupCapture wrapper only reads `capturePerf`, - // `beginFrameHasDamageCount`, `beginFrameNoDamageCount`. Everything else - // is unused — leave it as bare-minimum stubs cast through `unknown`. + // Capture is injected; provide the state needed for restoration and perf summaries. return { browser: {} as unknown, page: {} as unknown, @@ -38,12 +39,14 @@ function makeFakeSession(): CaptureSession { onBeforeCapture: null, isInitialized: true, browserConsoleBuffer: [], + warnings: [], capturePerf: { frames: 7, seekMs: 100, beforeCaptureMs: 50, screenshotMs: 200, totalMs: 350, + frameMs: [11, 13, 17, 19, 23, 29, 31], }, captureMode: "beginframe", beginFrameTimeTicks: 0, @@ -90,7 +93,8 @@ describe("discardWarmupCapture", () => { it("restores perf counters after the inner capture mutates them", async () => { const session = makeFakeSession(); - const before = { ...session.capturePerf }; + const before = { ...session.capturePerf, frameMs: [...session.capturePerf.frameMs] }; + const summaryBefore = getCapturePerfSummary(session); try { await discardWarmupCapture(session, 0, 0, async (s) => { s.capturePerf.frames += 1; @@ -98,9 +102,11 @@ describe("discardWarmupCapture", () => { s.capturePerf.beforeCaptureMs += 5; s.capturePerf.screenshotMs += 33; s.capturePerf.totalMs += 50; + s.capturePerf.frameMs.push(50); return { buffer: Buffer.alloc(0), quantizedTime: 0, captureTimeMs: 50 }; }); expect(session.capturePerf).toEqual(before); + expect(getCapturePerfSummary(session)).toEqual(summaryBefore); } finally { rmSync(session.outputDir, { recursive: true, force: true }); } @@ -125,24 +131,24 @@ describe("discardWarmupCapture", () => { it("restores state even when the inner capture throws", async () => { const session = makeFakeSession(); - const perfBefore = { ...session.capturePerf }; + const perfBefore = { ...session.capturePerf, frameMs: [...session.capturePerf.frameMs] }; + const summaryBefore = getCapturePerfSummary(session); const hasBefore = session.beginFrameHasDamageCount; const noBefore = session.beginFrameNoDamageCount; try { - let thrown: unknown; - try { - await discardWarmupCapture(session, 0, 0, async (s) => { + const failure = new Error("simulated capture failure"); + await expect( + discardWarmupCapture(session, 0, 0, async (s) => { s.capturePerf.frames += 5; + s.capturePerf.frameMs.push(50); s.beginFrameNoDamageCount += 2; - throw new Error("simulated capture failure"); - }); - } catch (err) { - thrown = err; - } - expect((thrown as Error).message).toBe("simulated capture failure"); + throw failure; + }), + ).rejects.toBe(failure); // The whole point of `finally { restore }`: failure must not leak // inflated counters into the real capture summary. expect(session.capturePerf).toEqual(perfBefore); + expect(getCapturePerfSummary(session)).toEqual(summaryBefore); expect(session.beginFrameHasDamageCount).toBe(hasBefore); expect(session.beginFrameNoDamageCount).toBe(noBefore); } finally { @@ -150,6 +156,29 @@ describe("discardWarmupCapture", () => { } }); + it.each([false, true])( + "restores samples after their contents are replaced (throws=%s)", + async (throws) => { + const session = makeFakeSession(); + const samplesBefore = [...session.capturePerf.frameMs]; + const summaryBefore = getCapturePerfSummary(session); + const failure = new Error("discarded capture failed"); + try { + const capture = discardWarmupCapture(session, 240, 8, async (s) => { + s.capturePerf.frameMs.splice(0, s.capturePerf.frameMs.length, 9000); + if (throws) throw failure; + return { buffer: Buffer.alloc(0), quantizedTime: 8, captureTimeMs: 0 }; + }); + if (throws) await expect(capture).rejects.toBe(failure); + else await capture; + expect(session.capturePerf.frameMs).toEqual(samplesBefore); + expect(getCapturePerfSummary(session)).toEqual(summaryBefore); + } finally { + rmSync(session.outputDir, { recursive: true, force: true }); + } + }, + ); + it("writes no output file to the session's outputDir", async () => { const session = makeFakeSession(); try { diff --git a/packages/engine/src/services/frameCapture.ts b/packages/engine/src/services/frameCapture.ts index c864018f4b..fb12542a38 100644 --- a/packages/engine/src/services/frameCapture.ts +++ b/packages/engine/src/services/frameCapture.ts @@ -4686,10 +4686,8 @@ export async function discardWarmupCapture( time: number = 0, innerCapture: DiscardWarmupInnerCapture = captureFrameCore, ): Promise { - // Snapshot the side-effect counters captureFrameCore mutates. We use a - // shallow `{...}` for capturePerf because all five fields are primitive - // numbers — no nested state to deep-copy. - const perfBefore = { ...session.capturePerf }; + // The discarded capture mutates frameMs, so preserve its samples by value. + const perfBefore = { ...session.capturePerf, frameMs: [...session.capturePerf.frameMs] }; const hasDamageBefore = session.beginFrameHasDamageCount; const noDamageBefore = session.beginFrameNoDamageCount; const dedupCountBefore = session.staticDedupCount;