Repository navigation
fix: a failed screenshot names the line that took it, once - #303
Merged
Merged
Conversation
A failed screenshot's message ended with the full, unfiltered call stack of the screenshot call. snap-diff#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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Contributor
Reviewer's GuideThe PR replaces full screenshot call stacks with one resolved user call-site line, including through RSpec matchers and DSL wrappers, and suppresses RSpec cause chaining so each failed screenshot is printed only once. It adds focused unit and subprocess integration coverage for path filtering, fallbacks, entry points, and output formatting, along with updated documentation and changelog notes. Sequence diagram for resolving a failed screenshot call sitesequenceDiagram
participant Test as Test spec
participant DSL as ScreenshotAssertion
participant Caller as Ruby caller frames
participant Filter as Call-site filter
participant Error as ExpectationNotMet
Test->>DSL: screenshot(name)
DSL->>Caller: caller
Caller-->>Filter: stack frames
Filter->>Filter: user_code?
Filter->>Filter: display_path(path)
Filter-->>DSL: path:line
DSL->>Error: validate!
Error-->>Test: message with one call-site line
Sequence diagram for preventing duplicate RSpec screenshot failuressequenceDiagram
participant Spec as RSpec example
participant Adapter as RSpec adapter
participant SnapDiff as SnapDiff assertion
participant RSpecError as RSpec expectation error
Spec->>Adapter: screenshot(name)
Adapter->>SnapDiff: validate!
SnapDiff-->>Adapter: SnapDiff::ExpectationNotMet
Adapter->>RSpecError: new(message)
Adapter->>RSpecError: set_backtrace(backtrace)
Adapter-->>Spec: raise cause: nil
Spec-->>Spec: print failure once
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change appears mergeable after normal checks; no actionable issue was identified. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: failed screenshots report their call-site line once. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/snap_diff/screenshot_assertion.rb" line_range="163" />
<code_context>
+ # 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
</code_context>
<issue_to_address>
**Path errors replace screenshot failures**
When a qualifying caller frame and `SnapDiff.config.root` are on different Windows drives, `display_path` calls `Pathname#relative_path_from` before checking whether the frame is outside the configured root. It raises `ArgumentError`, so `validate` stops formatting the mismatch and callers see a path error instead of the screenshot failure.
Handle different-drive paths before calling `relative_path_from`, returning the original path when a relative path cannot be computed.
</issue_to_address>
### Comment 2
<location path="lib/snap_diff/screenshot_assertion.rb" line_range="81" />
<code_context>
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]}"
</code_context>
<issue_to_address>
**Call site is not the final line**
When a registered contribution provider returns an annotation for a failed screenshot, `validate` appends contribution annotations after `call_site`, so the message ends with triage text instead of the promised `path:line`; callers relying on the final line lose the screenshot call site.
Append contribution annotations before `call_site` so the call site remains the final line.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: lib/snap_diff/screenshot_assertion.rb:163, lib/snap_diff/screenshot_assertion.rb:81
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 <noreply@anthropic.com>
Collaborator
|
Thanks @oboxodo I’ll check today and merge |
Collaborator
This was referenced Oct 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A failed screenshot's message ends with the whole call stack of the
screenshotcall, and under RSpec the entire failure is then printed a second time, underCaused by:. One spec with two failing screenshots prints 218 lines. After this PR it prints 39, and each screenshot names the line of the spec that took it. That includesexpect(page).to match_screenshot, where the spec's line is buried 10 frames deep today.Before (real run,
master, a spec withscreenshot "b_1"on line 44 andexpect(:page).to match_screenshot("b_2")on line 45)match_screenshot's stack starts inside rspec-expectations and snap_diff's own matcher; the spec line is the 11th frame.After (same run)
screenshot+match_screenshotfailingMinitest shows only one
[file:line]per failed test, so for the second screenshot the message is the only place its line appears. That line stays.How it got here
#84 asked for "which test source line the screenshot was called from", and 4f34873 appended exactly that one line. 80ba12d widened it to
caller(skip_stack_frames + 1)with a filter that only knewactivesupport|minitest|railtiesframes. When filtering moved intoBacktraceFilter(#141), which only touches exception backtraces, the message started carrying every frame, unfiltered.Commit 1: name the screenshot's call site instead of the whole stack
The message ends with one
path:line: the first frame in a real file outside snap_diff'slib/, installed gems (Gem.path,Gem.default_dir) and Ruby's library dirs (rubylibdir,sitelibdir,vendorlibdir). That's the splitActiveSupport::BacktraceCleanermakes for its gem filter and stdlib silencer.File.file?) also drops<internal:…>,(eval)and JRuby's Java frames without a per-platform rule.BacktraceFilter).config.rootwhen the file is inside it, like the feat: make the failure message legible #264 artifact paths; otherwise it's left as Ruby recorded it, which resolves from where the test command ran, as RSpec's own backtrace lines do. The:in '…'label is dropped, as inRemoval.origin_for's deprecation origins.caller = ["my_test.rb:42"]reads exactly as before (the existing message tests pass unchanged). No caller at all: the message has no trailing line; building it from anilcaller used to raiseNoMethodError. (validate!and the registry'sverifystill passcalleras the raised error's backtrace, unchanged.)It's unlabelled, in the spot where the stack was. A label (
called at:) would read better, but wouldn't line up with the artifact block's column and changes a line people may grep; happy to add one if you prefer.One behaviour to know about: a project helper that wraps
screenshotis user code, so it's named unless it passesskip_stack_frames: 1. That's the overridedocs/configuration.mdalready shows. Before, the full stack showed the helper's line first and the test line further down. The docs section and the DSL override comment now say why to pass it. Listing every non-gem frame instead would also pull in binstubs likebin/rspec.Commit 2: RSpec prints a failed screenshot once
The adapter's
append_aftertranslatesSnapDiff::ExpectationNotMetintoRSpec::Expectations::ExpectationNotMetErrorinside therescue, so Ruby attaches the original ascause, and rspec-core prints a cause in full. The translated error already copies the message and backtrace, so it's raised withcause: nil. Minitest's adapter re-raises the same way but Minitest prints no cause (checked), and Cucumber's never re-raises, so neither changes.Commit 3: keep the call site last, and its path readable across drives
From the bot review on this PR:
display_path'sPathname#relative_path_fromraisesArgumentError("different prefix") when it can't relate the frame to the root, 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.Not changed
ScreenshotAssertion#caller(public; still the raised error's backtrace),BacktraceFilter, and every exception backtrace.skip_stack_framessemantics.Evidence
test/unit/screenshot_assertion_call_site_test.rb(10 tests), frames built from real files so they hold on any machine and on JRuby:<internal:kernel>and(eval)before the test's own line → that line, relative to root;…/foo/doesn't exclude…/foobar/); a wrapper helper; the fallback;niland[]callers;screenshot,assert_matches_screenshot,assert_no_screenshot_changesanddelayed: falsepaths, each naming the test's own line.test/unit/rspec_call_site_test.rb: a browser-free RSpec run in a subprocess (ScreenshoterStub, baselines placed where a checkout would put them) throughscreenshotandmatch_screenshot, run once. Each call site appears once on its own line, no rspec or snap_diff frame appears, and there is noCaused by:.master'slib/, all 8 unit tests fail and the RSpec test fails. Dropping the gem roots, the directory boundary, the real-file check or the relative rendering each turns at least one test red. Withoutcause: nil, the RSpec test fails on itsCaused by:check. Without commit 3'srescue, or with the notes after the call site, its matching test fails.rakein thebin/dtestimage (Ruby 4.0), at commit 2: 819 runs, 0 failures, 1 skip. At commit 3:rake test:unit780 runs, 0 failures.standardrb: 181 files, no offenses.raise …, cause: nil, the frame parsing): green in a manual CI run on this branch atbf63890, https://github.com/oboxodo/snap_diff-capybara/actions/runs/38211137915. Since thenlib/changed only in review tidy-ups:FRAMEbecame a private constant andcall_site/library_rootswere restructured with the same regex and roots; theraiseis unchanged. Commit 3 adds a plainrescue ArgumentErrorand reorders the message lines. In the same run, every other job passed except the twoselenium_headless(Firefox) jobs, which fail exactly as onmaster: the same 7 tests,Dimensions have changed: 800x514 => 800x526(Firefox 157 breaks the selenium_headless baselines, and 8 browser tests that can't fail hid part of it #300, fixed by fix: stop blur_active_element clicking inputs, and green selenium_headless on Firefox 157 #301).Deliberately not here
baseline:/actual:labels); separate change.README.md:147still shows a pre-feat: make the failure message legible #264 example message; unrelated to the stack.Reporters::Default#display_pathmakes the samerelative_path_fromcall, for the artifact paths. Those live under the root by construction, so it's left alone.Dir.chdirbefore the message is built (relative frames then fail the real-file check, so the first frame is used). Neither breaks the message.Summary by Sourcery
Report concise screenshot failure call sites and eliminate duplicate RSpec failure output.
Bug Fixes:
Caused by:.Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
Bug Fixes
Documentation
The PR appears safe to merge; no actionable new issue remains.Summary
Failed screenshots now name one source line instead of printing the whole stack.
Reviews (3) · Last reviewed commit: "Merge branch 'master' into fix/failure-m..." · Reviewed by Greptile