refactor: cut every core→legacy dependency + reverse gate (3.0 readiness) - #235
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (36)
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 GuideRefactors SnapDiff’s core to eliminate all dependencies from core code into the legacy Capybara trees, centralize legacy behavior in a single shim file, and move configuration/driver behavior to SnapDiff namespaces while adding tests that enforce these constraints and document 3.0 deletion readiness. Sequence diagram for standalone driver detectionsequenceDiagram
participant Caller
participant Drivers as SnapDiff::Drivers
participant Vips
participant ChunkyPNG
Caller->>Drivers: require snap_diff/drivers
Drivers->>Vips: require("vips")
Vips-->>Drivers: available or LoadError
Drivers->>ChunkyPNG: require("chunky_png")
ChunkyPNG-->>Drivers: available or LoadError
Drivers->>Drivers: detect_available()
Drivers-->>Caller: available() => AVAILABLE_DRIVERS
Flow diagram for the 3.0 legacy-tree deletion dry runflowchart TD
Start["Delete legacy Capybara trees and LegacyShims"] --> CoreLoad["Load SnapDiff entry points"]
CoreLoad --> Surface["Core APIs work"]
Surface --> StartAPI["SnapDiff.start remains unresolved"]
StartAPI --> Decision{"Choose 3.0 API direction"}
Decision --> Drop["Drop start and update upgrade guidance"]
Decision --> Replace["Give start a non-legacy shape"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
legacy_tree_is_alias_only_test.rb proves the v1 trees hold no logic. Nothing proved the mirror image, and it was false: 64 core->legacy edges (6 backward requires, AVAILABLE_DRIVERS, ~55 config reads through the legacy view) meant `git rm lib/capybara*` would break the gem. This gate reports every one as file:line and starts with them all allowlisted, so it lands green and the following commits can only shrink it. Red on master with an empty allowlist; mutation-checked by adding a Capybara::Screenshot::Diff reference to region.rb (gate named it). Comments are ignored (history, not dependency); strings are not.
SnapDiff::Drivers.available is documented canonical API, but the list it read was defined only in config_legacy.rb, so ruby -Ilib -e 'require "snap_diff/drivers"; SnapDiff::Drivers.available' raised NameError: uninitialized constant Capybara::Screenshot::Diff. It now answers. Detection moved to Drivers.detect_available and runs at drivers.rb load (same load moment in every entry-point path); SnapDiff::Utils .detect_available_drivers one-lines into it, so the documented Utils name and its tests are unchanged. config_legacy keeps AVAILABLE_DRIVERS as an eager same-object alias -- test_helper still reads it at boot, and namespace_forwarding_test still pins assert_same. The stubbing point moves with the value: image_compare_test now stubs SnapDiff::Drivers::AVAILABLE_DRIVERS. Stubbing the legacy alias would only rebind the alias, which is the whole point of cutting the edge. utils.rb requires drivers and drivers now needs Utils at call time, so drivers.rb requires utils at the bottom -- either file can be required first.
Twenty-nine reads across twelve core files went through Capybara::Screenshot / Capybara::Screenshot::Diff -- the legacy VIEW of a storage that has lived in SnapDiff::Config since #230. Same storage, canonical name; behaviour is unchanged and the legacy delegators stay for users. Two follow-ons: - browser_helpers dropped its respond_to?(:window_size) guard: that existed because the mattr_accessor might not be installed yet, and Config always has the attribute. - the fail_if_new error message now tells users 'SnapDiff.config.fail_if_new = false' -- it named an accessor that 3.0 deletes. Tests that STUBBED the legacy accessors now stub SnapDiff.config: a delegator write still reaches the storage, but a stubbed delegator method does not, so those stubs were silently no-ops against the new reads. The legacy accessors keep their own coverage in snap_diff_config_test.rb (every MAPPING writer) and config_default_timing_test.rb.
dsl.rb, reporters/html.rb and screenshot_matcher.rb pulled their dependencies through capybara/screenshot/diff/* and capybara_screenshot_diff/* forwarders -- files whose only content is a require of the snap_diff/* unit the core actually wanted. Point at the units directly. snap_diff.rb's own two backward requires are the last ones left and go with the v1-surface consolidation.
Three legacy edges were left in the canonical core: - lib/snap_diff/config.rb DEFINED the Capybara::Screenshot::Diff module skeleton so Config::MAPPING could name it, and generated the legacy mattr_accessors. After a 3.0 `git rm` that would have survived: the core would still define a phantom v1 namespace, so adopters' `defined?(Capybara::Screenshot::Diff)` checks would keep passing. - lib/snap_diff.rb required config_legacy.rb and image_compare.rb -- the only reason being that those forwarders happened to install the v1 surface for bare `require "snap_diff"` processes. - SnapDiff.start yields the two legacy holders, so it cannot outlive them. All three move to lib/snap_diff/legacy_shims.rb, which now holds the whole v1 surface as code: const_missing forwarders, CONFIG_MAPPING and its generator, the derived forwarders that were in config_legacy.rb (Screenshot.active?, Diff.configure/.compare/.default_options), SnapDiff.start, and the eager AVAILABLE_DRIVERS / Comparison aliases. config_legacy.rb and image_compare.rb are now requires only. Config keeps the setting list as Config::SETTINGS and names nothing legacy; a new test pins CONFIG_MAPPING.keys == SETTINGS so the split cannot drift into storage with no accessor (or the reverse). Behaviour is unchanged, including the awkward part: bare `require "snap_diff"` still answers Capybara::Screenshot.active?, Diff.default_options, Diff::AVAILABLE_DRIVERS and Diff::Comparison, exactly as it did when it reached through the v1 forwarders (verified before/after in a fresh process). The reverse gate's allowlist is now EMPTY.
The rule was `body.include?("SnapDiff")` plus a following `end`, which
accepted anything that merely mentioned the canonical namespace:
def x
SnapDiff.config.a ? b : c # a conditional -- real behaviour
end
def x; SnapDiff.config.a; File.write(...); end # two statements
Both are now rejected: a semicolon is never alias-shaped (it is how
several statements, or a whole def, hide inside one "line"), and a
forwarder body must match the whole of FORWARDER_BODY -- one method-call
chain rooted at SnapDiff, at most one argument list and one block.
Mutation-checked with exactly those two shapes plus a genuine forwarder,
which still passes.
The gemspec required capybara/screenshot/diff/version and read Capybara::Screenshot::Diff::VERSION -- a build-time dependency on a file 3.0 deletes, so `gem build` would have failed the moment it did. Same value (the legacy constant is an alias of SnapDiff::VERSION), one fewer 3.0 blocker. The legacy constant stays for adopters who read it. Found by the 3.0 dry run; fixed here because it is one line and in scope.
Capybara::Screenshot::Diff::VERSION raised NameError -- and defined? returned nil -- under "snap_diff", snap_diff/dsl, both integrations, snap_diff/static and, worst, the LEGACY capybara_screenshot_diff/dsl. Cause: the constant was assigned by capybara/screenshot/diff/version.rb, and the 3.0-readiness pass stopped the core requiring that forwarder. legacy_shims deliberately omits VERSION from its const_missing map -- it is one of the documented eager exceptions -- so nothing filled the gap and adopters' `defined?` feature detection silently went false. Assign it in legacy_shims next to the other eager aliases: that file is required by every entry point, canonical and legacy, so it is the only place the eager exceptions can actually be eager. version.rb drops the assignment (a second one is a duplicate-constant warning, not a safety net) and becomes a require. The suite could not have caught it: EAGER_USER_FACING was only probed for the four legacy entry points, and VERSION was not in the list at all. New EAGER_EVERYWHERE probe covers all 15 entry points including capybara_screenshot_diff/dsl, which is in none of the existing lists -- exactly why it was the legacy entry that lost VERSION unnoticed. Red without the alias, naming all six. Also corrects two now-false claims in the legacy_shims header: it said AVAILABLE_DRIVERS was aliased in config_legacy.rb (it is aliased in this file) and that VERSION/Comparison were defined by their own forwarder files (nothing requires those anymore).
LEGACY_REQUIRE anchored straight on the opening quote, so
require_relative "../capybara/screenshot/diff/version"
passed the gate while genuinely loading the v1 file (confirmed via
$LOADED_FEATURES). Every core file sits one directory below lib/, so that
is a one-line escape, not a hypothetical. Allow an optional (\.{1,2}/)* --
mutation-checked with exactly that line.
Two smaller fixes while here: the header claimed comments are ignored when
only WHOLE-LINE ones are (trailing notes on a live code line are scanned,
which is the behaviour worth keeping -- the header now says so), and the
stale-allowlist check now reports a deleted allowlisted file instead of
raising Errno::ENOENT on it.
The tightened rule rejected ternaries and semicolons but left `(.*)` and
`{ .* }` unbounded, so both of these still passed as "forwarders":
SnapDiff.config.x(File.exist?("/etc/passwd") ? raise("boom") : ENV.fetch("HOME"))
SnapDiff.config.tap { |c| File.write("/tmp/pwned", c.inspect); exit 1 }
The block case also dodged the semicolon check, because the walk steps
over a def's body line without re-examining it -- that scan now runs over
every line up front.
An argument list is now names, commas, splats and keyword colons, or
Ruby's `...` forwarding. No parens, so no nested call; no `.`, so no bare
receiver call; no `?`/quote/`=`, so no conditional, literal or assignment.
The block form is gone entirely: nothing in these trees has a def left,
and an unbounded block is exactly the hole above.
Mutation-checked with both shapes above (rejected, the block twice) plus
three genuine forwarders that must not false-positive: a bare chain, an
argument pass-through, and CapybaraScreenshotDiff.serve(...) -- which the
first draft of this rule did reject.
Both are read-identical and only break on write, so nothing warns: - stubbing Capybara::Screenshot::Diff::AVAILABLE_DRIVERS now only rebinds an alias -- the gem reads SnapDiff::Drivers::AVAILABLE_DRIVERS, so a downstream test stubbing it to [] stops exercising the no-drivers path and passes for the wrong reason; - SnapDiff::Config::MAPPING is gone mid-beta, split into Config::SETTINGS and the @api-private LegacyShims::CONFIG_MAPPING.
ab853f6 to
c50d197
Compare
Caught by independent review: `rake test:canonical` in the deleted tree is 1F, not the 0F I published. CANONICAL_SURFACE listed `start`, applied to all 7 canonical entry points. SnapDiff.start is defined only in lib/snap_diff/legacy_shims.rb:169 and yields the two v1 config holders, so it cannot outlive them (#235 decided this). A canonical gate demanding a method 3.0 deletes is a gate that goes red the day the deletion lands -- and I widened it in e789c8a by adding snap_diff-capybara. My own PR body filed .start under "safe to lose at 3.0". mutation (start put back, deleted tree): require "snap_diff" -> missing: start require "snap_diff/dsl" -> missing: start require "snap_diff/integrations/minitest" -> missing: start ... all 7 entry points .start keeps full coverage on the legacy side: legacy_forwarders_test pins what it yields and that it applies a setting, and a new per-entry-point probe in legacy_entry_point_probe_test pins the availability claim the canonical gate used to make -- for the entries that actually keep it. Also, per review: - "bare require never loads the umbrella" moves to legacy_forwarders_test. Its subject is lib/capybara_screenshot_diff.rb; once 3.0 deletes that file the $LOADED_FEATURES grep is empty by construction and the guard can never fail again. (-1 canonical run: 458 -> 457.) - backtrace_filter_test built synthetic paths under lib/capybara_screenshot_diff/. Pure string inputs to a prefix matcher, so no assertion changes -- but one of them named the real file the filter defaults to, which 3.0 deletes. rake test:unit 547 runs, 1550 assertions, 0F/0E/0S rake test 575 runs, 1595 assertions, 0F/0E/1S rake test:canonical 457 runs, 1288 assertions, 0F/0E/1S ... and 457/1288/0F/0E/1S in the deleted tree, identical.
* test: move the v1-surface tests into test/legacy/ and add rake test:canonical
The five tests whose SUBJECT is the v1 compatibility surface now live in
test/legacy/, so the 3.0 deletion is one more path on the same git rm:
git rm -r lib/capybara* ... test/legacy
A directory rather than a list in the Rakefile: nothing to keep in sync.
- rake test unchanged, runs everything (today's gate)
- rake test:canonical NEW, everything except test/legacy (the 3.0 gate)
- rake test:unit test/unit + test/legacy, so the release gate keeps
its coverage (legacy/ marks lifetime, not kind)
errors_alias_test.rb was mixed: the four CapybaraScreenshotDiff::* alias
pairs are v1 surface, the hierarchy assertions outlive them. Split rather
than moved whole -- test/unit/errors_test.rb keeps the two canonical tests
verbatim, so no assertion is lost at 3.0.
530 runs, 1519 assertions, 0 failures (unchanged).
* test: point the whole canonical suite at SnapDiff names
The suite still spoke v1 everywhere, so it would have broken on the 3.0
deletion even though the gem no longer does. Mechanical, no behaviour and
no assertion values changed:
- harness: test_helper + system_test_case load snap_diff/integrations/*
and configure through SnapDiff.config; the support stubs (DSLStub,
ScreenshoterStub, TestDoubles, DriverCoverage, NonMinitest) stop
reopening gem namespaces and become plain top-level modules
- 33 test files were defined inside module Capybara::Screenshot(::Diff) /
CapybaraScreenshotDiff -- de-nested to top-level classes, so no bare
constant resolves into a namespace 3.0 deletes
- 270 legacy constant/accessor/session call sites repointed
(CapybaraScreenshotDiff.registry -> SnapDiff.session, .reporters ->
SnapDiff::Reporting.reporters, Capybara::Screenshot.root ->
SnapDiff.config.root, ...) and 25 legacy require paths
- legacy-surface tests now require the v1 entry point themselves, since
the shared harness no longer loads it
Three claims would have become tautologies under a blind repoint
(assert_same SnapDiff.session, SnapDiff.session and friends): they were
forwarder-identity claims about the v1 view. Preserved verbatim in the new
test/legacy/legacy_forwarders_test.rb together with SnapDiff.start, which
yields the two v1 holders and cannot outlive them.
rake test:unit 534 runs, 1526 assertions, 0F/0E (was 530/1519)
rake test 562 runs, 1571 assertions, 0F/0E/1S (was 558/1564)
+4 runs: legacy_forwarders_test keeps the v1 claim where the canonical
file also kept its own version (register-appends, reporters_mutex, serve).
* test: split the mixed config/entry-point files along the same line
Four files asserted the canonical behaviour AND the v1 view of it in one
place, so a receiver repoint turned real claims into tautologies. Each is
now two files; the v1 half is verbatim, and the canonical half stands on
its own after 3.0:
- snap_diff_config_test -> + test/legacy/legacy_config_accessors_test
(CONFIG_MAPPING completeness, the mattr_accessor round trips, active?
through the legacy forwarder, SnapDiff.start)
- config_default_timing_test -> + test/legacy/legacy_config_default_timing_test
canonical keeps snap_diff + snap_diff/integrations/minitest and reads
SnapDiff.config only; the legacy file re-runs the SAME probe scripts
under the v1 entries and adds the both-surfaces-agree loop, which is
exactly what check_both asserted -- one source of truth, no drift
- support_load_probe_test -> + test/legacy/legacy_entry_point_probe_test
(advertised v1 constants, the CapybaraScreenshotDiff session surface,
EAGER_USER_FACING / EAGER_EVERYWHERE under their OLD names)
- errors_alias_test (earlier commit) -> test/unit/errors_test
snap_diff-capybara joins CANONICAL_ENTRY_POINTS: 3.0 keeps that entry
point (repointed at snap_diff/integrations/minitest), and it was covered
only as a legacy entry, so it would have lost all coverage.
Also repointed the last legacy call sites the sweep left: the rspec
fixtures stubbed Capybara::Screenshot::Diff.pending_if_new, which the core
stopped reading in #235 -- a silent no-op stub, now SnapDiff.config.
rake test:unit 544 runs, 1544 assertions, 0F/0E
rake test 572 runs, 1589 assertions, 0F/0E/1S
* test: port the canonical claims that only a legacy-surface test was pinning
Audit of every assertion moving into test/legacy/, asking: if this file
vanished at 3.0, would any CANONICAL behaviour become untested? Two hits,
both now duplicated (not moved) into a canonical test -- the v1 originals
stay put, they still guard the v1 contract for all of 2.x:
- namespace_forwarding_test was the only place proving SnapDiff::Drivers
.loaded is ONE hash mutated in place (it asserted the v1 LOADED_DRIVERS
constant is that same object, and that registering through it shows up
canonically). Utils.find_driver_class_for caches through .loaded, so a
copy-returning refactor would break user driver registration silently.
-> drivers_test ".loaded is a single hash mutated in place"
mutation: `.loaded.dup[...] = ...` -> red, Expected :probe_driver, got nil
- the entry-point probe was the only place asserting an entry point defines
its advertised CONSTANTS when it is the ONLY require (the f89cea2 bug
class) -- but only for the v1 names.
-> support_load_probe_test "every canonical entry point defines its
advertised constants standalone", same claim over snap_diff/dsl,
/integrations/minitest, /integrations/rspec, snap_diff-capybara.
Entry-specific, because bare snap_diff carries neither DSL nor
reporters by design.
mutation: a bogus constant in the list -> red, naming it
Also: attempts_reporter_test now requires snap_diff/attempts_reporter --
stable_screenshoter pulls it in lazily and the v1 umbrella was what loaded
it eagerly, so it was the one canonical test the deletion actually broke.
Judged legacy-only and safe to lose at 3.0: const_missing/eager alias
semantics, deprecation warn-once + silencing, CONFIG_MAPPING completeness,
the alias-only scan of lib/capybara*, and the CapybaraScreenshotDiff
session/reporter forwarders -- every one is about a name 3.0 deletes, and
its canonical counterpart is pinned in test/unit/.
rake test 574 runs, 1593 assertions, 0F/0E/1S
* fix: the canonical gate demanded SnapDiff.start, which 3.0 deletes
Caught by independent review: `rake test:canonical` in the deleted tree is
1F, not the 0F I published.
CANONICAL_SURFACE listed `start`, applied to all 7 canonical entry points.
SnapDiff.start is defined only in lib/snap_diff/legacy_shims.rb:169 and
yields the two v1 config holders, so it cannot outlive them (#235 decided
this). A canonical gate demanding a method 3.0 deletes is a gate that goes
red the day the deletion lands -- and I widened it in e789c8a by adding
snap_diff-capybara. My own PR body filed .start under "safe to lose at 3.0".
mutation (start put back, deleted tree):
require "snap_diff" -> missing: start
require "snap_diff/dsl" -> missing: start
require "snap_diff/integrations/minitest" -> missing: start
... all 7 entry points
.start keeps full coverage on the legacy side: legacy_forwarders_test pins
what it yields and that it applies a setting, and a new per-entry-point
probe in legacy_entry_point_probe_test pins the availability claim the
canonical gate used to make -- for the entries that actually keep it.
Also, per review:
- "bare require never loads the umbrella" moves to legacy_forwarders_test.
Its subject is lib/capybara_screenshot_diff.rb; once 3.0 deletes that
file the $LOADED_FEATURES grep is empty by construction and the guard can
never fail again. (-1 canonical run: 458 -> 457.)
- backtrace_filter_test built synthetic paths under lib/capybara_screenshot_diff/.
Pure string inputs to a prefix matcher, so no assertion changes -- but one
of them named the real file the filter defaults to, which 3.0 deletes.
rake test:unit 547 runs, 1550 assertions, 0F/0E/0S
rake test 575 runs, 1595 assertions, 0F/0E/1S
rake test:canonical 457 runs, 1288 assertions, 0F/0E/1S
... and 457/1288/0F/0E/1S in the deleted tree, identical.
test/unit/legacy_tree_is_alias_only_test.rbproves the v1 trees hold no logic. Nothing proved the mirror image — and it was false. On master,git rm lib/capybara*breaks the gem.This PR cuts every core→legacy edge and adds the gate that keeps them cut, then dry-runs the 3.0 deletion and reports exactly what is left.
No version or CHANGELOG changes: this line ships nothing until 3.0-readiness is complete.
Rebased onto
e1c5962. Five conflicts, two of them semantic because #233 deleted code this branch was canonicalizing:screenshot_namer.rb#screenshot_areaand theactive?/fail_on_differenceguard inscreenshot_assertion.rb#verify. The deletion wins in both — a naive "take ours" would have resurrected dead code. That is also why the config-read count below is 25, not the 29 measured pre-rebase.1. The reverse gate —
test/unit/core_tree_has_no_legacy_deps_test.rbScans
lib/snap_diff.rb+lib/snap_diff/**(minus the files 3.0 deletes with the v1 trees) for two things: arequireof a v1 tree path, and a reference toCapybara::Screenshot/CapybaraScreenshotDiff. Reportsfile:line: reason -- code, not a count. Comments are ignored (history, not dependency); strings are not — a user-facing message naming an accessor that 3.0 deletes starts lying the day it does.Red on master, 64 offenders (
7a0bbc6seeds them all so the gate lands green; every later commit only takes entries away):Final allowlist: EMPTY.
A second sub-test fails on allowlist entries that no longer match a real line, so the list cannot rot into cover for future regressions. (It caught two of my own shrink steps mid-PR.)
2.
AVAILABLE_DRIVERS— canonical home isSnapDiff::Drivers(065c8f4)SnapDiff::Drivers.availableis documented canonical API (docs/snapdiff.md), but the value it read was defined only inconfig_legacy.rb. Proof it was already broken standalone, on master:SnapDiff::Drivers.detect_available, running atdrivers.rbload;SnapDiff::Utils.detect_available_driversone-lines into it, so the documented Utils name andtest/unit/drivers/utils_test.rbare untouched.config_legacykeepsAVAILABLE_DRIVERSas an eager same-object alias. Both consumers named in the brief still work:test_helperreads it at boot (unchanged), andnamespace_forwarding_test'sassert_samestill passes.image_compare_testnow stubsSnapDiff::Drivers::AVAILABLE_DRIVERS. Stubbing the legacy alias would only rebind the alias — that is precisely what cutting the edge means.test/unit/drivers_test.rbpins all three: standalone-loadable in a fresh subprocess (assertingconfig_legacy.rbis absent from$LOADED_FEATURES), live constant read so it stays stubbable, and both detection names agreeing.drivers.rbrequiresutilsat the bottom (utils requires drivers back), so either file can be required first.3. Core config reads →
SnapDiff.config(7d4a469)25 reads across 11 files went through the legacy view of storage that has lived in
SnapDiff::Configsince #230. Behaviour-preserving; the legacy delegators stay for users.Two follow-ons worth noting:
browser_helpersdropped itsrespond_to?(:window_size)guard — that existed because themattr_accessormight not be installed yet;Configalways has the attribute.fail_if_newerror message now saysSnapDiff.config.fail_if_new = false(it named an accessor 3.0 deletes).Tests that stubbed the legacy accessors now stub
SnapDiff.config(17 sites + one relativeScreenshot.stub). A delegator write still reaches storage, but a stubbed delegator method does not — those stubs were silently no-ops against the new reads, which is how they surfaced. The legacy accessors keep their own coverage insnap_diff_config_test.rb(every mapped writer) andconfig_default_timing_test.rb.4. Backward requires (
3cd2c18,9f310ca)All six repointed.
dsl.rb,reporters/html.rbandscreenshot_matcher.rbwere pulling dependencies through v1 forwarder files whose only content is a require of thesnap_diff/*unit they actually wanted.snap_diff.rb's two were load-bearing for a different reason — they were how barerequire "snap_diff"got the v1 surface — so they came with a consolidation:lib/snap_diff/legacy_shims.rbis now the single file holding the v1 surface as code. It gainedCONFIG_MAPPING+ the accessor generator (fromconfig.rb), the derived forwarders (Screenshot.active?,Diff.configure/.compare/.default_options, fromconfig_legacy.rb),SnapDiff.start, and the eagerAVAILABLE_DRIVERS/Comparisonaliases.config_legacy.rbandimage_compare.rbare now requires only.That closes a defect the naive fix would have left:
config.rbdefined theCapybara::Screenshot::Diffmodule skeleton soMAPPINGcould name it. After a 3.0git rmthe core would still have defined a phantom v1 namespace, so adopters'defined?(Capybara::Screenshot::Diff)feature detection would keep passing against a gem that no longer has it.Confignow declaresConfig::SETTINGSand names nothing legacy. A new test pinsLegacyShims::CONFIG_MAPPING.keys == Config::SETTINGS, so the split cannot drift into storage with no accessor (or an accessor delegating to a setting that does not exist).Behaviour verified unchanged, including the awkward part — bare
require "snap_diff"in a fresh process still answers:5. Alias-only gate: a real single-expression forwarder rule (
d76d8e7,9a00ef9)The rule was
body.include?("SnapDiff")plus a followingend, which waved through anything that merely mentioned the canonical namespace.Mutation-checked with exactly the two shapes from the brief, plus a genuine forwarder as a false-positive check:
def x/SnapDiff.config.enabled ? :on : :off/endinclude?("SnapDiff") == true)is not a single-expression forwarder into SnapDiffdef x; SnapDiff.config.enabled; File.write("x","y"); endputs more than one statement on a linedef x/SnapDiff.config.enabled/endA semicolon is now never alias-shaped (it is how several statements — or a whole
def— hide inside one "line"), and a forwarder body must match the whole ofFORWARDER_BODY: one method-call chain rooted atSnapDiff, at most one argument list and one block. Nothing in the v1 trees has adefleft, so the allowlist there stays empty.6. The 3.0 dry run — this is the important output
Scratch copy of the branch,
rm -rf lib/capybara lib/capybara-screenshot-diff.rb lib/capybara_screenshot_diff lib/capybara_screenshot_diff.rb lib/snap_diff/legacy_shims.rb lib/snap_diff/deprecation.rb. Not committed, nogit rmon the branch.The gem needs exactly two edits, then it fully works
lib/snap_diff.rb: droprequire "snap_diff/legacy_shims"(the one line in that file 3.0 removes — it is commented as such).lib/snap_diff-capybara.rb:require "capybara_screenshot_diff/minitest"→require "snap_diff/integrations/minitest". Left alone here on purpose:support_load_probe_testrequires this entry to keep theCapybaraScreenshotDiffsession surface, so repointing it now is a v2 compat break.(The gemspec was a third blocker — it required
capybara/screenshot/diff/versionat build time, sogem buildwould have failed. One line, in scope, fixed in37f63b5. Verified: gemspec loads in the deleted tree and reports2.0.0.beta3.)After those two edits, in a fresh process with the v1 trees gone:
Remaining 3.0 work
A.
SnapDiff.startis dropped in 3.0 — decided. It yields the two legacy holders, so it cannot outlive them; it now lives inlegacy_shims.rband disappears with it, leavingSnapDiff.configureas the single entry point.Reshaping it into a single-yield
startwas considered and rejected:screenshot_enabledandenabledare different settings (active?isscreenshot_enabled || (screenshot_enabled.nil? && enabled)), soscreenshot.enabled = falseagainst a one-object yield would silently land on the other setting — same syntax, different meaning. Removal in a major is honest; a silent semantic swap is not.SnapDiff.silence_deprecationsgoes the same way withdeprecation.rb.B. The test suite needs a mechanical rewrite. No gem defects behind it — every failure is a test naming something 3.0 deleted. Sequenced list, from driving it to a running suite in the scratch copy:
namespace_forwarding_test,legacy_namespace_deprecation_test,errors_alias_test,legacy_tree_is_alias_only_test,snap_diff_deprecation_test. (Andcore_tree_has_no_legacy_deps_test'sDELETED_IN_3_0list becomes moot — the gate simplifies to "all oflib/".)test_helper.rb(legacy require + 7 config accessors +AVAILABLE_DRIVERS),support/driver_coverage.rb,support/capybara_screenshot_diff/dsl_stub.rb,support/non_minitest_assertions.rb— all define or consumeCapybaraScreenshotDiff::*.module Capybara::Screenshot(::Diff).CapybaraScreenshotDiff.reset/.registry/.reporters/.reporters_mutex/.assertions/.verify/ … →SnapDiff.reset/SnapDiff.session.*/SnapDiff::Reporting.*.CapybaraScreenshotDiff::{ExpectationNotMet,UnstableImage,WindowSizeMismatchError},CapybaraScreenshotDiff::Minitest::Assertions,Capybara::Screenshot::Diff::{ImageCompare,Difference,VERSION,…}, plus old require paths (snap_diff/difference→comparison_result,snap_diff/image_compare→comparison,snap_diff/drivers/base_driver→driver,capybara_screenshot_diff/static→snap_diff/static).config_default_timing_testandsupport_load_probe_testboth enumerate legacy entry points explicitly (LEGACY_ENTRY_POINTS, thecapybara*PROBE_ENTRYcases) — those rows go.snap_diff_config_test'sCONFIG_MAPPINGhalf goes withlegacy_shims.rb; theConfig::SETTINGShalf stays.After steps 1–5 the suite runs (452 runs) with 17 failures / 24 errors, and every one is in category 6 or 7 — i.e. a test still asserting the v1 surface exists.
Review round: three holes and a live regression
Adversarial review found one shipped regression and two gates that did not bite. All fixed on this branch, each red→green.
Capybara::Screenshot::Diff::VERSIONraised NameError on six entry points (ac2ae68)Live today, silent, and it includes a legacy entry point. The constant was assigned by
capybara/screenshot/diff/version.rb; §4 stopped the core requiring that forwarder, andlegacy_shimsdeliberately omitsVERSIONfrom itsconst_missingmap (it is one of the eager exceptions), so nothing filled the gap.defined?just returned nil.Fix: assign it in
legacy_shims.rbbeside the other eager aliases — that file is on every entry point's require path, so it is the only place the eager exceptions can actually be eager.version.rbdrops the assignment (a second one is a duplicate-constant warning, not a safety net).Why the suite missed it:
EAGER_USER_FACINGwas probed only for the four legacy entry points, andVERSIONwas not in the list at all. NewEAGER_EVERYWHEREprobe covers all 15 entry points — includingcapybara_screenshot_diff/dsl, which appears in neither existing list, which is precisely why it was the legacy entry that lostVERSIONunnoticed. Mutation: removing the alias turns it red naming all six.Reverse gate missed
require_relativeescapes (84b525e)LEGACY_REQUIREanchored on the opening quote, so this passed the gate and really loaded the v1 file (confirmed via$LOADED_FEATURES):Now allows an optional
(\.{1,2}/)*. Mutation-checked with that exact line: silent before,snap_diff/region.rb:119: requires a v1 tree pathafter.Alias-only forwarder rule still accepted arbitrary code (
9a00ef9)§5 closed ternaries and semicolons but left
(.*)and{ .* }unbounded. Both of these passed as "forwarders":SnapDiff.config.x(File.exist?("/etc/passwd") ? raise("boom") : ENV.fetch("HOME"))SnapDiff.config.tap { |c| File.write("/tmp/pwned", c.inspect); exit 1 }The block case also dodged the semicolon check, because the walk does
index += 3past a def's body line and never re-examines it — that scan now runs over every line up front.An argument list is now names, commas, splats, keyword colons, or Ruby's
...forwarding. No parens (no nested call), no.(no bare receiver call), no?/quote/=(no conditional, literal or assignment). The block form is gone entirely.False-positive check: three genuine forwarders still pass — a bare chain, an argument pass-through, and
CapybaraScreenshotDiff.serve(...), which the first draft of the rule did reject and which is why...is spelled out explicitly.Smaller fixes
legacy_shims.rbheader claimedAVAILABLE_DRIVERSis aliased inconfig_legacy.rb(it is aliased in that file itself) and thatVERSION/Comparisonare defined by their own forwarders (nothing requires those anymore). Both corrected.Errno::ENOENT; it now fails cleanly.docs/UPGRADING.md(c50d197) flags the two moves that fail silently for downstream suites: stubbing the legacyAVAILABLE_DRIVERSnow only rebinds an alias (a test stubbing it to[]passes for the wrong reason), andSnapDiff::Config::MAPPINGvanishing mid-beta.Verification
rake test:unite1c5962: 529/0)rake teste1c5962: 557/0/1)standardrb lib testmise x ruby@4.0.6Mutations run, each reverted by targeted edit:
SNAP_DIFF_MUTATION = Capybara::Screenshot::Diffadded tolib/snap_diff/region.rb→ reverse gate red:snap_diff/region.rb:120: references a v1 namespace constant.AVAILABLE_DRIVERSaliased as.dup.freeze→namespace_forwarding_testred (oid=1184vsoid=1192); alias removed entirely →test_helper.rb:57red at boot. Both named consumers bite.require_relative "../capybara/screenshot/diff/version"in a core file -> reverse gate red (silent before the fix).VERSION = SnapDiff::VERSIONremoved from legacy_shims -> the new entry-point guard red, naming all six.Do not merge.
🤖 Generated with Claude Code