diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e634b44..ac139364 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -363,6 +363,19 @@ 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 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 +- **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 - **`CI: ""` means "not CI" for the HTML report too.** The report embedded every image whenever the `CI` variable merely existed, so the documented opt-out (`CI: ""` in the update-baselines job) still produced a report with inlined images while the record mode 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/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/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/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 8621fc90..c1e5822f 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/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/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/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/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 74056b2d..55c5ea0f 100644 --- a/test/unit/reporter_interplay_test.rb +++ b/test/unit/reporter_interplay_test.rb @@ -69,6 +69,98 @@ 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 + + # 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 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.