diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b5a1ee4..c6e554de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -396,6 +396,16 @@ 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 +- **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/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..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 @@ -115,18 +122,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..00707524 100644 --- a/lib/snap_diff/reporters/templates/report.html.erb +++ b/lib/snap_diff/reporters/templates/report.html.erb @@ -129,6 +129,9 @@ .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; } + .img-empty a { color: var(--accent); } /* Single image */ .view-single { width: 100%; height: 100%; display: flex; flex-direction: column; } @@ -266,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; - var badgeText = hasDiff - ? (item.diff_level != null ? item.diff_level + '%' : 'changed') + 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 ? '' : '') + '
' + '
' + '
' + esc(item.name) + '
' + (item.annotations || []).map(function(note) { @@ -303,6 +307,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 +328,12 @@ 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 || item.error; + if (item.error) { + topBadge.textContent = 'could not render'; + } else 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,11 +374,20 @@ 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) { + /* The report failed to build this entry: say so in every view instead of empty images */ + switch (item.error ? 'error' : state.view) { + case 'error': + html = '

' + + 'Could not render this screenshot: ' + esc(item.error) + '. ' + + 'The test output has its result. Please report this error at ' + + 'github.com/snap-diff/snap_diff-capybara/issues.' + + '

'; + break; case 'both': html = '
' + '
' + baseLbl + '
Base
' + @@ -371,7 +401,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/diff_test.rb b/test/unit/diff_test.rb index a7fb6055..7c47a2f4 100644 --- a/test/unit/diff_test.rb +++ b/test/unit/diff_test.rb @@ -70,7 +70,7 @@ class DiffTest < ActiveSupport::TestCase test "does not fail when fail_on_difference is false and screenshots differ" do SnapDiff.config.stub(:fail_on_difference, false) do test_case = SampleMiniTestCase.new(:_test_sample_screenshot_error) - test_case.run + without_reporters { test_case.run } assert_equal 0, test_case.failures.size end end @@ -123,7 +123,7 @@ class DiffTest < ActiveSupport::TestCase test "aggregates failures on teardown for Minitest" do test_case = SampleMiniTestCase.new(:_test_sample_screenshot_error) - test_case.run + without_reporters { test_case.run } assert_equal 1, test_case.failures.size assert_includes test_case.failures.first.message, "expected error message" @@ -135,7 +135,7 @@ class DiffTest < ActiveSupport::TestCase expected_message = "Screenshot does not match for 'sample_screenshot' expected error message for non minitest" - assert_raises(SnapDiff::ExpectationNotMet, expected_message) { test_case.teardown } + assert_raises(SnapDiff::ExpectationNotMet, expected_message) { without_reporters { test_case.teardown } } assert_empty(SnapDiff.session.assertions) end @@ -234,6 +234,18 @@ class ScreenshotFormatTest < ActiveSupport::TestCase end end + # The sample cases script a Minitest::Mock comparison for exactly what + # verification reads. The registered reporters (HTML) read more, run off + # the script and warn about it, so keep them out of these cases. + def without_reporters + original = SnapDiff::Reporting.mutex.synchronize do + SnapDiff::Reporting.reporters.dup.tap { SnapDiff::Reporting.reporters.clear } + end + yield + ensure + SnapDiff::Reporting.mutex.synchronize { SnapDiff::Reporting.reporters.replace(original) } if original + end + def screenshot_dir File.join(SnapDiff.config.screenshot_area, *screenshot_namer.directory_parts) end 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..8cdbc6b9 100644 --- a/test/unit/reporters/html_reporter_test.rb +++ b/test/unit/reporters/html_reporter_test.rb @@ -69,21 +69,69 @@ class HTMLReporterTest < ActiveSupport::TestCase assert_includes html, "3 total" end - test "#record tolerates broken assertions without crashing" do + # An entry that fails to build must not vanish, nor sink the report: it is + # warned about, kept as "could not render", and counted as failed. + test "#record keeps an entry it could not build, says so, and renders the rest" do reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) + broken = SnapDiff::ScreenshotAssertion.new("broken").tap { |a| a.compare = Object.new } + _out, err = capture_io { reporter.record([broken, build_failing_assertion("valid")]) } + + assert_match(/\[snap_diff\] HTML report: could not render 'broken' \(NoMethodError: .*difference/, err) + entry = reporter.failures.find { |e| e[:name] == "broken" } + assert_match(/\ANoMethodError: .*difference/, entry[:error]) + assert_equal [2, 0], [reporter.failed, reporter.passed], "an entry the report can't vouch for is not a pass" + + reporter.finalize + assert_includes @output_path.read, "valid" + end + + test "#record counts an entry that fails before its comparison is read" do + reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) broken = SnapDiff::ScreenshotAssertion.new("broken") - broken.compare = Object.new # will raise on .difference + broken.define_singleton_method(:compare) { raise "no compare" } - valid = build_failing_assertion("valid") + capture_io { reporter.record([broken]) } - assert_nothing_raised do - reporter.record([broken, valid]) - end + assert_equal [1, 1, 0], [reporter.total, reporter.failed, reporter.passed] + 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? - assert_includes @output_path.read, "valid" + 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 @@ -226,4 +274,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