diff --git a/.github/workflows/update-snapshots.yml b/.github/workflows/update-snapshots.yml index 3252bfe..cc37fb5 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,22 @@ jobs: env: SITE_PATH: fixtures/_build/html + # 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 + # --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 + - name: Commit and Push Updated Snapshots id: commit run: | @@ -140,14 +173,33 @@ 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 verifyOk = process.env.VERIFY === 'success'; const built = `\n\nBuilt against fixtures \`${process.env.FIXTURES}\`.`; + // 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}\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.` - : `ℹ️ No missing visual snapshots — nothing was committed. Every test already has a baseline, so this PR is unchanged.${built}`; + ? `✅ 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}${verified}`; await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, @@ -182,11 +234,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,12 +314,30 @@ 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 + # --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 + - name: Upload Old vs New Diff uses: actions/upload-artifact@v7 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 @@ -287,17 +374,35 @@ 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 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.)`; 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` + + `${verified}\n\n` + `📦 [Run log](${runUrl})`; await github.rest.issues.createComment({ owner: context.repo.owner, diff --git a/CHANGELOG.md b/CHANGELOG.md index de193c0..f62ea64 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 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`. - **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..d2e8294 100644 --- a/docs/developer/visual-testing.md +++ b/docs/developer/visual-testing.md @@ -71,20 +71,71 @@ 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 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. 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 + +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 +147,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.