refactor: split canonical vs legacy-surface tests (3.0 readiness) - #236
Merged
Conversation
…anonical
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).
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).
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
…inning
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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @pftg, your pull request is larger than the review limit of 150000 diff characters
Contributor
Reviewer's GuideRefactors the test suite to clearly separate legacy v1-surface coverage from canonical SnapDiff coverage via a new test/legacy directory and a test:canonical rake task, while repointing most tests and support code to canonical SnapDiff APIs and duplicating any unique canonical coverage that was previously embedded in legacy-focused tests. Flow diagram for canonical and legacy test scopesflowchart TD
all["test/**/*_test.rb"] --> test[rake test]
all --> canonicalFilter["FileList exclude test/legacy"]
canonicalFilter --> canonical[rake test:canonical]
unitFiles["test/unit/**/*_test.rb"] --> unit[rake test:unit]
legacy["test/legacy/**/*_test.rb"] --> unit
legacy -.-> deletion["3.0 deletion: remove v1 surface and test/legacy"]
canonical --> gate["3.0 gate"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (70)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe test suite now separates canonical SnapDiff tests from legacy compatibility tests. Test support, integrations, fixtures, configuration checks, namespaces, reporters, sessions, and error assertions use canonical SnapDiff APIs. ChangesSnapDiff test migration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This test-only reorganization adds a canonical gate while preserving the existing suites; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
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.
This was referenced Aug 23, 2026
Merged
pftg
added a commit
that referenced
this pull request
Aug 23, 2026
`rake test:canonical` is defined as "exactly what must still pass once test/legacy/ and the v1 trees are gone". Three times in one day a test asserting LEGACY behaviour was written into test/unit/, i.e. into that suite: a canonical surface table demanding the shim-only SnapDiff.start (#236), three legacy-constant probes in a canonical file (#237), and a pre-existing umbrella guard #236 had to relocate. Each would have failed the day the deletion landed, long after its author moved on. Reviews caught all three; the fourth would ship. The test-tree twin of core_tree_has_no_legacy_deps_test.rb: no file under test/unit/ or test/integration/ may require a doomed path, name a v1 namespace constant, or use a shim-only name (SnapDiff.start, .silence_deprecations, SnapDiff::Deprecation, suppress_migration_notice!). test/legacy/ is deliberately not policed -- exercising the legacy surface is its job. Same conventions as the twin: file:line: reason -- `code`, whole-line comments ignored, a vacuity guard, and a sub-test that fails on stale allowlist entries. The allowlist holds two entries, both gates rather than tests of behaviour (deletion_3_0_test.rb, which names the deletion set by construction, and the twin gate's own pattern literal). A third entry means canonical tests are still entangled and needs a decision, not a green build. This file cannot scan itself: a line-level allowlist has to quote the lines it blesses, and every quote is itself an offence -- no fixed point exists.
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.
#235 proved the gem survives the 3.0 deletion with two edits. The suite did not: ~17F/24E, every one a test still asserting the v1 surface exists.
This PR makes the split mechanical. No version or CHANGELOG changes; no
lib/changes at all — the diff isRakefile+test/.The mechanism: a directory, not a list
test/legacy/holds every test whose subject is the v1 compatibility surface. They stay and stay green for the whole 2.x line; in 3.0 they are deleted by the same commit that deletes what they test:rake testrake test:canonicaltest/legacy— the 3.0 gaterake test:unittest/unit+test/legacy— the release gate keeps its coverageWhy a directory over a
LEGACY_SURFACE_TESTSlist in the Rakefile: nothing to keep in sync (a list is a second place to edit, and a stale entry fails only at 3.0), the exclusion is oneFileList#exclude, and the deletion is onegit rm -rinstead of five paths a stranger has to look up.test/legacyis intest:uniton purpose:legacy/marks lifetime, not kind — these are unit-sized tests andrelease.ymlrunsrake test:unit.The money shot — scratch copy, v1 trees +
test/legacydeleted, the 2 known gem edits appliedGreen. Identical to
rake test:canonicalon this branch with everything present (457/1288/0F/0E/1S), which is the point: the task is a deletion, not a fix-up.The gate runs first on purpose — it is the check whose absence produced the wrong number the first time.
Two prerequisites for reproducing it, both learned the hard way:
BUNDLE_GEMFILEmust point at the scratch copy's owngems.rb.gems.rbhasgemspec path: __dir__, so bundler unshifts that gemfile's directory onto$LOAD_PATH— ahead of-Ilib. Point it at the original checkout and every subprocess probe silently loads the un-deleted gem.vcs_testshells out togit show HEAD:(lib/snap_diff/vcs.rb:10), so a plain copy fails there for reasons unrelated to 3.0.How the first money shot lied
I published
458 runs … 0 failures. Independent review got 1 failure on the same counts. The review was right about the defect and understated the harness bug: myBUNDLE_GEMFILEpointed at the worktree'sgems.rb, so bundler put the worktree'slibahead of-Ilib. Proved directly, from inside the scratch copy:So it was not "0 where it should have been 1" — the deletion was never exercised at all. Every subprocess probe in that run measured the intact tree. A run that cannot distinguish the deleted tree from the intact one is not evidence, and I published it as the evidence base for the 3.0 decision. The gate line now in the transcript exists so that failure mode announces itself.
The real defect it was hiding
CANONICAL_SURFACElistedstart, applied to all 7 canonical entry points — ande789c8awidened it by addingsnap_diff-capybara.SnapDiff.startis defined only atlib/snap_diff/legacy_shims.rb:169and yields the two v1 config holders, so it cannot outlive them (#235 decided this); this very PR body files it under "safe to lose at 3.0". A canonical gate demanding a method 3.0 deletes goes red the day the deletion lands.Fix: drop
startfromCANONICAL_SURFACE. Mutation — putting it back, in the deleted tree — reproduces the reviewer's failure exactly:.startloses nothing:legacy_forwarders_testpins what it yields and that it applies a setting, and a new per-entry-point probe inlegacy_entry_point_probe_testkeeps the availability claim the canonical gate used to make — for the entries that actually keep it.Per-category counts
test/legacy/test_helper.rb,system_test_case.rb+ 7 support filesmodule Capybara::Screenshot(::Diff)/CapybaraScreenshotDiffCategory 4 is 270, not ~330: the ~330 figure counted every
CapybaraScreenshotDifftoken, including the 33 module wrappers that category 3 removed outright.Category 2: the support stubs stop reopening gem namespaces
CapybaraScreenshotDiff::DSLStub,::DriverCoverage,::NonMinitest,Capybara::Screenshot::ScreenshoterStub,Capybara::Screenshot::Diff::TestDoublesare all test scaffolding. They now sit at top level (DSLStub,DriverCoverage, …) andtest/support/capybara_screenshot_diff/dsl_stub.rbmoves totest/support/dsl_stub.rb. Defining a namespace 3.0 deletes, from a test support file, would have left a phantomCapybaraScreenshotDiffalive in the suite after the gem stopped shipping one.Legacy-surface tests now
require "capybara_screenshot_diff"themselves — the require goes with the file.What a blind repoint would have broken
Five claims would have become tautologies (
assert_same X, X) or quietly changed meaning. Each was a forwarder-identity claim about the v1 view, and each is preserved verbatim intest/legacy/:assert_same SnapDiff.session, CapybaraScreenshotDiff.registryassert_same SnapDiff.session, SnapDiff.sessionassert_same SnapDiff::Reporting.reporters, CapybaraScreenshotDiff.reportersassert_same SnapDiff::Comparison, ...::ImageCompareassert_same SnapDiff::Comparison, SnapDiff::ComparisonCapybara::Screenshot::Diff.fail_if_new = truethen read viaconfigEAGER_USER_FACING = %w[Capybara::Screenshot::Diff::Reporters::Default ...]Found with a scan for assertions whose two sides became textually identical, plus a read of every test whose name mentions legacy / old / mattr / alias / forwards. Four self-comparisons remain and are all genuine memoization checks (
SnapDiff.session,Reporting.mutex,SnapManager.instance,Drivers.loaded); the first three were self-comparing before this PR, the fourth is the newly-ported.loadedtest below.The rspec fixtures were stubbing
Capybara::Screenshot::Diff.pending_if_new, which the core stopped reading in #235 — a silently no-op stub. NowSnapDiff.config.Unique canonical coverage found inside legacy-surface tests (and ported)
Audit rule: for every assertion moving to
test/legacy/, if this file vanished at 3.0, would any canonical behaviour become untested? Two hits. Both are duplicated, not moved — the v1 originals stay put and keep guarding the v1 contract for all of 2.x.namespace_forwarding_testSnapDiff::Drivers.loadedis ONE hash mutated in place (it asserted the v1LOADED_DRIVERSis that object, and that registering through it is visible canonically).Utils.find_driver_class_forcaches through.loaded, so a copy-returning refactor breaks user driver registration silently.drivers_test— ".loaded is a single hash mutated in place, so registrations stick".loaded.dup[…] = …→ red:Expected: :probe_driver / Actual: nilon the second assertion (theassert_sameoid mismatch fires first when the mutation is on the accessor itself)support_load_probe_test's legacy rowssupport_load_probe_test— "every canonical entry point defines its advertised constants standalone", oversnap_diff/dsl,/integrations/minitest,/integrations/rspec,snap_diff-capybaraThree more gaps closed while splitting:
snap_diff-capybarajoinsCANONICAL_ENTRY_POINTS. 3.0 keeps that entry point (edit 2 repoints it), but it was covered only as a legacy entry — it would have lost all coverage.errors_alias_testwas mixed. The fourCapybaraScreenshotDiff::*alias pairs are v1 surface; "error hierarchy is preserved" and "every error the gem defines inherits SnapDiff::Error" are not. Split verbatim intotest/unit/errors_test.rb.attempts_reporter_testnow requiressnap_diff/attempts_reporter—stable_screenshoterpulls it in lazily and the v1 umbrella was what loaded it eagerly. This was the one canonical test the deletion actually broke.And one guard moved the other way, per review: "bare require never loads the umbrella" is now in
test/legacy/legacy_forwarders_test.rb. Its subject islib/capybara_screenshot_diff.rb; once 3.0 deletes that file the$LOADED_FEATURESgrep is empty by construction and the guard can never fail again. (This is the −1 that takestest:canonicalfrom 458 to 457.)Explicitly judged legacy-only, safe to lose at 3.0
const_missing/ eager-alias semantics · deprecation warn-once, silencing, caller attribution,Deprecation.reset!·LegacyShims::CONFIG_MAPPINGcompleteness and its agreement withConfig::SETTINGS· the alias-only scan oflib/capybara*· theCapybaraScreenshotDiffsession/reporter forwarders ·SnapDiff.start·Capybara::Screenshot::Diff::VERSION/::Comparisoneagerness.Every one is about a name 3.0 deletes, and every one has its canonical counterpart pinned in
test/unit/—active?'s full truth table,Config::SETTINGS's ivars,SnapDiff.configure,Reporting.register,SnapDiff.compare,SnapDiff.serve, the error hierarchy,Comparison::Images,Reporters::Default.How the mixed files were split (6 + 7)
snap_diff_config_test→+ test/legacy/legacy_config_accessors_test. Canonical keepsConfig::SETTINGS↔ivars, theactive?truth table (Config column), root coercion,configure, and screenshot_enabled/enabled independence. Legacy keepsCONFIG_MAPPINGcompleteness, the mattr round trips, and theCapybara::Screenshot.active?column.config_default_timing_test→+ test/legacy/legacy_config_default_timing_test. Canonical keeps the whole 27-value default snapshot, require-time freezing offail_if_new/root, anddefault_options[:wait]liveness — oversnap_diffandsnap_diff/integrations/minitest, read throughSnapDiff.configonly. The legacy file re-runs the canonical file's own probe scripts under the v1 entries and adds the both-surfaces-agree loop; together that is exactly whatcheck_bothasserted, with one source of truth and no drift.support_load_probe_test→+ test/legacy/legacy_entry_point_probe_test. Canonical keeps the support-file probe, the canonical surface probe, the dual-install guard, and the new advertised-constants probe. Legacy keeps the v1 advertised constants, theCapybaraScreenshotDiffsession surface across 9 entries,SnapDiff.start's per-entry availability, andEAGER_USER_FACING/EAGER_EVERYWHEREunder their old names. It reusesSupportLoadProbeTest.proberather than keeping a second copy.Verification
9822cc6)rake test:unitrake testrake test:canonicalrake test:canonicalin the deleted treestandardrb lib test Rakefilemise x ruby@4.0.6+17 runs, no assertion value changed. Attribution measured, not estimated — per-test-class counts from
rake test TESTOPTS=--verboseon9822cc6vs this branch, grouped by the split that produced them:config_default_timing+legacy_config_default_timingsnap_diff/integrations/minitest(×3) and the both-surfaces-agree probe per legacy entry (×3)snap_diff_config+legacy_config_accessorsactive?, screenshot_enabled/enabled, root coercion,configure— canonical column and legacy columnsnap_diff_test→legacy_forwarders.start×2, umbrella guard); 4 v1 forwarder claims duplicated where the canonical file kept its own version (registry,register,reporters_mutex,serve)support_load_probe+legacy_entry_point_probe.start's per-entry probe moving to the legacy sidedrivers_testDrivers.loadedcoverageerrors_alias→+ errors_testReporters::DefaultTest→DefaultReporterTestThe reverse gate (
core_tree_has_no_legacy_deps_test) and the alias-only gate (legacy_tree_is_alias_only_test, now intest/legacy/) are green throughout — bothALLOWEDlists are still empty.Do not merge.
🤖 Generated with Claude Code