feat: make the failure message legible - #264
Conversation
A differing screenshot printed a JSON blob and four unlabelled absolute
paths. Nothing said whether `region` was [x,y,w,h] or edge coordinates,
`area_size` and `difference_level` had no denominator or unit, and the
paths -- pressed against ~20 lines of minitest/activesupport backtrace --
read as noise. The one path a reader wants most, the committed baseline
`.base.png`, was not among them: the file listed first was the capture
that had just overwritten it.
Now, from live state only:
Screenshot does not match for 'changed': the change spans 100 of 6400 px (1.56% of the 80x80 image)
changed region: [20.0,15.0,30.0,25.0] (left,top,right,bottom edges)
difference level: 0.00765625 (0.77% of the image area)
judged against: tolerance 0.001
baseline: screenshots/changed.base.png
actual: screenshots/changed.png
baseline annotated: screenshots/changed.base.diff.png
actual annotated: screenshots/changed.diff.png
heatmap: screenshots/changed.heatmap.diff.png
- every artifact is labelled, baseline first, and a label is printed only
when that file is on disk (chunky_png writes no heatmap; issue #260 is
the message naming a file CI never wrote)
- paths are relative to SnapDiff.config.root, absolute when outside it
- `judged against` restates the thresholds that were actually applied, or
says plainly that none were configured
Pass/fail behaviour and exit codes are untouched; so is the end-of-run
summary line. The two DSL expectations that pinned the old wording are
updated -- and one of them was asserting a heatmap path that chunky_png
never writes.
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideRefactors the default screenshot comparison failure message to be structured, explanatory, and artifact-aware, including denominated metrics, threshold restatement, and labeled, relative artifact paths, with tests updated and expanded to lock in the new behavior. Sequence diagram for structured screenshot failure reportingsequenceDiagram
participant Comparison
participant DefaultReporter
participant Driver
participant SnapDiffConfig
participant Filesystem
Comparison->>DefaultReporter: build_error_message()
DefaultReporter->>Driver: dimension(comparison.base_image)
Driver-->>DefaultReporter: width, height
DefaultReporter->>Comparison: difference.region_area_size
DefaultReporter->>Comparison: difference.coordinates
DefaultReporter->>Comparison: difference.ratio
DefaultReporter->>Comparison: difference.options
DefaultReporter->>Comparison: difference.meta
DefaultReporter->>SnapDiffConfig: config.root
DefaultReporter->>Filesystem: path.exist? for each artifact
Filesystem-->>DefaultReporter: on-disk artifact paths
DefaultReporter-->>Comparison: headline, metric_lines, artifact_lines
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
The v2.0.0 section was written before #250, #253, #254, #255, #256, #261, #263, #264, #266 and #267 landed, and three of its claims had gone false: - "Known limitations: fork-based parallel tests produce no HTML report ... Fixed in 2.1" -- fixed in 2.0 by #266. Reproduced both sides here: 1.15.1 + `parallelize(workers: 2, threshold: 0)` writes NO report and prints no summary line; master writes one merged report and `4 verified, 4 changed, 0 new`. - "a suite whose only contact with the v1 API is `require \"capybara_screenshot_diff/minitest\"` + `include ...Assertions` still prints nothing" -- #263 made the require doors warn. That exact setup now prints the migration notice; verified in a scratch project. - "Two removals 2.0 cannot warn about ... `driver:` as a setting" -- #263 made both the setting writer and the per-screenshot key warn. Verified: `Capybara::Screenshot::Diff.driver = :vips` prints the removal line with a call site. And the silent-by-design constant list repeated the shape of the beta2 `defined?` mistake: it listed "Os, Region" inside a run of `Capybara::Screenshot::Diff::` names. Probed on master -- `defined?(Capybara::Screenshot::Diff::Os)` and `defined?(Capybara::Screenshot::Diff::Region)` are both nil. The real names are `Capybara::Screenshot::Os` and the top-level `Region`, neither of which existed under `::Diff` in 1.15.1 either. Fully qualified now, and `::Comparison` added to match docs/UPGRADING.md. New material, every claim checked against the code or a live run: - a "why upgrade" section for the four green-suite-testing-nothing bugs (#255, #256, #254, #266), plus the unfollowable CI message (#267) and the fail_if_new precedence change - before/after transcripts of the failure message (#264), taken from the same page rendered on 1.15.1 and on master - the summary line (#261), with the fact that it comes from the HTML reporter and needs its one-line require -- an omission that would have read as a missing feature - the #250 / #253 perf table, attributed to its harness, with columns labelled before/after rather than 1.x/2.0 - the libvips fix is stated as guarded on libvips 8.15+, so a reader on an older libvips knows the bug is still theirs Install snippets stay pinned to 2.0.0.beta3 on purpose: `~> 2.0` resolves to nothing on rubygems today. docs/RELEASE_PREP.md already carries a precise step to swap all five (its grep finds exactly those five), and gains one line so the record-modes placeholder in the entry cannot ship unfilled. `rake test:unit` 651 runs / 0 failures, `standardrb lib test` clean.
beta3 fixed the canonical entry points and shipped almost none of the behaviour. beta4 is the prerelease the 2.0.0 entry actually describes: the four green-suite bugs (#254 #255 #256 #258), the accept workflow (#259), the legible failure message (#264), and the deprecation warnings that make 2.1's removals visible (#246 #263). - `lib/snap_diff/version.rb` -> 2.0.0.beta4. Nothing else holds a version; the gemspec, the legacy version file and the mirror gemspec all read it. Verified with the release workflow's own guard command: `ruby -I lib -r capybara/screenshot/diff/version -e "puts Capybara::Screenshot::Diff::VERSION"` => 2.0.0.beta4 - CHANGELOG: a `[v2.0.0.beta4]` section written as the delta from beta3, plus the record-modes PLACEHOLDER filled from #259 now that it has shipped. `grep -n PLACEHOLDER CHANGELOG.md` returns nothing. - Install snippets moved beta3 -> beta4 in README, docs/UPGRADING.md and docs/migration-guide.md. They stay PINNED: `~> 2.0` resolves to nothing while only prereleases exist, so unpinning belongs to 2.0.0 final, not here. The notes name the #272 caveat explicitly. Removing `skip_area`'s implicit stabilization wait (10.012 s -> 0.009 s measured) means a selector not yet in the DOM now yields no mask, silently, where it previously resolved after the wait. #277's run-level tally shipped in the same beta as the replacement signal, and the notes say so rather than leaving it to be discovered. Gates: `rake test:unit` 720 runs / 2124 assertions / 0 failures / 0 skips under CI=true on 4.0.6, `standardrb lib test` clean over 161 files, and `gem build` produces capybara-screenshot-diff-2.0.0.beta4.gem (93 files, 13 docs, RELEASE_PREP correctly excluded).
Two people ran the product and could not read its most important output.
Before (real run,
test/fixtures/summary_line_case.rb, real git baselines)regionis four bare numbers.area_sizehas no denominator. The four paths areunlabelled and sit against the backtrace, so they read as stack noise -- and the
baseline (
.base.png) is not among them: the file listed first is the capture thatjust overwrote it.
After (same run)
With a tolerance set, the metric block also carries its unit:
What changed
on them. A label is printed only when that file is on disk -- chunky_png
produces no diff mask, so it gets no heatmap line (the old message printed one
regardless; same family as The new-screenshot error prints a path that will never exist in CI #260).
area_sizegets a denominator and a percentage oftotal pixels plus the image dimensions.
regionis stated as edge coordinates --verified in
Region#to_edge_coordinates, not guessed.difference_levelis thechanged share of the image area, so it is printed with that unit and only when a
tolerance made us compute it.
was configured and any difference fails.
SnapDiff.config.root), absolute when the file is outsideit. Chosen because most terminals resolve relative paths against the working
directory, so they stay click-through-able while being short enough to read --
and short paths visually separate the artifact block from the absolute backtrace
frames below it, which was half the "reads as noise" complaint.
Not changed
Pass/fail behaviour, exit codes, and the end-of-run summary line
(
[snap_diff] 1 verified, 1 changed, 0 new) are untouched -- the after-run abovestill prints it verbatim.
Evidence
test/unit/reporters/default_test.rb, each mutation-checked:the denominator, the region interpretation, the difference-level unit, the
threshold line, the labels, the on-disk filter, and both directions of the
relative/absolute path rule. Every mutation went red; each was restored by a
targeted edit and confirmed green.
processes (RSpec indents the block under
Failure/Error:and it holds up).Cucumber re-raises the same
ExpectationNotMetstring with no message handlingof its own.
rake test:unit(617 runs),rake test:canonical(512 runs),standardrb lib testall green.
Expected test churn
Two expectations in
test/unit/dsl_test.rbpinned the old wording verbatim and areupdated -- a commissioned wording change, not a behaviour break. One of them was
asserting a
heatmap.diff.pngpath that chunky_png never writes.Summary by Sourcery
Improve screenshot mismatch failure messages so their metrics, thresholds, and diagnostic artifacts are immediately understandable.
New Features:
Bug Fixes:
Enhancements:
Tests: