ci: verify regenerated snapshots and let the bot's commit trigger CI - #425
Merged
Conversation
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the /update-snapshots and /update-new-snapshots GitHub Actions workflow so regenerated visual snapshot baselines are re-validated in the same run, and (optionally) bot-pushed snapshot commits can trigger normal CI by using a PAT/App token instead of GITHUB_TOKEN. This aligns the visual testing automation with the repo’s goal of keeping snapshot baselines trustworthy and reducing manual “empty commit” retriggers.
Changes:
- Add a second Playwright run (without
--update-snapshots) to verify regenerated baselines reproduce, and surface the result in PR summary comments. - Make snapshot pushes use
secrets.SNAPSHOT_BOT_TOKEN || secrets.GITHUB_TOKEN, plus report which token mode was used (and what the next step is). - Update developer docs and changelog to describe the new verification step, token/CI behavior, and fixtures-pin source of truth.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
docs/developer/visual-testing.md |
Documents baseline verification and the optional SNAPSHOT_BOT_TOKEN to allow bot commits to trigger CI. |
CHANGELOG.md |
Adds unreleased entries describing snapshot verification and optional CI-triggering bot pushes. |
.github/workflows/update-snapshots.yml |
Adds verification reruns and optional PAT token usage for pushing snapshot updates, plus enhanced PR summary comments. |
Suppressed comments (1)
.github/workflows/update-snapshots.yml:363
- The verification outcome is only included in the summary comment when snapshots are committed. Since the workflow runs the verification step unconditionally, it’s useful to surface that result even when no snapshot files changed/committed. Also, the failure text currently asserts a specific cause;
steps.verify.outcomeonly tells pass/fail, so the message should be a bit more general and point readers to the logs.
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.`;
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) <noreply@anthropic.com>
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
=======================================
Coverage ? 47.75%
=======================================
Files ? 2
Lines ? 423
Branches ? 0
=======================================
Hits ? 202
Misses ? 221
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
🎭 Visual Regression Test Results
Details
Skipped testsdesktop-chrome › theme.spec.ts › Visual Regression Tests › prob-matrix - full page screenshot |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #358.
Two problems with one symptom: regenerated baselines reach a PR with nothing having checked them, and no CI run to notice.
Verify the baselines before committing them
The workflow wrote new baselines and pushed them with nothing establishing they were any good. Both jobs now re-run the suite without
--update-snapshots, against the same built site that produced the images.Passing shows the render is deterministic. Failing means the images the job just wrote already don't reproduce — which is non-deterministic rendering (animation, font loading, a timing-dependent layout) rather than a stale baseline. That's worth knowing before the commit lands, not after it reaches
main. The summary comment reports the outcome either way.This is the closest thing to real validation that doesn't require a new secret, and it catches the failure mode the issue was actually worried about.
Let the push trigger CI
Commits pushed with
GITHUB_TOKENnever raisepush/pull_requestevents — GitHub's loop guard. So the baselines landed while the PR'svisualcheck still showed its previous, stale failure, and the author had to push an empty commit by hand. That is exactly what happened on #404 earlier today.Both checkouts now use
secrets.SNAPSHOT_BOT_TOKEN || secrets.GITHUB_TOKEN. Set that secret to a fine-grained PAT with Contents: read and write and CI re-runs automatically; leave it unset and the workflow behaves exactly as before. The summary comments say which path was taken and what, if anything, is still needed.I've deliberately made the secret opt-in rather than required — it's your call whether to create it, and nothing breaks in the meantime.
One correction to the issue
#358's first stated cause is stale. It says the commit message contains
[skip ci]; it doesn't — I read the workflow end to end. TheGITHUB_TOKENevent-suppression rule is the whole of it, which also means solution 4 in the issue ("remove[skip ci]") is already done.Docs
docs/developer/visual-testing.mdcovers the new secret and the verification step, and corrects two things #424 changed that the docs hadn't caught up with:ci.ymlis now the source of truth for the fixtures pin, and that pin is the one thing read from the PR branch rather than the default branch.Testing note
The
update-snapshotsworkflow only runs onissue_comment, andissue_commentalways loads the workflow from the default branch — so this change cannot be exercised until it is onmain. The YAML validates and all step/output references cross-check, but the first real run will be the next/update-snapshotsafter merge. Worth watching that one.🤖 Generated with Claude Code