From 9c238cf8f58a2a3deb3c4003d0d0d4a52b3fdc28 Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Mon, 12 Oct 2026 03:04:28 -0300 Subject: [PATCH 1/3] fix: name the screenshot's call site instead of the whole stack A failed screenshot's message ended with the full, unfiltered call stack of the screenshot call. #84 asked for the one source line, and its fix appended exactly that; a later filter only knew Minitest and Rails frames, and once filtering moved into BacktraceFilter (which only touches exception backtraces) the message carried every frame. The message now ends with one path:line: the first frame in a real file outside this gem, installed gems (Gem.path, Gem.default_dir) and Ruby's library dirs, the same split ActiveSupport::BacktraceCleaner makes. That finds the user's line without counting frames, which matters for `expect(page).to match_screenshot`, where rspec-expectations and the matcher sit in between. Roots are compared on directory boundaries; the path is shown relative to config.root like the artifact paths. With no qualifying frame it falls back to the first frame, so a stubbed caller reads as before; with no caller the message has no trailing line (building it from a nil caller used to raise). ScreenshotAssertion#caller is unchanged: it is public, and the raised error's backtrace still comes from it. A project helper wrapping `screenshot` is user code too, so it is named unless it passes skip_stack_frames: 1, as the documented override already does; the docs and the DSL override comment now say why. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 + docs/configuration.md | 5 +- lib/snap_diff/dsl.rb | 4 +- lib/snap_diff/screenshot_assertion.rb | 43 ++++++- test/fixtures/rspec_call_site_spec.rb | 47 ++++++++ test/unit/rspec_call_site_test.rb | 35 ++++++ .../screenshot_assertion_call_site_test.rb | 112 ++++++++++++++++++ 7 files changed, 247 insertions(+), 4 deletions(-) create mode 100644 test/fixtures/rspec_call_site_spec.rb create mode 100644 test/unit/rspec_call_site_test.rb create mode 100644 test/unit/screenshot_assertion_call_site_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c36dd68..37e1e083 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -392,6 +392,11 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV carried the `diff_mask` image into the error text (`"diff_mask":"#"`, or a `{"ptr":{}…}` struct depending on the driver); it is excluded now - Reporter failure warnings use one brand and name the failing reporter class +- **A failed screenshot names the line that took it, instead of the whole call stack.** + The message ends with one `path:line`: the first frame outside snap_diff, installed + gems and Ruby's own library, so `expect(page).to match_screenshot` names the spec line + too. A project helper wrapping `screenshot` is reported itself unless it passes + `skip_stack_frames: 1` (see docs/configuration.md) ### Known limitations - **One process per worker** (`parallel_tests`, RSpec, CI sharding) still writes one diff --git a/docs/configuration.md b/docs/configuration.md index 9d4f91c3..49d756a0 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -663,8 +663,9 @@ screenshot 'index', skip_area: [[0, 0, 64, 48], [17, 6, 27, 16], 'css_selector . ### Skipping stack frames in the error output -If you would like to override the `screenshot` method or for some other reason would like to skip stack -frames when reporting image differences, you can use the `skip_stack_frames` option: +A failed screenshot's message ends with the line of your code that took it: the first frame outside +snap_diff, installed gems and Ruby's own library. A helper in your project that wraps `screenshot` is +your code too, so its own line is reported unless it skips its frame with the `skip_stack_frames` option: ```ruby test 'test visiting the index' do diff --git a/lib/snap_diff/dsl.rb b/lib/snap_diff/dsl.rb index 8730e2cd..4e938d1d 100644 --- a/lib/snap_diff/dsl.rb +++ b/lib/snap_diff/dsl.rb @@ -141,7 +141,9 @@ def capture_screenshot(name, **options) # Asserts the current page has no visual changes from the baseline. # Override in your base test class to add project-specific behavior - # (e.g., waiting for Turbo, default skip areas). + # (e.g., waiting for Turbo, default skip areas). Pass + # skip_stack_frames: 1 to super so a failure names the test's line, + # not the override's. def assert_no_screenshot_changes(name, skip_stack_frames: 0, **opts, &readiness) assert_matches_screenshot(name, skip_stack_frames: skip_stack_frames + 1, **opts, &readiness) end diff --git a/lib/snap_diff/screenshot_assertion.rb b/lib/snap_diff/screenshot_assertion.rb index 32d0d304..a659a424 100644 --- a/lib/snap_diff/screenshot_assertion.rb +++ b/lib/snap_diff/screenshot_assertion.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true require "fileutils" +require "rbconfig" +require_relative "error_with_filtered_backtrace" require_relative "screenshot_namer" require_relative "reporting" @@ -76,7 +78,7 @@ def validate return nil end - message = "Screenshot does not match for '#{name}': #{compare.error_message}\n#{caller.join("\n")}" + message = ["Screenshot does not match for '#{name}': #{compare.error_message}", call_site].compact.join("\n") Contributions.annotations_for(name).each do |note| message += "\n #{note[:source].upcase} triage: #{note[:text]}" end @@ -122,6 +124,45 @@ def self.verify_screenshots!(screenshots) test_screenshot_errors.empty? ? nil : test_screenshot_errors end + + private + + FRAME = /\A(.+?):(\d+)(?::in |\z)/ + private_constant :FRAME + + # The line of the user's code that took the screenshot: the first frame + # in a real file outside this gem, installed gems and Ruby's own library. + # Matchers and helpers in between (e.g. rspec-expectations under + # match_screenshot) are skipped without counting their depth. + def call_site + frames = Array(caller) + frame = frames.find { |candidate| user_code?(candidate) } || frames.first + path, line = frame&.match(FRAME)&.captures + path ? "#{display_path(path)}:#{line}" : frame + end + + def user_code?(frame) + path = frame[FRAME, 1] or return false + return false unless File.file?(path) + + expanded = File.expand_path(path) + library_roots.none? { |root| expanded.start_with?(root) } + end + + def library_roots + ruby_dirs = RbConfig::CONFIG.values_at("rubylibdir", "sitelibdir", "vendorlibdir") + [BacktraceFilter::LIB_DIRECTORY, *Gem.path, Gem.default_dir, *ruby_dirs] + .reject { |dir| dir.to_s.empty? } + .map { |dir| File.expand_path(dir) + File::SEPARATOR } + end + + # Relative to the configured root when the file is inside it, as the + # failure message's artifact paths are; otherwise as Ruby recorded it, + # which resolves from where the test command ran. + def display_path(path) + relative = Pathname(path).expand_path.relative_path_from(SnapDiff.config.root).to_path + relative.start_with?("..") ? path : relative + end end class AssertionRegistry diff --git a/test/fixtures/rspec_call_site_spec.rb b/test/fixtures/rspec_call_site_spec.rb new file mode 100644 index 00000000..cd9038ef --- /dev/null +++ b/test/fixtures/rspec_call_site_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +# Run in a subprocess by test/unit/rspec_call_site_test.rb. Browser-free: +# ScreenshoterStub "captures" a fixture image, and each baseline is placed +# where a VCS checkout would put it. Deliberately does not require +# test_helper, whose minitest/autorun at_exit would take over this +# process's exit code. + +require "tmpdir" +require "snap_diff" +require "snap_diff/integrations/rspec" + +TEST_IMAGES_DIR = Pathname(File.expand_path("images", __dir__)) +require "support/screenshoter_stub" + +# Capture asks Capybara which driver it runs; rack_test needs an app. +Capybara.app = ->(_env) { [200, {}, [""]] } + +RSpec.describe "a failed screenshot's call site", type: :feature do + around do |example| + Dir.mktmpdir do |root| + SnapDiff.config.root = Pathname(root) + SnapDiff.config.save_path = "doc/screenshots" + SnapDiff.config.screenshoter = ScreenshoterStub + SnapDiff.config.record = :once + # Baselines are already in place, as a VCS checkout would leave them. + SnapDiff::Vcs.define_singleton_method(:checkout_vcs) { |*| true } + example.run + end + end + + # ScreenshoterStub captures images/.png; + # the baseline is a different image, so each screenshot fails. + def differing_baseline_for(name) + dir = SnapDiff.config.root / SnapDiff.config.save_path + FileUtils.mkdir_p(dir) + FileUtils.cp(TEST_IMAGES_DIR / "a.png", dir / "#{name}.base.png") + end + + it "fails two screenshots" do + differing_baseline_for("b_1") + differing_baseline_for("b_2") + + screenshot "b_1" # call site: screenshot + expect(:page).to match_screenshot("b_2") # call site: match_screenshot + end +end diff --git a/test/unit/rspec_call_site_test.rb b/test/unit/rspec_call_site_test.rb new file mode 100644 index 00000000..37235827 --- /dev/null +++ b/test/unit/rspec_call_site_test.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require "test_helper" +require "open3" + +# Runs a browser-free RSpec fixture in a subprocess, through the real +# `screenshot` and `match_screenshot` entry points, and reads what RSpec +# prints for the failed screenshots. +class RspecCallSiteTest < ActiveSupport::TestCase + SPEC_FILE = File.expand_path("../fixtures/rspec_call_site_spec.rb", __dir__) + + test "each failed screenshot names its own line of the spec, and nothing else of the stack" do + out = run_fixture + + %w[screenshot match_screenshot].each do |entry_point| + line = File.readlines(SPEC_FILE).index { |l| l.include?("# call site: #{entry_point}") } + 1 + assert_equal 1, out.scan(/^ *#{Regexp.escape("#{SPEC_FILE}:#{line}")}$/).size, + "expected the #{entry_point} call site on its own line, once:\n#{out}" + end + refute_match %r{/gems/rspec-}, out, "no rspec frame belongs in the message" + refute_match %r{lib/snap_diff/}, out, "no snap_diff frame belongs in the message" + end + + private + + def run_fixture + script = <<~RUBY + require "rspec/core" + exit RSpec::Core::Runner.run([#{SPEC_FILE.inspect}], $stderr, $stdout) + RUBY + out, status = Open3.capture2e(RbConfig.ruby, "-Ilib", "-Itest", "-e", script) + refute status.success?, "expected the fixture's screenshots to fail:\n#{out}" + out + end +end diff --git a/test/unit/screenshot_assertion_call_site_test.rb b/test/unit/screenshot_assertion_call_site_test.rb new file mode 100644 index 00000000..2c367368 --- /dev/null +++ b/test/unit/screenshot_assertion_call_site_test.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require "test_helper" +require "snap_diff" + +module SnapDiff + # The failure message names the line of the user's code that took the + # screenshot, once, instead of the whole call stack. Frames are built + # from real files, because a frame only counts as user code when its + # file exists. + class ScreenshotAssertionCallSiteTest < ActiveSupport::TestCase + include SnapDiff::DSL + include DSLStub + + def setup + super + @comparison = make_comparison(:a, :b) + end + + test "names the first frame outside the gem, installed gems and Ruby's library" do + user_frame = "#{__FILE__}:#{__LINE__}:in 'block in '" + frames = [ + "#{Gem.loaded_specs.fetch("rspec-expectations").full_gem_path}/lib/rspec/matchers/dsl.rb:535:in 'method_missing'", + "#{BacktraceFilter::LIB_DIRECTORY}snap_diff/integrations/rspec.rb:24:in 'block (2 levels) in '", + "#{Dir[File.join(RbConfig::CONFIG["rubylibdir"], "*.rb")].first}:1:in 'require'", + ":91:in 'tap'", + "(eval at #{__FILE__}:1):1:in 'block'", + user_frame, + "#{__FILE__}:1:in 'later frame'" + ] + + SnapDiff.config.stub(:root, Pathname(Dir.pwd)) do + assert_equal "test/unit/screenshot_assertion_call_site_test.rb:#{user_frame[/:(\d+):/, 1]}", call_site_line(frames) + end + end + + test "shows a user file outside the root as an absolute path" do + Dir.mktmpdir do |dir| + file = File.join(dir, "outside_spec.rb").tap { |f| File.write(f, "") } + + assert_equal "#{file}:7", call_site_line(["#{file}:7:in 'block'"]) + end + end + + test "matches library roots on directory boundaries" do + Dir.mktmpdir do |dir| + library = File.join(dir, "foo", "lib.rb") + user = File.join(dir, "foobar", "user_spec.rb") + [library, user].each { |f| FileUtils.mkdir_p(File.dirname(f)) && File.write(f, "") } + + Gem.stub(:path, [File.join(dir, "foo")]) do + assert_equal "#{user}:3", call_site_line(["#{library}:1:in 'x'", "#{user}:3:in 'y'"]) + end + end + end + + test "names a wrapper helper that does not skip its own frame" do + helper = "#{__FILE__}:10:in 'screenshot'" + + assert_equal "#{__FILE__}:10", call_site_line([helper, "#{__FILE__}:20:in 'test_page'"]) + end + + test "falls back to the first frame when no frame is user code" do + assert_equal "my_test.rb:42", call_site_line(["my_test.rb:42"]) + assert_equal ":91", call_site_line([":91:in 'tap'"]) + end + + test "adds no line when there is no caller" do + [nil, []].each do |frames| + message = build_assertion(frames).validate + + assert_equal "Screenshot does not match for 'name': #{@comparison.error_message}", message + end + end + + test "each DSL entry point names the test's own line" do + SnapDiff::Vcs.stub(:checkout_vcs, true) do + first_line = __LINE__ + 1 + screenshot(create_snapshot_for(:a, :c).full_name) + assert_matches_screenshot(create_snapshot_for(:a, :c).full_name) + assert_no_screenshot_changes(create_snapshot_for(:a, :c).full_name) + + assert_equal (first_line..first_line + 2).map { |line| "#{__FILE__}:#{line}" }, + SnapDiff.session.assertions.map { |assertion| assertion.validate.lines.last } + end + end + + test "a screenshot checked immediately names the test's own line" do + SnapDiff::Vcs.stub(:checkout_vcs, true) do + line = __LINE__ + 2 + error = assert_raises(SnapDiff::ExpectationNotMet) do + screenshot(create_snapshot_for(:a, :c).full_name, delayed: false) + end + + assert_equal "#{__FILE__}:#{line}", error.message.lines.last + end + end + + private + + def call_site_line(frames) + build_assertion(frames).validate.lines.last + end + + def build_assertion(frames) + SnapDiff::ScreenshotAssertion.new("name").tap do |assertion| + assertion.compare = @comparison + assertion.caller = frames + end + end + end +end From 6550addfe9a279615ddf65b3173cbbc6f3941a1d Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Mon, 12 Oct 2026 03:06:04 -0300 Subject: [PATCH 2/3] fix: RSpec prints a failed screenshot once, without a Caused by copy The RSpec adapter translates SnapDiff::ExpectationNotMet into RSpec's ExpectationNotMetError inside the rescue, which makes Ruby attach the original as its cause, and rspec-core prints a cause in full under "Caused by:". The translated error already copies the message and the backtrace, so the cause only repeated them; it is raised with cause: nil now. Minitest's adapter re-raises the same way but prints no cause, and Cucumber's does not re-raise, so neither changes. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ lib/snap_diff/integrations/rspec.rb | 4 +++- test/unit/rspec_call_site_test.rb | 4 +++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37e1e083..df8b3010 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -397,6 +397,8 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV gems and Ruby's own library, so `expect(page).to match_screenshot` names the spec line too. A project helper wrapping `screenshot` is reported itself unless it passes `skip_stack_frames: 1` (see docs/configuration.md) +- **RSpec prints a failed screenshot once.** The adapter re-raised it inside a `rescue`, + so RSpec repeated the whole failure under `Caused by:` ### Known limitations - **One process per worker** (`parallel_tests`, RSpec, CI sharding) still writes one diff --git a/lib/snap_diff/integrations/rspec.rb b/lib/snap_diff/integrations/rspec.rb index 732b9148..71866a9e 100644 --- a/lib/snap_diff/integrations/rspec.rb +++ b/lib/snap_diff/integrations/rspec.rb @@ -69,7 +69,9 @@ skip(msg) end rescue SnapDiff::ExpectationNotMet => e - raise RSpec::Expectations::ExpectationNotMetError.new(e.message).tap { |ex| ex.set_backtrace(e.backtrace) } + # Translated, not wrapped: it already carries the message and + # backtrace, so a cause would make RSpec print the failure twice. + raise RSpec::Expectations::ExpectationNotMetError.new(e.message).tap { |ex| ex.set_backtrace(e.backtrace) }, cause: nil ensure SnapDiff.reset end diff --git a/test/unit/rspec_call_site_test.rb b/test/unit/rspec_call_site_test.rb index 37235827..6cd8d1d1 100644 --- a/test/unit/rspec_call_site_test.rb +++ b/test/unit/rspec_call_site_test.rb @@ -9,7 +9,7 @@ class RspecCallSiteTest < ActiveSupport::TestCase SPEC_FILE = File.expand_path("../fixtures/rspec_call_site_spec.rb", __dir__) - test "each failed screenshot names its own line of the spec, and nothing else of the stack" do + test "each failed screenshot is printed once, naming its own line of the spec and nothing else of the stack" do out = run_fixture %w[screenshot match_screenshot].each do |entry_point| @@ -19,6 +19,8 @@ class RspecCallSiteTest < ActiveSupport::TestCase end refute_match %r{/gems/rspec-}, out, "no rspec frame belongs in the message" refute_match %r{lib/snap_diff/}, out, "no snap_diff frame belongs in the message" + refute_includes out, "Caused by:", "the translated failure must not carry the original as its cause" + assert_equal 1, out.scan("Screenshot does not match for 'b_1'").size, out end private From 9b749ccc458b837542a50c9b1328d9c6642c2a12 Mon Sep 17 00:00:00 2001 From: Diego Algorta Date: Mon, 12 Oct 2026 03:58:55 -0300 Subject: [PATCH 3/3] fix: keep the call site last, and its path readable across drives Two gaps in the call-site line, raised in review: - Contributor notes (AI triage) were appended after the call site, so with triage on the message no longer ended with the location the docs and CHANGELOG promise. The notes now sit between the diff details they describe and the call site. - display_path relates the frame to config.root with Pathname#relative_path_from, which raises ArgumentError ("different prefix") when the two can't be related, e.g. a test file on another Windows drive. That replaced the screenshot failure with an unrelated error. Such a frame now stays as Ruby recorded it, which the method's comment already promised for paths outside the root. The test for the second reproduces Ruby's error on any OS with a relative root. Reporters::Default#display_path has the same call, but renders artifact paths, which live under the root by construction; it is left alone here. Co-Authored-By: Claude Opus 5.5 --- lib/snap_diff/screenshot_assertion.rb | 13 ++++++------ .../screenshot_assertion_call_site_test.rb | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/snap_diff/screenshot_assertion.rb b/lib/snap_diff/screenshot_assertion.rb index a659a424..4a31df01 100644 --- a/lib/snap_diff/screenshot_assertion.rb +++ b/lib/snap_diff/screenshot_assertion.rb @@ -78,11 +78,9 @@ def validate return nil end - message = ["Screenshot does not match for '#{name}': #{compare.error_message}", call_site].compact.join("\n") - Contributions.annotations_for(name).each do |note| - message += "\n #{note[:source].upcase} triage: #{note[:text]}" - end - message + # The call site goes last, after what contributors say about the diff. + notes = Contributions.annotations_for(name).map { |note| " #{note[:source].upcase} triage: #{note[:text]}" } + ["Screenshot does not match for '#{name}': #{compare.error_message}", *notes, call_site].compact.join("\n") else archive_baseline! nil @@ -158,10 +156,13 @@ def library_roots # Relative to the configured root when the file is inside it, as the # failure message's artifact paths are; otherwise as Ruby recorded it, - # which resolves from where the test command ran. + # which resolves from where the test command ran. That includes a path + # Pathname can't relate to the root at all, such as one on another drive. def display_path(path) relative = Pathname(path).expand_path.relative_path_from(SnapDiff.config.root).to_path relative.start_with?("..") ? path : relative + rescue ArgumentError + path end end diff --git a/test/unit/screenshot_assertion_call_site_test.rb b/test/unit/screenshot_assertion_call_site_test.rb index 2c367368..c88a2071 100644 --- a/test/unit/screenshot_assertion_call_site_test.rb +++ b/test/unit/screenshot_assertion_call_site_test.rb @@ -42,6 +42,27 @@ def setup end end + # A path Pathname can't relate to the root (another Windows drive; a + # relative root gives Ruby's same "different prefix" error anywhere) + # must not replace the screenshot failure with an ArgumentError. + test "keeps a frame it can't relate to the root as Ruby recorded it" do + frame = "#{__FILE__}:5:in 'block'" + @comparison.processed # the artifact paths are rendered against the real root + + SnapDiff.config.stub(:root, Pathname("spec/dummy")) do + assert_equal "#{__FILE__}:5", call_site_line([frame]) + end + end + + test "stays the last line when contributors annotate the failure" do + Contributions.stub(:annotations_for, [{source: "ai", text: "a font change"}]) do + message = build_assertion(["#{__FILE__}:9:in 'block'"]).validate + + assert_includes message, "\n AI triage: a font change\n" + assert_equal "#{__FILE__}:9", message.lines.last + end + end + test "matches library roots on directory boundaries" do Dir.mktmpdir do |dir| library = File.join(dir, "foo", "lib.rb")