From c266c4123a7f206f264c515de23213784bf95994 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:52:55 -0400 Subject: [PATCH] Fix screenshot summary publication --- .../test_update_visual_failure_comment.py | 22 ++++ .../scripts/update_visual_failure_comment.py | 2 +- .github/workflows/_static-checks.yaml | 3 + .github/workflows/_visual-regression.yaml | 11 ++ .github/workflows/pr-screenshot-summary.yaml | 39 ++++-- .../workflows/update-visual-baselines.yaml | 16 +++ scripts/update-pr-screenshot-summary.mjs | 19 +-- scripts/update-pr-screenshot-summary.test.mjs | 118 ++++++++++++++++++ 8 files changed, 207 insertions(+), 23 deletions(-) create mode 100644 scripts/update-pr-screenshot-summary.test.mjs diff --git a/.github/scripts/test_update_visual_failure_comment.py b/.github/scripts/test_update_visual_failure_comment.py index 669a2126..3e7854d0 100644 --- a/.github/scripts/test_update_visual_failure_comment.py +++ b/.github/scripts/test_update_visual_failure_comment.py @@ -92,6 +92,28 @@ def test_rejects_oversized_failure_detail(self): with self.assertRaises(ArtifactValidationError): load_failures(archive_path) + def test_loads_compact_details_without_report_images(self): + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + archive_path = write_artifact( + root, {"failure.md": PIXEL_FAILURE} + ) + + self.assertEqual(len(load_failures(archive_path)), 1) + + def test_deduplicates_full_and_compact_failure_details(self): + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + archive_path = write_artifact( + root, + { + "playwright-report/data/failure.md": PIXEL_FAILURE, + "failure.md": PIXEL_FAILURE, + }, + ) + + self.assertEqual(len(load_failures(archive_path)), 1) + def test_rejects_symlink(self): with tempfile.TemporaryDirectory() as temp_dir: root = Path(temp_dir) diff --git a/.github/scripts/update_visual_failure_comment.py b/.github/scripts/update_visual_failure_comment.py index a83ff326..ddbeb299 100644 --- a/.github/scripts/update_visual_failure_comment.py +++ b/.github/scripts/update_visual_failure_comment.py @@ -167,7 +167,7 @@ def load_failures(artifact_path: Path) -> list[Failure]: "the Playwright artifact is too large" ) if re.fullmatch( - r"playwright-report/data/[^/]+\.md", info.filename + r"(?:playwright-report/data/)?[^/]+\.md", info.filename ): if info.file_size > MAX_DETAIL_BYTES: raise ArtifactValidationError( diff --git a/.github/workflows/_static-checks.yaml b/.github/workflows/_static-checks.yaml index 807fcfa2..3411df5b 100644 --- a/.github/workflows/_static-checks.yaml +++ b/.github/workflows/_static-checks.yaml @@ -34,5 +34,8 @@ jobs: - name: Test release PR body renderer run: node --test scripts/upstream-release/render-pr-body.test.mjs + - name: Test screenshot summary updater + run: node --test scripts/update-pr-screenshot-summary.test.mjs + - name: Test Helm reference generator run: node --test scripts/upstream-release/generate-helm-reference.test.mjs diff --git a/.github/workflows/_visual-regression.yaml b/.github/workflows/_visual-regression.yaml index 6f18b0c2..93195826 100644 --- a/.github/workflows/_visual-regression.yaml +++ b/.github/workflows/_visual-regression.yaml @@ -99,6 +99,17 @@ jobs: PLAYWRIGHT_SKIP_BUILD: '1' run: npm run test:visual + # Keep failure summaries independent of large PNGs, traces, and videos. + # The privileged consumer validates these files as untrusted text. + - name: Upload compact failure details + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + if: failure() + with: + name: visual-failure-details + path: playwright-report/data/*.md + retention-days: 7 + if-no-files-found: ignore + - name: Upload test artifacts uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 if: failure() diff --git a/.github/workflows/pr-screenshot-summary.yaml b/.github/workflows/pr-screenshot-summary.yaml index 7d2da2ea..4933c91f 100644 --- a/.github/workflows/pr-screenshot-summary.yaml +++ b/.github/workflows/pr-screenshot-summary.yaml @@ -9,9 +9,19 @@ on: # zizmor: ignore[dangerous-triggers] workflow_run: workflows: ['On PR'] types: [completed] + # Baseline publication calls this directly: a GITHUB_TOKEN push cannot + # reliably start the workflow_run chain, even after the PR run is approved. + workflow_call: + inputs: + pr_number: + required: true + type: string + head_sha: + required: true + type: string concurrency: - group: ${{ github.workflow }}-${{ github.event.workflow_run.pull_requests[0].number }} + group: ${{ github.workflow }}-${{ inputs.pr_number || github.event.workflow_run.pull_requests[0].number }} cancel-in-progress: true permissions: {} @@ -20,8 +30,9 @@ jobs: update-description: name: Update PR description with screenshot summary if: | - github.event.workflow_run.event == 'pull_request' && - github.event.workflow_run.pull_requests[0] != null + inputs.pr_number != '' || + (github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.pull_requests[0] != null) runs-on: ubuntu-latest timeout-minutes: 5 permissions: @@ -33,9 +44,9 @@ jobs: id: pr env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + PR_NUMBER: ${{ inputs.pr_number || github.event.workflow_run.pull_requests[0].number }} REPO: ${{ github.repository }} - RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + RUN_HEAD_SHA: ${{ inputs.head_sha || github.event.workflow_run.head_sha }} run: | PR_JSON="$(gh api "repos/$REPO/pulls/$PR_NUMBER")" BASE_SHA="$(echo "$PR_JSON" | jq -r '.base.sha')" @@ -63,7 +74,7 @@ jobs: if: steps.pr.outputs.current == 'true' env: EXPECTED_HEAD_SHA: ${{ steps.pr.outputs.head_sha }} - PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + PR_NUMBER: ${{ inputs.pr_number || github.event.workflow_run.pull_requests[0].number }} run: | git fetch --no-tags origin \ "+refs/pull/$PR_NUMBER/head:refs/remotes/origin/pr/$PR_NUMBER/head" @@ -74,7 +85,7 @@ jobs: fi - name: Resolve visual regression job - if: steps.pr.outputs.current == 'true' + if: github.event_name == 'workflow_run' && steps.pr.outputs.current == 'true' id: visual env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} @@ -87,8 +98,10 @@ jobs: gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE" CONCLUSION="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].conclusion // "missing"' "$JOBS_FILE")" JOB_URL="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].html_url // ""' "$JOBS_FILE")" - ARTIFACT_ID="$(jq -r '[.artifacts[] | select(.name == "playwright-report" and .expired == false)][0].id // ""' "$ARTIFACTS_FILE")" - ARTIFACT_SIZE="$(jq -r '[.artifacts[] | select(.name == "playwright-report" and .expired == false)][0].size_in_bytes // "0"' "$ARTIFACTS_FILE")" + jq '.artifacts |= ([.artifacts[] | select(.name == "visual-failure-details" and .expired == false)] + [.artifacts[] | select(.name == "playwright-report" and .expired == false)])' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected" + mv "$ARTIFACTS_FILE.selected" "$ARTIFACTS_FILE" + ARTIFACT_ID="$(jq -r '.artifacts[0].id // ""' "$ARTIFACTS_FILE")" + ARTIFACT_SIZE="$(jq -r '.artifacts[0].size_in_bytes // "0"' "$ARTIFACTS_FILE")" { echo "conclusion=$CONCLUSION" echo "job_url=$JOB_URL" @@ -115,19 +128,19 @@ jobs: if: steps.pr.outputs.current == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + PR_NUMBER: ${{ inputs.pr_number || github.event.workflow_run.pull_requests[0].number }} BASE_SHA: ${{ steps.pr.outputs.base_sha }} HEAD_SHA: ${{ steps.pr.outputs.head_sha }} # This script comes from the checked-out base revision, never the PR. run: node scripts/update-pr-screenshot-summary.mjs - name: Update visual failure comment - if: always() && steps.pr.outputs.current == 'true' + if: always() && github.event_name == 'workflow_run' && steps.pr.outputs.current == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + PR_NUMBER: ${{ inputs.pr_number || github.event.workflow_run.pull_requests[0].number }} REPO: ${{ github.repository }} - RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + RUN_HEAD_SHA: ${{ inputs.head_sha || github.event.workflow_run.head_sha }} RUN_URL: ${{ github.event.workflow_run.html_url }} VISUAL_CONCLUSION: ${{ steps.visual.outputs.conclusion }} VISUAL_JOB_URL: ${{ steps.visual.outputs.job_url }} diff --git a/.github/workflows/update-visual-baselines.yaml b/.github/workflows/update-visual-baselines.yaml index 8cd583a1..6337ad16 100644 --- a/.github/workflows/update-visual-baselines.yaml +++ b/.github/workflows/update-visual-baselines.yaml @@ -149,6 +149,7 @@ jobs: contents: write outputs: updated: ${{ steps.commit.outputs.updated }} + head_sha: ${{ steps.commit.outputs.head_sha }} steps: - name: Download untrusted baseline data uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 @@ -232,6 +233,7 @@ jobs: 'tests/visual/*.spec.ts-snapshots/*.png' if git diff --cached --quiet; then echo "No snapshot diff reproduced against this branch." + echo "head_sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT" echo "updated=false" >> "$GITHUB_OUTPUT" exit 0 fi @@ -241,8 +243,22 @@ jobs: git config --local core.hooksPath /dev/null git commit --no-verify -m "Update visual baselines for ${HEAD_SHA:0:12}" git push origin "HEAD:$HEAD_REF" + echo "head_sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT" echo "updated=true" >> "$GITHUB_OUTPUT" + update-description: + name: Summarize published baselines + needs: publish + permissions: + actions: read + contents: read + pull-requests: write + # issue_comment loads both workflows from the trusted default revision. + uses: $/.github/workflows/pr-screenshot-summary.yaml + with: + pr_number: ${{ format('{0}', github.event.issue.number) }} + head_sha: ${{ needs.publish.outputs.head_sha }} + comment-result: name: Comment result needs: [authorize, generate, publish] diff --git a/scripts/update-pr-screenshot-summary.mjs b/scripts/update-pr-screenshot-summary.mjs index 01992f39..92929042 100644 --- a/scripts/update-pr-screenshot-summary.mjs +++ b/scripts/update-pr-screenshot-summary.mjs @@ -65,15 +65,16 @@ function main() { { encoding: 'utf-8' } ).trim(); - const currentBody = gh([ - 'pr', - 'view', - prNumber, - '--json', - 'body', - '-q', - '.body', - ]); + const currentPR = JSON.parse( + gh(['pr', 'view', prNumber, '--json', 'body,headRefOid,state']) + ); + if (currentPR.headRefOid !== headSha || currentPR.state !== 'OPEN') { + console.log( + 'PR head or state changed — skipping stale screenshot summary.' + ); + return; + } + const currentBody = currentPR.body; let newBody; if (summary.startsWith('No visual-regression')) { diff --git a/scripts/update-pr-screenshot-summary.test.mjs b/scripts/update-pr-screenshot-summary.test.mjs new file mode 100644 index 00000000..ec1cfb9c --- /dev/null +++ b/scripts/update-pr-screenshot-summary.test.mjs @@ -0,0 +1,118 @@ +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import test from 'node:test'; + +const scripts = path.dirname(fileURLToPath(import.meta.url)); + +function fixture(t) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'screenshot-summary-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const git = (...args) => + execFileSync('git', args, { cwd: root, encoding: 'utf8' }).trim(); + git('init', '--quiet'); + git('config', 'user.name', 'Test'); + git('config', 'user.email', 'test@example.com'); + git('remote', 'add', 'origin', 'https://github.com/example/docs.git'); + fs.mkdirSync(path.join(root, 'scripts')); + for (const name of [ + 'update-pr-screenshot-summary.mjs', + 'pr-screenshot-summary.mjs', + ]) { + fs.copyFileSync(path.join(scripts, name), path.join(root, 'scripts', name)); + } + const snapshot = + 'tests/visual/pages.spec.ts-snapshots/home-light-desktop-linux'; + fs.mkdirSync(path.dirname(path.join(root, snapshot)), { recursive: true }); + fs.writeFileSync(path.join(root, `${snapshot}.png`), 'before'); + fs.writeFileSync( + path.join(root, `${snapshot}.json`), + JSON.stringify({ + url: { path: '/' }, + testFile: 'tests/visual/pages.spec.ts', + }) + ); + git('add', '.'); + git('commit', '--quiet', '-m', 'Base'); + const base = git('rev-parse', 'HEAD'); + fs.writeFileSync(path.join(root, `${snapshot}.png`), 'after'); + git('commit', '--quiet', '-am', 'Updated baseline'); + const head = git('rev-parse', 'HEAD'); + const bin = path.join(root, 'bin'); + fs.mkdirSync(bin); + fs.writeFileSync( + path.join(bin, 'gh'), + `#!/usr/bin/env node +const fs = require('node:fs'); +const args = process.argv.slice(2); +if (args[1] === 'view') { + console.log(process.env.MOCK_PR); +} else if (args[1] === 'edit') { + fs.copyFileSync(args[args.indexOf('--body-file') + 1], process.env.SAVED_BODY); +} else { + throw new Error('Unexpected gh command'); +} +`, + { mode: 0o755 } + ); + const savedBody = path.join(root, 'saved-body.md'); + return { + savedBody, + head, + run(overrides = {}) { + return execFileSync( + 'node', + ['scripts/update-pr-screenshot-summary.mjs'], + { + cwd: root, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}${path.delimiter}${process.env.PATH}`, + PR_NUMBER: '123', + BASE_SHA: base, + HEAD_SHA: head, + SAVED_BODY: savedBody, + MOCK_PR: JSON.stringify({ + body: 'Author description', + headRefOid: head, + state: 'OPEN', + ...overrides, + }), + }, + } + ); + }, + }; +} + +test('appends baseline images and replaces the existing summary once', (t) => { + const f = fixture(t); + f.run(); + const body = fs.readFileSync(f.savedBody, 'utf8'); + assert.ok(body.startsWith('Author description\n\n')); + assert.match(body, /Before/); + assert.match(body, /After/); + assert.ok(body.includes(`/raw/${f.head}/tests/visual/`)); + assert.match(f.run({ body }), /summary unchanged/); + f.run({ + body: body.replace('## Visual regression screenshots', 'Stale summary'), + }); + const refreshed = fs.readFileSync(f.savedBody, 'utf8'); + assert.equal(refreshed, body); +}); + +test('skips an advanced PR head', (t) => { + const f = fixture(t); + assert.match(f.run({ headRefOid: 'newer-head' }), /skipping stale/); + assert.equal(fs.existsSync(f.savedBody), false); +}); + +test('skips a closed PR', (t) => { + const f = fixture(t); + assert.match(f.run({ state: 'CLOSED' }), /skipping stale/); + assert.equal(fs.existsSync(f.savedBody), false); +});