From 9db1d5a95a4bd6cc8560ea7dee1fa398e3782834 Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Sun, 11 Oct 2026 19:18:24 -0300 Subject: [PATCH 1/2] fix: the HTML report shows screenshots that changed size A screenshot whose capture changed size (Dimensions have changed: 1034x111 => 1040x111) was counted in the run summary but missing from snap_diff_report.html. Comparison returns a frozen null ComparisonResult for that case, and ComparisonResult#region_area_size memoized into an ivar, so the reporter's read of area_size raised FrozenError. #record rescues per assertion and only warns under DEBUG, so the entry vanished silently, and the report header counted it as passed (passed is total minus entries). The same raise hit #to_h and #inspect on every null result. region_area_size no longer memoizes: Region#size is two multiplications, and the result stays frozen as build_null intends. For a dimension change the entry carries the baseline, the new capture and dimensions_changed ("WxH => WxH", worded by Reporters::Default#dimension_change, the same as the failure message). It links no annotated or heatmap images: none are drawn for different-sized images, and any on disk are left over from an earlier same-size failure. The template says what it shows and why: the entry is badged "resized" (it showed a green "pass" for any entry without diff images), the heatmap view explains a missing heatmap (a size change, or an image driver that produced no difference mask, which only chunky_png does) and what to do next, and the Annotated toggle labels plain images as not annotated instead of "Annotated Base/New". Why the failure was silent is the next commit. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++ lib/snap_diff/comparison_result.rb | 3 +- lib/snap_diff/reporters/default.rb | 10 ++-- lib/snap_diff/reporters/html.rb | 19 ++++++-- .../reporters/templates/report.html.erb | 36 +++++++++++---- scripts/generate_sample_report.rb | 3 +- test/unit/difference_test.rb | 11 +++++ test/unit/reporters/ai_simple_test.rb | 1 + test/unit/reporters/html_reporter_test.rb | 46 +++++++++++++++++++ 9 files changed, 119 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b5a1ee4..ed1c3bc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -396,6 +396,12 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV After each capture it used to click the input it had blurred. The click moved the pointer onto it, so the input rendered hovered in later screenshots (Firefox 157 draws that darker), and it fired the page's click handlers. It now calls `focus()` +- **A screenshot that changed size is in the HTML report.** It was counted in the + summary line but left out of `snap_diff_report.html` (and a run whose only failures + were size changes wrote no report at all): the null comparison result is frozen, and + reading its `area_size` raised `FrozenError`, which the reporter swallowed unless + `DEBUG` was set. The entry shows the baseline, the new capture and both sizes, and + says why it has no heatmap or annotations ### Known limitations - **One process per worker** (`parallel_tests`, RSpec, CI sharding) still writes one diff --git a/lib/snap_diff/comparison_result.rb b/lib/snap_diff/comparison_result.rb index b01010a9..ada5e381 100644 --- a/lib/snap_diff/comparison_result.rb +++ b/lib/snap_diff/comparison_result.rb @@ -59,8 +59,9 @@ def blank? region.nil? || region_area_size.zero? end + # Not memoized: build_null freezes its result, and Region#size is cheap. def region_area_size - @region_area_size ||= region&.size || 0 + region&.size || 0 end def ratio diff --git a/lib/snap_diff/reporters/default.rb b/lib/snap_diff/reporters/default.rb index 9bcc439c..dbe1988d 100644 --- a/lib/snap_diff/reporters/default.rb +++ b/lib/snap_diff/reporters/default.rb @@ -52,11 +52,15 @@ def annotate_and_save_images end def build_error_for_different_dimensions - change_msg = [comparison.base_image, comparison.new_image] + "Dimensions have changed: #{dimension_change}\n#{base_image_path.to_path}\n#{image_path.to_path}" + end + + # "WxH => WxH", baseline first. Public so Reporters::HTML says it the + # same way. + def dimension_change + [comparison.base_image, comparison.new_image] .map { |image| driver.dimension(image).join("x") } .join(" => ") - - "Dimensions have changed: #{change_msg}\n#{base_image_path.to_path}\n#{image_path.to_path}" end NEW_LINE = "\n" diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index ca82a2ed..2ab9baee 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -115,18 +115,31 @@ def self.default_output_path # Build a report entry from a comparison, omitting unavailable images and metrics. def failure_entry_for(name, compare) - difference = compare.difference + details = compare.dimensions_changed? ? dimension_change_details(compare) : difference_details(compare) { name: name, original: resolve_image(compare.base_image_path), - new: resolve_image(compare.image_path), + new: resolve_image(compare.image_path) + }.merge(details).compact + end + + # Images of different sizes get no annotated or heatmap images, and + # no region metrics. Any annotated images on disk for this name are + # left over from an earlier same-size failure, so they are not linked. + def dimension_change_details(compare) + {dimensions_changed: compare.reporter.dimension_change} + end + + def difference_details(compare) + difference = compare.difference + { base_diff: resolve_image(compare.reporter.annotated_base_image_path), diff: resolve_image(compare.reporter.annotated_image_path), heatmap: resolve_image(compare.reporter.heatmap_diff_path), diff_level: difference.ratio && (difference.ratio * 100).round(2), area_size: difference.region_area_size, max_color_distance: difference.meta[:max_color_distance]&.round(1) - }.compact + } end # Contributed annotations (AI triage, ...), attached at RENDER diff --git a/lib/snap_diff/reporters/templates/report.html.erb b/lib/snap_diff/reporters/templates/report.html.erb index 86c5aa34..46a0b098 100644 --- a/lib/snap_diff/reporters/templates/report.html.erb +++ b/lib/snap_diff/reporters/templates/report.html.erb @@ -129,6 +129,8 @@ .img-box img { display: block; object-fit: contain; transform-origin: 0 0; } /* At 100% zoom, image fills the box */ .img-box[data-zoom="100"] img { width: 100%; height: 100%; } + .img-empty { height: 100%; display: flex; align-items: center; justify-content: center; padding: 1rem; font-size: .75rem; line-height: 1.5; color: var(--text2); } + .img-empty p { max-width: 42em; margin: 0; } /* Single image */ .view-single { width: 100%; height: 100%; display: flex; flex-direction: column; } @@ -266,9 +268,9 @@ btn.dataset.idx = idx; /* Thumbnail: annotated mode → heatmap; normal → new image (no annotation) */ var thumb = state.annotated ? (item.heatmap || item.new || item.original || '') : (item.new || item.original || ''); - var hasDiff = item.diff || item.heatmap; - var badgeText = hasDiff - ? (item.diff_level != null ? item.diff_level + '%' : 'changed') + var hasDiff = item.diff || item.heatmap || item.dimensions_changed; + var badgeText = item.dimensions_changed ? 'resized' + : hasDiff ? (item.diff_level != null ? item.diff_level + '%' : 'changed') : 'pass'; btn.innerHTML = '
' + @@ -303,6 +305,19 @@ function selectNext() { var p = filtered.indexOf(state.current); if (p < filtered.length - 1) selectItem(filtered[p + 1]); } function selectPrev() { var p = filtered.indexOf(state.current); if (p > 0) selectItem(filtered[p - 1]); } + /* A failure has no heatmap when the images differ in size, or when the + image driver produced no difference mask. Say which, and what to do. */ + function noHeatmapReason(item) { + if (item.dimensions_changed) { + return 'No heatmap: the screenshot changed size (' + item.dimensions_changed + '). ' + + 'Heatmaps and annotations compare pixel by pixel, so they need both images at the same size; ' + + 'compare Baseline and Current instead. If the new size is intended, commit the new screenshot to accept it. ' + + 'If not, the layout changed, or the browser window size or pixel ratio differs from the run that recorded the baseline.'; + } + return 'No heatmap: the image driver produced no difference mask for this comparison. ' + + 'The chunky_png driver never produces one; vips does.'; + } + /* ── Render ───────────────────────────────────────────────────────── */ function render() { var item = DATA[state.current]; @@ -311,8 +326,10 @@ topTitle.textContent = item.name; var pos = filtered.indexOf(state.current); navCounter.textContent = (pos + 1) + '/' + filtered.length; - var hasDiff = item.diff || item.heatmap; - if (hasDiff && item.diff_level != null) { + var hasDiff = item.diff || item.heatmap || item.dimensions_changed; + if (item.dimensions_changed) { + topBadge.textContent = 'dimensions changed: ' + item.dimensions_changed; + } else if (hasDiff && item.diff_level != null) { topBadge.textContent = item.diff_level + '% diff'; } else { topBadge.textContent = hasDiff ? 'changed' : 'pass'; @@ -353,8 +370,9 @@ var newImg = ann ? (item.diff || item.new || '') : (item.new || ''); var heatImg = item.heatmap || ''; - var baseLbl = ann ? 'Annotated Base' : 'Baseline'; - var newLbl = ann ? 'Annotated New' : 'Current'; + /* Label what is shown: without annotated files the toggle falls back to the plain images */ + var baseLbl = !ann ? 'Baseline' : item.base_diff ? 'Annotated Base' : 'Baseline (not annotated)'; + var newLbl = !ann ? 'Current' : item.diff ? 'Annotated New' : 'Current (not annotated)'; var html = ''; switch (state.view) { @@ -371,7 +389,9 @@ html = '
' + newLbl + '
New
'; break; case 'heatmap': - html = '
Heatmap
Heatmap
'; + html = '
Heatmap
' + + (heatImg ? 'Heatmap' : '

