Skip to content

ci: verify regenerated snapshots and let the bot's commit trigger CI #425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mmcky merged 2 commits into from
Aug 6, 2026
Merged

ci: verify regenerated snapshots and let the bot's commit trigger CI #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c55cd21
ci: verify regenerated snapshots and let the bot's commit trigger CI
mmcky Aug 6, 2026
2ea8109
ci: stop the verify run wiping the snapshot diff artifact
mmcky Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 112 additions & 7 deletions .github/workflows/update-snapshots.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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: |
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
58 changes: 55 additions & 3 deletions docs/developer/visual-testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.

Expand Down
Loading