Skip to content

Commit 968ec91

Browse files
committed
Stabilise visual regression snapshot testing
Unrelated PRs turned red from snapshot "changes" that were only a couple of bytes. The Playwright config set updateSnapshots: 'all', so every run unconditionally rewrote all 1482 PNGs and 471 ARIA snapshots instead of comparing against the committed baseline. The workflow then auto-committed any byte diff and hard-failed, so non-deterministic Chromium anti-aliasing produced spurious commits and red CI on PRs that changed no component behaviour. Switches the model to assert-against-baseline, regenerate on intent: - Removes updateSnapshots: 'all' so runs compare against the committed baseline, and adds maxDiffPixelRatio: 0.01 so trivial anti-aliasing noise passes instead of failing. - Gates regeneration and the auto-commit behind a regen-snapshots PR label; normal runs only assert and fail loudly on a real diff. - Forwards extra args through script/run-playwright so the regen path can pass --update-snapshots. - Raises timeout-minutes from 20 to 45 and drops continue-on-error and the blanket exit-1 step so a partial or timed-out run fails instead of committing a partial baseline on 2-core runners. Implements DREAM-773.
1 parent 22f9fc0 commit 968ec91

3 files changed

Lines changed: 34 additions & 12 deletions

File tree

.github/workflows/test-visual.yml

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,18 @@ concurrency:
1313

1414
env:
1515
FERRUM_PROCESS_TIMEOUT: 30
16+
# Regeneration is opt-in: add the `regen-snapshots` label to a PR to rewrite
17+
# and commit the baseline. Without it the job only asserts against the
18+
# committed snapshots and fails loudly on a real diff — it never commits.
19+
REGEN: ${{ contains(github.event.pull_request.labels.*.name, 'regen-snapshots') }}
1620

1721
jobs:
1822
visual:
1923
name: Visual and Semantic Markup Regressions
2024
if: ${{ github.event_name == 'pull_request' }}
21-
timeout-minutes: 20
25+
# 2-core ubuntu-latest: a full suite does not finish inside 20 min. Raised
26+
# so a run completes instead of timing out and committing a partial baseline.
27+
timeout-minutes: 45
2228
runs-on: ubuntu-latest
2329
steps:
2430
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
@@ -52,12 +58,20 @@ jobs:
5258
gem install overmind
5359
- name: Install Playwright Browsers
5460
run: npx playwright install --with-deps
55-
- name: Run Playwright tests
56-
id: playwright-run
57-
continue-on-error: true
61+
# Default path: compare against the committed baseline. No
62+
# continue-on-error — a genuine over-threshold diff fails the job so drift
63+
# surfaces loudly instead of being silently auto-committed.
64+
- name: Run Playwright tests (assert)
65+
id: playwright-assert
66+
if: ${{ env.REGEN != 'true' }}
5867
run: ./script/run-playwright
68+
# Opt-in path (regen-snapshots label): rewrite the baseline, then commit it.
69+
- name: Run Playwright tests (regenerate)
70+
id: playwright-regen
71+
if: ${{ env.REGEN == 'true' }}
72+
run: ./script/run-playwright --update-snapshots
5973
- name: Update to latest branch tip
60-
if: ${{ github.event.pull_request.head.repo.full_name == github.repository }}
74+
if: ${{ env.REGEN == 'true' && github.event.pull_request.head.repo.full_name == github.repository }}
6175
run: |
6276
set -e
6377
BRANCH="${GITHUB_EVENT_PULL_REQUEST_HEAD_REF}"
@@ -73,15 +87,15 @@ jobs:
7387
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
7488
GITHUB_EVENT_PULL_REQUEST_HEAD_REF: ${{ github.event.pull_request.head.ref }}
7589
- id: auto-commit
76-
if: ${{ github.event.pull_request.head.repo.full_name == github.repository }}
90+
if: ${{ env.REGEN == 'true' && github.event.pull_request.head.repo.full_name == github.repository }}
7791
uses: stefanzweifel/git-auto-commit-action@04702edda442b2e678b25b537cec683a1493fcb9 # v7.1.0
7892
with:
7993
commit_message: Generating component snapshots
8094
file_pattern: .playwright/screenshots/**/*.png .playwright/screenshots/**/*.yml
8195
push_options: --force-with-lease
8296
token: ${{ secrets.GITHUB_TOKEN }}
8397
- name: "Changes detected"
84-
if: steps.auto-commit.outputs.changes_detected == 'true'
98+
if: ${{ env.REGEN == 'true' && steps.auto-commit.outputs.changes_detected == 'true' }}
8599
uses: phulsechinmay/rewritable-pr-comment@a1b041997fa84ec3e524ee9dee5ba0b5f4e73604 # v0.3.0
86100
with:
87101
message: |
@@ -92,11 +106,10 @@ jobs:
92106
[Review differences](https://github.com/opf/primer_view_components/pull/${{ github.event.number }}/files?file-filters%5B%5D=.png&file-filters%5B%5D=.yml&show-viewed-files=false)
93107
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
94108
COMMENT_IDENTIFIER: "visual-comparison-diff"
109+
# Uploaded on assert failures too, so authors can inspect the diff report.
95110
- uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7
111+
if: ${{ !cancelled() }}
96112
with:
97113
name: playwright-report
98114
path: .playwright/report/
99115
retention-days: 30
100-
- name: Failure
101-
if: ${{ steps.auto-commit.outputs.changes_detected == 'true' || steps.playwright-run.outcome == 'failure' }}
102-
run: exit 1

playwright.config.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@ const config: PlaywrightTestConfig = {
1818
/* Run tests in files in parallel */
1919
fullyParallel: true,
2020
workers: process.env.CI ? 4 : undefined,
21-
updateSnapshots: 'all',
21+
// Assert against the committed baseline by default (Playwright's 'missing').
22+
// Snapshots are regenerated only on intent via `--update-snapshots` (the
23+
// `regen-snapshots` PR label in test-visual.yml), never unconditionally — an
24+
// unconditional 'all' rewrites every file every run and commits byte-churn.
2225
use: {
2326
baseURL: 'http://127.0.0.1:4000',
2427
browserName: 'chromium',
@@ -30,7 +33,12 @@ const config: PlaywrightTestConfig = {
3033
animations: 'disabled',
3134
},
3235
toMatchSnapshot: {
36+
// `threshold` sets per-pixel colour sensitivity; `maxDiffPixelRatio` sets
37+
// how many pixels may differ before failing. Without a max-diff budget a
38+
// single anti-aliased pixel of non-deterministic Chromium rasterisation
39+
// fails the run. Tune if genuine small changes slip through.
3340
threshold: 0.1,
41+
maxDiffPixelRatio: 0.01,
3442
},
3543
},
3644
/* Retry on CI only */

script/run-playwright

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#! /bin/bash
22

33
./script/stop-existing-processes
4-
exec npx playwright test --workers 6
4+
# Forward extra args (e.g. --update-snapshots for the regen path in CI).
5+
exec npx playwright test --workers 6 "$@"

0 commit comments

Comments
 (0)