fix: never print a path or command that is not derived from live state (#260) - #267
Conversation
…260) `No existing screenshot found for <path>! To record it: run the test, then `git add <path>`' was unfollowable on the exact run that printed it. In CI -- the one place `fail_if_new` is on by default -- `check_base_screenshot` raised BEFORE `capture_screenshot`, so nothing was ever written to that path. Measured end to end against a real browser: $ CI=true ... => ExpectationNotMet, `ls <path>` -> no such file $ CI= ... => no raise, `ls <path>` -> file IS on disk The raise now runs after the capture, so the path exists whenever we name it and `git add` is a command the user can run right now -- and a failing CI job leaves the new screenshot behind for an artifact upload instead of nothing. Only the raise moved: the git checkout that drives `need_to_compare?`, and the no-baseline warning that has to read `@snapshot.path` before the capture overwrites it, both stay put. The two now-separate paths mean the warning would fire alongside the raise, so `check_base_screenshot` bows out when `fail_if_new` is on -- the raise says the same thing with the fix attached. Also guards a seam found by mutation: deleting `@snapshot.checkout_base_screenshot` outright left the ENTIRE unit suite green. Everything downstream hangs off that one call, and every test stubs `Vcs.checkout_vcs`, so nothing asserted the matcher ever asks git.
`fail_if_new` stored `!ENV["CI"].nil?` at require time, which made "the
user asked for false" and "CI was absent when the gem loaded" the same
false -- so the environment, not the user, had the last word. It now has
no stored default: nil means nobody said, and only then does the CI sniff
answer, read live. An explicit setting wins in both directions, whenever
the variable appears; assigning nil hands the setting back to the
environment.
SnapDiff.config.fail_if_new = false # false, even under CI=true
Same ordering insta narrowed to in insta#924 after Ruff hit it ("normally,
CLI flags take precedence over environment variables"), and the inverse of
jest#12288, where reading argv.ci instead of detected CI state made
`CI=1 jest` and `jest --ci` disagree for a whole major version.
The default itself is unchanged: failing only under CI stays, deliberately
-- a locally recorded baseline is often worthless across OS.
config_default_timing_test.rb is updated, not deleted: it still pins the
two contracts that did not change (root frozen at require time,
default_options[:wait] live per call), and its CI probe now pins the new
rule instead of the old freeze. The probes run from both the canonical and
the legacy entry points, so the v1 mattr_accessor view gets the same
precedence for free.
|
Warning Review limit reachedNext included review available in 18 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 (10)
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 GuideThe PR fixes misleading missing-screenshot guidance by capturing before enforcing fail_if_new, adds a guard that preserves baseline checkout behavior, and changes fail_if_new so explicit configuration outranks a live CI environment fallback while retaining CI-only failure as the default. Sequence diagram for screenshot capture before missing-baseline enforcementsequenceDiagram
participant Matcher as ScreenshotMatcher
participant Snapshot
participant Config
participant Filesystem
Matcher->>Snapshot: checkout_base_screenshot()
Snapshot->>Filesystem: Check base_path.exists?
Matcher->>Matcher: check_base_screenshot()
Matcher->>Matcher: capture_screenshot(capture_options, comparison_options)
Matcher->>Filesystem: Write screenshot to path
Matcher->>Matcher: fail_if_new_screenshot()
Matcher->>Config: fail_if_new()
Config-->>Matcher: CI fallback or explicit setting
alt New screenshot and fail_if_new enabled
Matcher-->>Matcher: Raise ExpectationNotMet with git add path
else Existing baseline or failure disabled
Matcher->>Matcher: need_to_compare?()
end
Flow diagram for fail_if_new configuration precedenceflowchart TD
A[Read fail_if_new] --> B{@fail_if_new is nil?}
B -- No --> C[Use explicit setting]
B -- Yes --> D[Read ENV CI live]
D --> E{CI is non-empty?}
E -- Yes --> F[Return true]
E -- No --> G[Return false]
C --> H[Explicit value outranks environment]
F --> H
G --> H
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
* feat: VCR-shaped record modes -- the accept workflow (#259) Accepting an intentional UI change is the most frequent action in the product and had no verb: `grep -rni "def accept|approve|update_baseline" lib/` returned nothing, both documented recipes were wrong, and two customer personas ended up reading vcs.rb to work out that the answer is `git add` + commit. SnapDiff.config.record = :once # default -- record when no baseline SnapDiff.config.record = :none # strict -- a missing baseline fails SnapDiff.config.record = :all # re-record everything, compare nothing `:all` is the genuinely new capability: the bulk-accept verb for the redesign that changed forty screenshots at once. Modes rather than a CLI flag because there is no runner to hang a flag on -- VCR's shape, and ours. PRECEDENCE: an explicitly set mode outranks `fail_if_new`; `fail_if_new` decides only when no mode was set. Same property #267 gave `fail_if_new` over the CI sniff. :once IS today, structurally. Config#record reads `@record || (fail_if_new ? :none : :once)`, so with nothing set `record_mode == :none` is true exactly when `config.fail_if_new` was, and the two decision points in ScreenshotMatcher swapped one for the other 1:1. No new branch on the default path; the missing-baseline default is untouched (CI-only failure, as Jest/AVA/Vitest/testthat all chose). `:all` refuses to run under CI. It accepts every rendering by design, so left in a committed config it is a build that compares nothing and passes forever -- Percy's failure mode. A CI job that needs to record NEW baselines uses `:once`, which still compares everything that has one. `#capture` is exempt: it never compares, so the mode is inert there. Additive only (ADR-010). `fail_if_new`, `pending_if_new` and `fail_on_difference` keep working for the whole 2.x line; each warns once per process through the existing Removal channel, naming the mode that replaces it -- and only where that is true. `record:` is also a per-screenshot option, carved out before Comparison so it cannot become an accepted-but-unread key. Also fixes a stale claim in docs/ci-integration.md: since #267 the screenshot IS written before the raise. * test: record-mode examples must run as a laptop, not as CI The `:all` examples failed on CI for exactly the reason they exist: `:all` refuses to run when ENV["CI"] is set, and the runner sets it. They passed locally because CI is unset there. The class now clears ENV["CI"] in setup and restores it in teardown, so it runs as a developer's laptop by default; the CI-refusal cases opt IN through the existing `with_ci` helper. Verified both ways: 25 runs green with CI=true and with it unset. Local green is half the bar -- this repo has been here before.
Two 2.0 items, one commit each.
1. The error names a file that will never exist — fixes #260
No existing screenshot found for <path>! To record it: run the test, then \git add `was unfollowable on the exact run that printed it.build_screenshot_assertioncalledcheck_base_screenshot*before*capture_screenshot, andfail_if_new` defaults to on under CI — so in CI the raise came first and nothing was ever written to the path the message named.Measured end to end against a real browser (cuprite), before:
After: the file is on disk in both cases, so
git add <path>is a command the user can run right now — and a failing CI job leaves the new screenshot behind for an artifact upload instead of nothing.Chosen fix: capture first. The alternative — branching the message on whether the file was written — needs two messages kept in sync, and the CI branch still could not offer
git add; it would have to send the user somewhere else. Capturing first makes the state match the message in every case, with one code path.Only the raise moved. The git checkout that drives
need_to_compare?stays incheck_base_screenshot, and so does the no-baseline warning, which has to read@snapshot.pathbefore the capture overwrites it. Because the two are now separate, the warning would otherwise fire alongside the raise, socheck_base_screenshotbows out whenfail_if_newis on — the raise says the same thing with the fix attached.Unguarded seam found by mutation: deleting
@snapshot.checkout_base_screenshotoutright left the entire unit suite green. Every test stubsVcs.checkout_vcs, so nothing asserted the matcher ever asks git for the baseline — whileneed_to_compare?, the warning and the raise all hang off that one call. Added the missing guard.2. Explicit config outranks the env sniff
fail_if_newstored!ENV["CI"].nil?at require time, which made "the user asked for false" and "CI was absent when the gem loaded" the samefalse— so the environment, not the user, had the last word. It now has no stored default:nilmeans nobody said, and only then does the CI sniff answer, read live.Same ordering insta narrowed to in insta#924 after Ruff hit it ("normally, CLI flags take precedence over environment variables"), and the inverse of jest#12288, where reading
argv.ciinstead of detected CI state madeCI=1 jestandjest --cidisagree for a whole major version.The default itself is unchanged. Failing only under CI stays, deliberately — a locally recorded baseline is often worthless across OS. This is a precedence rule only.
test/unit/config_default_timing_test.rbis updated, not deleted: it still pins the two contracts that did not change (rootfrozen at require time,default_options[:wait]live per call), and its CI probe now pins the new rule instead of the old freeze. The probes are re-run from the v1 entry points bytest/legacy/legacy_config_default_timing_test.rb, so the legacymattr_accessorview gets the same precedence for free.Evidence
Guard test first for each item; every guard mutation-checked (break it → red → restore with a targeted edit → green):
the message says \git add …` -- that file has to exist by then`fail_if_newbail-out incheck_base_screenshot@snapshot.checkout_base_screenshotfail_if_newreader ignores the ivar (env always wins)explicit false outranks CI=1fail_if_newfrozen back into storage at requireCI unset after require is seenrake test:unit(643 runs),rake test:canonical(527 runs),standardrb lib test— all green.Summary by Sourcery
Make missing-screenshot errors actionable by capturing the screenshot first and give explicit
fail_if_newsettings precedence over live CI detection.Bug Fixes:
git addinstruction.fail_if_newraises.Enhancements:
fail_if_newconfiguration take precedence over the liveCIenvironment while preserving CI-based defaults.Documentation:
Tests: