From 7901b4894545a23b4c8398a3052b2881b37afad1 Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Fri, 16 Oct 2026 16:54:08 -0300 Subject: [PATCH 1/3] fix: count and report assertions made with delayed: false `assert_matches_screenshot(..., delayed: false)` validated the assertion on the spot and never added it to the session registry. Only the registry reaches `Reporting.notify` at reset, so an immediate assertion was invisible to the end-of-run tally and to every reporter: a suite run with `SnapDiff.config.delayed = false` ended on "0 verified ... NOTHING WAS VERIFIED", and a mismatch raised inside the test but never made the HTML report. The DSL now registers every assertion and, for the immediate path, also calls `validate!` right away. `validate!` marks the assertion so the registry's teardown pass treats it as done; without that, a mismatch would fail the test twice (once in the body, once in `verify`). The flag is set in `validate!` only, so the delayed path (`session.verify`, which re-raises on repeated calls) is unchanged. Not changed: what gets registered. Captures without a baseline still return nil from the matcher and never become assertions; that is a separate change. --- CHANGELOG.md | 4 ++++ docs/architecture.md | 6 +++--- lib/snap_diff/dsl.rb | 13 +++++------ lib/snap_diff/screenshot_assertion.rb | 9 +++++--- test/unit/reporter_interplay_test.rb | 31 +++++++++++++++++++++++++++ 5 files changed, 49 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e33905d..02038d97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -363,6 +363,10 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV and omitted the README ### Fixed +- **Screenshots asserted with `delayed: false` are now counted and reported.** They + validated immediately but never reached the registry, so a suite run that way ended + on `0 verified ... NOTHING WAS VERIFIED` and its failures were missing from the + report. The failure still raises inside the test, once - **A screenshot with no committed baseline no longer passes silently** — it warns, names the file, and is counted as `new (not verified)` rather than as a pass - **An inherited `GIT_DIR` / `GIT_WORK_TREE` / `GIT_INDEX_FILE` no longer redirects diff --git a/docs/architecture.md b/docs/architecture.md index a972161f..dd16ca4d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -62,7 +62,7 @@ The entry point for test code. `assert_matches_screenshot` is the primary assert 1. Checks if screenshots are `active?` (returns `false` if disabled) 2. Builds a full screenshot name via `ScreenshotNamer` (handles sections, groups, counters) 3. Delegates to `ScreenshotMatcher` to capture and prepare comparison -4. Creates a `ScreenshotAssertion` — either adds it to the thread-local registry (delayed validation) or validates immediately +4. Creates a `ScreenshotAssertion` and adds it to the thread-local registry; with `delayed: false` it also validates it right away (the registry skips it at teardown) ### 2. ScreenshotMatcher (`lib/snap_diff/screenshot_matcher.rb`) @@ -187,8 +187,8 @@ Test begins ├─ test body: │ └─ screenshot("name") │ ├─ ScreenshotMatcher builds assertion - │ ├─ delayed=true → add to Thread-local registry - │ └─ delayed=false → validate immediately + │ ├─ add to Thread-local registry (always) + │ └─ delayed=false → also validate immediately │ ├─ teardown: │ ├─ SnapDiff.session.verify diff --git a/lib/snap_diff/dsl.rb b/lib/snap_diff/dsl.rb index 4e938d1d..f92f8a32 100644 --- a/lib/snap_diff/dsl.rb +++ b/lib/snap_diff/dsl.rb @@ -96,14 +96,11 @@ def assert_matches_screenshot(name, skip_stack_frames: 0, **options) return false unless assertion - # Determine if validation should be delayed or immediate - delayed = options.fetch(:delayed, SnapDiff.config.delayed) - - if delayed - SnapDiff.session.add_assertion(assertion) - else - assertion.validate! - end + # Registered either way: the end-of-run tally and the reporters see + # every finished assertion, immediate ones included. An immediate one + # validates here and is a no-op when the registry verifies at teardown. + SnapDiff.session.add_assertion(assertion) + assertion.validate! unless options.fetch(:delayed, SnapDiff.config.delayed) true end diff --git a/lib/snap_diff/screenshot_assertion.rb b/lib/snap_diff/screenshot_assertion.rb index 60bc9b21..ac243ea9 100644 --- a/lib/snap_diff/screenshot_assertion.rb +++ b/lib/snap_diff/screenshot_assertion.rb @@ -65,6 +65,7 @@ def inspect # Return an annotated failure message for an unsuppressed screenshot mismatch. # Return nil for absent, matching, or suppressed comparisons; archive matching baselines. def validate + return if @validated return unless compare if compare.different? @@ -98,12 +99,14 @@ def archive_baseline! FileUtils.mv(compare.base_image_path, compare.image_path, force: true) end + # Raise on an unsuppressed mismatch. An assertion raised here (delayed: + # false) is still registered, so the registry re-visits it at teardown: + # the flag makes that second #validate a no-op instead of a second failure. def validate! error_msg = validate + @validated = true - if error_msg - raise SnapDiff::ExpectationNotMet.new(error_msg, caller) - end + raise SnapDiff::ExpectationNotMet.new(error_msg, caller) if error_msg end # Verifies that all scheduled screenshots do not show any unintended differences. diff --git a/test/unit/reporter_interplay_test.rb b/test/unit/reporter_interplay_test.rb index 74056b2d..a6dc34d2 100644 --- a/test/unit/reporter_interplay_test.rb +++ b/test/unit/reporter_interplay_test.rb @@ -69,6 +69,37 @@ def summary end end + # The same wiring for the immediate path. An assertion validated with + # delayed: false is just as finished as a delayed one; a suite that runs + # that way used to end on "0 verified" with its failures missing from + # the report. + test "an immediate assertion reaches reporters and the tally too" do + SnapDiff::Reporting.reset_run_totals! + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :a) + + assert_matches_screenshot(snap.full_name, delayed: false) + SnapDiff.reset + + assert_equal [snap.full_name], @spy.recorded.flatten.map(&:name) + assert_equal 1, SnapDiff::Reporting.verified + end + end + + test "an immediate failure is reported once and raised once" do + SnapDiff::Reporting.reset_run_totals! + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:c, :a) + + assert_raises(SnapDiff::ExpectationNotMet) { assert_matches_screenshot(snap.full_name, delayed: false) } + assert_nil SnapDiff.session.verify, "teardown must not fail the test a second time" + SnapDiff.reset + + assert_equal [snap.full_name], @spy.recorded.flatten.map(&:name) + assert_equal 1, SnapDiff::Reporting.changed + end + end + test "reset without assertions does not notify reporters" do SnapDiff.reset From 429736df81be54f08b324a92c78b1e8046cd6970 Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Sat, 17 Oct 2026 02:56:11 -0300 Subject: [PATCH 2/3] fix: a capture that vanishes before the comparison is a difference, not a crash Comparison#find_difference answers "an image is missing" with a null difference whose failed_by was the String "missing_image". Every reader indexes failed_by with a Symbol (Comparison#dimensions_changed?, Reporters::Default#generate), and String#[] with a Symbol raises TypeError, so `different?` crashed on a capture that disappeared between capture and comparison (two tests writing the same screenshot name, a cleanup racing the run). failed_by is now a Hash like the other failure reason ({missing_image: true}). Three readers had to follow: - quick_equal? destructures find_difference's answer into [equal, result] and first stats both files for identical_files?. A missing input now skips the stat and gets a proper [false, result] pair; before, the bare Struct splatted into its fields and the next `different?` raised NoMethodError. - `different?` runs the default reporter, which tried to annotate images that do not exist. It gets an annotation-free message in the same style as the dimension change one: which input is missing, plus the one left. - The HTML entry carries that message as its error. The page infers "pass" from absent diff keys, so without it a vanished capture rendered green. Not changed: when a comparison runs, and what any other failure reason looks like. --- CHANGELOG.md | 4 +++ lib/snap_diff/comparison.rb | 10 ++++-- lib/snap_diff/reporters/default.rb | 16 ++++++++-- lib/snap_diff/reporters/html.rb | 21 +++++++++++-- test/unit/compare_api_test.rb | 37 +++++++++++++++++++++++ test/unit/reporters/html_reporter_test.rb | 19 ++++++++++++ 6 files changed, 100 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02038d97..1396de1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -367,6 +367,10 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV validated immediately but never reached the registry, so a suite run that way ended on `0 verified ... NOTHING WAS VERIFIED` and its failures were missing from the report. The failure still raises inside the test, once +- **A capture that disappears before it is compared no longer crashes the comparison.** + `different?` raised `TypeError` (the missing-image result carried a String where + the readers index a Hash). It now reads as different, and the error message names + the missing file - **A screenshot with no committed baseline no longer passes silently** — it warns, names the file, and is counted as `new (not verified)` rather than as a pass - **An inherited `GIT_DIR` / `GIT_WORK_TREE` / `GIT_INDEX_FILE` no longer redirects diff --git a/lib/snap_diff/comparison.rb b/lib/snap_diff/comparison.rb index 5dbb7b6e..ed8df179 100644 --- a/lib/snap_diff/comparison.rb +++ b/lib/snap_diff/comparison.rb @@ -108,7 +108,7 @@ def initialize(image_path, base_image_path, options = {}) # # @note This method will raise ArgumentError if either image file is missing. def quick_equal? - return true if identical_files? + return true if images_exist? && identical_files? result, difference = find_difference(quick_mode: true) self.difference = difference @@ -169,8 +169,12 @@ def image_preprocessor end def find_difference(quick_mode: false) - # Validate images exist - return build_null_difference("missing_image") unless images_exist? + # A missing input is a failed comparison, in the same shape the + # caller asked for: quick mode expects an [equal, result] pair. + unless images_exist? + missing = build_null_difference(missing_image: true) + return quick_mode ? [false, missing] : missing + end # Step 1 of the layered strategy documented on this class: byte-identical # files cannot have different pixels. quick_equal? has always checked diff --git a/lib/snap_diff/reporters/default.rb b/lib/snap_diff/reporters/default.rb index 021a9fa8..c1a72438 100644 --- a/lib/snap_diff/reporters/default.rb +++ b/lib/snap_diff/reporters/default.rb @@ -35,8 +35,9 @@ def generate return nil end - if difference.failed? && difference.failed_by[:different_dimensions] - return build_error_for_different_dimensions + if difference.failed? + return build_error_for_missing_image if difference.failed_by[:missing_image] + return build_error_for_different_dimensions if difference.failed_by[:different_dimensions] end annotate_and_save_images @@ -57,6 +58,17 @@ def build_error_for_different_dimensions ["Dimensions have changed: #{dimension_change}", *artifact_lines(INPUT_ARTIFACTS)].join(NEW_LINE) end + # A capture that vanished before the comparison ran has no pixels to + # annotate: say which input is gone and list the one that is left. + def build_error_for_missing_image + missing = INPUT_ARTIFACTS.filter_map do |label, path_method| + path = send(path_method) + "#{label} #{display_path(path)}" unless path.exist? + end + + ["Cannot compare, missing: #{missing.join(", ")}", *artifact_lines(INPUT_ARTIFACTS)].join(NEW_LINE) + end + # "WxH => WxH", baseline first. Public so Reporters::HTML says it the # same way. def dimension_change diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index d2b990e3..e1df9a32 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -129,12 +129,29 @@ def self.default_output_path # Build a report entry from a comparison, omitting unavailable images and metrics. def failure_entry_for(name, compare) - 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) - }.merge(details).compact + }.merge(failure_details(compare)).compact + end + + def failure_details(compare) + if compare.difference.failed_by&.[](:missing_image) + missing_input_details(compare) + elsif compare.dimensions_changed? + dimension_change_details(compare) + else + difference_details(compare) + end + end + + # An input that vanished before the comparison leaves nothing to diff + # or annotate. The entry carries the error so the page says so, instead + # of inferring "pass" from the absent diff keys. resolve_image already + # drops the missing file from original/new. + def missing_input_details(compare) + {error: compare.error_message.lines.first.strip} end # Images of different sizes get no annotated or heatmap images, and diff --git a/test/unit/compare_api_test.rb b/test/unit/compare_api_test.rb index 811dbf97..f95d4708 100644 --- a/test/unit/compare_api_test.rb +++ b/test/unit/compare_api_test.rb @@ -54,4 +54,41 @@ class CompareApiTest < ActiveSupport::TestCase ) assert_not result.different? end + + # The capture can vanish between capture and comparison (two tests writing + # the same screenshot name, a cleanup racing the run). That is a difference + # to report, not a crash: `different?` used to raise TypeError because the + # missing-image result carried a String where every reader indexes a Hash. + test "a capture that disappears before the comparison runs reads as different" do + Dir.mktmpdir do |dir| + base = Pathname(dir) / "x.base.png" + capture = Pathname(dir) / "x.png" + FileUtils.cp TEST_IMAGES_DIR / "a.png", base + FileUtils.cp TEST_IMAGES_DIR / "b.png", capture + comparison = SnapDiff.compare(base, capture) + FileUtils.rm capture + + assert comparison.different? + assert_match(/missing: actual .*x\.png/, comparison.error_message) + end + end + + # quick_equal? destructures find_difference's answer into [equal, result]. + # The missing-image path used to return the bare result, which a Struct + # splits into its fields, so `difference` ended up holding the meta Hash + # and the next `different?` raised NoMethodError. + test "#quick_equal? on a vanished capture is false and leaves a usable difference" do + Dir.mktmpdir do |dir| + base = Pathname(dir) / "x.base.png" + capture = Pathname(dir) / "x.png" + FileUtils.cp TEST_IMAGES_DIR / "a.png", base + FileUtils.cp TEST_IMAGES_DIR / "b.png", capture + comparison = SnapDiff.compare(base, capture) + FileUtils.rm capture + + assert_not comparison.quick_equal? + assert comparison.different? + assert comparison.difference.failed_by[:missing_image] + end + end end diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb index 0d975dfd..86d4fc9b 100644 --- a/test/unit/reporters/html_reporter_test.rb +++ b/test/unit/reporters/html_reporter_test.rb @@ -118,6 +118,25 @@ class HTMLReporterTest < ActiveSupport::TestCase assert_includes @output_path.read, "resized" end + # A capture that vanished before the comparison has nothing to diff or + # annotate. Without an explicit error the template infers "pass" from the + # missing diff keys, so the entry says what went wrong instead. + test "#record shows a vanished capture as an error, not a pass" do + reporter = SnapDiff::Reporters::HTML.new(output_path: @output_path) + compare = make_comparison(:a, :b, destination: "gone") + FileUtils.rm compare.image_path + compare.processed + assertion = SnapDiff::ScreenshotAssertion.new("gone").tap { |a| a.compare = compare } + + reporter.record([assertion]) + + entry = reporter.failures.sole + assert_match(/missing/i, entry[:error]) + assert_nil entry[:diff] + assert_nil entry[:new], "there is no capture to show" + assert entry[:original], "the baseline is still there to show" + 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. From 78b243b59cf891e445da9f8f60607202ef9bdd68 Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Sat, 17 Oct 2026 01:08:03 -0300 Subject: [PATCH 3/3] fix: never count an unprocessed comparison as verified A Comparison is lazy: it does no pixel work until something asks whether it differs. Reporting.count read the memo (`difference&.different?`), so a comparison nobody ran counted as verified and matching. The gap opens when teardown does not validate. With the legacy `fail_on_difference = false`, verify_screenshots! returns before calling validate, so a differing screenshot ended the run as "1 verified, 0 changed", with no diff files and no report entry. count now asks `compare.different?`, which runs the comparison once and is a memo read on the normal path, where validate already forced it. Forcing a comparison can raise, so count rescues per assertion: the screenshot that cannot be compared is warned about by name and left out of the tally, and the other screenshots of the same test still count. Before, the one rescue in notify dropped the whole test from the summary with a message that named nothing. The two diff_test.rb mocks that counted `different?` reads now expect the extra read. Cost: a suite that resets without validating now pays one comparison per screenshot when the run is tallied -- the same cost the default modes pay at teardown. Not changed: the record modes, validate, or what the legacy flag does to the test result (it still never fails one). --- CHANGELOG.md | 5 +++ lib/snap_diff/reporting.rb | 10 ++++- test/unit/diff_test.rb | 7 ++-- test/unit/reporter_interplay_test.rb | 61 ++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1396de1a..8f930586 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -371,6 +371,11 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV `different?` raised `TypeError` (the missing-image result carried a String where the readers index a Hash). It now reads as different, and the error message names the missing file +- **The summary never counts a comparison that did not run.** With + `fail_on_difference = false` nothing validated the screenshots at teardown, so a + differing one was tallied as `verified`, with no diff files and no report entry. The + tally now runs the comparison, so the summary says `changed` and the report lists it. + The flag still never fails a test - **A screenshot with no committed baseline no longer passes silently** — it warns, names the file, and is counted as `new (not verified)` rather than as a pass - **An inherited `GIT_DIR` / `GIT_WORK_TREE` / `GIT_INDEX_FILE` no longer redirects diff --git a/lib/snap_diff/reporting.rb b/lib/snap_diff/reporting.rb index e95f166f..93539cca 100644 --- a/lib/snap_diff/reporting.rb +++ b/lib/snap_diff/reporting.rb @@ -199,6 +199,11 @@ def notify(assertions) # Tallies a finished test's assertions. An assertion with no # `compare` never reached a baseline, so it is neither verified nor # changed -- it is counted, if at all, by {record_missing_baseline}. + # A comparison that has not run yet (nothing validated it: teardown + # skips that when fail_on_difference is off) runs here, so a verified + # screenshot is always one that was actually compared. One that cannot + # run is warned about by name and left out of the tally; the other + # screenshots of the same test still count. def count(assertions) verified = 0 changed = 0 @@ -207,8 +212,11 @@ def count(assertions) compare = assertion.compare next unless compare + different = compare.different? verified += 1 - changed += 1 if compare.difference&.different? + changed += 1 if different + rescue => e + warn "[snap_diff] Could not tally '#{assertion.name}' (#{e.class}: #{e.message})" end @mutex.synchronize do diff --git a/test/unit/diff_test.rb b/test/unit/diff_test.rb index 7c47a2f4..b7152570 100644 --- a/test/unit/diff_test.rb +++ b/test/unit/diff_test.rb @@ -147,8 +147,8 @@ def _test_sample_screenshot_error mock = ::Minitest::Mock.new mock.expect(:different?, true) mock.expect(:different?, true) - # Read by SnapDiff::Reporting.count when the test ends. - mock.expect(:difference, TestDoubles::TestDifference.new(true)) + # The third read is SnapDiff::Reporting.count's, when the test ends. + mock.expect(:different?, true) mock.expect(:dimensions_changed?, false) mock.expect(:base_image_path, Pathname.new("screenshot.base.png")) mock.expect(:error_message, "expected error message") @@ -184,8 +184,7 @@ def _test_sample_screenshot_error comparison = ::Minitest::Mock.new comparison.expect(:different?, true) # to find backtrace comparison.expect(:different?, true) # to find messages - # Read by SnapDiff::Reporting.count when the test ends. - comparison.expect(:difference, TestDoubles::TestDifference.new(true)) + comparison.expect(:different?, true) # SnapDiff::Reporting.count, when the test ends comparison.expect(:dimensions_changed?, false) comparison.expect(:base_image_path, Pathname.new("screenshot.base.png")) comparison.expect(:error_message, "expected error message for non minitest") diff --git a/test/unit/reporter_interplay_test.rb b/test/unit/reporter_interplay_test.rb index a6dc34d2..55c5ea0f 100644 --- a/test/unit/reporter_interplay_test.rb +++ b/test/unit/reporter_interplay_test.rb @@ -100,6 +100,67 @@ def summary end end + # A comparison nobody ran has no verdict yet. With fail_on_difference + # off, teardown never validates, so the tally has to force the comparison + # itself or it reports a differing screenshot as verified and matching. + test "the tally runs a comparison nobody forced" do + SnapDiff::Reporting.reset_run_totals! + SnapDiff::Vcs.stub(:checkout_vcs, true) do + SnapDiff.config.stub(:fail_on_difference, false) do + differing = create_snapshot_for(:c, :a) + matching = create_snapshot_for(:a, :a) + + assert_matches_screenshot(differing.full_name) + assert_matches_screenshot(matching.full_name) + assert_nil SnapDiff.session.verify, "fail_on_difference = false never fails the test" + SnapDiff.reset + + assert_equal 2, SnapDiff::Reporting.verified + assert_equal 1, SnapDiff::Reporting.changed + end + end + end + + # A comparison that blows up costs its own row, not the test's whole tally: + # the other screenshots of the same test still count, and the warning names + # the one that failed. + test "a comparison that raises is skipped on its own, with a warning naming it" do + SnapDiff::Reporting.reset_run_totals! + SnapDiff::Vcs.stub(:checkout_vcs, true) do + good = create_snapshot_for(:a, :a) + assert_matches_screenshot(good.full_name) + + broken = SnapDiff::ScreenshotAssertion.new("broken_shot") + broken.compare = Object.new.tap { |compare| compare.define_singleton_method(:different?) { raise "boom" } } + SnapDiff.session.add_assertion(broken) + + _out, err = capture_io { SnapDiff.reset } + + assert_equal 1, SnapDiff::Reporting.verified + assert_match(/Could not tally 'broken_shot' \(RuntimeError: boom\)/, err) + assert_equal 2, @spy.recorded.flatten.size, "reporters still get every assertion" + end + end + + # Nothing the tally does on a healthy run, or on a capture that vanished + # before it was compared, is worth a line on stderr. + test "reset tallies a match, a difference and a vanished capture without a word" do + SnapDiff::Reporting.reset_run_totals! + SnapDiff::Vcs.stub(:checkout_vcs, true) do + matching = create_snapshot_for(:a, :a) + differing = create_snapshot_for(:c, :a) + vanished = create_snapshot_for(:c, :a) + [matching, differing, vanished].each { |snap| assert_matches_screenshot(snap.full_name) } + FileUtils.rm(vanished.path) + + _out, err = capture_io { SnapDiff.reset } + + assert_empty err + assert_equal 3, SnapDiff::Reporting.verified + assert_equal 2, SnapDiff::Reporting.changed + end + end + test "reset without assertions does not notify reporters" do SnapDiff.reset