Skip to content

Commit 76acdf7

Browse files
authored
Fix visual failure comment artifact selection (#1296)
* Fix visual failure comment artifact selection * Exclude stale visual artifacts on reruns --------- Co-authored-by: Dan Barr <6922515+danbarr@users.noreply.github.com>
1 parent 251482f commit 76acdf7

7 files changed

Lines changed: 231 additions & 21 deletions

‎.github/workflows/_static-checks.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ jobs:
3434
- name: Test release PR body renderer
3535
run: node --test scripts/upstream-release/render-pr-body.test.mjs
3636

37-
- name: Test screenshot summary updater
38-
run: node --test scripts/update-pr-screenshot-summary.test.mjs
37+
- name: Test screenshot summary helpers
38+
run: node --test scripts/update-pr-screenshot-summary.test.mjs scripts/pr-screenshot-workflow.test.mjs
3939

4040
- name: Test Helm reference generator
4141
run: node --test scripts/upstream-release/generate-helm-reference.test.mjs

‎.github/workflows/pr-screenshot-summary.yaml‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,16 +95,34 @@ jobs:
9595
JOBS_FILE="$RUNNER_TEMP/jobs.json"
9696
ARTIFACTS_FILE="$RUNNER_TEMP/artifacts.json"
9797
gh api "repos/$REPO/actions/runs/$RUN_ID/jobs?per_page=100" > "$JOBS_FILE"
98-
gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE"
9998
CONCLUSION="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].conclusion // "missing"' "$JOBS_FILE")"
10099
JOB_URL="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].html_url // ""' "$JOBS_FILE")"
101-
jq '.artifacts |= ([.artifacts[] | select(.name == "visual-failure-details" and .expired == false)] + [.artifacts[] | select(.name == "playwright-report" and .expired == false)])' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected"
100+
# Preserve the conclusion so the always() comment step can still
101+
# post a fallback if artifact discovery or parsing fails.
102+
{
103+
echo "conclusion=$CONCLUSION"
104+
echo "job_url=$JOB_URL"
105+
} >> "$GITHUB_OUTPUT"
106+
JOB_STARTED_AT="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].started_at // ""' "$JOBS_FILE")"
107+
JOB_COMPLETED_AT="$(jq -r '[.jobs[] | select(.name == "Visual regression / Playwright visual snapshots")][0].completed_at // ""' "$JOBS_FILE")"
108+
gh api "repos/$REPO/actions/runs/$RUN_ID/artifacts?per_page=100" > "$ARTIFACTS_FILE"
109+
# A run's artifacts include prior attempts. GitHub returns these
110+
# timestamps in UTC; accept only files created during this job.
111+
jq --arg started_at "$JOB_STARTED_AT" --arg completed_at "$JOB_COMPLETED_AT" '
112+
.artifacts = [.artifacts[] | select(
113+
.expired == false and
114+
$started_at != "" and $completed_at != "" and
115+
.created_at >= $started_at and .created_at <= $completed_at
116+
)] |
117+
.artifacts = (
118+
[.artifacts[] | select(.name == "visual-failure-details")] +
119+
[.artifacts[] | select(.name == "playwright-report")]
120+
)
121+
' "$ARTIFACTS_FILE" > "$ARTIFACTS_FILE.selected"
102122
mv "$ARTIFACTS_FILE.selected" "$ARTIFACTS_FILE"
103123
ARTIFACT_ID="$(jq -r '.artifacts[0].id // ""' "$ARTIFACTS_FILE")"
104124
ARTIFACT_SIZE="$(jq -r '.artifacts[0].size_in_bytes // "0"' "$ARTIFACTS_FILE")"
105125
{
106-
echo "conclusion=$CONCLUSION"
107-
echo "job_url=$JOB_URL"
108126
echo "artifact_id=$ARTIFACT_ID"
109127
echo "artifact_size=$ARTIFACT_SIZE"
110128
} >> "$GITHUB_OUTPUT"
Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
import assert from 'node:assert/strict';
5+
import { spawnSync } from 'node:child_process';
6+
import fs from 'node:fs';
7+
import os from 'node:os';
8+
import path from 'node:path';
9+
import test from 'node:test';
10+
import { parse } from 'yaml';
11+
12+
const workflow = parse(
13+
fs.readFileSync(
14+
new URL('../.github/workflows/pr-screenshot-summary.yaml', import.meta.url),
15+
'utf8'
16+
)
17+
);
18+
const resolveStep = workflow.jobs['update-description'].steps.find(
19+
(step) => step.id === 'visual'
20+
);
21+
22+
function resolveArtifacts(t, artifacts, { unavailable = false } = {}) {
23+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'visual-job-'));
24+
t.after(() => fs.rmSync(root, { recursive: true, force: true }));
25+
const bin = path.join(root, 'bin');
26+
fs.mkdirSync(bin);
27+
fs.writeFileSync(
28+
path.join(root, 'jobs.json.fixture'),
29+
JSON.stringify({
30+
jobs: [
31+
{
32+
name: 'Static checks / Lint and format checks',
33+
conclusion: 'success',
34+
},
35+
{
36+
name: 'Visual regression / Playwright visual snapshots',
37+
conclusion: 'failure',
38+
html_url: 'https://github.com/example/docs/actions/runs/1/job/2',
39+
started_at: '2026-10-30T17:00:00Z',
40+
completed_at: '2026-10-30T17:05:00Z',
41+
},
42+
],
43+
})
44+
);
45+
fs.writeFileSync(
46+
path.join(root, 'artifacts.json.fixture'),
47+
typeof artifacts === 'string' ? artifacts : JSON.stringify({ artifacts })
48+
);
49+
fs.writeFileSync(
50+
path.join(bin, 'gh'),
51+
`#!/bin/sh
52+
case "$2" in
53+
*/jobs?*) cat "$RUNNER_TEMP/jobs.json.fixture" ;;
54+
*/artifacts?*)
55+
if [ "$ARTIFACTS_UNAVAILABLE" = true ]; then exit 1; fi
56+
cat "$RUNNER_TEMP/artifacts.json.fixture" ;;
57+
*) exit 1 ;;
58+
esac
59+
`,
60+
{ mode: 0o755 }
61+
);
62+
const outputFile = path.join(root, 'outputs');
63+
const result = spawnSync('bash', ['-eo', 'pipefail', '-c', resolveStep.run], {
64+
encoding: 'utf8',
65+
env: {
66+
...process.env,
67+
PATH: `${bin}${path.delimiter}${process.env.PATH}`,
68+
RUNNER_TEMP: root,
69+
GITHUB_OUTPUT: outputFile,
70+
REPO: 'example/docs',
71+
RUN_ID: '1',
72+
ARTIFACTS_UNAVAILABLE: String(unavailable),
73+
},
74+
});
75+
const outputs = Object.fromEntries(
76+
fs
77+
.readFileSync(outputFile, 'utf8')
78+
.trim()
79+
.split('\n')
80+
.map((line) => {
81+
const separator = line.indexOf('=');
82+
return [line.slice(0, separator), line.slice(separator + 1)];
83+
})
84+
);
85+
assert.equal(outputs.conclusion, 'failure');
86+
assert.equal(
87+
outputs.job_url,
88+
'https://github.com/example/docs/actions/runs/1/job/2'
89+
);
90+
return { result, outputs };
91+
}
92+
93+
const fullReport = {
94+
name: 'playwright-report',
95+
expired: false,
96+
id: 10,
97+
size_in_bytes: 55_544_517,
98+
created_at: '2026-10-30T17:04:00Z',
99+
};
100+
const compactReport = {
101+
name: 'visual-failure-details',
102+
expired: false,
103+
id: 20,
104+
size_in_bytes: 21_006,
105+
created_at: '2026-10-30T17:03:00Z',
106+
};
107+
108+
test('selects compact details ahead of a large full report', (t) => {
109+
const { result, outputs } = resolveArtifacts(t, [fullReport, compactReport]);
110+
assert.equal(result.status, 0, result.stderr);
111+
assert.equal(outputs.artifact_id, '20');
112+
assert.equal(outputs.artifact_size, '21006');
113+
});
114+
115+
test('falls back to the full report when compact details are expired', (t) => {
116+
const { result, outputs } = resolveArtifacts(t, [
117+
{ ...compactReport, expired: true },
118+
fullReport,
119+
]);
120+
assert.equal(result.status, 0, result.stderr);
121+
assert.equal(outputs.artifact_id, '10');
122+
});
123+
124+
test('preserves the failure conclusion when no artifact is available', (t) => {
125+
const { result, outputs } = resolveArtifacts(t, []);
126+
assert.equal(result.status, 0, result.stderr);
127+
assert.equal(outputs.artifact_id, '');
128+
assert.equal(outputs.artifact_size, '0');
129+
});
130+
131+
test('preserves the failure conclusion when artifact discovery fails', (t) => {
132+
const { result, outputs } = resolveArtifacts(t, [], { unavailable: true });
133+
assert.notEqual(result.status, 0);
134+
assert.equal(outputs.artifact_id, undefined);
135+
});
136+
137+
test('preserves the failure conclusion when artifact parsing fails', (t) => {
138+
const { result, outputs } = resolveArtifacts(t, '{malformed');
139+
assert.notEqual(result.status, 0);
140+
assert.equal(outputs.artifact_id, undefined);
141+
});
142+
143+
test('uses the current full report ahead of prior-attempt compact details', (t) => {
144+
const { result, outputs } = resolveArtifacts(t, [
145+
{ ...compactReport, created_at: '2026-10-30T16:55:00Z' },
146+
fullReport,
147+
]);
148+
assert.equal(result.status, 0, result.stderr);
149+
assert.equal(outputs.artifact_id, '10');
150+
});
151+
152+
test('uses the fallback when a rerun produces no new artifacts', (t) => {
153+
const { result, outputs } = resolveArtifacts(t, [
154+
{ ...compactReport, created_at: '2026-10-30T16:55:00Z' },
155+
{ ...fullReport, created_at: '2026-10-30T16:56:00Z' },
156+
]);
157+
assert.equal(result.status, 0, result.stderr);
158+
assert.equal(outputs.artifact_id, '');
159+
});
160+
161+
test('selects current compact details when older compact details appear first', (t) => {
162+
const { result, outputs } = resolveArtifacts(t, [
163+
{ ...compactReport, id: 19, created_at: '2026-10-30T16:55:00Z' },
164+
compactReport,
165+
]);
166+
assert.equal(result.status, 0, result.stderr);
167+
assert.equal(outputs.artifact_id, '20');
168+
});
169+
170+
test('rejects artifacts uploaded after the current visual job finished', (t) => {
171+
const { result, outputs } = resolveArtifacts(t, [
172+
{ ...compactReport, created_at: '2026-10-30T17:10:00Z' },
173+
]);
174+
assert.equal(result.status, 0, result.stderr);
175+
assert.equal(outputs.artifact_id, '');
176+
});
177+
178+
test('rejects artifacts with no creation timestamp', (t) => {
179+
const { result, outputs } = resolveArtifacts(t, [
180+
{ ...compactReport, created_at: undefined },
181+
]);
182+
assert.equal(result.status, 0, result.stderr);
183+
assert.equal(outputs.artifact_id, '');
184+
});

