From 98035e087363805c347686fdd37209b3b16e50c7 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 23 Aug 2026 00:55:56 +0200 Subject: [PATCH 1/4] 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. --- lib/{capybara/screenshot/diff => snap_diff}/reporters/default.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename lib/{capybara/screenshot/diff => snap_diff}/reporters/default.rb (100%) diff --git a/lib/capybara/screenshot/diff/reporters/default.rb b/lib/snap_diff/reporters/default.rb similarity index 100% rename from lib/capybara/screenshot/diff/reporters/default.rb rename to lib/snap_diff/reporters/default.rb From c75ec3360b989ada92a65b7b8f4b4aa2b119bbb8 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 23 Aug 2026 00:57:36 +0200 Subject: [PATCH 2/4] 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. --- docs/architecture.md | 3 +-- lib/capybara/screenshot/diff/reporters/default.rb | 7 +++++++ lib/snap_diff/comparison.rb | 4 ++-- lib/snap_diff/legacy_shims.rb | 6 ++++++ lib/snap_diff/reporters/default.rb | 4 +--- test/unit/image_compare_test.rb | 4 ++-- test/unit/namespace_forwarding_test.rb | 8 +++++--- test/unit/reporters/default_test.rb | 6 +++--- 8 files changed, 27 insertions(+), 15 deletions(-) create mode 100644 lib/capybara/screenshot/diff/reporters/default.rb diff --git a/docs/architecture.md b/docs/architecture.md index eeca6d06..96072d22 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -158,7 +158,7 @@ Drivers abstract image processing operations. Shared default behavior lives in t Handles baseline retrieval from git. Uses `git show HEAD:` to extract the committed version. Supports Git LFS via `git lfs smudge`. Returns `false` if the file doesn't exist in VCS (first-run scenario). -### 9. Reporters (`lib/capybara/screenshot/diff/reporters/default.rb`, `lib/snap_diff/reporters/html.rb`) +### 9. Reporters (`lib/snap_diff/reporters/default.rb`, `lib/snap_diff/reporters/html.rb`) **Default reporter:** Generates annotated diff images: - `image.diff.png` — new screenshot with diff region outlined in red @@ -281,7 +281,6 @@ lib/ capybara/screenshot/diff/ config_legacy.rb # mattr_accessor settings storage (source of truth) region.rb # Bounding box region value object (top-level Region) - reporters/default.rb # Default annotated-image reporter version.rb # Capybara::Screenshot::Diff::VERSION (gemspec reads it) ... # Everything else forwards to snap_diff/ ``` diff --git a/lib/capybara/screenshot/diff/reporters/default.rb b/lib/capybara/screenshot/diff/reporters/default.rb new file mode 100644 index 00000000..0d890b32 --- /dev/null +++ b/lib/capybara/screenshot/diff/reporters/default.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +# Forwarder (ADR-008 step 4): the default reporter lives at +# SnapDiff::Reporters::Default; the old name now resolves lazily via +# snap_diff/legacy_shims' const_missing, with a deprecation warning. +require "snap_diff/reporters/default" +require "snap_diff/legacy_shims" diff --git a/lib/snap_diff/comparison.rb b/lib/snap_diff/comparison.rb index 4e93cfbb..0fde3cde 100644 --- a/lib/snap_diff/comparison.rb +++ b/lib/snap_diff/comparison.rb @@ -6,7 +6,7 @@ require "snap_diff/comparison_result" require "snap_diff/drivers" require "snap_diff/image_preprocessor" -require "capybara/screenshot/diff/reporters/default" +require "snap_diff/reporters/default" # The internal images-holder struct and the driver cache keep their legacy # Capybara::Screenshot::Diff homes for now: SnapDiff::Comparison is the @@ -195,7 +195,7 @@ def difference=(new_difference) def build_reporter current_difference = difference || build_null_difference - Capybara::Screenshot::Diff::Reporters::Default.new(current_difference) + Reporters::Default.new(current_difference) end # Loads and preprocesses images for detailed comparison. diff --git a/lib/snap_diff/legacy_shims.rb b/lib/snap_diff/legacy_shims.rb index 6e6d86dd..b4fa6637 100644 --- a/lib/snap_diff/legacy_shims.rb +++ b/lib/snap_diff/legacy_shims.rb @@ -47,6 +47,8 @@ def self.install(namespace, old_prefix, mapping) module Capybara module Screenshot module Diff + module Reporters + end end end end @@ -74,6 +76,10 @@ module Reporters Difference: "SnapDiff::ComparisonResult" }.freeze) +SnapDiff::LegacyShims.install(Capybara::Screenshot::Diff::Reporters, "Capybara::Screenshot::Diff::Reporters", { + Default: "SnapDiff::Reporters::Default" +}.freeze) + SnapDiff::LegacyShims.install(CapybaraScreenshotDiff, "CapybaraScreenshotDiff", { RED_RGBA: "SnapDiff::RED_RGBA", ORANGE_RGBA: "SnapDiff::ORANGE_RGBA", diff --git a/lib/snap_diff/reporters/default.rb b/lib/snap_diff/reporters/default.rb index 89254c2a..668b2e4e 100644 --- a/lib/snap_diff/reporters/default.rb +++ b/lib/snap_diff/reporters/default.rb @@ -1,10 +1,8 @@ # frozen_string_literal: true require "snap_diff/annotation_service" -# Defines the Capybara::Screenshot::Diff namespace this file reopens. -require "snap_diff/legacy_shims" -module Capybara::Screenshot::Diff +module SnapDiff module Reporters class Default attr_reader :difference diff --git a/test/unit/image_compare_test.rb b/test/unit/image_compare_test.rb index c0ff8e27..d90826dc 100644 --- a/test/unit/image_compare_test.rb +++ b/test/unit/image_compare_test.rb @@ -207,7 +207,7 @@ class ImageCompareRefactorTest < ActiveSupport::TestCase comparison.processed assert_predicate comparison, :dimensions_changed? - assert_kind_of Reporters::Default, comparison.reporter + assert_kind_of SnapDiff::Reporters::Default, comparison.reporter end test "#dimensions_changed? returns false when images have same dimensions" do @@ -220,7 +220,7 @@ class ImageCompareRefactorTest < ActiveSupport::TestCase # Test reporter configuration test "#reporter returns Default reporter by default" do comparison = make_comparison(:a, :a) - assert_kind_of Reporters::Default, comparison.reporter + assert_kind_of SnapDiff::Reporters::Default, comparison.reporter end end end diff --git a/test/unit/namespace_forwarding_test.rb b/test/unit/namespace_forwarding_test.rb index a0ced252..221ff1c5 100644 --- a/test/unit/namespace_forwarding_test.rb +++ b/test/unit/namespace_forwarding_test.rb @@ -49,6 +49,7 @@ class NamespaceForwardingTest < ActiveSupport::TestCase "Capybara::Screenshot::Diff::Drivers::BaseDriver" => "SnapDiff::Driver", "Capybara::Screenshot::Diff::Drivers::ChunkyPNGDriver" => "SnapDiff::Drivers::ChunkyPNGDriver", "Capybara::Screenshot::Diff::Drivers::VipsDriver" => "SnapDiff::Drivers::VipsDriver", + "Capybara::Screenshot::Diff::Reporters::Default" => "SnapDiff::Reporters::Default", "Capybara::Screenshot::Diff::ImageCompare" => "SnapDiff::Comparison", "Capybara::Screenshot::Diff::Difference" => "SnapDiff::ComparisonResult", "CapybaraScreenshotDiff::RED_RGBA" => "SnapDiff::RED_RGBA", @@ -58,7 +59,7 @@ class NamespaceForwardingTest < ActiveSupport::TestCase # Explicit requires: a dedicated forwarder-identity test shouldn't rely # on incidental transitive loads from other test files (or on rake's # file-load order within a single process) to make every one of these - # 29 constants resolvable. Most of these are already pulled in by + # constants resolvable. Most of these are already pulled in by # test_helper's own "capybara_screenshot_diff/minitest" require; listed # here anyway so this file passes standalone. require "capybara/screenshot/diff/os" @@ -80,6 +81,7 @@ class NamespaceForwardingTest < ActiveSupport::TestCase require "capybara_screenshot_diff/error_with_filtered_backtrace" require "capybara_screenshot_diff/reporters/html" require "capybara_screenshot_diff/screenshot_assertion" + require "capybara/screenshot/diff/reporters/default" require "capybara/screenshot/diff/drivers" require "capybara/screenshot/diff/drivers/base_driver" require "capybara/screenshot/diff/drivers/chunky_png_driver" @@ -105,7 +107,7 @@ class NamespaceForwardingTest < ActiveSupport::TestCase end end - test "MAPPING covers all 29 documented forwarders" do - assert_equal 29, MAPPING.size + test "MAPPING covers all 30 documented forwarders" do + assert_equal 30, MAPPING.size end end diff --git a/test/unit/reporters/default_test.rb b/test/unit/reporters/default_test.rb index b749b308..bbd9d43e 100644 --- a/test/unit/reporters/default_test.rb +++ b/test/unit/reporters/default_test.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "test_helper" -require "capybara/screenshot/diff/reporters/default" +require "snap_diff/reporters/default" require "capybara/screenshot/diff/drivers/vips_driver" if defined?(Vips) @@ -19,7 +19,7 @@ class Reporters::DefaultTest < ActiveSupport::TestCase test "for vips driver generates heatmap diff file" do driver = SnapDiff::Drivers::VipsDriver.new comparison = build_comparison_for(driver, "a.png", "b.png") - reporter = Reporters::Default.new(driver.find_difference_region(comparison)) + reporter = SnapDiff::Reporters::Default.new(driver.find_difference_region(comparison)) reporter.generate @@ -29,7 +29,7 @@ class Reporters::DefaultTest < ActiveSupport::TestCase test "#clean_tmp_files removes heatmap diff along with other diff artifacts" do driver = SnapDiff::Drivers::VipsDriver.new comparison = build_comparison_for(driver, "a.png", "b.png") - reporter = Reporters::Default.new(driver.find_difference_region(comparison)) + reporter = SnapDiff::Reporters::Default.new(driver.find_difference_region(comparison)) reporter.generate assert_predicate reporter.heatmap_diff_path, :exist? From b8ec193dda33e7fb5f2a7b7138459f1743bf0b52 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 23 Aug 2026 01:00:11 +0200 Subject: [PATCH 3/4] 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. --- lib/snap_diff/comparison.rb | 33 ++++++++++++-------------- lib/snap_diff/legacy_shims.rb | 1 + test/support/driver_contract_tests.rb | 8 +++---- test/unit/annotation_service_test.rb | 6 ++--- test/unit/image_preprocessor_test.rb | 8 +++---- test/unit/namespace_forwarding_test.rb | 5 ++-- test/unit/reporters/default_test.rb | 2 +- 7 files changed, 31 insertions(+), 32 deletions(-) diff --git a/lib/snap_diff/comparison.rb b/lib/snap_diff/comparison.rb index 0fde3cde..ccebd6c2 100644 --- a/lib/snap_diff/comparison.rb +++ b/lib/snap_diff/comparison.rb @@ -8,23 +8,12 @@ require "snap_diff/image_preprocessor" require "snap_diff/reporters/default" -# The internal images-holder struct and the driver cache keep their legacy -# Capybara::Screenshot::Diff homes for now: SnapDiff::Comparison is the -# comparison class below (ex-ImageCompare), so the struct cannot take the -# same name. Its SnapDiff home arrives only when it is folded into -# Comparison as a nested value (v2 design section 2) -- renaming it here -# would exceed step 5's two approved renames. +# The driver cache keeps its legacy Capybara::Screenshot::Diff home for +# now; it moves in ADR-008 step 5b. module Capybara module Screenshot module Diff LOADED_DRIVERS = {} - - # Holds the two images (and their paths/options/driver) being compared. - class Comparison < Struct.new(:new_image, :base_image, :options, :driver, :new_image_path, :base_image_path) - def skip_area - options[:skip_area] - end - end end end end @@ -53,6 +42,14 @@ module SnapDiff # - Only performing expensive operations when absolutely necessary # - Maintaining high accuracy for complex comparisons class Comparison + # Holds the two images (and their paths/options/driver) being compared + # (ADR-008 step 5: ex-Capybara::Screenshot::Diff::Comparison struct). + Images = Struct.new(:new_image, :base_image, :options, :driver, :new_image_path, :base_image_path) do + def skip_area + options[:skip_area] + end + end + TOLERABLE_OPTIONS = [:tolerance, :color_distance_limit, :shift_distance_limit, :area_size_limit].freeze attr_reader :driver, :driver_options @@ -136,7 +133,7 @@ def without_tolerable_options? def load_images_and_build_comparison(base_path, new_path, options) base_img, new_img = driver.load_images(base_path, new_path) - Capybara::Screenshot::Diff::Comparison.new(new_img, base_img, options, driver, new_path, base_path) + Images.new(new_img, base_img, options, driver, new_path, base_path) end def image_preprocessor @@ -155,7 +152,7 @@ def find_difference(quick_mode: false) # Analyzes the comparison and determines if images are different. # - # @param comparison [Capybara::Screenshot::Diff::Comparison] The comparison object containing images to analyze. + # @param comparison [Comparison::Images] The comparison object containing images to analyze. # @param quick_mode [Boolean] When true, performs minimal checks and returns early. # In quick mode, returns [is_equal, difference] where: # - is_equal is true if images are considered equal @@ -203,7 +200,7 @@ def build_reporter # This method is responsible for: # 1. Loading both images using the configured driver # 2. Applying any necessary preprocessing (cropping, normalization) - # 3. Creating a Capybara::Screenshot::Diff::Comparison object that holds the image data + # 3. Creating a Comparison::Images object that holds the image data # # @param base_path [String,Pathname] Path to the baseline/reference image # @param new_path [String,Pathname] Path to the new/candidate image @@ -211,7 +208,7 @@ def build_reporter # - :crop [Array] Optional crop area [x, y, width, height] # - :skip_area [Array] Areas to exclude from comparison # - :tolerance [Numeric] Color tolerance threshold - # @return [Capybara::Screenshot::Diff::Comparison] Prepared comparison object ready for analysis + # @return [Comparison::Images] Prepared comparison object ready for analysis # @raise [ArgumentError] If image files are invalid or unreadable def load_comparison(base_path, new_path, options) comparison = load_images_and_build_comparison(base_path, new_path, options) @@ -219,7 +216,7 @@ def load_comparison(base_path, new_path, options) end def build_null_difference(failed_by = nil) - comparison = Capybara::Screenshot::Diff::Comparison.new(nil, nil, driver_options, driver, image_path, base_image_path).freeze + comparison = Images.new(nil, nil, driver_options, driver, image_path, base_image_path).freeze ComparisonResult.build_null(comparison, base_image_path, image_path, failed_by) end diff --git a/lib/snap_diff/legacy_shims.rb b/lib/snap_diff/legacy_shims.rb index b4fa6637..de08d1c8 100644 --- a/lib/snap_diff/legacy_shims.rb +++ b/lib/snap_diff/legacy_shims.rb @@ -73,6 +73,7 @@ module Reporters ScreenshotMatcher: "SnapDiff::ScreenshotMatcher", Drivers: "SnapDiff::Drivers", ImageCompare: "SnapDiff::Comparison", + Comparison: "SnapDiff::Comparison::Images", Difference: "SnapDiff::ComparisonResult" }.freeze) diff --git a/test/support/driver_contract_tests.rb b/test/support/driver_contract_tests.rb index b69793ed..4ebbe9d4 100644 --- a/test/support/driver_contract_tests.rb +++ b/test/support/driver_contract_tests.rb @@ -103,7 +103,7 @@ module DriverContractTests test "[contract] #same_dimension? returns true when images share dimensions" do driver = make_comparison(:a, :a).driver old_image, new_image = driver.load_images(TEST_IMAGES_DIR / "a.png", TEST_IMAGES_DIR / "b.png") - comparison = Capybara::Screenshot::Diff::Comparison.new(new_image, old_image, {}, driver) + comparison = SnapDiff::Comparison::Images.new(new_image, old_image, {}, driver) assert driver.same_dimension?(comparison) end @@ -111,7 +111,7 @@ module DriverContractTests test "[contract] #same_dimension? returns false when images differ in dimensions" do driver = make_comparison(:a, :a).driver old_image, new_image = driver.load_images(TEST_IMAGES_DIR / "a.png", TEST_IMAGES_DIR / "a_cropped.png") - comparison = Capybara::Screenshot::Diff::Comparison.new(new_image, old_image, {}, driver) + comparison = SnapDiff::Comparison::Images.new(new_image, old_image, {}, driver) assert_not driver.same_dimension?(comparison) end @@ -130,11 +130,11 @@ module DriverContractTests driver = make_comparison(:a, :a).driver old_image, new_image = driver.load_images(TEST_IMAGES_DIR / "a.png", TEST_IMAGES_DIR / "a.png") - same_comparison = Capybara::Screenshot::Diff::Comparison.new(new_image, old_image, {}, driver) + same_comparison = SnapDiff::Comparison::Images.new(new_image, old_image, {}, driver) assert driver.same_pixels?(same_comparison) other_old_image, other_new_image = driver.load_images(TEST_IMAGES_DIR / "a.png", TEST_IMAGES_DIR / "c.png") - different_comparison = Capybara::Screenshot::Diff::Comparison.new(other_new_image, other_old_image, {}, driver) + different_comparison = SnapDiff::Comparison::Images.new(other_new_image, other_old_image, {}, driver) assert_not driver.same_pixels?(different_comparison) end diff --git a/test/unit/annotation_service_test.rb b/test/unit/annotation_service_test.rb index b22c77e5..096a9614 100644 --- a/test/unit/annotation_service_test.rb +++ b/test/unit/annotation_service_test.rb @@ -52,9 +52,9 @@ class AnnotationServiceTest < ActiveSupport::TestCase new_image = driver.from_file(TEST_IMAGES_DIR.join("a.png")) base_image = driver.from_file(TEST_IMAGES_DIR.join("b.png")) - with_skip_area = Comparison.new(new_image, base_image, {skip_area: [Region.new(0, 0, 10, 10)]}, driver, + with_skip_area = SnapDiff::Comparison::Images.new(new_image, base_image, {skip_area: [Region.new(0, 0, 10, 10)]}, driver, @_tmpdir / "with_skip_area.png", @_tmpdir / "with_skip_area_base.png") - without_skip_area = Comparison.new(new_image, base_image, {}, driver, + without_skip_area = SnapDiff::Comparison::Images.new(new_image, base_image, {}, driver, @_tmpdir / "without_skip_area.png", @_tmpdir / "without_skip_area_base.png") service_with = SnapDiff::AnnotationService.new(driver.find_difference_region(with_skip_area)) @@ -72,7 +72,7 @@ def build_comparison_for(driver, *images) new_image = driver.from_file(TEST_IMAGES_DIR.join(images.first)) base_image = driver.from_file(TEST_IMAGES_DIR.join(images.last)) - Comparison.new(new_image, base_image, {}, driver, @_tmpdir / images.first, @_tmpdir / images.last) + SnapDiff::Comparison::Images.new(new_image, base_image, {}, driver, @_tmpdir / images.first, @_tmpdir / images.last) end end end diff --git a/test/unit/image_preprocessor_test.rb b/test/unit/image_preprocessor_test.rb index a0cf3045..99350e37 100644 --- a/test/unit/image_preprocessor_test.rb +++ b/test/unit/image_preprocessor_test.rb @@ -18,7 +18,7 @@ def setup test "#process_comparison returns comparison unchanged when no preprocessing options are provided" do preprocessor = SnapDiff::ImagePreprocessor.new(@driver, {}) - comparison = Comparison.new(:new_image, :base_image, {}, @driver) + comparison = SnapDiff::Comparison::Images.new(:new_image, :base_image, {}, @driver) result = preprocessor.process_comparison(comparison) @@ -30,7 +30,7 @@ def setup test "#process_comparison applies black box to skip areas when skip_area option is provided" do skip_area = [{x: 10, y: 20, width: 30, height: 40}] preprocessor = SnapDiff::ImagePreprocessor.new(@driver, skip_area: skip_area) - comparison = Comparison.new(:new_image, :base_image, {}, @driver) + comparison = SnapDiff::Comparison::Images.new(:new_image, :base_image, {}, @driver) result = preprocessor.process_comparison(comparison) @@ -53,7 +53,7 @@ def setup window_size = 3 options = {median_filter_window_size: window_size} preprocessor = SnapDiff::ImagePreprocessor.new(@driver, options) - comparison = Comparison.new(:new_image, :base_image, {}, @driver) + comparison = SnapDiff::Comparison::Images.new(:new_image, :base_image, {}, @driver) result = preprocessor.process_comparison(comparison) @@ -78,7 +78,7 @@ def setup expected_warning = /Median filter has been skipped for.*because it is not supported/ - comparison = Comparison.new(:new_image, :base_image, {}, @driver) + comparison = SnapDiff::Comparison::Images.new(:new_image, :base_image, {}, @driver) warning_output = capture_io do preprocessor = SnapDiff::ImagePreprocessor.new(@driver, options) diff --git a/test/unit/namespace_forwarding_test.rb b/test/unit/namespace_forwarding_test.rb index 221ff1c5..86ed5b2c 100644 --- a/test/unit/namespace_forwarding_test.rb +++ b/test/unit/namespace_forwarding_test.rb @@ -51,6 +51,7 @@ class NamespaceForwardingTest < ActiveSupport::TestCase "Capybara::Screenshot::Diff::Drivers::VipsDriver" => "SnapDiff::Drivers::VipsDriver", "Capybara::Screenshot::Diff::Reporters::Default" => "SnapDiff::Reporters::Default", "Capybara::Screenshot::Diff::ImageCompare" => "SnapDiff::Comparison", + "Capybara::Screenshot::Diff::Comparison" => "SnapDiff::Comparison::Images", "Capybara::Screenshot::Diff::Difference" => "SnapDiff::ComparisonResult", "CapybaraScreenshotDiff::RED_RGBA" => "SnapDiff::RED_RGBA", "CapybaraScreenshotDiff::ORANGE_RGBA" => "SnapDiff::ORANGE_RGBA" @@ -107,7 +108,7 @@ class NamespaceForwardingTest < ActiveSupport::TestCase end end - test "MAPPING covers all 30 documented forwarders" do - assert_equal 30, MAPPING.size + test "MAPPING covers all 31 documented forwarders" do + assert_equal 31, MAPPING.size end end diff --git a/test/unit/reporters/default_test.rb b/test/unit/reporters/default_test.rb index bbd9d43e..edd7199d 100644 --- a/test/unit/reporters/default_test.rb +++ b/test/unit/reporters/default_test.rb @@ -47,7 +47,7 @@ def build_comparison_for(driver, *images) new_image = driver.from_file(TEST_IMAGES_DIR.join(images.first)) base_image = driver.from_file(TEST_IMAGES_DIR.join(images.last)) - Comparison.new(new_image, base_image, {}, driver, @_tmpdir / images.first, @_tmpdir / images.last) + SnapDiff::Comparison::Images.new(new_image, base_image, {}, driver, @_tmpdir / images.first, @_tmpdir / images.last) end end end From 67d5b400586c34868ce0f287518f4508cae8d466 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 23 Aug 2026 01:01:49 +0200 Subject: [PATCH 4/4] 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. --- lib/capybara/screenshot/diff/image_compare.rb | 7 +++++-- lib/snap_diff/comparison.rb | 10 ---------- lib/snap_diff/drivers.rb | 20 +++++++++++++++++++ lib/snap_diff/legacy_shims.rb | 11 ++++++++++ lib/snap_diff/utils.rb | 8 +++++--- test/unit/namespace_forwarding_test.rb | 17 ++++++++++++++++ 6 files changed, 58 insertions(+), 15 deletions(-) diff --git a/lib/capybara/screenshot/diff/image_compare.rb b/lib/capybara/screenshot/diff/image_compare.rb index 09cb6a3c..0b0bd2f0 100644 --- a/lib/capybara/screenshot/diff/image_compare.rb +++ b/lib/capybara/screenshot/diff/image_compare.rb @@ -6,7 +6,10 @@ # snap_diff/comparison itself pulls in the ComparisonResult and Drivers # units, and the shims keep the old ::Difference / ::Drivers names # resolvable, so this path still provides everything the pre-move -# image_compare.rb did. The internal Comparison struct and LOADED_DRIVERS -# keep their legacy names and are defined by snap_diff/comparison.rb itself. +# image_compare.rb did. The internal images-holder struct lives at +# SnapDiff::Comparison::Images (its old ::Comparison name resolves via the +# shims) and the driver cache at SnapDiff::Drivers.loaded, with +# LOADED_DRIVERS kept as an eager same-object alias by legacy_shims +# (ADR-008 step 5). require "snap_diff/comparison" require "snap_diff/legacy_shims" diff --git a/lib/snap_diff/comparison.rb b/lib/snap_diff/comparison.rb index ccebd6c2..b0d60240 100644 --- a/lib/snap_diff/comparison.rb +++ b/lib/snap_diff/comparison.rb @@ -8,16 +8,6 @@ require "snap_diff/image_preprocessor" require "snap_diff/reporters/default" -# The driver cache keeps its legacy Capybara::Screenshot::Diff home for -# now; it moves in ADR-008 step 5b. -module Capybara - module Screenshot - module Diff - LOADED_DRIVERS = {} - end - end -end - module SnapDiff # Handles comparison of two images with a focus on performance and accuracy. # diff --git a/lib/snap_diff/drivers.rb b/lib/snap_diff/drivers.rb index d6c26539..f1e02183 100644 --- a/lib/snap_diff/drivers.rb +++ b/lib/snap_diff/drivers.rb @@ -10,5 +10,25 @@ def self.for(driver_options = {}) Utils.find_driver_class_for(driver_option).new end + + # Canonical driver-class cache (ADR-008 step 5b, ex + # Capybara::Screenshot::Diff::LOADED_DRIVERS): driver name => driver + # class, filled lazily by Utils.find_driver_class_for. Mutated in + # place -- including by user registration through the legacy constant, + # which legacy_shims pins as an EAGER same-object alias of this hash + # (a lazy copy would silently drop such registrations). + def self.loaded + @loaded ||= {} + end + + # Canonical read API for the detected-drivers list. The value itself + # stays on Capybara::Screenshot::Diff::AVAILABLE_DRIVERS (assigned in + # config_legacy.rb at load time, exactly when detection historically + # ran); this reads it live rather than caching, because that constant + # is the published stubbing point (image_compare_test stubs it to [] + # to exercise the no-drivers error path). + def self.available + Capybara::Screenshot::Diff::AVAILABLE_DRIVERS + end end end diff --git a/lib/snap_diff/legacy_shims.rb b/lib/snap_diff/legacy_shims.rb index de08d1c8..f465b634 100644 --- a/lib/snap_diff/legacy_shims.rb +++ b/lib/snap_diff/legacy_shims.rb @@ -24,6 +24,14 @@ # shared SnapDiff::Drivers module (the Drivers alias is same-object by # contract), so const_missing can never fire for the leaf names; # resolving them through the old path still warns for ...::Drivers. +# - Diff::LOADED_DRIVERS: user code registers custom drivers by mutating +# this hash in place, so it must be the exact same object as the +# canonical SnapDiff::Drivers.loaded -- a lazy warn-once shim could not +# keep a mutable alias, and warning on a supported registration surface +# would be noise. Assigned eagerly below. +# - Diff::AVAILABLE_DRIVERS: stays a real constant defined by +# config_legacy.rb (detection runs at that load moment); +# SnapDiff::Drivers.available is the canonical reader. module SnapDiff # @api private module LegacyShims @@ -47,6 +55,9 @@ def self.install(namespace, old_prefix, mapping) module Capybara module Screenshot module Diff + # EAGER same-object alias of the canonical driver cache (see header). + LOADED_DRIVERS = SnapDiff::Drivers.loaded + module Reporters end end diff --git a/lib/snap_diff/utils.rb b/lib/snap_diff/utils.rb index 57178b54..4124b7da 100644 --- a/lib/snap_diff/utils.rb +++ b/lib/snap_diff/utils.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "snap_diff/drivers" + module SnapDiff module Utils def self.detect_available_drivers @@ -20,9 +22,9 @@ def self.detect_available_drivers end def self.find_driver_class_for(driver) - driver = Capybara::Screenshot::Diff::AVAILABLE_DRIVERS.first if driver == :auto + driver = Drivers.available.first if driver == :auto - Capybara::Screenshot::Diff::LOADED_DRIVERS[driver] ||= + Drivers.loaded[driver] ||= case driver when :chunky_png require "snap_diff/drivers/chunky_png_driver" @@ -31,7 +33,7 @@ def self.find_driver_class_for(driver) require "snap_diff/drivers/vips_driver" SnapDiff::Drivers::VipsDriver else - fail "Wrong adapter #{driver.inspect}. Available adapters: #{Capybara::Screenshot::Diff::AVAILABLE_DRIVERS.inspect}" + fail "Wrong adapter #{driver.inspect}. Available adapters: #{Drivers.available.inspect}" end end end diff --git a/test/unit/namespace_forwarding_test.rb b/test/unit/namespace_forwarding_test.rb index 86ed5b2c..ce6fac9c 100644 --- a/test/unit/namespace_forwarding_test.rb +++ b/test/unit/namespace_forwarding_test.rb @@ -111,4 +111,21 @@ class NamespaceForwardingTest < ActiveSupport::TestCase test "MAPPING covers all 31 documented forwarders" do assert_equal 31, MAPPING.size end + + # Driver registries (ADR-008 step 5b): not part of MAPPING because the + # old names are EAGER aliases (never warn) of the canonical + # SnapDiff::Drivers accessors -- LOADED_DRIVERS is mutated in place by + # user driver registration, so it must stay the exact same object. + test "driver registries are the same object under old and canonical names" do + assert_same SnapDiff::Drivers.loaded, Capybara::Screenshot::Diff::LOADED_DRIVERS + assert_same SnapDiff::Drivers.available, Capybara::Screenshot::Diff::AVAILABLE_DRIVERS + end + + test "driver registration through the legacy LOADED_DRIVERS constant is visible canonically" do + Capybara::Screenshot::Diff::LOADED_DRIVERS[:forwarding_probe] = :probe_driver + + assert_equal :probe_driver, SnapDiff::Drivers.loaded[:forwarding_probe] + ensure + SnapDiff::Drivers.loaded.delete(:forwarding_probe) + end end