test: migrate suite to SnapDiff canonical names - #221
Conversation
v2 step 8 (ADR-004): the test suite now exercises the canonical SnapDiff:: names everywhere except tests whose purpose IS the legacy surface. - Migrate every deprecated old-namespace constant reference in test/ to its SnapDiff:: equivalent (ImageCompare -> Comparison, Difference -> ComparisonResult, SnapManager, ScreenshotAssertion, Vcs, StableScreenshoter, Screenshoter, BrowserHelpers, Reporters::HTML, Snap, DSL, ...). - Replace the suite-wide SnapDiff.silence_deprecations = true (from #218) with the opposite guard: a Warning hook in test_helper that RAISES on any [snap_diff deprecation] warning, so accidental old-name use inside the suite fails loud at the resolution site. Legacy-surface tests opt out per test: namespace_forwarding_test silences deprecations in its own setup; legacy_namespace_deprecation_test and snap_diff_deprecation_test flag their captured warnings as expected (module flag, not thread-local, so warnings from spawned threads are covered too). - Deliberate legacy keeps, each scoped and documented in place: namespace_forwarding_test / legacy_namespace_deprecation_test / support_load_probe_test (their purpose), snap_diff_test's alias-pinning test, one static_test case pinning the CapybaraScreenshotDiff.serve forwarder, the legacy entry-point requires in the rspec fixtures, and system_test_case's 'include Capybara::Screenshot::Diff' v1 mixin path. rake test:unit 488/0/0 and rake test 521/0/6 -- unchanged run counts, zero deprecation output. Mutation-checked: reintroducing one old constant in a migrated file makes the guard raise.
Reviewer's GuideMigrates the test suite from legacy CapybaraScreenshotDiff/Capybara::Screenshot::Diff constant names to the canonical SnapDiff:: API, and replaces the prior global deprecation silencing with an explicit guard that raises on unexpected shim usage, while keeping a handful of targeted legacy-surface tests and entry points intact and documented. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 18 minutes 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 (27)
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 |
…napDiff homes (ADR-008 steps 4+5) (#227) * refactor: move reporters/default.rb to its snap_diff home (mv only) Pure git mv so the rename survives with history; the rewrap to SnapDiff::Reporters::Default and the old-path forwarder land in the next commit. * refactor: default reporter becomes SnapDiff::Reporters::Default (ADR-008 step 4) The last old-namespace CLASS dependency in the core: comparison.rb now requires and constructs SnapDiff::Reporters::Default. The old constant Capybara::Screenshot::Diff::Reporters::Default joins the legacy_shims mapping (lazy, warn-once, same-object -- pinned by the MAPPING-driven forwarding and deprecation tests, now 30 pairs). The old file path stays requirable as a forwarder. Test edits are canonical-name migrations only (per the #221 policy -- the raise-on-deprecation guard would trip on the old name otherwise): default_test.rb and image_compare_test.rb reference SnapDiff::Reporters::Default; no expectations changed. * refactor: images-holder struct becomes SnapDiff::Comparison::Images (ADR-008 step 5a) Resolves the Comparison name collision: the images-holder struct that lived at Capybara::Screenshot::Diff::Comparison (blocked from a SnapDiff home because SnapDiff::Comparison is the comparator class) is now nested inside the comparator as SnapDiff::Comparison::Images. The old constant joins the legacy_shims mapping (lazy, warn-once, same-object -- 31 pairs now). comparison.rb constructs Images directly; no old-namespace class reference remains in the compare path. Test edits are canonical-name migrations only (per the #221 policy; the raise-on-deprecation guard would otherwise trip): the struct constructions in driver_contract_tests, image_preprocessor_test, annotation_service_test and default_test now spell the canonical name. No expectations changed. * refactor: driver registry gets its SnapDiff home (ADR-008 step 5b) SnapDiff::Drivers.loaded is now the canonical driver-class cache (ex Capybara::Screenshot::Diff::LOADED_DRIVERS, which comparison.rb no longer defines); the old constant stays as an EAGER same-object alias in legacy_shims because user code registers custom drivers by mutating the hash in place -- a lazy copy would silently drop registrations, and warning on a supported surface would be noise. SnapDiff::Drivers.available is the canonical reader for the detected drivers list; the value itself stays on the eager Capybara::Screenshot::Diff::AVAILABLE_DRIVERS constant (config_legacy detection moment unchanged, and image_compare_test pins that constant as the stub point for the no-drivers error path). Utils' find_driver_class_for now reads both registries through the canonical accessors, removing the last old-namespace reads from the core compare path. Guard tests pin same-object identity and legacy-write -> canonical-read visibility.
v2 step 8 (ADR-004): the test suite now exercises the canonical
SnapDiff::names everywhere except tests whose purpose IS the legacy surface. Test-only change — nolib/edits.What changed
1. Old-name references migrated to
SnapDiff::equivalents (26 test files):ImageCompare→Comparison,Difference→ComparisonResult,SnapManager,ScreenshotAssertion,Vcs,StableScreenshoter,Screenshoter,BrowserHelpers,Reporters::HTML,Snap,DSL, plus theScreenshoterStubsuperclass. Test-class nesting under the old modules was kept (references migrated, classes not renamed, per proportionate-churn).2. Suite-wide deprecation silencing replaced with a loud guard. #218's global
SnapDiff.silence_deprecations = trueis gone. Insteadtest_helperinstalls aWarning.extendhook that raises on any[snap_diff deprecation]warning, so accidental old-name use inside the suite fails at the resolution site. Legacy-surface tests opt out per test:namespace_forwarding_testsilences deprecations in its own setup/teardown (it resolves every mapped old name on purpose);legacy_namespace_deprecation_test/snap_diff_deprecation_testsetSnapDiffDeprecationGuard.expectedaround the warnings they capture and assert on (a module flag, not a thread-local, so warnings emitted from threads a test spawns are covered — the deprecation thread-safety test needs exactly that).3. Deliberate legacy keeps (each scoped + documented in place):
test/unit/namespace_forwarding_test.rb,test/unit/legacy_namespace_deprecation_test.rb,test/unit/support_load_probe_test.rb— their purpose is the legacy surface.test/unit/snap_diff_test.rb— the alias-pinning test ("SnapDiff::Comparison aliases …ImageCompare") keeps the old name with local silencing; its sibling assertions migrated.test/unit/static_test.rb— one of three cases now pins the legacyCapybaraScreenshotDiff.serveforwarder; the other two use canonicalSnapDiff.serve.test/fixtures/rspec_*.rb— keeprequire "capybara_screenshot_diff/rspec"(legacy entry point under test); incidentalSnapManagerrefs migrated.test/system_test_case.rb— keepsinclude Capybara::Screenshot::Diff(the v1 mixin path) andinclude CapybaraScreenshotDiff::Minitest::Assertions, so the integration suite still exercises the legacy DSL entry end-to-end.Capybara::Screenshot/Diffmattr config accessors (storage of record perSnapDiff::Configdelegation),Capybara::Screenshot::Diff::AVAILABLE_DRIVERS,CapybaraScreenshotDiffsession surface (reset/verify/registry/…) and error classes (ExpectationNotMetetc.), top-levelRegion,Capybara::Screenshot::Diff::Reporters::Default.Shim-coverage gaps checked: the legacy DSL mixin (
include Capybara::Screenshot::Diff) stays covered viasystem_test_case; the legacy rspec/minitest entry points via the fixtures and entry-point probes;TestMethodsdoes not exist in lib — no new tests needed. Finding (out of scope for this test-only PR): the session surface (reset/verify/registry) and error classes exist only onCapybaraScreenshotDiff, with noSnapDiffequivalent yet.Evidence
rake test:unit488 runs / 0 failures / 0 errors / 0 skips;rake test521 runs / 0 failures / 6 skips — unchanged run counts, zero deprecation output on stderr.Capybara::Screenshot::Diff::Vcsin migrateddsl_test.rb→RuntimeError: old-namespace constant resolved inside the test suite …raised from the guard (then restored).standardrb test/— 62 files, no offenses.🤖 Generated with Claude Code
Summary by Sourcery
Make the test suite validate canonical SnapDiff APIs and fail loudly on unintended legacy namespace usage.
Enhancements:
SnapDiff::names while retaining explicitly scoped coverage for legacy namespaces and entry points.Tests: