diff --git a/CHANGELOG.md b/CHANGELOG.md index c6e554de..4a174da2 100644 --- a/CHANGELOG.md +++ b/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 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/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/lib/snap_diff/screenshot_assertion.rb b/lib/snap_diff/screenshot_assertion.rb index 32d0d304..4a31df01 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,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 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..6cd8d1d1 --- /dev/null +++ b/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 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..c88a2071 --- /dev/null +++ b/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 '" + 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 + + # 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 ":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