From d088eb4497e254bcd74b9711d19734a30a72e776 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:32:52 +0200 Subject: [PATCH] feat: everything 2.1 removes now warns in 2.0, and nothing is silent ADR-010: 2.0 deletes nothing and warns about everything; 2.1 deletes. That only works if the warnings actually reach a v1 user, and beta3 proved they did not -- a v1-only suite produced zero deprecation output while UPGRADING.md described three channels. Three holes, found by probing each door a v1 user can come through in its own subprocess (before/after table in the PR body): REQUIRE DOORS WERE SILENT. Every existing channel needs the user to CALL something -- a config delegator, a shimmed constant, a legacy include. A suite that requires the gem and only calls `screenshot` touches none of them. Requiring a v1-NAMED file is itself use of the v1 API, so the migration notice now fires there, at the top of the entry file. The canonical gem-name entry loads the v1 umbrella itself, so it claims the process first and stays silent. THE `driver` SETTING WAS SILENT. It is on the 2.1 removal list, and 2.0 said nothing for any value -- UPGRADING.md documented that as unfixable. It is not: the writer warns (Config#initialize seeds the ivar directly, so booting stays quiet) and so does the raw per-screenshot `driver:` key, at the two places raw user options exist. `driver: :vips` warns too: the warning is about the setting existing, not the value. UNRECOGNISED OPTIONS WERE SILENT -- the general case behind `shift_distance_limit=` behind a respond_to? guard. The per-screenshot hash was frozen but never validated, so `tolerence:` bought a green suite that compared nothing. Validated at SnapDiff::Comparison, the one funnel every option hash reaches: 2.0 warns once per key, 2.1 raises. Running that check over the whole suite found one live no-op: a `dimensions: [80, 80]` in image_compare_test that nothing has ever read. No new machinery -- SnapDiff::Removal / ::Deprecation already exist, are once-per-process, and are silenced by SNAP_DIFF_SILENCE_DEPRECATIONS. Which surfaced one honest limitation, now documented: the `silence_deprecations` accessor cannot silence a require-time notice, because setting it needs the require. Under Bundler.require the env var is the only channel. --- docs/UPGRADING.md | 67 +++++++--- lib/capybara-screenshot-diff.rb | 6 + lib/capybara_screenshot_diff.rb | 7 + lib/capybara_screenshot_diff/dsl.rb | 5 + lib/snap_diff-capybara.rb | 6 + lib/snap_diff.rb | 4 + lib/snap_diff/comparison.rb | 32 +++++ lib/snap_diff/config.rb | 20 ++- lib/snap_diff/deprecation.rb | 31 +++++ lib/snap_diff/removal.rb | 19 +++ lib/snap_diff/screenshot_matcher.rb | 4 + .../legacy_namespace_deprecation_test.rb | 12 +- test/legacy/legacy_tree_is_alias_only_test.rb | 7 +- test/legacy/snap_diff_deprecation_test.rb | 76 ++++++++++- ...canonical_suite_has_no_legacy_refs_test.rb | 2 + test/unit/dsl_test.rb | 33 +++++ test/unit/image_compare_test.rb | 10 +- test/unit/legacy_deletion_test.rb | 8 +- test/unit/option_validation_test.rb | 126 ++++++++++++++++++ test/unit/removed_in_2_1_deprecation_test.rb | 65 +++++++-- 20 files changed, 499 insertions(+), 41 deletions(-) create mode 100644 test/unit/option_validation_test.rb diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 41f54eab..ca016321 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -172,8 +172,13 @@ This means you can migrate your codebase incrementally **now**, before opting in ### Deprecation Warnings -v2.0 emits three different things, and it is worth knowing which is which. The first two are -about the old namespaces; the third is about the driver features 2.1 removes. +v2.0 emits four different things, and it is worth knowing which is which. The first two are +about the old namespaces; the third is about the driver features 2.1 removes; the fourth is +about options that never did anything. + +Everything 2.1 removes warns in 2.0, and every warning names 2.1. Nothing you can still write +in 2.0 does nothing quietly — if a setting is on its way out, or was never read at all, you +hear about it once per process. #### 1. The migration notice — one line per process @@ -185,12 +190,29 @@ The first time a process touches *any* hookable legacy API, you get a single lin It fires once and never again, whichever door you came through: +- **requiring a v1-named entry point** — `require "capybara/screenshot/diff"`, + `"capybara-screenshot-diff"`, `"capybara_screenshot_diff"`, or any of + `capybara_screenshot_diff/{dsl,minitest,rspec,cucumber}`. With `gem "capybara-screenshot-diff"` + in the Gemfile, `Bundler.require` opens this door for you at boot. - a legacy config accessor — `Capybara::Screenshot.window_size = ...`, `Capybara::Screenshot::Diff.tolerance` - a lazily shimmed legacy constant (see below) - `include Capybara::Screenshot` / `include Capybara::Screenshot::Diff` It exists because most of the v1 surface **cannot** warn per use, so without it a 2.x app could -be entirely silent right up to the bare `NameError` it would get on 2.1. +be entirely silent right up to the bare `NameError` it would get on 2.1. The require door is +what makes the rest of the list a safety net rather than the only mechanism: a suite that +requires the gem and only calls `screenshot` touches none of the other four. + +The canonical entry points — `require "snap_diff"`, `"snap_diff-capybara"`, `"snap_diff/dsl"`, +`"snap_diff/integrations/*"` — never fire it, even though `snap_diff-capybara` loads the v1 +compatibility files internally. + +> **Silencing a require-time notice needs the env var.** `SnapDiff.silence_deprecations = true` +> only takes effect from the line that sets it, and you cannot set it before the require that +> defines `SnapDiff`. Under `Bundler.require` there is no earlier moment at all. Use +> `SNAP_DIFF_SILENCE_DEPRECATIONS=1` in the environment, or +> `require "snap_diff/deprecation"; SnapDiff.silence_deprecations = true` ahead of everything +> else. #### 2. Per-constant warnings — one line per lazily shimmed constant @@ -227,19 +249,15 @@ warns once per process per subject, through the same channel and the same silenc | read `SnapDiff::Drivers.loaded` (the custom-driver registry) | the registry | nothing — custom drivers are removed, see below | | read `SnapDiff::Drivers.available` | driver detection | require `ruby-vips` instead of branching on a detected list | | `include SnapDiff::Driver` in your own driver class | the driver mixin | nothing — see below | +| set a driver **at all** — `SnapDiff.config.driver =`, the legacy `Capybara::Screenshot::Diff.driver =`, or `screenshot "index", driver: …` | the `driver` setting and the `driver:` option | delete the line. With libvips the only backend there is nothing to select | -> **The one removal on this list that 2.0 cannot warn you about: the `driver:` setting -> itself.** `SnapDiff.config.driver = :vips` and the legacy -> `Capybara::Screenshot::Diff.driver = :vips` are **silent** in 2.0 and raise -> `NoMethodError: undefined method 'driver='` in 2.1, at config time before any test runs. -> The per-screenshot form — `screenshot "index", driver: :vips` — is silent in 2.0 **and** -> in 2.1: per-screenshot options are a free-form hash, so an unknown key is simply inert. -> Warning on any of this would fire on the recommended configuration, so this note is the -> warning: **delete the line, and grep for the per-screenshot one.** With libvips the only -> backend there is nothing to select, and the default just works. The same goes for -> `driver: :auto` on a machine that *has* `ruby-vips` — the `:auto` warning above only -> fires when `:auto` actually falls back to ChunkyPNG, because that is the case where 2.1 -> stops the process comparing at all. +> **`driver: :vips` warns too, and that is deliberate.** The warning is not about the *value* +> you picked — it is about the setting existing. `SnapDiff.config.driver = :vips` raises +> `NoMethodError: undefined method 'driver='` in 2.1, at config time before any test runs, and +> `screenshot "index", driver: :vips` becomes an `ArgumentError` for an unknown option. Both +> are lines to delete, not lines to change. `driver: :auto` on a machine that *has* libvips is +> the one place the option still says something — and the `:auto` row above covers the case +> that matters, where `:auto` silently lands on ChunkyPNG. ``` [snap_diff deprecation] `driver: :auto` selected chunky_png because libvips is not available in this process. The chunky_png driver is REMOVED in 2.1, when libvips (the `ruby-vips` gem) becomes required -- install it now, or this setup stops comparing on 2.1. See docs/drivers.md. Silence with `SnapDiff.silence_deprecations = true` or SNAP_DIFF_SILENCE_DEPRECATIONS=1. (shown once per process) (called from /app/test/test_helper.rb:12) @@ -259,12 +277,27 @@ so warning there would fire on setups that are not affected by anything on this detection (`SnapDiff::Drivers.detect_available` / `SnapDiff::Utils.detect_available_drivers`) runs at load, before any user code. +#### 4. Unknown screenshot options — warned in 2.0, raised in 2.1 + +Per-screenshot options used to be a free-form hash: anything the gem did not read was frozen, +carried around, and ignored. A misspelt `tolerence:` bought you a green suite that compared +nothing and never said so. 2.0 warns once per unknown key; 2.1 raises `ArgumentError`. + +``` +[snap_diff deprecation] `:tolerence` is not a recognised screenshot option, so it does nothing. 2.1 raises ArgumentError for it. Check the spelling against the option list in docs/configuration.md. Silence with `SnapDiff.silence_deprecations = true` or SNAP_DIFF_SILENCE_DEPRECATIONS=1. (shown once per process) (called from /app/test/features/home_test.rb:14) +``` + +It applies to every route into a comparison — `screenshot`, `assert_matches_screenshot`, +`capture_screenshot` and `SnapDiff.compare`. Recognised keys are `area_size_limit`, +`capybara_screenshot_options`, `color_distance_limit`, `crop`, `delayed`, `driver`, +`median_filter_window_size`, `perceptual_threshold`, `screenshot_format`, +`shift_distance_limit`, `skip_area`, `stability_time_limit`, `tolerance` and `wait`. + #### Silent by design Some legacy names never warn individually, and that is deliberate — the migration notice above is the signal for all of them: -- **Requiring the gem.** `require "capybara_screenshot_diff/minitest"` etc. is not deprecated. - **The DSL.** `screenshot`, `assert_matches_screenshot`, `capture_screenshot` are never deprecated. - **Settings access.** `Capybara::Screenshot.blur_active_element`, `Capybara::Screenshot::Diff.tolerance=` and the `Diff.configure` block are plain delegators onto `SnapDiff.config`. There is no @@ -363,7 +396,7 @@ All settings and baselines are compatible with v1.x. Simply pin your Gemfile bac - [ ] Run your system tests (`bin/rails test:system`, not `rake test`) to verify no regressions - [ ] Read the warnings it prints — each one names something 2.1 removes - [ ] Add `gem "ruby-vips"` if you are not already on it (2.1 makes libvips the only backend) -- [ ] Drop `driver:` from your config — it is silent in 2.0 and gone in 2.1 +- [ ] Drop `driver:` from your config and your `screenshot` calls — 2.0 warns about it, 2.1 removes it - [ ] (Optional, but do it before 2.1) Migrate config and constants to the `SnapDiff` namespace - [ ] (Optional) Silence deprecation warnings if not ready to migrate - [ ] Report anything surprising on [the issue tracker](https://github.com/snap-diff/snap_diff-capybara/issues) diff --git a/lib/capybara-screenshot-diff.rb b/lib/capybara-screenshot-diff.rb index eb46ea11..a7ff06bc 100644 --- a/lib/capybara-screenshot-diff.rb +++ b/lib/capybara-screenshot-diff.rb @@ -3,4 +3,10 @@ # Bundler.require entry point for `gem "capybara-screenshot-diff"` -- the v1 # gem name, deleted in 3.0. The surviving name owns the logic (including the # minitest feature detection this door needs just as much). +# +# The marker goes BEFORE that require: snap_diff-capybara claims the process +# as canonical, and a marker after it would be swallowed. +require "snap_diff/deprecation" +SnapDiff::Deprecation.legacy_entry_point! + require "snap_diff-capybara" diff --git a/lib/capybara_screenshot_diff.rb b/lib/capybara_screenshot_diff.rb index 7ec0d800..adfca34f 100644 --- a/lib/capybara_screenshot_diff.rb +++ b/lib/capybara_screenshot_diff.rb @@ -1,5 +1,12 @@ # frozen_string_literal: true +# The v1 umbrella, and the file every legacy entry point except +# capybara_screenshot_diff/dsl reaches. Marked here rather than in each of +# them; snap_diff-capybara.rb (which loads this umbrella for canonical +# users) claims the process first, so this stays quiet for them. +require "snap_diff/deprecation" +SnapDiff::Deprecation.legacy_entry_point! + require "capybara/dsl" require "capybara/screenshot/diff/config_legacy" require "capybara/screenshot/diff/version" diff --git a/lib/capybara_screenshot_diff/dsl.rb b/lib/capybara_screenshot_diff/dsl.rb index 33535a3e..78942cd3 100644 --- a/lib/capybara_screenshot_diff/dsl.rb +++ b/lib/capybara_screenshot_diff/dsl.rb @@ -1,5 +1,10 @@ # frozen_string_literal: true +# The one legacy entry point that does NOT go through the umbrella, so it +# carries its own marker. +require "snap_diff/deprecation" +SnapDiff::Deprecation.legacy_entry_point! + require "snap_diff/dsl" # Deliberately EAGER and silent (v2 step 6 exception): DSL is an advertised diff --git a/lib/snap_diff-capybara.rb b/lib/snap_diff-capybara.rb index 87bf04ac..43c612a1 100644 --- a/lib/snap_diff-capybara.rb +++ b/lib/snap_diff-capybara.rb @@ -18,6 +18,12 @@ # zero-require Rails path and still activates the assertions. Absent gets a # line saying so -- a gem that loads and then does nothing, silently, is its # own bug report. +# This IS the canonical door, and it loads the v1 umbrella below -- so claim +# the process before that require, or every canonical user is told to +# migrate off an API they never touched. +require "snap_diff/deprecation" +SnapDiff::Deprecation.canonical_entry_point! + require "capybara_screenshot_diff" begin diff --git a/lib/snap_diff.rb b/lib/snap_diff.rb index 1bc84e7b..c0ce8493 100644 --- a/lib/snap_diff.rb +++ b/lib/snap_diff.rb @@ -65,6 +65,10 @@ module SnapDiff # 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) + # BEFORE the merge, which is the last moment `driver:` still means "the + # caller asked for a backend" rather than "config.default_options + # carries the key for everyone". + Removal.warn_once(:driver_setting, Removal::DRIVER_REMOVED) if options.key?(:driver) Comparison.new(current_path, baseline_path, config.default_options.merge(options)) end diff --git a/lib/snap_diff/comparison.rb b/lib/snap_diff/comparison.rb index 1a0965a4..5dbb7b6e 100644 --- a/lib/snap_diff/comparison.rb +++ b/lib/snap_diff/comparison.rb @@ -43,6 +43,31 @@ def skip_area TOLERABLE_OPTIONS = [:tolerance, :color_distance_limit, :shift_distance_limit, :area_size_limit].freeze + # Every key anything downstream of here actually reads -- capture + # options included, because ScreenshotMatcher hands the same hash to the + # screenshoter and only carves :crop / :stability_time_limit / :wait out + # of the copy it passes on. + # + # Written out rather than derived from Config#default_options: that hash + # is what the gem merges in, so deriving from it would make the check + # agree with itself and validate nothing about the keys a USER adds. + KNOWN_OPTIONS = %i[ + area_size_limit + capybara_screenshot_options + color_distance_limit + crop + delayed + driver + median_filter_window_size + perceptual_threshold + screenshot_format + shift_distance_limit + skip_area + stability_time_limit + tolerance + wait + ].freeze + attr_reader :driver, :driver_options attr_reader :image_path, :base_image_path attr_reader :difference, :error_message @@ -54,6 +79,13 @@ def initialize(image_path, base_image_path, options = {}) ensure_files_exist! @driver_options = options.freeze + # THE no-silent-no-op check (ADR-010). Every option hash in the gem + # reaches this constructor, so a key nothing reads is caught here + # whichever entry point produced it. Frozen-but-unvalidated is how a + # misspelt `tolerence:` bought a green suite that compared nothing. + (options.keys - KNOWN_OPTIONS).each do |key| + Removal.warn_once(:"unknown_option_#{key}", Removal.unknown_option(key)) + end # The per-comparison half of the shift_distance_limit removal (the # global half is Config#shift_distance_limit=). Presence is not enough: # config.default_options carries the key on EVERY comparison, nil for diff --git a/lib/snap_diff/config.rb b/lib/snap_diff/config.rb index 367e0d3c..1fd97d80 100644 --- a/lib/snap_diff/config.rb +++ b/lib/snap_diff/config.rb @@ -79,11 +79,12 @@ class Config manager ].freeze - # shift_distance_limit is excluded from the generated writers and hand - # written below (it announces its 2.1 removal); generating it here too - # would print Ruby's "method redefined" warning on every load. - attr_accessor(*(SETTINGS - %i[root shift_distance_limit])) - attr_reader :root, :shift_distance_limit + # shift_distance_limit and driver are excluded from the generated + # writers and hand written below (they announce their 2.1 removal); + # generating them here too would print Ruby's "method redefined" + # warning on every load. + attr_accessor(*(SETTINGS - %i[root shift_distance_limit driver])) + attr_reader :root, :shift_distance_limit, :driver def initialize # Every setting gets its ivar up front (nil-defaulted ones included) @@ -126,6 +127,15 @@ def shift_distance_limit=(value) @shift_distance_limit = value end + # Same shape, same reason: the writer only. #default_options reads + # +driver+ on every comparison for everyone, and #initialize seeds the + # +:auto+ default straight into the ivar, so only a user deliberately + # picking a backend hears about it. + def driver=(value) + Removal.warn_once(:driver_setting, Removal::DRIVER_REMOVED) + @driver = value + 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. diff --git a/lib/snap_diff/deprecation.rb b/lib/snap_diff/deprecation.rb index d4941600..b691797a 100644 --- a/lib/snap_diff/deprecation.rb +++ b/lib/snap_diff/deprecation.rb @@ -33,6 +33,7 @@ module Deprecation @seen = {} @notified = false @notice_suppressed = false + @canonical_entry = false # The ONE line a v1 user gets, whichever door they came through. Most of # the v1 surface cannot warn per use -- the config accessors are plain @@ -60,6 +61,36 @@ def notice Kernel.warn(MIGRATION_NOTICE) if first_time end + # @api private + # + # Called at the TOP of every v1-NAMED entry file, before that file's + # own requires. Requiring one of those paths IS use of the v1 API, + # and it is the ONE door a suite that merely calls `screenshot` goes + # through: every other door ({LEGACY_DOORS} in the probe test) needs + # the user to call something, which is how beta3 shipped a v1-only + # app that produced zero deprecation output. + # + # Position matters. The marker must run before the file's requires, + # because lib/capybara-screenshot-diff.rb reaches the canonical entry + # point below on its way in, and a marker placed after that require + # would be swallowed by {canonical_entry_point!}. + # @return [void] + def legacy_entry_point! + notice unless @canonical_entry + end + + # @api private + # + # Claimed by lib/snap_diff-capybara.rb -- the canonical gem-name + # entry -- which loads the v1 umbrella itself. Without this, "the v1 + # files got loaded" would be indistinguishable from "a v1 user", and + # every `gem "snap_diff-capybara"` app would be told to migrate off + # an API it never touched. + # @return [void] + def canonical_entry_point! + @canonical_entry = true + end + # @api private # # Suppresses {MIGRATION_NOTICE} for the rest of the process, without diff --git a/lib/snap_diff/removal.rb b/lib/snap_diff/removal.rb index 1b22d7a5..ff756e70 100644 --- a/lib/snap_diff/removal.rb +++ b/lib/snap_diff/removal.rb @@ -40,6 +40,25 @@ module Removal "driver, which is removed with it. libvips has no shift-distance comparison -- drop the " \ "option and tune `tolerance` / `color_distance_limit` instead. See docs/configuration.md." + # Also one subject with several call sites -- the setting's writer + # (Config) and the raw per-screenshot / per-compare option hashes. + # Deliberately value-blind: `driver: :vips` warns too. It is the knob + # that picks between implementations, and 2.1 leaves one implementation, + # so the line goes whatever it currently says. + DRIVER_REMOVED = + "The `driver` setting and the per-screenshot `driver:` option are REMOVED in 2.1: libvips " \ + "becomes the only backend, so there is nothing left to select. Drop the option and depend " \ + "on the `ruby-vips` gem instead. See docs/drivers.md." + + # Not a removal: 2.0 announces it and 2.1 turns it into an ArgumentError. + # The whole reason unrecognised keys need announcing is that the options + # hash was frozen but never validated, so a typo -- or a v1 option that + # no longer exists -- configured nothing, silently, forever. + def self.unknown_option(key) + "`#{key.inspect}` is not a recognised screenshot option, so it does nothing. 2.1 raises " \ + "ArgumentError for it. Check the spelling against the option list in docs/configuration.md." + end + MUTEX = Mutex.new @seen = {} @suppressed = false diff --git a/lib/snap_diff/screenshot_matcher.rb b/lib/snap_diff/screenshot_matcher.rb index c3865596..6cc21eb1 100644 --- a/lib/snap_diff/screenshot_matcher.rb +++ b/lib/snap_diff/screenshot_matcher.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require "snap_diff/removal" require "snap_diff/snap_manager" require_relative "screenshoter" require_relative "stable_screenshoter" @@ -15,6 +16,9 @@ class ScreenshotMatcher def initialize(screenshot_full_name, options = {}) @screenshot_full_name = screenshot_full_name + # BEFORE the merge below -- see SnapDiff.compare. Afterwards `:driver` + # is present for every caller and proves nothing. + Removal.warn_once(:driver_setting, Removal::DRIVER_REMOVED) if options.key?(:driver) @driver_options = SnapDiff.config.default_options.merge(options) @screenshot_format = @driver_options[:screenshot_format] diff --git a/test/legacy/legacy_namespace_deprecation_test.rb b/test/legacy/legacy_namespace_deprecation_test.rb index 5063fbdd..0ef58dac 100644 --- a/test/legacy/legacy_namespace_deprecation_test.rb +++ b/test/legacy/legacy_namespace_deprecation_test.rb @@ -146,10 +146,18 @@ def capture_warnings _out, err, status = Open3.capture3(RbConfig.ruby, "-Ilib", "-e", body, chdir: PROJECT_ROOT) + # ADR-010: requiring a v1-named entry point is itself use of the v1 + # API and now announces itself once. That line is the ENTRY's, not + # internal code's -- what this test is about is a per-constant warning, + # which can only come from the gem naming an old constant internally. + # Canonical entries stay held to zero output of any kind. + noise = err.lines.grep(/deprecation/) + noise = noise.grep_v(/This process uses the v1/) if kind == :legacy + if !status.success? "require \"#{entry}\": probe failed:\n#{err}" - elsif err.include?("deprecation") - "require \"#{entry}\": internal use emitted deprecation output:\n#{err}" + elsif !noise.empty? + "require \"#{entry}\": internal use emitted deprecation output:\n#{noise.join}" end end diff --git a/test/legacy/legacy_tree_is_alias_only_test.rb b/test/legacy/legacy_tree_is_alias_only_test.rb index d2dfe43c..beeeb61b 100644 --- a/test/legacy/legacy_tree_is_alias_only_test.rb +++ b/test/legacy/legacy_tree_is_alias_only_test.rb @@ -77,7 +77,12 @@ class LegacyTreeIsAliasOnlyTest < ActiveSupport::TestCase private\z | (extend|include)\s | [A-Z]\w*\s*=\s | # constant alias: Foo = SnapDiff::Foo - def_delegators?\s + def_delegators?\s | + # The v1 entry-point marker (ADR-010). Deliberately spelled out to the + # exact two calls rather than admitting bare method calls generally: + # these files must stay `git rm`-able, and announcing "a v1 door was + # used" is compatibility plumbing that dies with the door. + SnapDiff::Deprecation\.(legacy|canonical)_entry_point!\z )/x test "every legacy file is aliases and forwarders only" do diff --git a/test/legacy/snap_diff_deprecation_test.rb b/test/legacy/snap_diff_deprecation_test.rb index d9685df1..0a0d15ce 100644 --- a/test/legacy/snap_diff_deprecation_test.rb +++ b/test/legacy/snap_diff_deprecation_test.rb @@ -176,6 +176,53 @@ def capture_warnings end end + # --- the require doors ------------------------------------------------ + # + # THE beta3 HOLE. Every door above needs the user to CALL something. A v1 + # suite that requires a v1 entry point and then only calls `screenshot` + # touches no config delegator, no const_missing and no legacy include -- + # so beta3 shipped with docs describing three warning channels and a + # v1-only app producing zero output. Requiring a v1-NAMED file is itself + # use of the v1 API, and is the one door every such app goes through. + # + # capybara_screenshot_diff/cucumber is absent for the same reason as in + # legacy_entry_point_probe_test: it calls World(...) at load. + V1_REQUIRE_DOORS = %w[ + capybara/screenshot/diff + capybara-screenshot-diff + capybara_screenshot_diff + capybara_screenshot_diff/dsl + capybara_screenshot_diff/minitest + capybara_screenshot_diff/rspec + ].freeze + + V1_REQUIRE_DOORS.each do |entry| + test "the migration notice fires for `require #{entry.inspect}` alone" do + out = run_probe(%(require #{entry.inspect})) + + assert_equal 1, out.scan(NOTICE_MARKER).size, "expected exactly one migration notice, got:\n#{out}" + end + end + + # The mirror image, and the reason the doors above cannot simply be "the + # v1 files got loaded": the canonical gem-name entry point loads the whole + # v1 umbrella itself, so a `gem "snap_diff-capybara"` user would eat a + # notice about an API they never touched. + CANONICAL_REQUIRE_DOORS = %w[ + snap_diff + snap_diff-capybara + snap_diff/dsl + snap_diff/integrations/minitest + ].freeze + + CANONICAL_REQUIRE_DOORS.each do |entry| + test "`require #{entry.inspect}` alone stays silent" do + out = run_probe(%(require #{entry.inspect})) + + assert_equal "", out.strip, "canonical entry points must not warn" + end + end + test "the migration notice fires exactly once per process, however many legacy APIs are used" do out = run_probe(<<~RUBY) require "capybara_screenshot_diff" @@ -202,16 +249,43 @@ def capture_warnings assert_equal "", out.strip, "canonical-only usage must stay silent" end + # The accessor silences everything from the moment it is set -- which is + # necessarily AFTER the entry point that loaded SnapDiff in the first + # place. Setting it needs the constant, and the constant needs a require. test "the migration notice is silenced by the SnapDiff.silence_deprecations accessor" do out = run_probe(<<~RUBY) - require "capybara_screenshot_diff" + require "snap_diff/deprecation" SnapDiff.silence_deprecations = true + require "capybara_screenshot_diff" #{LEGACY_USE} RUBY assert_equal "", out.strip end + # ...which is why the env var is the channel UPGRADING.md points a + # Bundler.require user at: with `gem "capybara-screenshot-diff"` in the + # Gemfile, the v1 entry point loads before any line of their code runs, + # so the accessor has no moment early enough to cover it. + test "the accessor cannot silence the require-time notice, and the env var can" do + late = run_probe(<<~RUBY) + require "capybara_screenshot_diff" + SnapDiff.silence_deprecations = true + #{LEGACY_USE} + RUBY + + assert_equal 1, late.scan(NOTICE_MARKER).size, + "the require itself warns before the accessor can be set" + + early = run_probe(<<~RUBY, "SNAP_DIFF_SILENCE_DEPRECATIONS" => "1") + require "capybara_screenshot_diff" + SnapDiff.silence_deprecations = true + #{LEGACY_USE} + RUBY + + assert_equal "", early.strip + end + test "the migration notice is silenced by SNAP_DIFF_SILENCE_DEPRECATIONS" do out = run_probe(<<~RUBY, "SNAP_DIFF_SILENCE_DEPRECATIONS" => "1") require "capybara_screenshot_diff" diff --git a/test/unit/canonical_suite_has_no_legacy_refs_test.rb b/test/unit/canonical_suite_has_no_legacy_refs_test.rb index a5e220dd..afd00019 100644 --- a/test/unit/canonical_suite_has_no_legacy_refs_test.rb +++ b/test/unit/canonical_suite_has_no_legacy_refs_test.rb @@ -90,6 +90,8 @@ class CanonicalSuiteHasNoLegacyRefsTest < ActiveSupport::TestCase '["snap_diff.rb", %(require "snap_diff/legacy_shims"), nil],', '%(require "capybara_screenshot_diff/minitest" if defined?(::Minitest)),', '%(require "capybara_screenshot_diff"),', + '["snap_diff-capybara.rb", %(require "snap_diff/deprecation"), nil],', + '["snap_diff-capybara.rb", %(SnapDiff::Deprecation.canonical_entry_point!), nil]', 'gate << "SnapDiff.start is still defined" if SnapDiff.respond_to?(:start)', 'gate << "CapybaraScreenshotDiff is still defined" if defined?(CapybaraScreenshotDiff)', 'assert_includes failure, "SnapDiff.start is still defined"' diff --git a/test/unit/dsl_test.rb b/test/unit/dsl_test.rb index 39015f35..c783a682 100644 --- a/test/unit/dsl_test.rb +++ b/test/unit/dsl_test.rb @@ -222,6 +222,22 @@ def take_comparison_screenshot(snapshot) end end + # ADR-010: the per-screenshot `driver:` key is REMOVED in 2.1, so a user + # who passes it has to hear about it -- and the DSL is how most of them + # pass it. The message, once-per-process and silencing contracts are + # proven in a subprocess by removed_in_2_1_deprecation_test; what THIS + # example guards is the seam that test cannot reach, because the DSL + # resolves `:driver` to a driver instance before Comparison ever sees the + # hash. Asserting the routing, not the output: SnapDiff::Removal is + # suppressed suite-wide (test_helper) and cannot be un-suppressed. + test "#capture_screenshot routes a user's driver: option to the removal channel" do + assert_includes removal_subjects_for(driver: :vips), :driver_setting + end + + test "#capture_screenshot does not announce the driver removal when the user did not ask for one" do + assert_not_includes removal_subjects_for, :driver_setting + end + test "#capture_screenshot does not raise even when a differing baseline exists" do SnapDiff::Vcs.stub(:checkout_vcs, true) do snap = create_snapshot_for(:a, :c) @@ -245,6 +261,23 @@ def take_comparison_screenshot(snapshot) private + # Every removal subject the channel is asked to announce while the DSL + # captures one screenshot with +options+. + def removal_subjects_for(**options) + subjects = [] + recorder = ->(subject, _message) { subjects << subject } + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + SnapDiff::Removal.stub(:warn_once, recorder) do + # :c, like the other capture examples -- ScreenshoterStub copies the + # like-named fixture, so the name has to be one that exists. + capture_screenshot(:c, **options) + end + end + + subjects + end + def our_screenshot(name, skip_stack_frames) screenshot(name, skip_stack_frames: skip_stack_frames) end diff --git a/test/unit/image_compare_test.rb b/test/unit/image_compare_test.rb index 89c2bc07..9ade5850 100644 --- a/test/unit/image_compare_test.rb +++ b/test/unit/image_compare_test.rb @@ -63,11 +63,11 @@ class ImageCompareTest < ActiveSupport::TestCase assert_equal 0.02, comp.driver_options[:tolerance] end - test "#initialize with dimensions creates valid comparison" do - comp = make_comparison(:b, dimensions: [80, 80]) - assert comp.quick_equal? - assert_not comp.different? - end + # `dimensions: [80, 80]` used to be here, and was the only thing this + # example added over the identical-images case above. Nothing in the gem + # has ever read that key -- it configured nothing, silently, which is the + # exact class OptionValidationTest now closes. The option validation found + # it; the example went with it rather than pinning a no-op. test "#initialize with :auto driver selects vips when available" do skip "VIPS not present. Skipping VIPS driver tests." unless defined?(Vips) diff --git a/test/unit/legacy_deletion_test.rb b/test/unit/legacy_deletion_test.rb index 1795e221..a11b7166 100644 --- a/test/unit/legacy_deletion_test.rb +++ b/test/unit/legacy_deletion_test.rb @@ -59,7 +59,13 @@ class LegacyDeletionTest < ActiveSupport::TestCase %(require "snap_diff/integrations/minitest" if defined?(::Minitest))], ["snap_diff-capybara.rb", %(require "capybara_screenshot_diff"), - %(require "snap_diff")] + %(require "snap_diff")], + # The ADR-010 canonical-entry marker: it exists only to keep the v1 + # migration notice off canonical users while this file still loads the + # v1 umbrella. With the umbrella gone there is nothing to suppress, and + # snap_diff/deprecation.rb is deleted above -- so both lines go. + ["snap_diff-capybara.rb", %(require "snap_diff/deprecation"), nil], + ["snap_diff-capybara.rb", %(SnapDiff::Deprecation.canonical_entry_point!), nil] ].freeze ENTRY_POINTS = SupportLoadProbeTest::CANONICAL_ENTRY_POINTS diff --git a/test/unit/option_validation_test.rb b/test/unit/option_validation_test.rb new file mode 100644 index 00000000..3f5a75ee --- /dev/null +++ b/test/unit/option_validation_test.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require "test_helper" +require "open3" + +# NO SILENT NO-OPS (ADR-010). +# +# The per-screenshot options hash was frozen but never validated, so any key +# the gem does not read -- a typo, a v1 option that no longer exists, an +# option meant for a different gem -- configured nothing and said nothing. +# A tolerance you believe is set and is not is worse than no tolerance at +# all: the suite stays green and stops testing. +# +# 2.0 warns once per unknown key; 2.1 raises ArgumentError. Guarded at +# SnapDiff::Comparison, the one funnel EVERY option hash reaches -- the DSL +# (screenshot -> ScreenshotMatcher -> Comparison) and the file-to-file +# SnapDiff.compare both end up there, so a second entry point cannot be +# added past this check by accident. +# +# Subprocess probes: "once per process" is the contract, and this suite's +# test_helper suppresses these warnings globally (it exercises the doomed +# options by design), so an in-process assertion could measure neither. +class OptionValidationTest < ActiveSupport::TestCase + PROJECT_ROOT = File.expand_path("../..", __dir__) + IMAGE_A = File.join(PROJECT_ROOT, "test/fixtures/images/a.png") + IMAGE_B = File.join(PROJECT_ROOT, "test/fixtures/images/b.png") + + test "an unrecognised option warns, naming the key and 2.1" do + lines = probe(<<~RUBY) + require "snap_diff" + #{compare(tolerence: 0.5)} + RUBY + + unknown = lines.grep(/tolerence/) + assert_equal 1, unknown.size, lines.join + assert_match(/does nothing/, unknown.first) + assert_match(/2\.1 raises/, unknown.first) + end + + test "an unrecognised option warns once per process, not once per comparison" do + lines = probe(<<~RUBY) + require "snap_diff" + 3.times { #{compare(tolerence: 0.5)} } + RUBY + + assert_equal 1, lines.grep(/tolerence/).size, lines.join + end + + test "each distinct unrecognised key gets its own warning" do + lines = probe(<<~RUBY) + require "snap_diff" + #{compare(tolerence: 0.5, colour_distance_limit: 3)} + RUBY + + assert_equal 1, lines.grep(/tolerence/).size, lines.join + assert_equal 1, lines.grep(/colour_distance_limit/).size, lines.join + end + + # The v1 option people most often still have in a setup file. It is not a + # "typo" at all -- it was real once -- which is exactly why an unknown-key + # check has to cover the general case rather than a blocklist. + test "a per-screenshot option through the DSL is validated too" do + lines = probe(<<~RUBY) + require "snap_diff" + SnapDiff::Comparison.new( + #{IMAGE_A.inspect}, #{IMAGE_B.inspect}, + SnapDiff.config.default_options.merge(shift_distance_limits: 3) + ) + RUBY + + assert_equal 1, lines.grep(/shift_distance_limits/).size, lines.join + end + + # The half that decides whether anyone will listen: config.default_options + # merges a dozen keys into EVERY comparison, and the matcher adds more on + # its way through. If any of those tripped the check, the warning would be + # unconditional noise and users would filter the channel out entirely. + test "a fully-specified real comparison produces no unknown-option warning" do + out = probe_stderr(<<~RUBY) + require "snap_diff" + #{compare( + tolerance: 0.01, + color_distance_limit: 8, + area_size_limit: 100, + perceptual_threshold: 2, + median_filter_window_size: 3, + skip_area: nil, + crop: nil, + delayed: false, + screenshot_format: "png", + capybara_screenshot_options: {} + )} + RUBY + + assert_empty out.lines.grep(/not a recognised/), "recognised options must not warn" + end + + test "silenced by SNAP_DIFF_SILENCE_DEPRECATIONS" do + out = probe_stderr(<<~RUBY, "SNAP_DIFF_SILENCE_DEPRECATIONS" => "1") + require "snap_diff" + #{compare(tolerence: 0.5)} + RUBY + + assert_equal "", out + end + + private + + def compare(**options) + args = [IMAGE_A.inspect, IMAGE_B.inspect] + options.each { |key, value| args << "#{key}: #{value.inspect}" } + "SnapDiff.compare(#{args.join(", ")})" + end + + def probe(script, env = {}) + probe_stderr(script, env).lines.reject { |line| line.strip.empty? } + end + + def probe_stderr(script, env = {}) + _out, err, status = Open3.capture3( + env, RbConfig.ruby, "-Ilib", "-e", script, chdir: PROJECT_ROOT + ) + assert_predicate status, :success?, err + err.lines.grep(/\[snap_diff/).join + end +end diff --git a/test/unit/removed_in_2_1_deprecation_test.rb b/test/unit/removed_in_2_1_deprecation_test.rb index f10cf3c7..b1748e78 100644 --- a/test/unit/removed_in_2_1_deprecation_test.rb +++ b/test/unit/removed_in_2_1_deprecation_test.rb @@ -44,10 +44,13 @@ def require(name) 3.times { #{compare(driver: :chunky_png)} } RUBY - assert_equal 1, lines.size, lines.join - assert_match(/chunky_png/, lines.first) - assert_match(/REMOVED in 2\.1/, lines.first) - assert_match(/vips/, lines.first) + # Counts the chunky lines, not every line: picking a driver AT ALL is + # its own removal (see the driver-setting examples below), so this + # caller legitimately hears two different things. + chunky = lines.grep(/chunky_png driver/) + assert_equal 1, chunky.size, lines.join + assert_match(/REMOVED in 2\.1/, chunky.first) + assert_match(/vips/, chunky.first) end test "selecting chunky_png through SnapDiff.config.driver warns once" do @@ -57,8 +60,7 @@ def require(name) 3.times { #{compare} } RUBY - assert_equal 1, lines.size, lines.join - assert_match(/chunky_png/, lines.first) + assert_equal 1, lines.grep(/chunky_png driver/).size, lines.join end # THE CASE THAT MATTERS MOST: these users never asked for chunky_png and @@ -142,18 +144,63 @@ class MyOtherDriver assert_match(/REMOVED in 2\.1/, lines.first) end + # --- the `driver` setting and the `driver:` option ------------------- + # + # The abstraction goes, so the knob that picks between implementations + # goes with it -- whichever value it is set to. `driver: :vips` is the + # case that needs saying out loud: it warns about nothing today (it is + # the surviving backend) and it is exactly the line that stops existing + # in 2.1. + + test "setting the driver on the config warns once, whatever the value" do + lines = probe(<<~RUBY) + require "snap_diff" + 3.times { SnapDiff.config.driver = :vips } + RUBY + + assert_equal 1, lines.size, lines.join + assert_match(/`driver`/, lines.first) + assert_match(/REMOVED in 2\.1/, lines.first) + end + + test "passing driver: per comparison warns once, whatever the value" do + lines = probe(<<~RUBY) + require "snap_diff" + 3.times { #{compare(driver: :vips)} } + RUBY + + driver = lines.grep(/`driver`/) + assert_equal 1, driver.size, lines.join + assert_match(/REMOVED in 2\.1/, driver.first) + end + + # The trap that made shift_distance_limit hard: config.default_options + # carries :driver on EVERY comparison. Presence in the merged hash is not + # evidence that the user asked for it. + test "the driver: key config always merges in does not warn by itself" do + out = probe_stderr(<<~RUBY) + require "snap_diff" + 3.times { #{compare} } + RUBY + + assert_empty out.lines.grep(/`driver`/), "config's own default must not warn at the user" + end + # --- what must stay silent ------------------------------------------- # The mutation that matters for everyone who is NOT affected: a plain vips # setup, comparing images, must not gain a single line of stderr -- and the # gem's own drivers include the mixin themselves, so an unscoped `included` # hook would fire here. + # + # No `config.driver = :vips` here any more: that line is itself removed in + # 2.1 and now warns (see the driver-setting examples above). Leaving vips + # to :auto is what an unaffected setup looks like on this box. test "a plain vips setup with no chunky or shift usage stays silent" do skip "libvips not available on this box" unless SnapDiff::Drivers::AVAILABLE_DRIVERS.include?(:vips) out = probe_stderr(<<~RUBY) require "snap_diff" - SnapDiff.config.driver = :vips 3.times { #{compare} } SnapDiff::Drivers::VipsDriver RUBY @@ -188,8 +235,8 @@ class MyOtherDriver end RUBY - assert_equal 5, lines.size, lines.join - assert_equal 5, lines.uniq.size, "duplicate warning text: #{lines.join}" + assert_equal 6, lines.size, lines.join + assert_equal 6, lines.uniq.size, "duplicate warning text: #{lines.join}" end # --- silencing --------------------------------------------------------