' + esc(noHeatmapReason(item)) + '

') + + '
'; break; } viewInner.innerHTML = html; diff --git a/scripts/generate_sample_report.rb b/scripts/generate_sample_report.rb index 9b9245e1..af716ff0 100644 --- a/scripts/generate_sample_report.rb +++ b/scripts/generate_sample_report.rb @@ -16,7 +16,8 @@ pairs = [ {name: "islands-map", base: "a", new: "b"}, {name: "islands-variant", base: "a", new: "c"}, - {name: "portrait-layout", base: "portrait", new: "portrait_b"} + {name: "portrait-layout", base: "portrait", new: "portrait_b"}, + {name: "header-resized", base: "a", new: "a_cropped"} # dimensions change: 80x80 => 80x60 ] embed = ARGV.include?("--embed") || !!ENV["CI"] diff --git a/test/unit/difference_test.rb b/test/unit/difference_test.rb index 3acde90f..f6bb9b93 100644 --- a/test/unit/difference_test.rb +++ b/test/unit/difference_test.rb @@ -25,4 +25,15 @@ class DifferenceTest < ActiveSupport::TestCase assert_includes line, "difference_level=" assert_not_includes line, "\n" end + + # build_null freezes its result, so every reader must work on a frozen + # instance. + test "a frozen null result answers its readers" do + null = SnapDiff::ComparisonResult.build_null(nil, nil, nil, {different_dimensions: true}) + + assert_predicate null, :frozen? + assert_equal 0, null.region_area_size + assert_equal 0, null.to_h[:area_size] + assert_includes null.inspect, "area_size=0" + end end diff --git a/test/unit/reporters/ai_simple_test.rb b/test/unit/reporters/ai_simple_test.rb index 2bee8693..49751ab1 100644 --- a/test/unit/reporters/ai_simple_test.rb +++ b/test/unit/reporters/ai_simple_test.rb @@ -308,6 +308,7 @@ def heatmap_diff_path = nil HtmlCompare = Struct.new(:difference) do def base_image_path = Pathname("/nonexistent/base.png") def image_path = Pathname("/nonexistent/current.png") + def dimensions_changed? = false def reporter = HtmlReporterStub.new end HtmlAssertion = Struct.new(:name, :compare) diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb index e1250f57..8cc64dab 100644 --- a/test/unit/reporters/html_reporter_test.rb +++ b/test/unit/reporters/html_reporter_test.rb @@ -86,6 +86,44 @@ class HTMLReporterTest < ActiveSupport::TestCase assert_includes @output_path.read, "valid" end + # A dimension change fails with a frozen null ComparisonResult (no + # region, failed_by: {different_dimensions: true}). Recording it used to + # raise FrozenError, which #record swallowed, so the screenshot was + # counted but silently left out of the report. + test "#record reports a dimension change instead of skipping it" do + reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) + + reporter.record([build_dimension_change_assertion("resized")]) + + entry = reporter.failures.sole + assert_equal "resized", entry[:name] + assert_equal "80x80 => 80x60", entry[:dimensions_changed] + assert entry[:original], "the baseline must be shown" + assert entry[:new], "the new capture must be shown" + + reporter.finalize + assert @output_path.exist?, "a run whose only failure is a dimension change still gets a report" + assert_includes @output_path.read, "resized" + end + + # Annotated and heatmap images are only drawn for same-size images. Any + # on disk for this name are left over from an earlier, same-size failure + # and describe a different comparison, so the entry must not link them. + test "#record links no annotated images for a dimension change" do + reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) + assertion = build_dimension_change_assertion("stale") + stale = assertion.compare.reporter + [stale.annotated_image_path, stale.annotated_base_image_path, stale.heatmap_diff_path] + .each { |path| FileUtils.cp(assertion.compare.image_path, path) } + + reporter.record([assertion]) + + entry = reporter.failures.sole + assert_nil entry[:diff] + assert_nil entry[:base_diff] + assert_nil entry[:heatmap] + end + test "HTML reporter defaults to screenshot root path" do reporter = SnapDiff::Reporters::HTML.new expected = SnapDiff.config.root / SnapDiff.config.save_path / "snap_diff_report.html" @@ -226,4 +264,12 @@ def build_failing_assertion(name) SnapDiff::ScreenshotAssertion.new(name).tap { |a| a.compare = compare } end + + # a.png is 80x80, a_cropped.png is 80x60. + def build_dimension_change_assertion(name) + compare = make_comparison(:a, :a_cropped, destination: "resized_#{name}") + compare.processed + + SnapDiff::ScreenshotAssertion.new(name).tap { |a| a.compare = compare } + end end From 29e9e21ca82befcd146c2247af39eec8c11aabfc Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Sun, 11 Oct 2026 20:10:12 -0300 Subject: [PATCH 2/2] fix: a report error no longer hides the screenshot HTML#record rescues each entry so one bad entry can't sink the whole report, but it warned only under DEBUG and dropped the entry. Because passed is total minus entries, the dropped screenshot was counted as passed: the size-change bug fixed in the previous commit showed up as a silent pass, not as an error. The rescue now always warns, in the same shape as Reporting's reporter warnings, and keeps the screenshot as {name:, error:}. The report shows it as "could not render" with the error and where to report it, and counts it as failed: the report can't vouch for it. People reading the report as a CI artifact never see stderr, so the entry is what tells them. The rescue itself stays: a report that refuses to render because of one entry would be worse. An entry that raises before its comparison is read is counted in the total too, so failed can't exceed total. README's "Debug mode" said DEBUG=1 shows skipped entries; nothing reads DEBUG any more, so it now describes the always-on warning. DiffTest's sample cases script a Minitest::Mock comparison for exactly what verification reads; the global HTML reporter reads past that script, which used to fail silently and now warns. Those three cases now run without registered reporters, as reporter_interplay_test does. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +++ README.md | 5 ++-- lib/snap_diff/reporters/html.rb | 9 +++++- .../reporters/templates/report.html.erb | 24 ++++++++++++---- test/unit/diff_test.rb | 18 ++++++++++-- test/unit/reporters/html_reporter_test.rb | 28 +++++++++++++------ 6 files changed, 67 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed1c3bc8..c6e554de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -402,6 +402,10 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV reading its `area_size` raised `FrozenError`, which the reporter swallowed unless `DEBUG` was set. The entry shows the baseline, the new capture and both sizes, and says why it has no heatmap or annotations +- **An error while building a report entry no longer hides the screenshot.** The HTML + reporter warns on stderr without `DEBUG` (`[snap_diff] HTML report: could not render + 'name' (Error: message)`) and keeps the screenshot in the report as "could not render", + counted as failed, instead of dropping it and counting it as passed ### Known limitations - **One process per worker** (`parallel_tests`, RSpec, CI sharding) still writes one diff --git a/README.md b/README.md index c6889602..162d8e70 100644 --- a/README.md +++ b/README.md @@ -332,8 +332,9 @@ You do not need a flag to keep the diff images — a failing run leaves `.diff.p deletes them (`SnapManager#cleanup!` is this repository's own test-harness call, not something your suite runs). -`DEBUG=1` does one thing: it makes the HTML reporter print why it skipped an assertion -instead of failing quietly — useful when `snap_diff_report.html` is missing entries. +Nor do you need a flag to see report problems: if the HTML reporter cannot render a +screenshot, it prints `[snap_diff] HTML report: could not render '' ()` and +still lists the screenshot in `snap_diff_report.html`, marked "could not render". ## Installation diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index 2ab9baee..dda8cb45 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -31,15 +31,22 @@ def record(assertions) total = 0 assertions.each do |assertion| + counted = false compare = assertion.compare next unless compare total += 1 + counted = true next unless compare.difference&.different? failures << failure_entry_for(assertion.name, compare) rescue => e - warn "[snap_diff] Reporter skipped '#{assertion.name}': #{e.message}" if ENV["DEBUG"] + # A report bug must not hide a screenshot: warn, and keep it listed + # (counted as failed: the report can't vouch for it). + error = "#{e.class}: #{e.message}" + warn "[snap_diff] HTML report: could not render '#{assertion.name}' (#{error})" + total += 1 unless counted + failures << {name: assertion.name, error: error} end @mutex.synchronize do diff --git a/lib/snap_diff/reporters/templates/report.html.erb b/lib/snap_diff/reporters/templates/report.html.erb index 46a0b098..00707524 100644 --- a/lib/snap_diff/reporters/templates/report.html.erb +++ b/lib/snap_diff/reporters/templates/report.html.erb @@ -131,6 +131,7 @@ .img-box[data-zoom="100"] img { width: 100%; height: 100%; } .img-empty { height: 100%; display: flex; align-items: center; justify-content: center; padding: 1rem; font-size: .75rem; line-height: 1.5; color: var(--text2); } .img-empty p { max-width: 42em; margin: 0; } + .img-empty a { color: var(--accent); } /* Single image */ .view-single { width: 100%; height: 100%; display: flex; flex-direction: column; } @@ -268,12 +269,13 @@ btn.dataset.idx = idx; /* Thumbnail: annotated mode → heatmap; normal → new image (no annotation) */ var thumb = state.annotated ? (item.heatmap || item.new || item.original || '') : (item.new || item.original || ''); - var hasDiff = item.diff || item.heatmap || item.dimensions_changed; - var badgeText = item.dimensions_changed ? 'resized' + var hasDiff = item.diff || item.heatmap || item.dimensions_changed || item.error; + var badgeText = item.error ? 'error' + : item.dimensions_changed ? 'resized' : hasDiff ? (item.diff_level != null ? item.diff_level + '%' : 'changed') : 'pass'; btn.innerHTML = - '
' + + '
' + (thumb ? '' : '') + '
' + '