From c55cd21598662e8759446a0fbbfb0eb783f3761c Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Thu, 6 Aug 2026 15:30:44 +1000 Subject: [PATCH 1/2] ci: verify regenerated snapshots and let the bot's commit trigger CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses #358. Two problems, one symptom — regenerated baselines reaching a PR with nothing having checked them and no CI run to notice. Verify before committing. The workflow wrote new baselines and pushed them with nothing establishing they were reproducible. Both jobs now re-run the suite without --update-snapshots against the same built site. Passing shows the render is deterministic; failing means the images just written already don't reproduce, which is non-deterministic rendering rather than a stale baseline, and is worth knowing before the commit lands rather than after it reaches main. Make the push able to trigger CI. Commits pushed with GITHUB_TOKEN never raise push/pull_request events, so the baselines landed while the PR's visual check still showed its previous failure and the author had to push an empty commit by hand. Both checkouts now use `secrets.SNAPSHOT_BOT_TOKEN || secrets.GITHUB_TOKEN`, so setting that secret makes CI re-run automatically while leaving the workflow working unchanged when it is absent. The summary comments report which path was taken and what, if anything, the author still needs to do. Note that #358's first stated cause is stale: the commit messages no longer carry `[skip ci]`. The GITHUB_TOKEN event-suppression rule is the whole of it. Docs cover the new secret, the verification step, and correct two points that #424 changed — ci.yml is now the source of truth for the fixtures pin, and the pin is the one thing read from the PR branch rather than the default branch. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/update-snapshots.yml | 84 ++++++++++++++++++++++++-- CHANGELOG.md | 2 + docs/developer/visual-testing.md | 43 ++++++++++++- 3 files changed, 121 insertions(+), 8 deletions(-) diff --git a/.github/workflows/update-snapshots.yml b/.github/workflows/update-snapshots.yml index 3252bfe..a249a7d 100644 --- a/.github/workflows/update-snapshots.yml +++ b/.github/workflows/update-snapshots.yml @@ -43,11 +43,28 @@ jobs: core.setOutput('ref', pr.data.head.ref); core.setOutput('sha', pr.data.head.sha); + # SNAPSHOT_BOT_TOKEN (optional) is what makes the bot's commit trigger CI. + # Commits pushed with GITHUB_TOKEN never raise `push`/`pull_request` events + # — GitHub's loop guard — so the regenerated baselines land with the PR's + # visual check still showing its old, stale failure. If the secret is not + # set this falls back to GITHUB_TOKEN and everything still works; the + # summary comment then tells the author to push an empty commit. - name: Checkout PR branch uses: actions/checkout@v7 with: ref: ${{ steps.pr.outputs.ref }} - token: ${{ secrets.GITHUB_TOKEN }} + token: ${{ secrets.SNAPSHOT_BOT_TOKEN || secrets.GITHUB_TOKEN }} + + - name: Detect push-token mode + id: token + env: + BOT_TOKEN: ${{ secrets.SNAPSHOT_BOT_TOKEN }} + run: | + if [ -n "$BOT_TOKEN" ]; then + echo "mode=pat" >> "$GITHUB_OUTPUT" + else + echo "mode=github_token" >> "$GITHUB_OUTPUT" + fi # Must run after the PR branch is checked out. On `issue_comment` events # the workflow definition comes from the default branch, so the top-level @@ -106,6 +123,18 @@ jobs: env: SITE_PATH: fixtures/_build/html + # The core of issue #358: nothing validated the baselines this job just + # wrote. Re-run the suite (no --update-snapshots) against the very site + # that produced them. Passing proves the render is deterministic; failing + # means the new baselines are already unreproducible, which is exactly the + # state that used to reach `main` unnoticed. + - name: Verify regenerated baselines reproduce + id: verify + continue-on-error: true + run: npx playwright test + env: + SITE_PATH: fixtures/_build/html + - name: Commit and Push Updated Snapshots id: commit run: | @@ -140,13 +169,20 @@ jobs: COMMITTED: ${{ steps.commit.outputs.committed }} CHANGED: ${{ steps.commit.outputs.changed }} FIXTURES: ${{ steps.pin.outputs.repo }}@${{ steps.pin.outputs.sha }} + VERIFY: ${{ steps.verify.outcome }} + TOKEN_MODE: ${{ steps.token.outputs.mode }} with: script: | const committed = process.env.COMMITTED === 'true'; const built = `\n\nBuilt against fixtures \`${process.env.FIXTURES}\`.`; + const verified = process.env.VERIFY === 'success' + ? `\n\n✅ Re-ran the suite against the new baselines — they reproduce.` + : `\n\n❌ Re-ran the suite against the new baselines and it **failed**. The baselines this job just wrote do not reproduce against the same build, which points at non-deterministic rendering rather than a stale baseline. Do not rely on them — investigate before merging.`; + const nextStep = process.env.TOKEN_MODE === 'pat' + ? `\n\nThe commit was pushed with \`SNAPSHOT_BOT_TOKEN\`, so CI re-runs on this PR automatically.` + : `\n\n⚠️ The commit was pushed with \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines. (Set a \`SNAPSHOT_BOT_TOKEN\` secret to make this automatic.)`; const body = committed - ? `✅ Added ${process.env.CHANGED} missing visual snapshot(s) and committed them to this PR.${built}\n\n` + - `⚠️ The bot's commit uses \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines.` + ? `✅ Added ${process.env.CHANGED} missing visual snapshot(s) and committed them to this PR.${built}${verified}${nextStep}` : `ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.${built}`; await github.rest.issues.createComment({ owner: context.repo.owner, @@ -182,11 +218,28 @@ jobs: core.setOutput('ref', pr.data.head.ref); core.setOutput('sha', pr.data.head.sha); + # SNAPSHOT_BOT_TOKEN (optional) is what makes the bot's commit trigger CI. + # Commits pushed with GITHUB_TOKEN never raise `push`/`pull_request` events + # — GitHub's loop guard — so the regenerated baselines land with the PR's + # visual check still showing its old, stale failure. If the secret is not + # set this falls back to GITHUB_TOKEN and everything still works; the + # summary comment then tells the author to push an empty commit. - name: Checkout PR branch uses: actions/checkout@v7 with: ref: ${{ steps.pr.outputs.ref }} - token: ${{ secrets.GITHUB_TOKEN }} + token: ${{ secrets.SNAPSHOT_BOT_TOKEN || secrets.GITHUB_TOKEN }} + + - name: Detect push-token mode + id: token + env: + BOT_TOKEN: ${{ secrets.SNAPSHOT_BOT_TOKEN }} + run: | + if [ -n "$BOT_TOKEN" ]; then + echo "mode=pat" >> "$GITHUB_OUTPUT" + else + echo "mode=github_token" >> "$GITHUB_OUTPUT" + fi # Must run after the PR branch is checked out. On `issue_comment` events # the workflow definition comes from the default branch, so the top-level @@ -245,6 +298,18 @@ jobs: env: SITE_PATH: fixtures/_build/html + # The core of issue #358: nothing validated the baselines this job just + # wrote. Re-run the suite (no --update-snapshots) against the very site + # that produced them. Passing proves the render is deterministic; failing + # means the new baselines are already unreproducible, which is exactly the + # state that used to reach `main` unnoticed. + - name: Verify regenerated baselines reproduce + id: verify + continue-on-error: true + run: npx playwright test + env: + SITE_PATH: fixtures/_build/html + - name: Upload Old vs New Diff uses: actions/upload-artifact@v7 if: always() @@ -287,15 +352,24 @@ jobs: COMMITTED: ${{ steps.commit.outputs.committed }} CHANGED: ${{ steps.commit.outputs.changed }} FIXTURES: ${{ steps.pin.outputs.repo }}@${{ steps.pin.outputs.sha }} + VERIFY: ${{ steps.verify.outcome }} + TOKEN_MODE: ${{ steps.token.outputs.mode }} with: script: | const runUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; const committed = process.env.COMMITTED === 'true'; + const verified = process.env.VERIFY === 'success' + ? `✅ Re-ran the suite against the new baselines — they reproduce.` + : `❌ Re-ran the suite against the new baselines and it **failed**. The baselines this job just wrote do not reproduce against the same build, which points at non-deterministic rendering rather than a stale baseline. Do not rely on them — investigate before merging.`; + const nextStep = process.env.TOKEN_MODE === 'pat' + ? `The commit was pushed with \`SNAPSHOT_BOT_TOKEN\`, so CI re-runs on this PR automatically.` + : `⚠️ The commit was pushed with \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines. (Set a \`SNAPSHOT_BOT_TOKEN\` secret to make this automatic.)`; const body = committed ? `✅ Regenerated visual snapshots and committed ${process.env.CHANGED} changed file(s) to this PR.\n\n` + `Built against fixtures \`${process.env.FIXTURES}\`.\n\n` + + `${verified}\n\n` + `📦 [Download snapshot-update-diff artifact](${runUrl}#artifacts) to review before/after images.\n\n` + - `⚠️ The bot's commit uses \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines.` + `${nextStep}` : `ℹ️ Snapshots were regenerated but came out identical to the existing baselines, so **nothing was committed** and this PR is unchanged.\n\n` + `Built against fixtures \`${process.env.FIXTURES}\`. If the visual job is still failing, check that this is the fixtures commit you expect — the pin is read from \`.github/workflows/ci.yml\` on this branch.\n\n` + `📦 [Run log](${runUrl})`; diff --git a/CHANGELOG.md b/CHANGELOG.md index de193c0..c7e93a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Developer setup troubleshooting for stale `.nodeenv`** — documented the `nodeenv-version-mismatch` error (an in-repo `.nodeenv/` left over from an older pinned Node.js version) and its fix (`rm -rf .nodeenv` then rebuild), which otherwise blocks `tox` and editable installs locally. Also clarified that `tox` keeps the toolchain fully repo-local (`.tox/`, `.nodeenv/`, `node_modules/` are all git-ignored and regenerated), so nothing is installed into the base/global environment. ### CI +- **Regenerated visual baselines are now verified before they are committed** (#358) — the snapshot workflow wrote new baselines and committed them with nothing checking that they were any good. Both jobs now re-run the suite without `--update-snapshots` against the same built site, so a baseline set that doesn't reproduce (non-deterministic rendering rather than a stale image) is caught in the same run and called out in the summary comment. +- **The snapshot bot's commit can now trigger CI** (#358) — commits pushed with `GITHUB_TOKEN` don't raise `push`/`pull_request` events, so regenerated baselines landed while the PR's `visual` check still showed its old failure, and the author had to push an empty commit. The workflow now pushes with an optional `SNAPSHOT_BOT_TOKEN` secret when one is set, falling back to `GITHUB_TOKEN` otherwise; the summary comment says which happened and what to do next. See `docs/developer/visual-testing.md` for how to add the secret. - **`/update-snapshots` now reads the fixtures pin from the PR branch** — GitHub always runs the *default branch's* copy of a workflow on `issue_comment` events, so `update-snapshots.yml` was resolving `FIXTURES_SHA` from main rather than from the PR under test. A PR that bumps the pin (because it needs a fixtures change to exercise a new feature) therefore regenerated against the *old* fixtures, produced byte-identical baselines, committed nothing — and still posted "✅ regenerated and committed", leaving the PR's visual job failing with no way to fix it. Both jobs now resolve the pin out of the checked-out branch's `ci.yml`, falling back to the workflow-level value. Both summary comments also report what actually happened (committed vs. unchanged, and which fixtures commit was built against) instead of unconditionally claiming success, and note that the bot's `GITHUB_TOKEN` commit does not itself trigger CI. - **`/update-snapshots` is now restricted to trusted actors** — both jobs check out the PR branch and execute its code (`pip install .` runs the PR's build backend) with a `contents: write` token, but had no author-association gate, so anyone able to comment on a PR could trigger that. Both now require `OWNER`, `MEMBER`, or `COLLABORATOR`. - **CI Node bumped 20 → 24** — Node 20 reached end-of-life in April 2026, and the grouped npm updates in #400 raised engine floors (`sass-loader` 17 requires Node ≥22.11). Node 24 is the current active LTS (supported to April 2028). Applies to `ci.yml`, `docs.yml`, and `update-snapshots.yml`; `.nvmrc` and the contributor docs (`CONTRIBUTING.md`, `docs/developer/setup.md`) move to Node 24 in step so local dev matches CI. diff --git a/docs/developer/visual-testing.md b/docs/developer/visual-testing.md index 2be0c13..764c413 100644 --- a/docs/developer/visual-testing.md +++ b/docs/developer/visual-testing.md @@ -71,20 +71,56 @@ Two commands are available as PR comments: | `/update-new-snapshots` | Adds only **missing** baselines — use for new tests | Both commands: -- Check out the fixtures repo at `FIXTURES_SHA` +- Check out the fixtures repo at the pin read from this PR branch's `ci.yml` - Build the fixtures site with the PR's theme applied - Run Playwright on Ubuntu for platform-consistent snapshots +- Re-run the suite against the freshly written baselines to confirm they + reproduce (see below) - Commit updated snapshots to the PR branch - Post a summary comment on the PR +Both are restricted to users with an `OWNER`, `MEMBER`, or `COLLABORATOR` +association — they check out and execute PR-branch code under a token with +write access. + The `/update-snapshots` command also uploads a `snapshot-update-diff` artifact with before/after images for review. +### Baselines are verified before they are committed + +After regenerating, the workflow runs the suite once more *without* +`--update-snapshots`, against the same built site that produced the images. A +pass means the render is deterministic. A failure means the baselines the job +just wrote already don't reproduce — which points at non-deterministic +rendering (animation, font loading, a timing-dependent layout) rather than a +stale baseline. The summary comment reports the outcome either way, so a bad +regeneration is visible immediately instead of surfacing later on `main`. + +### Making the bot's commit trigger CI + +Commits pushed with the default `GITHUB_TOKEN` do **not** raise `push` or +`pull_request` events — GitHub's loop-prevention rule. So by default the +regenerated baselines land while the PR's `visual` check still shows its +previous, now-stale failure, and you have to push an empty commit to re-run it. + +To make this automatic, add a repository secret named `SNAPSHOT_BOT_TOKEN` +holding a fine-grained personal access token (or GitHub App installation token) +with **Contents: read and write** on this repository. The workflow picks it up +automatically and pushes with it; CI then re-runs on the PR without any manual +step. If the secret is absent the workflow falls back to `GITHUB_TOKEN` and +still works — the summary comment just tells you to push the empty commit. + ```{note} `issue_comment` workflows always load the workflow file from the **default branch**, not from the PR branch. If a PR changes `update-snapshots.yml` itself, the new workflow takes effect only after that change has been merged to `main`. + +The fixtures pin is the deliberate exception: the workflow re-reads +`FIXTURES_SHA` out of the checked-out branch's `ci.yml`, so a PR that bumps +the pin regenerates against the fixtures it actually tests against. Without +that, such a PR regenerates against `main`'s fixtures, gets byte-identical +baselines, commits nothing, and can never go green. ``` ## Bumping the fixtures pin @@ -96,8 +132,9 @@ fixtures repo): [`quantecon-book-theme-fixtures`](https://github.com/QuantEcon/quantecon-book-theme-fixtures) adding the page. See its `real-world/README.md` for capture conventions. 2. Once merged, open a follow-up PR on this repo that bumps `FIXTURES_SHA` - in both workflow files and (if needed) adds a corresponding test entry - in `theme.spec.ts`. + in `ci.yml` (the source of truth — `update-snapshots.yml` reads it from + there, and its own copy is only a fallback) and, if needed, adds a + corresponding test entry in `theme.spec.ts`. 3. Comment `/update-new-snapshots` on that PR to seed baselines for any newly tested pages. From 2ea8109ad777d22c3939717d04f8494c45ef1df7 Mon Sep 17 00:00:00 2001 From: Matt McKay Date: Thu, 6 Aug 2026 16:31:00 +1000 Subject: [PATCH 2/2] ci: stop the verify run wiping the snapshot diff artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on #425, plus two defects found reviewing the result. Report the verify outcome in every case, not only when something was committed. The verify step has no `if:`, so it always runs; suppressing its result when nothing was committed hid the most useful signal of all — for /update-new-snapshots, a failure with nothing committed means the PR has real visual regressions. Give the verify run its own output directory. Playwright clears its output dir as the first task of every run, so the verify run was deleting the regeneration run's images out of test-results/ — precisely the content the snapshot-update-diff artifact exists to carry. The artifact would have uploaded nothing while the comment still linked to it, with only a buried upload-artifact warning as signal. Verified by repro: a sentinel file in test-results/ does not survive a plain `playwright test`, and does survive `playwright test --output=test-results-verify`. The upload now covers both directories, so a verify failure stays diagnosable. Stop asserting a cause the workflow cannot know. /update-new-snapshots runs --update-snapshots=missing, which creates only absent baselines and leaves existing-but-differing ones failing, while the verify run covers the whole suite. So `committed && !verified` there does not imply the new snapshots are at fault — it is reachable through the repo's own documented fixtures-bump procedure, where a bump seeds one new baseline correctly and perturbs unrelated existing pages. That job's message now gives both possibilities; the regenerate-all job, where every baseline was rewritten, still states the specific cause because there it is the only one available. Docs and CHANGELOG follow, and no longer imply the verification gates the commit. It reports; the commit happens either way, which is deliberate — the images stay available to inspect. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/update-snapshots.yml | 61 +++++++++++++++++++------- CHANGELOG.md | 2 +- docs/developer/visual-testing.md | 29 +++++++++--- 3 files changed, 69 insertions(+), 23 deletions(-) diff --git a/.github/workflows/update-snapshots.yml b/.github/workflows/update-snapshots.yml index a249a7d..cc37fb5 100644 --- a/.github/workflows/update-snapshots.yml +++ b/.github/workflows/update-snapshots.yml @@ -123,15 +123,19 @@ jobs: env: SITE_PATH: fixtures/_build/html - # The core of issue #358: nothing validated the baselines this job just - # wrote. Re-run the suite (no --update-snapshots) against the very site - # that produced them. Passing proves the render is deterministic; failing - # means the new baselines are already unreproducible, which is exactly the - # state that used to reach `main` unnoticed. + # Part of issue #358: nothing checked the baselines this job just wrote. + # Re-run the suite (no --update-snapshots) against the very site that + # produced them. Note this job adds only MISSING baselines, so a failure + # here is not necessarily about what was just written — see the summary + # comment below, which is careful not to assert a cause. - name: Verify regenerated baselines reproduce id: verify continue-on-error: true - run: npx playwright test + # --output is load-bearing: Playwright clears its output dir as the first + # task of every run, so without this the verify run would wipe the + # regeneration run's images out of test-results/ — the exact content the + # snapshot-update-diff artifact exists to carry. + run: npx playwright test --output=test-results-verify env: SITE_PATH: fixtures/_build/html @@ -173,17 +177,29 @@ jobs: TOKEN_MODE: ${{ steps.token.outputs.mode }} with: script: | + const runUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; const committed = process.env.COMMITTED === 'true'; + const verifyOk = process.env.VERIFY === 'success'; const built = `\n\nBuilt against fixtures \`${process.env.FIXTURES}\`.`; - const verified = process.env.VERIFY === 'success' - ? `\n\n✅ Re-ran the suite against the new baselines — they reproduce.` - : `\n\n❌ Re-ran the suite against the new baselines and it **failed**. The baselines this job just wrote do not reproduce against the same build, which points at non-deterministic rendering rather than a stale baseline. Do not rely on them — investigate before merging.`; + // Reported in all four cases, and deliberately non-committal about the + // cause. This job runs --update-snapshots=missing, which only creates + // ABSENT baselines and leaves existing-but-differing ones failing. The + // verify run is the whole suite, and steps.verify.outcome is only + // pass/fail — so a failure here may be in what was just added OR in a + // pre-existing baseline this PR legitimately invalidated. Don't guess. + const verified = committed + ? (verifyOk + ? `\n\n✅ Re-ran the full suite afterwards — everything passes.` + : `\n\n❌ Re-ran the full suite afterwards and it **failed**. This job only adds *missing* baselines and leaves existing ones untouched, so the failure may be in the snapshots just added or in an existing baseline this PR invalidated. See the [run log](${runUrl}).`) + : (verifyOk + ? `\n\n✅ Re-ran the full suite anyway — the existing baselines still pass.` + : `\n\n❌ Re-ran the full suite anyway and it **failed**. Nothing was added, so the failure is in existing baselines rather than in anything this job wrote — this PR has visual changes that need \`/update-snapshots\`. See the [run log](${runUrl}).`); const nextStep = process.env.TOKEN_MODE === 'pat' ? `\n\nThe commit was pushed with \`SNAPSHOT_BOT_TOKEN\`, so CI re-runs on this PR automatically.` : `\n\n⚠️ The commit was pushed with \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines. (Set a \`SNAPSHOT_BOT_TOKEN\` secret to make this automatic.)`; const body = committed ? `✅ Added ${process.env.CHANGED} missing visual snapshot(s) and committed them to this PR.${built}${verified}${nextStep}` - : `ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.${built}`; + : `ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.${built}${verified}`; await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, @@ -306,7 +322,11 @@ jobs: - name: Verify regenerated baselines reproduce id: verify continue-on-error: true - run: npx playwright test + # --output is load-bearing: Playwright clears its output dir as the first + # task of every run, so without this the verify run would wipe the + # regeneration run's images out of test-results/ — the exact content the + # snapshot-update-diff artifact exists to carry. + run: npx playwright test --output=test-results-verify env: SITE_PATH: fixtures/_build/html @@ -315,7 +335,9 @@ jobs: if: always() with: name: snapshot-update-diff - path: test-results/ + path: | + test-results/ + test-results-verify/ retention-days: 30 - name: Commit and Push Updated Snapshots @@ -358,9 +380,17 @@ jobs: script: | const runUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; const committed = process.env.COMMITTED === 'true'; - const verified = process.env.VERIFY === 'success' - ? `✅ Re-ran the suite against the new baselines — they reproduce.` - : `❌ Re-ran the suite against the new baselines and it **failed**. The baselines this job just wrote do not reproduce against the same build, which points at non-deterministic rendering rather than a stale baseline. Do not rely on them — investigate before merging.`; + const verifyOk = process.env.VERIFY === 'success'; + // Reported in all four cases. What a failure MEANS depends on whether + // anything was written, so don't assert one cause — steps.verify.outcome + // only carries pass/fail. Point at the log instead. + const verified = committed + ? (verifyOk + ? `✅ Re-ran the suite against the new baselines — they reproduce.` + : `❌ Re-ran the suite against the new baselines and it **failed** — the snapshots just written do not reproduce against the same build. See the [run log](${runUrl}) before relying on them.`) + : (verifyOk + ? `✅ Re-ran the suite against the existing baselines — they still pass.` + : `❌ Re-ran the suite and it **failed**, even though no baselines changed — the existing baselines do not reproduce against this build. See the [run log](${runUrl}).`); const nextStep = process.env.TOKEN_MODE === 'pat' ? `The commit was pushed with \`SNAPSHOT_BOT_TOKEN\`, so CI re-runs on this PR automatically.` : `⚠️ The commit was pushed with \`GITHUB_TOKEN\`, which does not trigger workflows — push an empty commit to re-run CI against the new baselines. (Set a \`SNAPSHOT_BOT_TOKEN\` secret to make this automatic.)`; @@ -372,6 +402,7 @@ jobs: `${nextStep}` : `ℹ️ Snapshots were regenerated but came out identical to the existing baselines, so **nothing was committed** and this PR is unchanged.\n\n` + `Built against fixtures \`${process.env.FIXTURES}\`. If the visual job is still failing, check that this is the fixtures commit you expect — the pin is read from \`.github/workflows/ci.yml\` on this branch.\n\n` + + `${verified}\n\n` + `📦 [Run log](${runUrl})`; await github.rest.issues.createComment({ owner: context.repo.owner, diff --git a/CHANGELOG.md b/CHANGELOG.md index c7e93a0..f62ea64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Developer setup troubleshooting for stale `.nodeenv`** — documented the `nodeenv-version-mismatch` error (an in-repo `.nodeenv/` left over from an older pinned Node.js version) and its fix (`rm -rf .nodeenv` then rebuild), which otherwise blocks `tox` and editable installs locally. Also clarified that `tox` keeps the toolchain fully repo-local (`.tox/`, `.nodeenv/`, `node_modules/` are all git-ignored and regenerated), so nothing is installed into the base/global environment. ### CI -- **Regenerated visual baselines are now verified before they are committed** (#358) — the snapshot workflow wrote new baselines and committed them with nothing checking that they were any good. Both jobs now re-run the suite without `--update-snapshots` against the same built site, so a baseline set that doesn't reproduce (non-deterministic rendering rather than a stale image) is caught in the same run and called out in the summary comment. +- **Regenerated visual baselines are now re-checked in the same run** (#358) — the snapshot workflow wrote new baselines and committed them with nothing checking that they were any good. Both jobs now re-run the suite without `--update-snapshots` against the same built site and report the outcome in the summary comment (it reports rather than gates — the commit still happens, so the images are always there to inspect). The re-run writes to its own `test-results-verify/` directory, since Playwright clears its output directory on every run and would otherwise erase the regeneration images that the `snapshot-update-diff` artifact carries. After `/update-new-snapshots` the comment deliberately does not attribute a failure to the snapshots just added — that job leaves existing baselines untouched, so the cause may be a baseline the PR legitimately invalidated. - **The snapshot bot's commit can now trigger CI** (#358) — commits pushed with `GITHUB_TOKEN` don't raise `push`/`pull_request` events, so regenerated baselines landed while the PR's `visual` check still showed its old failure, and the author had to push an empty commit. The workflow now pushes with an optional `SNAPSHOT_BOT_TOKEN` secret when one is set, falling back to `GITHUB_TOKEN` otherwise; the summary comment says which happened and what to do next. See `docs/developer/visual-testing.md` for how to add the secret. - **`/update-snapshots` now reads the fixtures pin from the PR branch** — GitHub always runs the *default branch's* copy of a workflow on `issue_comment` events, so `update-snapshots.yml` was resolving `FIXTURES_SHA` from main rather than from the PR under test. A PR that bumps the pin (because it needs a fixtures change to exercise a new feature) therefore regenerated against the *old* fixtures, produced byte-identical baselines, committed nothing — and still posted "✅ regenerated and committed", leaving the PR's visual job failing with no way to fix it. Both jobs now resolve the pin out of the checked-out branch's `ci.yml`, falling back to the workflow-level value. Both summary comments also report what actually happened (committed vs. unchanged, and which fixtures commit was built against) instead of unconditionally claiming success, and note that the bot's `GITHUB_TOKEN` commit does not itself trigger CI. - **`/update-snapshots` is now restricted to trusted actors** — both jobs check out the PR branch and execute its code (`pip install .` runs the PR's build backend) with a `contents: write` token, but had no author-association gate, so anyone able to comment on a PR could trigger that. Both now require `OWNER`, `MEMBER`, or `COLLABORATOR`. diff --git a/docs/developer/visual-testing.md b/docs/developer/visual-testing.md index 764c413..d2e8294 100644 --- a/docs/developer/visual-testing.md +++ b/docs/developer/visual-testing.md @@ -86,15 +86,30 @@ write access. The `/update-snapshots` command also uploads a `snapshot-update-diff` artifact with before/after images for review. -### Baselines are verified before they are committed +### Baselines are re-checked before the commit After regenerating, the workflow runs the suite once more *without* -`--update-snapshots`, against the same built site that produced the images. A -pass means the render is deterministic. A failure means the baselines the job -just wrote already don't reproduce — which points at non-deterministic -rendering (animation, font loading, a timing-dependent layout) rather than a -stale baseline. The summary comment reports the outcome either way, so a bad -regeneration is visible immediately instead of surfacing later on `main`. +`--update-snapshots`, against the same built site that produced the images. The +summary comment reports the outcome either way, so a bad regeneration is visible +immediately instead of surfacing later on `main`. Note this reports; it does not +gate — the commit and push happen regardless, so the images are always available +to inspect. + +What a failure means depends on which command you ran: + +- After `/update-snapshots`, every baseline was rewritten, so a failure means + the images just written don't reproduce against the same build — that is, + the render is not deterministic. +- After `/update-new-snapshots`, only *missing* baselines were created and + existing ones were left untouched, while the verify run covers the whole + suite. A failure there may be in what was just added, or in an existing + baseline the PR legitimately invalidated. The comment says so rather than + guessing; use `/update-snapshots` if the latter. + +The verify run writes to its own `test-results-verify/` directory. This matters: +Playwright clears its output directory at the start of every run, so sharing +`test-results/` would erase the regeneration images that the +`snapshot-update-diff` artifact exists to carry. ### Making the bot's commit trigger CI