diff --git a/lib/capybara/screenshot/diff/config_legacy.rb b/lib/capybara/screenshot/diff/config_legacy.rb index 27fd7bc3..0fed2093 100644 --- a/lib/capybara/screenshot/diff/config_legacy.rb +++ b/lib/capybara/screenshot/diff/config_legacy.rb @@ -3,11 +3,13 @@ # Legacy Capybara::Screenshot / Capybara::Screenshot::Diff config surface. # # Since ADR-008 step 1 the storage lives in SnapDiff::Config -- the require -# leaf of the config graph (see its own header) -- and this file installs -# the old accessor names as thin delegators onto SnapDiff.config, generated -# from SnapDiff::Config::MAPPING. The v1 write surface -# (Capybara::Screenshot.window_size = ..., Diff.configure { ... }) keeps -# working unchanged: one storage, two views. +# leaf of the config graph (see its own header) -- and since step 7b the +# DERIVED values (active?, screenshot_area, default_options) live there +# too. snap_diff/config.rb also generates the old accessor names as thin +# delegators from SnapDiff::Config::MAPPING, so nothing but forwarders is +# left here. The v1 surface (Capybara::Screenshot.window_size = ..., +# Diff.configure { ... }, Diff.compare) keeps working unchanged: one +# storage, two views. # # Load order: requiring snap_diff/config first also eagerly evaluates the # require-time defaults (ENV["CI"] for fail_if_new, Rails.root/pwd for @@ -23,18 +25,15 @@ module Capybara module Screenshot class << self def active? - enabled || (enabled.nil? && Diff.enabled) + SnapDiff.config.active? end def screenshot_area - parts = [Screenshot.save_path] - parts << SnapDiff::Os.name if Screenshot.add_os_path - parts << Capybara.current_driver.to_s if Screenshot.add_driver_path - File.join(*parts) + SnapDiff.config.screenshot_area end def screenshot_area_abs - root / screenshot_area + SnapDiff.config.screenshot_area_abs end end @@ -50,46 +49,18 @@ module Diff # diff.driver = :vips # diff.tolerance = 0.0005 # end + # The bare `yield` (rather than an explicit &block) keeps this + # method's published arity byte-identical to what it always had. def self.configure - yield Screenshot, self + SnapDiff.start { |screenshot, diff| yield screenshot, diff } end def self.compare(baseline_path, current_path, **options) - SnapDiff::Comparison.new(current_path, baseline_path, default_options.merge(options)) + SnapDiff.compare(baseline_path, current_path, **options) end def self.default_options - { - area_size_limit: area_size_limit, - color_distance_limit: color_distance_limit, - driver: driver, - screenshot_format: Screenshot.screenshot_format, - capybara_screenshot_options: Screenshot.capybara_screenshot_options, - perceptual_threshold: perceptual_threshold, - shift_distance_limit: shift_distance_limit, - skip_area: skip_area, - stability_time_limit: Screenshot.stability_time_limit, - tolerance: tolerance || ((driver == :vips) ? 0.001 : nil), - # Deliberately LIVE (pinned by config_default_timing_test.rb): - # read at call time, never frozen into storage. - wait: Capybara.default_max_wait_time - } - end - end - - # The old mattr_accessor surface, now delegating to the single storage. - # mattr_accessor used to define both singleton and instance accessors - # (the instance ones are what `include Capybara::Screenshot::Diff` - # picks up), so both are installed. root keeps its historical - # asymmetry -- readable everywhere, writable only at module level - # (it was mattr_reader plus a custom module-level writer) -- with the - # Pathname coercion now living in Config#root=. - SnapDiff::Config::MAPPING.each do |name, (mod, mattr)| - [mod, mod.singleton_class].each do |target| - target.define_method(mattr) { SnapDiff.config.public_send(name) } - next if name == :root && target == mod - - target.define_method(:"#{mattr}=") { |value| SnapDiff.config.public_send(:"#{name}=", value) } + SnapDiff.config.default_options end end end diff --git a/lib/snap_diff.rb b/lib/snap_diff.rb index cb23beb6..e8f6a6fd 100644 --- a/lib/snap_diff.rb +++ b/lib/snap_diff.rb @@ -34,7 +34,7 @@ def self.assert_single_gem!(loaded_specs = Gem.loaded_specs) # stay resolvable (now with deprecation warnings) even in processes that # only ever require "snap_diff". # "capybara/dsl" is needed directly (not just transitively) so -# `Capybara.default_max_wait_time` in Diff.default_options resolves even +# `Capybara.default_max_wait_time` in Config#default_options resolves even # when "snap_diff" is required standalone (SnapDiffTest's # "standalone-loadable in a fresh process" regression test). require "capybara/dsl" @@ -51,22 +51,28 @@ def self.assert_single_gem!(loaded_specs = Gem.loaded_specs) # shims (snap_diff/legacy_shims) that emit a deprecation warning once per # constant per process. See ADR-004 for the full migration plan. module SnapDiff - def self.compare(...) - Capybara::Screenshot::Diff.compare(...) + # Compare two images on disk with the configured defaults. Canonical home + # since ADR-008 step 7b; +Capybara::Screenshot::Diff.compare+ now forwards + # here rather than the other way round. + # + # Note the argument order swap: callers pass baseline first (reading + # "compare baseline against current"), Comparison takes current first. + def self.compare(baseline_path, current_path, **options) + Comparison.new(current_path, baseline_path, config.default_options.merge(options)) end - # v1-style configuration: yields the two existing mattr_accessor holders + # v1-style configuration: yields the two legacy accessor holders # (+Capybara::Screenshot+, +Capybara::Screenshot::Diff+) exactly as - # +Capybara::Screenshot::Diff.configure+ always has. Kept byte-for-byte - # identical to +Diff.configure+ for existing callers migrating namespaces - # without changing call shape. + # +Capybara::Screenshot::Diff.configure+ always has -- and, since ADR-008 + # step 7b, this is where that yield actually happens; Diff.configure + # forwards here. Both names stay identical in call shape. # # SnapDiff.start do |screenshot, diff| # screenshot.window_size = [1280, 1024] # diff.tolerance = 0.0005 # end - def self.start(&block) - Capybara::Screenshot::Diff.configure(&block) + def self.start + yield Capybara::Screenshot, Capybara::Screenshot::Diff end # Forward-looking configuration: yields the single consolidated diff --git a/lib/snap_diff/config.rb b/lib/snap_diff/config.rb index ed5311fe..7f9617ba 100644 --- a/lib/snap_diff/config.rb +++ b/lib/snap_diff/config.rb @@ -42,7 +42,7 @@ module SnapDiff # +Rails.root+ / pwd) must never become lazy read-time defaults, memoized # or not. The one deliberately LIVE value, +default_options[:wait]+, is # not storage at all: it stays a method-body read of - # +Capybara.default_max_wait_time+ in +Diff.default_options+. + # +Capybara.default_max_wait_time+ in +#default_options+. class Config # config attr name => [legacy module, legacy accessor name]. # @@ -118,6 +118,53 @@ def initialize def root=(path) @root = Pathname(path).expand_path end + + # --- Derived config (ADR-008 step 7b) ------------------------------- + # Read-only values computed from the storage above. They used to live + # on the legacy modules; those now one-line forward here. + + # ex +Capybara::Screenshot.active?+. The two +enabled+ settings are + # independent (see {MAPPING}): the Screenshot-side one wins whenever it + # was set at all, and only a nil there falls through to the Diff-side + # one. + def active? + screenshot_enabled || (screenshot_enabled.nil? && enabled) + end + + # ex +Capybara::Screenshot.screenshot_area+: the save_path, optionally + # segmented per OS and per Capybara driver. + def screenshot_area + parts = [save_path] + parts << Os.name if add_os_path + parts << Capybara.current_driver.to_s if add_driver_path + File.join(*parts) + end + + # ex +Capybara::Screenshot.screenshot_area_abs+. + def screenshot_area_abs + root / screenshot_area + end + + # ex +Capybara::Screenshot::Diff.default_options+: the capture/compare + # defaults handed to {SnapDiff::Comparison}. Carries the one literal + # that is not a stored setting -- the vips tolerance floor. + def default_options + { + area_size_limit: area_size_limit, + color_distance_limit: color_distance_limit, + driver: driver, + screenshot_format: screenshot_format, + capybara_screenshot_options: capybara_screenshot_options, + perceptual_threshold: perceptual_threshold, + shift_distance_limit: shift_distance_limit, + skip_area: skip_area, + stability_time_limit: stability_time_limit, + tolerance: tolerance || ((driver == :vips) ? 0.001 : nil), + # Deliberately LIVE (pinned by config_default_timing_test.rb): + # read at call time, never frozen into storage. + wait: Capybara.default_max_wait_time + } + end end # Instantiated eagerly so the require-time defaults above are evaluated @@ -129,4 +176,25 @@ def root=(path) def self.config @config end + + # Installs the old mattr_accessor surface onto the legacy modules, + # delegating to the single storage above. mattr_accessor used to define + # both singleton and instance accessors (the instance ones are what + # `include Capybara::Screenshot::Diff` picks up), so both are installed. + # root keeps its historical asymmetry -- readable everywhere, writable + # only at module level (it was mattr_reader plus a custom module-level + # writer) -- with the Pathname coercion living in Config#root=. + # + # Generated here rather than in config_legacy.rb (ADR-008 step 7b) for + # the same reason legacy_shims.rb generates the legacy constants here: + # the generator is code, and the v1 trees must stay alias-only so 3.0 is + # a `git rm`. Same technique, same side of the fence. + Config::MAPPING.each do |name, (mod, mattr)| + [mod, mod.singleton_class].each do |target| + target.define_method(mattr) { SnapDiff.config.public_send(name) } + next if name == :root && target == mod + + target.define_method(:"#{mattr}=") { |value| SnapDiff.config.public_send(:"#{name}=", value) } + end + end end diff --git a/test/unit/legacy_tree_is_alias_only_test.rb b/test/unit/legacy_tree_is_alias_only_test.rb index 4cf94fae..2838828c 100644 --- a/test/unit/legacy_tree_is_alias_only_test.rb +++ b/test/unit/legacy_tree_is_alias_only_test.rb @@ -19,22 +19,22 @@ class LegacyTreeIsAliasOnlyTest < ActiveSupport::TestCase Dir[LIB.join("capybara_screenshot_diff/**/*.rb")] ).map { |path| Pathname.new(path) }.sort.freeze - # THE ALLOWLIST. Every entry is a file that legitimately still holds code, - # with the reason. This list is the honest state of the tree -- keep it in - # sync with ADR-008, and never add to it to make a red build green without - # first asking whether the code belongs in lib/snap_diff/ instead. - ALLOWED_WITH_CODE = { - # ADR-008 step 1 moved config STORAGE to SnapDiff::Config and generates - # the old accessor names here from Config::MAPPING (delegation, but via - # define_method). It also still holds DERIVED config logic that never - # moved: .active? precedence, .screenshot_area path assembly and - # .default_options (which carries one literal default, the vips - # tolerance 0.001). Narrowed below by pinning the method inventory, so - # new logic here still reds. AVAILABLE_DRIVERS also lives here on - # purpose (see #227: test_helper reads it at boot, image_compare_test - # stubs it) -- the gate does not push it out. - "capybara/screenshot/diff/config_legacy.rb" => "generates delegating accessors from Config::MAPPING; retains derived config logic (ADR-008 step 1)" - }.freeze + # THE ALLOWLIST -- and as of ADR-008 step 7b it is EMPTY: not one file in + # the v1 trees holds logic any more. config_legacy.rb was the last entry; + # step 7b moved its derived config (.active? precedence, .screenshot_area + # path assembly, .default_options incl. the vips tolerance literal) into + # SnapDiff::Config, and the Config::MAPPING accessor generator into + # snap_diff/config.rb alongside it, leaving only one-line forwarders. + # + # Keep it empty. Adding an entry back is a decision to keep behaviour on + # the v1 side of the 3.0 deletion, so it needs a written reason here AND + # an ADR-008 update -- never just to turn a red build green. + # + # (Diff::AVAILABLE_DRIVERS still lives in config_legacy.rb, but as a bare + # constant assignment it is alias-shaped and needs no exemption. It stays + # there on purpose -- see #227: test_helper reads it at boot and + # image_compare_test stubs it as the published no-drivers hook.) + ALLOWED_WITH_CODE = {}.freeze # Shapes that are pure compatibility plumbing rather than behaviour. ALIAS_SHAPES = /\A( @@ -62,15 +62,13 @@ class LegacyTreeIsAliasOnlyTest < ActiveSupport::TestCase MSG end - test "config_legacy.rb keeps exactly its known set of methods" do - source = LIB.join("capybara/screenshot/diff/config_legacy.rb").read - defined_methods = source.scan(/^\s*def\s+(self\.)?([a-z_]\w*[?!]?)/).map { |receiver, name| "#{receiver}#{name}" } - - assert_equal( - ["active?", "screenshot_area", "screenshot_area_abs", "self.compare", "self.configure", "self.default_options"], - defined_methods.sort, - "config_legacy.rb is allowlisted for the logic it already had, not for new logic -- move new methods to lib/snap_diff/" - ) + # The pinned method inventory that used to narrow config_legacy.rb's + # allowlist entry is gone with the entry itself (ADR-008 step 7b): with + # nothing allowlisted, the general rule above already checks every `def` + # in the tree, config_legacy.rb's included. + test "the allowlist is empty, so nothing is exempt from the general rule" do + assert_empty ALLOWED_WITH_CODE, + "an exemption came back -- see the comment on ALLOWED_WITH_CODE before keeping it" end private diff --git a/test/unit/snap_diff_config_test.rb b/test/unit/snap_diff_config_test.rb index 01df70d7..e1d2dfc8 100644 --- a/test/unit/snap_diff_config_test.rb +++ b/test/unit/snap_diff_config_test.rb @@ -118,6 +118,41 @@ def config end end + # ADR-008 step 7b moved this precedence rule from + # Capybara::Screenshot.active? into Config#active?, and found it had no + # test at all: replacing the whole expression with a bare `enabled` kept + # all 529 unit tests green. The full truth table is pinned here, through + # both the canonical method and the legacy forwarder, so it cannot move + # again unnoticed. + # + # The rule: the Screenshot-side flag wins whenever it was set to anything + # at all; only a nil there falls through to the Diff-side flag. + ACTIVE_TRUTH_TABLE = [ + [true, true, true], + [true, false, true], + [false, true, false], + [false, false, false], + [nil, true, true], + [nil, false, false] + ].freeze + + test "active? gives Screenshot.enabled precedence and only falls through on nil" do + original_screenshot = Capybara::Screenshot.enabled + original_diff = Capybara::Screenshot::Diff.enabled + + ACTIVE_TRUTH_TABLE.each do |screenshot_enabled, enabled, expected| + config.screenshot_enabled = screenshot_enabled + config.enabled = enabled + context = "screenshot_enabled=#{screenshot_enabled.inspect}, enabled=#{enabled.inspect}" + + assert_equal expected, !!config.active?, "Config#active? with #{context}" + assert_equal expected, !!Capybara::Screenshot.active?, "Capybara::Screenshot.active? with #{context}" + end + ensure + Capybara::Screenshot.enabled = original_screenshot + Capybara::Screenshot::Diff.enabled = original_diff + end + test "writing root through config round-trips through the same Pathname coercion" do original = Capybara::Screenshot.root