chore(ui-scripts,regression-test): ignore whole-pixel shifts in visual regression diffs - #2710
Conversation
|
Visual regression reportCypress suite: ✅ Passing Visual diff:
Accessibility (axe): ✅ No violations. 📊 View full report — click a screenshot's ⚠ badge to see each violation boxed on the image, with the offending element named and contrast failures shown as color swatches. Diff images (2)badge-canvas.png — 1573 pixels differtooltip-light.png — 956 pixels differBaselines come from the |
2bbb988 to
29285f7
Compare
balzss
changed the title
matyasf
left a comment
matyasf
left a comment
There was a problem hiding this comment.
see my comments. Code looks OK, but I'd change the comments
| 'max-shift': { | ||
| type: 'number', | ||
| describe: | ||
| 'Treat a screenshot as unchanged when it matches its baseline exactly after being shifted by up to this many pixels. Absorbs whole-pixel layout rounding, which moves a component without altering it. 0 requires an exact match.', |
There was a problem hiding this comment.
"Absorbs whole-pixel layout rounding, which moves a component without altering it." is not needed
There was a problem hiding this comment.
it matches shift to ANY direction within the tolerance (including e.g. diagonal shifts)
There was a problem hiding this comment.
Both fixed. I removed that sentence and kept only "Absorbs whole-pixel layout rounding", so --help still says why the flag exists. And you are right about the direction — dx and dy both go from -maxShift to +maxShift, so diagonals too. The text says "in any direction" now.
| // Each offset is scored over the region the two images share, so the band that | ||
| // shifts in from outside is never counted. (0, 0) is included deliberately: the | ||
| // caller's comparison *pads* mismatched sizes, while this one *crops* to the | ||
| // overlap, which is what lets "one pixel taller, same content" pass. |
There was a problem hiding this comment.
way way too verbose. Please simplify this to 1-2 lines
There was a problem hiding this comment.
Cut from 16 lines to 8. I kept two things only: why a pixel-count tolerance cannot do this, and why (0, 0) is in the loop. The second one is not obvious — without it someone will remove it later and break the "one pixel taller, same content" case.
…l regression diffs The suite failed on screenshots that were pixel-identical to their baseline but translated by about a pixel. When a layout box rounds one device pixel differently the whole painted subtree moves, so a direct comparison lights up every edge in the image: on a real baseline, a 1px translation produces 2825 differing pixels. That is not a visual regression, and the suite should not report one. visual-diff now retries the comparison at small integer offsets before calling a screenshot changed, behind --max-shift (default 1, 0 restores exact matching). Scoring happens over the region the two images share, which also covers the related case of a screenshot one pixel taller with identical content. A pixel-count tolerance cannot do this job: the shift above and a genuine 40x40 recolour differ by less than a factor of two, so any threshold loose enough to absorb the first would hide the second. viewportHeight goes 800 to 2000 to cut the stitching that produces those shifts. capture: 'fullPage' scrolls the viewport down the document and stitches the slices, and the stitch is the least reproducible part of the capture. Measured against the baselines branch, this takes the suite from 58 stitch operations to 37, and from 17 of 32 pages captured in one pass to 28. The viewport change alters layout, so the first run shows a cascade of changed rows; merging refreshes the baselines. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…and revert viewport change Addresses review feedback on #2710. - Trim the `matchesWhenShifted` and status-check comments, and state that the match covers a shift in any direction, diagonals included. - Report a realigned screenshot as layout shifted in `summary.json` and the HTML report instead of silently calling it unchanged. - Revert `viewportHeight` to 800. The setting does not reach the browser in CI: captures are floored at 720px with both 800 and 2000, so the claimed stitch reduction never happened. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
29285f7 to
a1d1cc6
Compare
matyasf
left a comment
matyasf
left a comment
There was a problem hiding this comment.
Looks OK. I still prefer human written comments in the code though
3 participants
Summary
visual-diffretries the comparison at small integer offsets before calling a screenshot changed, behind--max-shift(default 1,0restores exact matching). It tries every offset within the budget in any direction, diagonals included. Scoring over the shared region also covers a screenshot one pixel taller with identical content.layout shiftedinsummary.jsonand in the HTML report, so absorbing a shift doesn't hide it.A pixel-count tolerance can't do this job. On a real baseline a 1px translation produces 2825 differing pixels and a genuine 40×40 recolour produces 1479 — less than a factor of two apart, so any threshold loose enough to absorb the first would hide the second.
Verified against four real CI baselines — identical, shifted 1px, 1px taller, and a genuine recolour:
--max-shift 0(current)Dropped from this PR: the
viewportHeightchangeAn earlier revision raised
viewportHeightfrom 800 to 2000 to cutfullPagestitching. That's reverted, because the setting never reached the browser.The body carries
min-height: 100vh, so every short page should be floored at exactly the viewport height. Across the 99 baselines, 35 sit at exactly 720 — not 800. The images this PR's own run produced are the same height as the baselines (alert-canvas 720 → 720), even though that run hadviewportHeight: 2000.viewportWidth: 1280does take effect, so the config loads; the height is clamped, most likely by the bundled Electron window since the suite runs with no--browser.So the stitch-reduction numbers in the earlier revision were arithmetic over baseline heights, not measurements. Sizing the viewport properly needs the setting to work first, and that's separate from this PR.
Test Plan
changedrows in the report (badge-canvas,tooltip-light,tooltip-dark) predate this PR and are not explained by it.badge-canvasis a single-pass 720px page, so it can't be a stitch artifact. Worth a look before merge.--max-shiftis covered by unit tests inpackages/ui-scripts/lib/__node_tests__/visual-diff.test.tsplus the four-baseline check above. CI hasn't exercised it, since no run has produced a real whole-pixel shift yet.Fixes INSTUI-5180
🤖 Generated with Claude Code