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 (3)badge-canvas.png — 1573 pixels differtooltip-dark.png — 881 pixels differtooltip-light.png — 956 pixels differBaselines come from the |
| run: npx wait-on http://localhost:3000 --timeout 120000 | ||
| working-directory: regression-test | ||
|
|
||
| # `trashAssetsBeforeRuns` wipes cypress/screenshots at the start of every |
There was a problem hiding this comment.
trashAssetsBeforeRuns -- what is this referencing?
…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>
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
| // Deliberately taller than a real browser window. `capture: 'fullPage'` | ||
| // scrolls the viewport down the document and stitches the slices together, | ||
| // and the stitch is the least reproducible part of the capture. Measured | ||
| // against the baselines on the `visual-baselines` branch, raising this from | ||
| // 800 takes the suite from 58 stitch operations to 37, and from 17 of 32 | ||
| // pages captured in one pass to 28. Only `min-height: 100vh` on the body | ||
| // depends on this value. |
There was a problem hiding this comment.
unnecessary comment IMO. Especially parts that talk about the past value. We should not keep history in comments.
Also why is this needed? Whats the issue with lots of stitch operations? If they are bad why not set this to 9999999?
| '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)
| // Straight comparison first, so identical screenshots cost nothing extra; | ||
| // the realignment check below only runs on one that already failed it. The | ||
| // size guard keeps a real layout change from being shifted away — only a | ||
| // delta within the shift budget is a rounding artifact. |
There was a problem hiding this comment.
A better explanation:
Check if the change is just a layout shift
| return out | ||
| } | ||
|
|
||
| // Is `actual` pixel-identical to `baseline` once shifted by up to `maxShift`? |
There was a problem hiding this comment.
As I see from the code, it takes into account shifting into any direction, not just up?
| // 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
| status === 'changed' && | ||
| maxShift > 0 && | ||
| Math.abs(baseline.width - actual.width) <= maxShift && | ||
| Math.abs(baseline.height - actual.height) <= maxShift && | ||
| matchesWhenShifted(baseline, actual, threshold, maxShift) |
There was a problem hiding this comment.
maybe add a layout shifted: true to the summary in this case?
2 participants
Summary
visual-diffretries the comparison at small integer offsets before calling a screenshot changed, behind--max-shift(default 1,0restores exact matching). Scoring over the shared region also covers a screenshot one pixel taller with identical content.viewportHeight800 → 2000, cutting thefullPagestitching that produces those shifts: 58 stitch operations → 37, and 17/32 → 28/32 pages captured in a single pass (measured against thevisual-baselinesbranch).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)Test Plan
changedrows on this PR — the viewport change alters layout, so it can't demonstrate its own success. Merging refreshes the baselines.changedrows in the report to confirm they're the viewport reflow and not something unexpected.Fixes INSTUI-5180
🤖 Generated with Claude Code