fix: stop passing green when a screenshot has no committed baseline - #255
Conversation
Baselines are read from git (`git show HEAD:<path>`), never from disk, and `fail_if_new` is false off CI by design. So a screenshot with no COMMITTED baseline was not compared at all: no assertion registered, the test passed whatever the page looked like, and the capture silently overwrote the PNG on disk. Recording a page, changing it, and re-running gave `1 runs, 1 assertions, 0 failures` with the file rewritten -- the product's core promise failing quietly, in the default local configuration, for every new user. `fail_if_new = false` stays a supported choice, so this warns rather than raises: once per screenshot, naming the real screenshot path and what to do about it, and calling out the confusing case where a PNG is already sitting there (it is not a baseline until it is committed). The end-of-run summary gets a matching line -- the reporters count what WAS compared, so their "N screenshots compared, no failures" was silent about exactly the screenshots that passed without being looked at. Three smaller lies fixed in the same messages: * The new-screenshot error promised `RECORD_SCREENSHOTS=1 bundle exec rake test`. Nothing in lib/ has ever read that variable -- it is this repo's own test-suite convention. Not implemented, deleted: a run already captures every changed screenshot to its real path (verified for several screenshots in one test, first one differing), so `git add <screenshot> && git commit` IS the bulk-record path, and an env var would only add a way to accept regressions without looking at them. * That error named `<name>.base.png`, a generated temp file nobody creates or commits, and pointed at the v1 `Capybara::Screenshot::Diff.fail_if_new` in the release whose headline is SnapDiff. Both corrected. * The Minitest activation warning keyed off lib/capybara-screenshot-diff.rb, the gem-NAME file. `Bundler.require` requires the gem's own name, so it fired for everyone with the gem in their Gemfile no matter what they required explicitly, with no action available to silence it. Only the v1 namespace entry (capybara/screenshot/diff) is a deprecated choice, and the remedy now names the canonical require.
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
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 |
Reviewer's GuideClarifies and scopes the deprecation warning source, introduces tracking and run‑summary reporting for screenshots without committed baselines, adjusts matcher behavior and messaging around missing baselines, and adds tests to lock in the new behavior and warning semantics. Sequence diagram for missing screenshot baseline handlingsequenceDiagram
participant Matcher as ScreenshotMatcher
participant VCS as SnapshotVCS
participant Reporting as SnapDiff::Reporting
participant FS as Filesystem
participant Test as TestRun
Matcher->>VCS: checkout_base_screenshot()
VCS-->>Matcher: committed baseline missing
alt fail_if_new enabled
Matcher->>Matcher: raise ExpectationNotMet
else fail_if_new disabled
Matcher->>Reporting: record_missing_baseline(screenshot_full_name)
Reporting-->>Matcher: first occurrence
Matcher->>FS: path.exist?
FS-->>Matcher: on-disk file status
Matcher-->>Test: warn no committed baseline
Matcher->>Matcher: capture_screenshot()
Test->>Reporting: finalize!()
Reporting-->>Test: missing_baselines_summary()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
`Bundler.require` -- the default -- requires the gem's own name, so
lib/capybara-screenshot-diff.rb and lib/snap_diff-capybara.rb load for every
consumer. Both hard-required capybara_screenshot_diff/minitest, which
requires "minitest", which is NOT a declared runtime dependency (the gemspec
declares capybara only). An RSpec-only bundle died at boot:
There was an error while trying to load the gem 'capybara-screenshot-diff'.
Gem Load Error is: cannot load such file -- minitest
from a gem that ships a first-class RSpec integration.
minitest is not added as a runtime dependency -- that would force it on RSpec
users to fix a problem caused by assuming it. The gem-name entry now loads
the gem, then feature-detects: minitest present keeps the documented
zero-require Rails path (assert_matches_screenshot with no explicit require),
minitest absent loads fine and says so, naming the integration to require and
the `require: false` that silences the line. A gem that loads and then does
nothing, silently, is its own bug report.
The v1 gem-name file now forwards to the surviving one, so there is one copy
of the detection rather than two that drift.
Explicit requires are untouched: `require "snap_diff/integrations/minitest"`
(and its legacy alias) still hard-require minitest and still fail loudly.
That is a line the user wrote, at a path they chose. `snap_diff/static` does
the same and is left alone for the same reason -- worth a docs note, not a
code change.
Also, from the release lane: the activation deprecation was a bare
Kernel#warn, so SnapDiff.silence_deprecations / SNAP_DIFF_SILENCE_DEPRECATIONS
did not reach it. It now honours the documented switch.
Guards: subprocess probes that chdir OUT of the project (inside it, RubyGems
re-adds -rbundler/setup and the gemspec unshifts the real lib/) and shadow
"minitest" with a raising stub on the load path, so `require "minitest"`
fails exactly as it does in a bundle without it. Every probe opens with a
gate line asserting the environment really is what it claims; both gate lines
were shown to abort when it is not. A real `bundle install` with rspec and no
minitest was used to confirm the crash and the fix by hand.
test/unit/legacy_deletion_test.rb's EDITS and the canonical-refs allowlist
track the new shape of snap_diff-capybara.rb -- both are deletion-simulation
fixtures whose stated contract is to go red when those lines change.
The v2.0.0 section was written before #250, #253, #254, #255, #256, #261, #263, #264, #266 and #267 landed, and three of its claims had gone false: - "Known limitations: fork-based parallel tests produce no HTML report ... Fixed in 2.1" -- fixed in 2.0 by #266. Reproduced both sides here: 1.15.1 + `parallelize(workers: 2, threshold: 0)` writes NO report and prints no summary line; master writes one merged report and `4 verified, 4 changed, 0 new`. - "a suite whose only contact with the v1 API is `require \"capybara_screenshot_diff/minitest\"` + `include ...Assertions` still prints nothing" -- #263 made the require doors warn. That exact setup now prints the migration notice; verified in a scratch project. - "Two removals 2.0 cannot warn about ... `driver:` as a setting" -- #263 made both the setting writer and the per-screenshot key warn. Verified: `Capybara::Screenshot::Diff.driver = :vips` prints the removal line with a call site. And the silent-by-design constant list repeated the shape of the beta2 `defined?` mistake: it listed "Os, Region" inside a run of `Capybara::Screenshot::Diff::` names. Probed on master -- `defined?(Capybara::Screenshot::Diff::Os)` and `defined?(Capybara::Screenshot::Diff::Region)` are both nil. The real names are `Capybara::Screenshot::Os` and the top-level `Region`, neither of which existed under `::Diff` in 1.15.1 either. Fully qualified now, and `::Comparison` added to match docs/UPGRADING.md. New material, every claim checked against the code or a live run: - a "why upgrade" section for the four green-suite-testing-nothing bugs (#255, #256, #254, #266), plus the unfollowable CI message (#267) and the fail_if_new precedence change - before/after transcripts of the failure message (#264), taken from the same page rendered on 1.15.1 and on master - the summary line (#261), with the fact that it comes from the HTML reporter and needs its one-line require -- an omission that would have read as a missing feature - the #250 / #253 perf table, attributed to its harness, with columns labelled before/after rather than 1.x/2.0 - the libvips fix is stated as guarded on libvips 8.15+, so a reader on an older libvips knows the bug is still theirs Install snippets stay pinned to 2.0.0.beta3 on purpose: `~> 2.0` resolves to nothing on rubygems today. docs/RELEASE_PREP.md already carries a precise step to swap all five (its grep finds exactly those five), and gains one line so the record-modes placeholder in the entry cannot ship unfilled. `rake test:unit` 651 runs / 0 failures, `standardrb lib test` clean.
beta3 fixed the canonical entry points and shipped almost none of the behaviour. beta4 is the prerelease the 2.0.0 entry actually describes: the four green-suite bugs (#254 #255 #256 #258), the accept workflow (#259), the legible failure message (#264), and the deprecation warnings that make 2.1's removals visible (#246 #263). - `lib/snap_diff/version.rb` -> 2.0.0.beta4. Nothing else holds a version; the gemspec, the legacy version file and the mirror gemspec all read it. Verified with the release workflow's own guard command: `ruby -I lib -r capybara/screenshot/diff/version -e "puts Capybara::Screenshot::Diff::VERSION"` => 2.0.0.beta4 - CHANGELOG: a `[v2.0.0.beta4]` section written as the delta from beta3, plus the record-modes PLACEHOLDER filled from #259 now that it has shipped. `grep -n PLACEHOLDER CHANGELOG.md` returns nothing. - Install snippets moved beta3 -> beta4 in README, docs/UPGRADING.md and docs/migration-guide.md. They stay PINNED: `~> 2.0` resolves to nothing while only prereleases exist, so unpinning belongs to 2.0.0 final, not here. The notes name the #272 caveat explicitly. Removing `skip_area`'s implicit stabilization wait (10.012 s -> 0.009 s measured) means a selector not yet in the DOM now yields no mask, silently, where it previously resolved after the wait. #277's run-level tally shipped in the same beta as the replacement signal, and the notes say so rather than leaving it to be discovered. Gates: `rake test:unit` 720 runs / 2124 assertions / 0 failures / 0 skips under CI=true on 4.0.6, `standardrb lib test` clean over 161 files, and `gem build` produces capybara-screenshot-diff-2.0.0.beta4.gem (93 files, 13 docs, RELEASE_PREP correctly excluded).
Two boot/behaviour bugs a customer persona hit in their first afternoon, plus the message lies around them.
1. The false green (release blocker)
Baselines are read from git (
git show HEAD:<path>), never from disk, andfail_if_newdefaults to!ENV["CI"].nil?— false locally. A screenshot with no committed baseline was therefore not compared at all:need_to_compare?false, no assertion registered, test green, screenshot written over whatever was on disk.Reproduced end to end in a scratch app (static page + real headless Chrome, git repo, no committed baseline):
Before
After
Positive control, same app:
git add doc/screenshots/heading.png && git commit, then change the page — the run fails, as it should. The feature is untouched; only the silence is gone.fail_if_new = falseis a deliberate, documented choice, so this warns rather than raises. Once per screenshot, naming the real path and the action. The end-of-run summary gets a matching line: the reporters count what was compared, soN screenshots compared, no failureswas silent about exactly the screenshots that passed unlooked-at.2. RSpec-only bundles could not boot the gem at all
Bundler.require— the default — requires the gem's own name, solib/capybara-screenshot-diff.rbandlib/snap_diff-capybara.rbload for every consumer. Both hard-requiredcapybara_screenshot_diff/minitest→require "minitest", which is not a declared runtime dependency (the gemspec declares capybara only):…from a gem that ships a first-class RSpec integration. Verified with a real
bundle install(rspec, no minitest) before and after.minitest is not added as a runtime dependency — that would force it on RSpec users to fix a problem caused by assuming it. The gem-name entry now loads the gem, then feature-detects:
assert_matches_screenshotis still there with no explicit require.require: falsethat silences the line. A gem that loads and then does nothing, silently, is its own bug report.The v1 gem-name file forwards to the surviving one, so there is one copy of the detection rather than two that drift. Explicit requires are deliberately untouched:
require "snap_diff/integrations/minitest"still hard-requires minitest and still fails loudly — that is a line the user wrote at a path they chose.snap_diff/staticdoes the same and is left alone for the same reason (docs note, not a code change).3. Messages that were not true
RECORD_SCREENSHOTS=1 bundle exec rake test— nothing inlib/has ever read that variable; it is this repo's own test-suite convention. Deleted rather than implemented. Measured first: one run captures every changed screenshot to its real path, including the second screenshot of a test whose first screenshot already differed (verification happens at teardown, not at the assert). Sogit add <screenshot> && git commitalready is the bulk-record path; an env var would only add a way to accept regressions without looking at them, and would need a CI guard to avoid being exactly that.<name>.base.pngin the new-screenshot error — a generated temp file nobody creates or commits. Now names the real screenshot.Capybara::Screenshot::Diff.fail_if_new = false— the v1 namespace, in the release whose headline is SnapDiff. Now the canonical name.4. Unsilenceable deprecation warning
integrations/minitest.rbwarned when the caller chain includedcapybara-screenshot-diff.rb— the gem-name Bundler entry point, whichBundler.requireloads for everyone with the gem in their Gemfile whatever they required explicitly. Verified through a realBundler.requirebefore and after. Onlycapybara/screenshot/diffis a genuinely deprecated choice; the remedy line now namessnap_diff/integrations/minitestinstead of another deprecated require. It was also a bareKernel#warn, soSnapDiff.silence_deprecations/SNAP_DIFF_SILENCE_DEPRECATIONSdid not reach it — it now honours the documented switch.Guards
Every fix has a test that failed before it, and each was mutation-checked (break the fix, watch the test red, restore with a targeted edit, watch it green):
.base.pngThe minitest-absence probes chdir out of the project (inside it, RubyGems re-adds
-rbundler/setupand the gemspec unshifts the reallib/) and shadowminitestwith a raising stub first on the load path, sorequire "minitest"fails exactly as in a bundle without it. Every probe opens with a gate line asserting the environment really is what it claims, and both gate lines were shown to abort when it is not (gate-true with the shim installed, gate-false without). A realbundle installreproduced the crash and confirmed the fix by hand; it has no place in the unit suite.rake test:unit(602 runs),rake test:canonical(494 runs) andstandardrb lib testare green. No existing test expectation was edited. Two deletion-simulation fixtures were updated to track the new shape ofsnap_diff-capybara.rb—legacy_deletion_test.rb'sEDITSand thecanonical_suite_has_no_legacy_refs_test.rballowlist — both of which document that going red on such a change is their job.Docs follow-ups (not touched here —
docs/is another lane's)docs/ci-integration.md:190,229,docs/docker-testing.md:19,docs/migration-guide.md:251still tell users to runRECORD_SCREENSHOTS=1. It never did anything. Replace with: run the suite, thengit add doc/screenshots/*.png && git commit.README.md:104— "If the change is intentional, delete the baseline and re-run to update it." Verified wrong: baselines come from git, so deleting the file changes nothing and the run fails again. The accept step isgit add <screenshot> && git commit(the failing run already left the new screenshot at that path).README.md:107-110lists three artifacts; a failing run leaves five:homepage.png,homepage.base.png,homepage.diff.png,homepage.base.diff.png,homepage.heatmap.diff.png. A passing run cleans up all buthomepage.png— verified, so.base.pngnever survives a green run and needs no code change, only a row in that table. The README gitignore block already covers all five (*.base.pngplus*.diff.png, which globs.base.diff.pngand.heatmap.diff.png).gem "capybara-screenshot-diff", require: false— it is also what silences the new "minitest is not in this bundle" boot line.snap_diff/static(andcapybara_screenshot_diff/static) still hard-require the Minitest integration, so an RSpec user who requires it gets a LoadError. Deliberate (loud failure at a line the user wrote), but worth documenting as minitest-only.🤖 Generated with Claude Code
https://claude.ai/code/session_014BQJX6eWzBj2UTm5zQsjEs