diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index e8b65964e9..cfd5823cd2 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -302,8 +302,9 @@ jobs: echo 'Files that can move a pixel:' printf '%s\n' "$HITS" - # Visual diff over src/dev/fixtures/registry.ts — 30 fixtures at four phone - # widths, 120 PNGs. No API, no database, no provider key and no secret: a + # Visual diff over src/dev/fixtures/registry.ts — 30 fixtures, four phone + # widths on baseline pushes and two on PRs (see the capture step). + # No API, no database, no provider key and no secret: a # fixture answers every request the app makes. # # ADVISORY. It is deliberately NOT in `ci-success.needs`, so it can never @@ -410,6 +411,23 @@ jobs: echo "::error::playwright install-deps failed after 3 attempts" exit 1 + # Next's incremental build cache — roughly halves the ~4 min build + # when warm. Key follows the Next.js docs recipe: exact key per + # source hash, restore-keys falls back to the newest cache for the + # same lockfile, which is the incremental rebuild Next optimizes. + # Restore-only on purpose: a save on every PR push would upload a + # fresh 100-400 MB entry that is branch-scoped (no other PR can + # read it), churning the shared 10 GB cache quota and spending + # 1-2 min on the upload. Push runs save below instead, so every + # PR restores a near-fresh dev-scoped entry via restore-keys. + - name: Restore the Next build cache + uses: actions/cache/restore@v4 + with: + path: .next/cache + key: ds-shots-next-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}-${{ hashFiles('src/**/*.ts', 'src/**/*.tsx', 'src/**/*.js', 'src/**/*.jsx') }} + restore-keys: | + ds-shots-next-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}- + - name: Build env: # Without this DEV_TOOLS_ENABLED is false, ?__fixture= is @@ -424,12 +442,32 @@ jobs: NEXT_PUBLIC_BASE_URL: http://127.0.0.1:3080 run: pnpm build + # Push runs only (dev/main/DS train land several times a day), so + # PRs always find a near-fresh entry without per-PR churn. Right + # after Build rather than a post step: a capture failure must not + # lose the cache. A re-run finds the key taken — losing that race + # costs nothing, the entry is already there. + - name: Save the Next build cache + if: github.event_name == 'push' + continue-on-error: true + uses: actions/cache/save@v4 + with: + path: .next/cache + key: ds-shots-next-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }}-${{ hashFiles('src/**/*.ts', 'src/**/*.tsx', 'src/**/*.js', 'src/**/*.jsx') }} + # The capture fails a shot that still has a spinner on camera, on # purpose. No continue-on-error: a red step is how anyone finds out. + # + # PR runs shoot only the two extreme widths — 320 finds the most + # overflow bugs, 430 is the widest device — which halves capture + # time. Baseline pushes keep all four widths so a later full + # capture always has something to diff against; visual-diff.mjs + # ignores baseline widths a partial capture never shot. The + # project names are the widths (playwright.shots.config.ts). - name: Capture screenshots env: SHOTS_OUT: e2e/__shots__/${{ github.event_name == 'push' && 'base' || 'head' }} - run: pnpm test:visual:capture + run: pnpm test:visual:capture ${{ github.event_name == 'pull_request' && '--project=320 --project=430' || '' }} - name: Save this commit as the baseline if: github.event_name == 'push' diff --git a/scripts/__tests__/visual-diff-widths.test.js b/scripts/__tests__/visual-diff-widths.test.js new file mode 100644 index 0000000000..fe393ccaf6 --- /dev/null +++ b/scripts/__tests__/visual-diff-widths.test.js @@ -0,0 +1,112 @@ +const { spawnSync } = require('child_process') +const fs = require('fs') +const os = require('os') +const path = require('path') +const { PNG } = require('pngjs') + +const SCRIPT_PATH = path.join(__dirname, '..', 'visual-diff.mjs') + +// PR captures shoot 2 of the baseline's 4 widths (tests.yml), so the diff +// must not report the widths a partial capture never shot as "removed" — +// while a screen that is really gone must still be reported. The script is a +// CI entrypoint, so drive it the way the workflow does: two directories in, +// JSON out. +function png(color) { + const img = new PNG({ width: 2, height: 2 }) + for (let i = 0; i < img.data.length; i += 4) { + img.data[i] = color + img.data[i + 3] = 255 + } + return PNG.sync.write(img) +} + +function capture(dir, shots) { + fs.mkdirSync(dir, { recursive: true }) + for (const [file, color] of Object.entries(shots)) fs.writeFileSync(path.join(dir, file), png(color)) +} + +function diff(before, after) { + const out = path.join(path.dirname(before), 'diff') + const result = spawnSync(process.execPath, [SCRIPT_PATH, before, after, `--out=${out}`, '--json'], { + encoding: 'utf-8', + }) + expect(result.status).toBe(0) + return JSON.parse(result.stdout) +} + +describe('visual-diff partial-width capture', () => { + let root + beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'visual-diff-')) + }) + + it('does not report baseline-only widths as removed', () => { + capture(path.join(root, 'base'), { + 'home@320.png': 0, + 'home@375.png': 0, + 'home@430.png': 0, + }) + capture(path.join(root, 'head'), { + 'home@320.png': 0, + 'home@430.png': 0, + }) + + const report = diff(path.join(root, 'base'), path.join(root, 'head')) + + expect(report.removed).toEqual([]) + expect(report.added).toEqual([]) + expect(report.changed).toEqual([]) + expect(report.unchanged).toBe(2) + }) + + it('still reports a screen that is really gone', () => { + capture(path.join(root, 'base'), { + 'home@320.png': 0, + 'home@375.png': 0, + 'gone@320.png': 0, + 'gone@375.png': 0, + }) + capture(path.join(root, 'head'), { + 'home@320.png': 0, + }) + + const report = diff(path.join(root, 'base'), path.join(root, 'head')) + + // 375 was never shot, so only the captured width proves the removal + expect(report.removed).toEqual(['gone@320.png']) + }) + + it('fails loud when the capture produced nothing', () => { + capture(path.join(root, 'base'), { 'home@320.png': 0 }) + capture(path.join(root, 'head'), {}) + + const out = path.join(root, 'diff') + const result = spawnSync( + process.execPath, + [SCRIPT_PATH, path.join(root, 'base'), path.join(root, 'head'), `--out=${out}`, '--json'], + { encoding: 'utf-8' } + ) + + // an empty after dir must never diff all-green — that is tool breakage + expect(result.status).toBe(2) + expect(result.stderr).toContain('capture produced nothing') + }) + + it('still detects pixel changes and additions at captured widths', () => { + capture(path.join(root, 'base'), { + 'home@320.png': 0, + 'home@375.png': 0, + }) + capture(path.join(root, 'head'), { + 'home@320.png': 255, + 'new-screen@320.png': 0, + }) + + const report = diff(path.join(root, 'base'), path.join(root, 'head')) + + expect(report.changed.map((c) => c.file)).toEqual(['home@320.png']) + expect(report.changed[0].percent).toBe(100) + expect(report.added).toEqual(['new-screen@320.png']) + expect(report.removed).toEqual([]) + }) +}) diff --git a/scripts/visual-diff.mjs b/scripts/visual-diff.mjs index 661dd944f7..d263ca80c0 100644 --- a/scripts/visual-diff.mjs +++ b/scripts/visual-diff.mjs @@ -63,6 +63,14 @@ const shots = (dir) => new Set(readdirSync(dir).filter((f) => f.endsWith('.png') const before = shots(beforeDir) const after = shots(afterDir) +// A capture that produced nothing must stay loud: with zero after-files the +// width-aware removal filter below would turn "every screenshot vanished" +// into an all-green report. That is tool breakage, not a diff result. +if (before.size > 0 && after.size === 0) { + console.error(`no PNGs in ${afterDir} — the capture produced nothing`) + process.exit(2) +} + mkdirSync(outDir, { recursive: true }) const changed = [] @@ -95,7 +103,15 @@ for (const file of [...after].filter((f) => before.has(f)).sort()) { } const added = [...after].filter((f) => !before.has(f)).sort() -const removed = [...before].filter((f) => !after.has(f)).sort() + +// A partial capture must not report the widths it never shot as "removed": +// PR runs shoot 2 of the baseline's 4 widths (see tests.yml), so only a width +// the after capture actually produced can prove a removal. A screen that is +// really gone is still reported — its files are missing at the captured +// widths too. +const widthOf = (f) => f.match(/@(\d+)\.png$/)?.[1] ?? '' +const afterWidths = new Set([...after].map(widthOf)) +const removed = [...before].filter((f) => !after.has(f) && afterWidths.has(widthOf(f))).sort() changed.sort((x, y) => y.percent - x.percent)