mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-22 15:15:18 +00:00
* ci(web-shell): before/after visual previews, showing only changed views The visual-preview bot posted the same fixed set of canned screenshots on every web-shell PR, so it could not show what a PR actually changed (a mermaid/split feature was invisible) and added noise on PRs that touch the UI only trivially. Render each scenario against BOTH the PR base (`main`) and the PR head, pixel-diff them, and post a stitched "main | this PR" composite for only the views that CHANGED. A PR with no visual impact composites nothing → "no visual change". This makes the preview feature-aware with no per-PR understanding: the diff finds exactly the surface the PR moved (and it subsumes the backend-PR noise #6959 pre-filtered, at the content level). - web-shell-visuals-compose.mjs: pixel-diff (canvas) + stitch a labelled composite; pure helpers (parseShot/isChanged/planWork) unit-tested. - web-shell-visuals.yml: also render the base — trusted `main` via pull_request.base.sha, so no secret exposure in the untrusted-PR job — then compose; composites replace the raw after-shots (same `<view>-<theme>.png` name the publisher already expects). - publish buildComment: list composites; "no visual change" when none. Verified locally by overlaying #6881's real changes onto main + a new mermaid scenario: the compositor flagged the mermaid view 6.5% changed (its new zoom controls) and correctly skipped the unchanged transcript. * ci(web-shell): address before/after review — lazy import, merge-base, robustness Addresses the /review findings on the before/after preview: - Lazy the @playwright/test import in the compositor so the pure exports (parseShot/isChanged/planWork) load dependency-free, and wire web-shell-visuals-compose.test.mjs into the github_ci_only test step — it was never actually running in CI. (finding 1) - Diff against the MERGE-BASE, not the base-branch tip, so a PR branch behind main doesn't render others' already-landed changes reversed as this PR's diff. (finding 2) - continue-on-error on the base checkout + install so a flaky base degrades to after-only instead of sinking the job; compose likewise degrades to the raw after-shots on failure. (finding 3) - timeout 20->30 (the job ~doubled) and scope the base render to screenshots.spec.ts, skipping the discarded flow videos. (finding 4) - diffPct: add img.onerror so a corrupt/truncated baseline PNG can't hang page.evaluate to the job timeout. (finding 5) - Lower CHANGED_PCT_THRESHOLD 0.1 -> 0.02 (~205px at 1280x800) so an icon swap or one-word label change isn't classified "no change". (finding 6) - Nits: correct the stdout comment, esc() the burned-in labels, and scope the comment wording to "screenshots" (flows are always head-only). * ci(web-shell): address before/after review round 2 (yiliang114) - diffPct: a dimension change IS a visual change — comparing only the overlapping rectangle hid it (a taller viewport with unchanged top pixels read 0%). Short-circuit any size mismatch to changed. (Critical) - Composite/comment label: "PR base (before)" not "main" — the workflow also runs for release/**, whose base is not main. (Critical) - Merge-base resolve: retry the compare API, then emit an EMPTY sha and SKIP the base render (after-only) rather than falling back to the base-branch tip, which reintroduces the reversed-diff bug. (Critical) - Base steps get ids; the before render runs only when the base checkout AND install both succeeded — else base/ (nested under head) resolves node_modules up to head's and produces a hybrid before. (Critical) - Publisher: a zero-change run now UPDATES the marker comment (image-less "no screenshot changes") instead of exiting, so a prior preview's stale images + SHA do not linger. (Critical) Finding 6 (helper tests skip full CI) is a pre-existing repo-wide gap for every .github/scripts test; left for a focused follow-up. * ci(web-shell): close the compositor browser in a finally (leak on error) A mid-loop rejection in diffPct (evaluate timeout / CDP disconnect on a corrupt or oversized PNG) exited composeCli via the exception and skipped browser.close(), leaking a ~200 MB Chromium child for the rest of the CI job. Wrap the page + loop in try/finally so the browser always closes. Also drop the stale "main (before)" labels from the docstring (the composite/comment say "PR base" now, since the workflow also runs for release/**). * ci(web-shell): catch the compositor CLI promise for a clean exit An unhandled composeCli() rejection (e.g. a missing @playwright/test) printed an UnhandledPromiseRejectionWarning and exited without a meaningful code; add a .catch that writes the error and exits 1. * ci: run .github/scripts helper tests in full CI + test planWork nullish guards - Finding 6: the compositor/publisher helper tests ran only in the github_ci_only profile, which a `full` PR skips (and vitest test:ci doesn't collect node:test files) — so a compositor change could pass CI without its regression tests. Run them in the full ubuntu Test job too. - Cover planWork's `?? []` guards with null/undefined inputs. * ci: extract HELPER_TESTS list so both CI profiles share one source of truth Round-3 F6 fix duplicated the .github/scripts node:test list across the github_ci_only and full-profile steps; a missed edit would silently drop coverage in one path. Hoist it to a workflow-level env var both reference. --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
91 lines
3 KiB
JavaScript
91 lines
3 KiB
JavaScript
/**
|
|
* @license
|
|
* Copyright 2025 Qwen
|
|
* SPDX-License-Identifier: Apache-2.0
|
|
*/
|
|
|
|
import assert from 'node:assert/strict';
|
|
import test from 'node:test';
|
|
|
|
import {
|
|
CHANGED_PCT_THRESHOLD,
|
|
isChanged,
|
|
parseShot,
|
|
planWork,
|
|
} from './web-shell-visuals-compose.mjs';
|
|
|
|
test('parseShot extracts view + theme, is case-insensitive, and strips dirs', () => {
|
|
assert.deepEqual(parseShot('mermaid-diagram-light.png'), {
|
|
view: 'mermaid-diagram',
|
|
theme: 'light',
|
|
});
|
|
assert.deepEqual(parseShot('permission-panel-DARK.png'), {
|
|
view: 'permission-panel',
|
|
theme: 'dark',
|
|
});
|
|
// A view name may itself contain a dash — only the final -light/-dark splits.
|
|
assert.deepEqual(parseShot('a/b/session-transcript-dark.png'), {
|
|
view: 'session-transcript',
|
|
theme: 'dark',
|
|
});
|
|
});
|
|
|
|
test('parseShot rejects non-screenshot names (gifs, manifest, themeless)', () => {
|
|
assert.equal(parseShot('model-switch.gif'), null);
|
|
assert.equal(parseShot('manifest.json'), null);
|
|
assert.equal(parseShot('mermaid-diagram.png'), null); // no -light/-dark
|
|
assert.equal(parseShot('random.png'), null);
|
|
});
|
|
|
|
test('isChanged: a view with no baseline (PR-added) always counts as changed', () => {
|
|
assert.equal(isChanged({ hasBefore: false, changedPct: 0 }), true);
|
|
});
|
|
|
|
test('isChanged: with a baseline, only meets threshold counts as changed', () => {
|
|
assert.equal(isChanged({ hasBefore: true, changedPct: 0 }), false);
|
|
// Below the threshold (relative, so it survives a threshold retune).
|
|
assert.equal(
|
|
isChanged({ hasBefore: true, changedPct: CHANGED_PCT_THRESHOLD / 2 }),
|
|
false,
|
|
);
|
|
assert.equal(
|
|
isChanged({ hasBefore: true, changedPct: CHANGED_PCT_THRESHOLD }),
|
|
true,
|
|
);
|
|
assert.equal(isChanged({ hasBefore: true, changedPct: 6.5 }), true);
|
|
});
|
|
|
|
test('planWork pairs after-shots with baseline presence, sorted, ignoring non-shots', () => {
|
|
const plan = planWork(
|
|
[
|
|
'mermaid-diagram-light.png',
|
|
'mermaid-diagram-dark.png',
|
|
'session-transcript-light.png',
|
|
'model-switch.gif', // ignored (not a shot)
|
|
'manifest.json', // ignored
|
|
],
|
|
['session-transcript-light.png', 'session-transcript-dark.png'],
|
|
);
|
|
assert.deepEqual(plan, [
|
|
{ name: 'mermaid-diagram-dark.png', hasBefore: false }, // PR-added → NEW
|
|
{ name: 'mermaid-diagram-light.png', hasBefore: false },
|
|
{ name: 'session-transcript-light.png', hasBefore: true }, // has baseline
|
|
]);
|
|
});
|
|
|
|
test('planWork is robust to a missing before set (first-ever PR → all NEW)', () => {
|
|
const plan = planWork(['home-light.png', 'home-dark.png'], []);
|
|
assert.deepEqual(
|
|
plan.map((p) => p.hasBefore),
|
|
[false, false],
|
|
);
|
|
});
|
|
|
|
test('planWork tolerates null/undefined args (exercises the ?? [] guards)', () => {
|
|
assert.deepEqual(planWork(null, null), []);
|
|
assert.deepEqual(planWork(undefined, undefined), []);
|
|
// after present, before nullish → every shot is NEW.
|
|
assert.deepEqual(planWork(['a-light.png'], null), [
|
|
{ name: 'a-light.png', hasBefore: false },
|
|
]);
|
|
});
|