‎tests/visual/pages.spec.ts‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
import { expect, test, type Page } from '@playwright/test';
5-
import { captureBothThemes, captureElementBothThemes } from './fixtures';
5+
import {
6+
captureBothThemes,
7+
captureColorScheme,
8+
captureElementBothThemes,
9+
} from './fixtures';
610

711
/**
812
* The documented representative-page matrix for issue #1163. Keep this
@@ -32,16 +36,20 @@ test('home page', async ({ page }, testInfo) => {
3236
await captureBothThemes(page, testInfo, 'Home page');
3337
});
3438

35-
test('theme preview page', async ({ page }, testInfo) => {
36-
await gotoSuccessful(page, '/theme-preview');
37-
// Mermaid diagrams render client-side and asynchronously — wait for
38-
// both of the page's diagrams to finish before capturing, or the
39-
// snapshot flakes between "still rendering" and "done".
40-
await expect(page.locator('.docusaurus-mermaid-container svg')).toHaveCount(
41-
2
42-
);
43-
await captureBothThemes(page, testInfo, 'Theme preview page');
44-
});
39+
for (const scheme of ['light', 'dark'] as const) {
40+
test(`theme preview page - ${scheme}`, async ({ page }, testInfo) => {
41+
// Mermaid rerenders asynchronously when the theme changes. Start each
42+
// page in its final theme so full-page capture cannot measure the page
43+
// while a theme switch temporarily removes a diagram.
44+
await page.emulateMedia({ colorScheme: scheme });
45+
await gotoSuccessful(page, '/theme-preview');
46+
await expect(page.locator('html')).toHaveAttribute('data-theme', scheme);
47+
await expect(page.locator('.docusaurus-mermaid-container svg')).toHaveCount(
48+
2
49+
);
50+
await captureColorScheme(page, testInfo, 'Theme preview page', scheme);
51+
});
52+
}
4553

4654
for (const { section, path } of NAV_PAGES) {
4755
test(`navigation loads - ${section}`, async ({ page }) => {
Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
{
22
"titlePath": [
33
"pages.spec.ts",
4-
"theme preview page"
4+
"theme preview page - dark"
55
],
66
"testFile": "tests/visual/pages.spec.ts",
77
"url": {
88
"origin": "http://localhost:3005",
99
"path": "/theme-preview"
1010
},
11-
"reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page\""
11+
"reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page - dark\""
1212
}
64.5 KB
Loading
Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
{
22
"titlePath": [
33
"pages.spec.ts",
4-
"theme preview page"
4+
"theme preview page - light"
55
],
66
"testFile": "tests/visual/pages.spec.ts",
77
"url": {
88
"origin": "http://localhost:3005",
99
"path": "/theme-preview"
1010
},
11-
"reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page\""
11+
"reproCommand": "npx playwright test tests/visual/pages.spec.ts -g \"theme preview page - light\""
1212
}

0 commit comments

Comments
 (0)