fix: a failed screenshot names the line that took it, once

CHANGELOG.md

@@ -406,6 +406,13 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV
   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
+- **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)
+- **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

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

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

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

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,11 +78,9 @@ def validate
           return nil
         end
 
-        message = "Screenshot does not match for '#{name}': #{compare.error_message}\n#{caller.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
@@ -122,6 +122,48 @@ 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. 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
 
   class AssertionRegistry

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/<name without its _N suffix>.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

test/unit/rspec_call_site_test.rb

@@ -0,0 +1,37 @@
+# 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 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|
+      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"
+    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
+
+  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

test/unit/screenshot_assertion_call_site_test.rb

@@ -0,0 +1,133 @@
+# 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 <class:ScreenshotAssertionCallSiteTest>'"
+      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 <top (required)>'",
+        "#{Dir[File.join(RbConfig::CONFIG["rubylibdir"], "*.rb")].first}:1:in 'require'",
+        "<internal:kernel>: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
+
+    # 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")
+        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 "<internal:kernel>:91", call_site_line(["<internal:kernel>: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