feat: VCR-shaped record modes — the accept workflow (#259) - #274
Merged
Conversation
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reachedNext included review available in 11 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 (14)
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 |
Contributor
Reviewer's GuideIntroduces config-driven Sequence diagram for screenshot record mode resolution and matchingsequenceDiagram
participant Test
participant Matcher as ScreenshotMatcher
participant Config
participant Snapshot as SnapManager
participant Reporting
Test->>Matcher: build_screenshot_assertion()
Matcher->>Config: record()
Config-->>Matcher: :once, :none, or :all
Note over Matcher: Per-screenshot record: overrides config
alt record_mode == :all
Matcher->>Matcher: refuse_bulk_record_under_ci!
Matcher->>Snapshot: capture screenshot
Matcher->>Snapshot: write baseline
Matcher->>Reporting: record_rerecorded_baseline(name)
else baseline exists
Matcher->>Snapshot: checkout_base_screenshot
Matcher->>Snapshot: capture screenshot
Matcher->>Snapshot: compare baseline
else record_mode == :none and baseline missing
Matcher->>Snapshot: capture screenshot
Matcher-->>Test: ExpectationNotMet
else record_mode == :once and baseline missing
Matcher->>Snapshot: capture screenshot
Matcher->>Snapshot: record new screenshot
end
Flow diagram for record mode precedence and baseline handlingflowchart TD
A[assert_matches_screenshot] --> B[Resolve per-screenshot record option]
B -->|unset| C[Config.record]
C -->|unset| D[fail_if_new fallback]
D -->|CI enabled| E[record_mode = none]
D -->|CI disabled| F[record_mode = once]
B -->|explicit mode| G[Use explicit mode]
E --> H{Baseline exists?}
F --> H
G --> I{Mode}
H -->|yes| J[Compare screenshot]
H -->|no| K[Record screenshot or fail]
I -->|once| K
I -->|none| L[Fail with git add guidance]
I -->|all| M[Re-record without comparison]
M --> N[Report rerecorded screenshots]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Screenshot diffs detected
|
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.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
These three checks could not have been written before the rebase: the counts line (#269) and record modes (#274) never met until now. - The fork-parallel fragment carries BOTH tallies, so a worker that counts assertions AND re-records a baseline must hand back both and neither may cost the other. The main merge case now does both. - A fragment with none of the keys added since #266 still merges. The fragments directory is keyed by pid under the system temp dir, so a recycled pid can hand the merge a fragment written by an older version of the gem; every key but "missing_baselines" is read with a default for exactly that. Mutation-checked: dropping one default fails the merge with `TypeError: nil can't be coerced into Integer`. - `record: :all` end to end, on a finished process. It re-records without comparing, which through the summary path is neither verified nor changed, and NOT "new" either -- there was a baseline, it just was not consulted.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
…on (#272, #269, #270) (#275) * fix: skip_area selectors that match nothing no longer block for 5s (#272) Capybara's `all` defaults to `minimum: 1` and blocks in `synchronize` until that count is satisfied, so every `skip_area` (or `crop`) selector matching nothing burned a full `Capybara.default_max_wait_time` -- 5s by default, per selector, per screenshot. Measured with a real browser at Capybara's shipped 5s default, `%w[picture img]` against an image-less page: 10.012s before, 0.009s after. One project reported that exact scenario as 44% of their whole suite. The cost is only half of it. `skip_area` is a MASK -- "exclude whatever is currently there". Waiting for an element to appear is the wrong semantic: a selector matching nothing has nothing to mask, and that answer is available immediately. `all_visible_regions_for` is the only Capybara finder in lib/; the rest of BrowserHelpers is execute_script/evaluate_script and driver introspection, none of which carry a count expectation to block on. So this is the class of bug, not one instance of it. The guard uses a REAL browser session. Every existing test of this path stubs the browser, and a stub answers instantly whether or not the selector matches -- which is exactly why a 5-second wait went unnoticed for years. Its budget is derived from the live `default_max_wait_time` rather than hardcoded, so lowering the suite's wait cannot quietly turn it into an assertion that passes while broken. * fix: the end-of-run summary prints without a reporter registered (#269) `SnapDiff::Reporting.register` appears exactly once in the whole gem, at reporters/html.rb:140. So the honest summary line shipped bundled with the HTML report, and the documented Rails setup registered nothing: $ ruby -e 'require "snap_diff/integrations/minitest" puts SnapDiff::Reporting.reporters.size' 0 That line exists to catch the failure modes no per-assertion rule can see -- a run where zero system tests executed, or where an inherited GIT_DIR redirected every baseline lookup. `0 verified` is the only signal for either, and it was behind an opt-in require. Separates the two concerns: counting is core honesty, writing an HTML file is a feature. Reporting owns `verified`/`changed` and prints `counts_summary` unconditionally from `finalize!`; Reporters::HTML keeps the report file, stays opt-in, and its `summary` is now just the path of the file it wrote -- on its own line, and nil when it wrote nothing. So the counts print exactly once whether or not the reporter is loaded, and `0 verified` still shouts NOTHING WAS VERIFIED. The fork-parallel merge (#266) carries the new counters in the same fragment as the missing-baseline names, and its guard now goes through `Reporting.notify` -- the real path -- so it checks both halves the worker has to hand back. `count` warns and skips rather than raising: `notify` runs inside every test's teardown, and a raise there aborts SnapDiff.reset before it clears the registry, leaking one test's assertions into the next. Same contract the reporter loop already applies, and just as loud (unconditional, not DEBUG-gated). Adding it surfaced three test doubles that never implemented Comparison's `difference` reader -- they had been blowing up unnoticed inside HTML#record, which swallows under `if ENV["DEBUG"]`. docs: `configuration.md` recommended the LEGACY `Capybara::Screenshot.enabled` spelling inside the canonical config reference, in three places. * fix: a disabled screenshot no longer counts as an assertion (#270) `integrations/minitest.rb` incremented the counter before the `active?` guard inside `super`, which returns false immediately when screenshots are disabled. So a test whose only assertion was a screenshot reported `1 runs, 1 assertions, 0 failures` -- nothing captured, nothing compared, and a green line claiming otherwise. Counting only when active hands the alarm to Rails for free. Rails unconditionally prepends ActiveSupport::Testing::TestsWithoutAssertions into every ActiveSupport::TestCase (test_case.rb:205), so those tests now warn: Test is missing assertions: `test_it` .../my_test.rb:12 Guarded in both directions with the real Rails module, not a stand-in: the alarm fires for a test whose sole assertion was a disabled screenshot, and stays quiet both for a test that asserts something else and for a screenshot that actually ran. Audited the other two adapters, neither needs a change: - RSpec's matcher returns a literal `true`, which is correct rather than the same bug: returning `assert_matches_screenshot`'s false would fail the example over a config switch the user set on purpose, and a real mismatch raises rather than returning false. RSpec has no assertion count to correct and no missing-assertion alarm to trigger, so only the end-of-run `0 verified` line can see a disabled run. Said so at the call site, since the next reader will otherwise "fix" it. - Cucumber counts nothing; `SnapDiff::DSL#assert_matches_screenshot` already returns false when inactive. * test: pin the #274 record-modes interactions the rebase created These three checks could not have been written before the rebase: the counts line (#269) and record modes (#274) never met until now. - The fork-parallel fragment carries BOTH tallies, so a worker that counts assertions AND re-records a baseline must hand back both and neither may cost the other. The main merge case now does both. - A fragment with none of the keys added since #266 still merges. The fragments directory is keyed by pid under the system temp dir, so a recycled pid can hand the merge a fragment written by an older version of the gem; every key but "missing_baselines" is read with a default for exactly that. Mutation-checked: dropping one default fails the merge with `TypeError: nil can't be coerced into Integer`. - `record: :all` end to end, on a finished process. It re-records without comparing, which through the summary path is neither verified nor changed, and NOT "new" either -- there was a baseline, it just was not consulted.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
…289) `jruby-10.0/rails80` timed out on master run 32763256451 and took the whole run red. Tests were still printing dots when SIGTERM landed, so the cell was SLOW, not hung -- and the same cell had passed the previous three runs. The 15-minute per-attempt budget was sized from run 32643567648, where a clean JRuby attempt was 545-713s: "~26% headroom over the slowest", as the comment says. That measurement is stale. Re-measured on run 32758898367, the last green one before this bit: rails71 652s rails81 740s rails80 870s rails72 881s <- against a 900s cap The suite grew from 646 to 757 runs in between (#274, #277, #278, #279, #283), and the headroom went with it: 881/900 is 2%. The cells have been passing by seconds, which is why this looked stable for three runs and then was not. 20 minutes restores the ~26% margin this was originally sized for, and the job cap follows to keep the arithmetic true: 1 + 20 + 20 = 41. Both numbers move together on purpose -- a per-attempt timeout that does not fit the cap kills the last attempt mid-run and reports `cancelled`, which reads as an absence rather than a failure. MRI is unchanged: 128s against 3 minutes. Costs nothing on a green run; it is a ceiling, not a sleep.
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
1 participant
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.
Closes #259.
Accepting an intentional UI change is the most frequent action in the product, forever — and it had no verb.
grep -rni "def accept|approve|update_baseline" lib/returns nothing; there are no rake tasks and noexe/. Both documented recipes were wrong, and two customer personas ended up readingvcs.rbto work out that the real answer isgit add+ commit.The design
VCR is the closest Ruby precedent, and its record modes are config, not CLI flags — which is our shape too, since we have no runner to hang a flag on.
Also a per-screenshot option:
assert_matches_screenshot "x", record: :none.:allis the genuinely new capability — the bulk-accept verb for the redesign that changed forty screenshots at once.Precedence
An explicitly set mode outranks
fail_if_new;fail_if_newdecides only when no mode was set. Per screenshot outranks the config; the config outranksfail_if_new. That is the same property #267 gavefail_if_newitself over theCIsniff — explicit outranks implicit, all the way down.recordreads:once:nonerecord = :once:oncerecord = :none:nonerecord = :once,fail_if_new = true:oncerecord = nil,fail_if_new = true:none:onceis today, structurally — not just by testConfig#recordreads@record || (fail_if_new ? :none : :once). With nothing set,record_mode == :noneis true exactly whenconfig.fail_if_newwas, so the two decision points inScreenshotMatcherare a 1:1 substitution:No new branch is added on the default path. A subprocess probe pins it: a setup that sets none of the settings is byte-for-byte silent and still reads
:noneunderCI=1. Mutating the fallback to a plain:oncereds 8 tests.The missing-baseline default is unchanged — CI-only failure is what Jest, AVA, Vitest, testthat and jest-image-snapshot all chose, and a locally recorded screenshot baseline is often worthless across OS.
:nonemakes strictness an explicit choice instead; that is the point of the mode.Guarding
:all:allaccepts every rendering by design, so left in a committed config file it is a build that compares nothing and passes forever, with the "recorded" screenshots discarded when the runner is torn down. That is precisely how Percy goes green on a job that lost its token.:allrefuses to run under CI. A CI job that needs to record new baselines does not need it::oncerecords those and still compares everything that has a baseline.#captureis exempt — it never compares against a baseline, so the mode is inert there and refusing would invent a failure out of a setting that changes nothing.Belt as well as braces: an end-of-run summary names exactly the screenshots that went down the re-record path, derived from live state.
Additive only (ADR-010)
fail_if_new,pending_if_newandfail_on_differenceall keep working exactly as they do. Each warns once per process through the existingRemovalchannel, from its writer (never its reader, which runs for everyone). Each message names the mode that replaces that setting and nothing more —record:supersedes the missing-baseline half squarely and the other two only obliquely, and a message that overclaims misroutes.record:is carved out of the options hash beforeComparison, rather than added toKNOWN_OPTIONS— a key that hash accepts and nothing downstream reads is the silent no-op ADR-010 exists to stop.Evidence
rake test:unit682 runs / 0 failures ·rake test:canonical566 runs / 0 failures ·standardrb lib testclean.record_mode != :allclause inneed_to_compare?reddened nothing —check_base_screenshotalready leaves:allwith no base file. Deleted; the guard that does hold the invariant now reds 5 tests instead of 1.:all-under-CI refusal sat in#initialize, so it also fired oncapture_screenshot. Moved tobuild_screenshot_assertion, with a test pinning that#captureunder:all+CI=truestill works.git show HEAD:lookups, realgit status) for all three modes, the bulk case, and the CI refusal.Docs
docs/configuration.md(new Record modes section),docs/UPGRADING.md(warning #5),README.md("Accepting many at once"), plusarchitecture.md/snapdiff.mdsetting counts.Incidentally fixes a stale claim in
docs/ci-integration.md: since #267 the screenshot is written before the raise, so thegit addthe message names is a command you can actually run.Does not touch
version.rborCHANGELOG.md.Summary by Sourcery
Introduce record modes as an explicit workflow for recording, rejecting, or bulk-accepting screenshot baselines while retaining backward-compatible defaults.
New Features:
:all.Bug Fixes:
Enhancements:
fail_if_new, and CI defaults while preserving existing behavior when no mode is configured.Documentation:
Tests: