diff --git a/README.md b/README.md index 28e94b4e..0c15993d 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,31 @@ bin/rails test:system # now green — HEAD holds the new baseline Reviewing the change is what the pull request is for — the updated `.png` shows up as an image diff next to the code that caused it. +### Accepting many at once + +One failure at a time is fine for one screenshot. After a redesign that changed forty, set the +**record mode** to `:all` and re-record them in a single run: + +```ruby +# test_helper.rb +SnapDiff.config.record = ENV["ACCEPT_SCREENSHOTS"] ? :all : :once +``` + +```bash +ACCEPT_SCREENSHOTS=1 bin/rails test:system +# [snap_diff] record: :all re-recorded 40 screenshots WITHOUT comparing: ... + +git status # forty modified baselines, and nothing else +git add doc/screenshots/ +git commit -m "chore: re-record baselines after the checkout redesign" +``` + +`:all` writes every capture straight to its baseline path and compares nothing, so `git status` +is the review surface — look at the images before you commit. It **refuses to run under CI**, +where there would be nobody to look. The other modes are `:once` (the default) and `:none` +(strict — a missing baseline always fails); see +[Record modes](docs/configuration.md#record-modes--accepting-changes). + ## Web UI for Reviewing Screenshot Changes Add one line to get an interactive dashboard for reviewing all screenshot differences: diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index ca016321..1cbc954c 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -6,7 +6,7 @@ Version 2.0 introduces a new canonical namespace (`SnapDiff`) for cleaner, more discoverable code. The public DSL remains unchanged — your existing `screenshot` and `assert_matches_screenshot` calls work without modification. This guide covers the optional migration path for settings and the new namespace. -**Status:** 2.0 is the **transitional** release — the v1 API and the canonical `SnapDiff` API both work. **2.1 removes** everything 2.0 warns about (the legacy namespaces, the ChunkyPNG driver, `shift_distance_limit`, the `driver:` setting and the driver abstraction). There is no 3.0. Migrating on 2.0 is optional; doing it before 2.1 is not. +**Status:** 2.0 is the **transitional** release — the v1 API and the canonical `SnapDiff` API both work. **2.1 removes** everything 2.0 warns about (the legacy namespaces, the ChunkyPNG driver, `shift_distance_limit`, the `driver:` setting and the driver abstraction, and the `fail_if_new` / `pending_if_new` / `fail_on_difference` booleans that `record` replaces). There is no 3.0. Migrating on 2.0 is optional; doing it before 2.1 is not. **Estimated upgrade time:** 5–15 minutes (most users need only the Gemfile pin) @@ -114,7 +114,7 @@ The most commonly-used settings and how to update them: | `save_path` | `Capybara::Screenshot.save_path = "doc/screenshots"` | `SnapDiff.config.save_path = "doc/screenshots"` | Where baseline screenshots are stored | | `window_size` | `Capybara::Screenshot.window_size = [1280, 1024]` | `SnapDiff.config.window_size = [1280, 1024]` | Browser viewport size for consistent screenshots | -**All 27 settings** from both legacy namespaces are available via `SnapDiff.config.` — see the [Configuration Reference](configuration.md) for the full list. One rename to note: `Capybara::Screenshot.enabled` becomes `SnapDiff.config.screenshot_enabled` (it would otherwise collide with `Capybara::Screenshot::Diff.enabled`, which keeps the bare `enabled` name). +**All 28 settings** from both legacy namespaces are available via `SnapDiff.config.` — see the [Configuration Reference](configuration.md) for the full list. One rename to note: `Capybara::Screenshot.enabled` becomes `SnapDiff.config.screenshot_enabled` (it would otherwise collide with `Capybara::Screenshot::Diff.enabled`, which keeps the bare `enabled` name). --- @@ -172,9 +172,10 @@ This means you can migrate your codebase incrementally **now**, before opting in ### Deprecation Warnings -v2.0 emits four different things, and it is worth knowing which is which. The first two are +v2.0 emits five 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. +about options that never did anything; the fifth is about the new-screenshot booleans that +`record` replaces. 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 @@ -293,6 +294,42 @@ It applies to every route into a comparison — `screenshot`, `assert_matches_sc `median_filter_window_size`, `perceptual_threshold`, `screenshot_format`, `shift_distance_limit`, `skip_area`, `stability_time_limit`, `tolerance` and `wait`. +#### 5. The new-screenshot booleans — superseded by `record`, removed in 2.1 + +`fail_if_new`, `pending_if_new` and `fail_on_difference` each answered part of "what happens when +there is no baseline, or when there is a difference" — and none of them named the action people +actually want, which is *accept this change*. 2.0 adds the verb: + +```ruby +SnapDiff.config.record = :once # default. Record a screenshot that has no baseline. +SnapDiff.config.record = :none # strict. A missing baseline always fails. +SnapDiff.config.record = :all # re-record everything. THE BULK-ACCEPT MODE. +``` + +| Old | New | Note | +|-----|-----|------| +| `fail_if_new = true` | `record = :none` | the mode means the same thing on CI and off it | +| `fail_if_new = false` | `record = :once` | | +| `pending_if_new = true` | `record = :none`, or `:once` | `:none` fails with the `git add` command attached; `:once` records and lists it in the end-of-run summary | +| `fail_on_difference = false` | `record = :all` | to *accept* the new rendering rather than ignore the difference | + +All three keep working for the whole 2.x line (2.0 deletes nothing) and each warns once per +process, from the point you set it: + +``` +[snap_diff deprecation] `fail_if_new` is REMOVED in 2.1: the record modes replace it. `SnapDiff.config.record = :none` is `fail_if_new = true`, `= :once` is `fail_if_new = false` -- and unlike the boolean, a mode means the same thing on CI and off it. See docs/configuration.md. Silence with `SnapDiff.silence_deprecations = true` or SNAP_DIFF_SILENCE_DEPRECATIONS=1. (shown once per process) (called from /app/test/test_helper.rb:9) +``` + +**Nothing changes if you set none of them.** With no `record` line, `record` reads back as `:none` +under CI and `:once` off it — exactly what `fail_if_new` already did, sniff and all. The +missing-baseline default is deliberately unchanged; `:none` is how you opt into strictness +explicitly. **Precedence:** an explicitly set mode outranks `fail_if_new`, and `fail_if_new` +decides only when no mode was set — the same rule `fail_if_new` itself has over the `CI` sniff. + +`record = :all` **refuses to run under CI**: it accepts every rendering by design, so left in a +committed config file it would be a build that compares nothing and passes forever. See +[Record modes](configuration.md#record-modes--accepting-changes). + #### Silent by design Some legacy names never warn individually, and that is deliberate — the migration notice above is diff --git a/docs/architecture.md b/docs/architecture.md index 87661b89..a972161f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -234,13 +234,13 @@ The two legacy views are organized into two namespaces: **`Capybara::Screenshot::Diff`** — comparison settings: - `driver`, `tolerance`, `color_distance_limit`, `perceptual_threshold`, `shift_distance_limit` -- `area_size_limit`, `skip_area`, `fail_if_new`, `fail_on_difference`, `delayed` +- `area_size_limit`, `skip_area`, `record`, `fail_if_new`, `fail_on_difference`, `delayed` -The canonical way in is `SnapDiff.configure { |config| ... }` (all 27 settings flat on one object). `SnapDiff.start` and `Capybara::Screenshot::Diff.configure` are the two-holder block shape over the same storage — since ADR-008 step 7b, `Diff.configure` forwards to `SnapDiff.start` rather than the other way round. +The canonical way in is `SnapDiff.configure { |config| ... }` (all 28 settings flat on one object). `SnapDiff.start` and `Capybara::Screenshot::Diff.configure` are the two-holder block shape over the same storage — since ADR-008 step 7b, `Diff.configure` forwards to `SnapDiff.start` rather than the other way round. `Config` also owns the derived values that used to live on the legacy modules: `active?` (ex `Capybara::Screenshot.active?`), `screenshot_area` / `screenshot_area_abs`, and `default_options` (ex `Capybara::Screenshot::Diff.default_options`, the option hash handed to `SnapDiff::Comparison`). The legacy module methods one-line forward here. -**Default timing contract:** every *stored* default is evaluated once, in `Config#initialize`, which runs at require time of `config.rb` — the same load moment the old `mattr_accessor` default blocks evaluated at. `root` (from `Rails.root`) must never become a lazy read-time default. Two values are deliberately live: `default_options[:wait]`, a method-body read of `Capybara.default_max_wait_time`, and `fail_if_new`, whose reader falls back to `ENV["CI"]` whenever nothing explicit was set — an explicit setting outranks the environment, so the sniff cannot be frozen into storage. +**Default timing contract:** every *stored* default is evaluated once, in `Config#initialize`, which runs at require time of `config.rb` — the same load moment the old `mattr_accessor` default blocks evaluated at. `root` (from `Rails.root`) must never become a lazy read-time default. Two values are deliberately live: `default_options[:wait]`, a method-body read of `Capybara.default_max_wait_time`, and `fail_if_new`, whose reader falls back to `ENV["CI"]` whenever nothing explicit was set — an explicit setting outranks the environment, so the sniff cannot be frozen into storage. `record` is the third: its reader falls back to `fail_if_new` (`:none` when it is on, `:once` when it is off), which is what makes the record modes a rename of the existing behaviour rather than a change to it — the matcher branches on the mode alone and a suite with no `record` line takes exactly the branches it always took. ## File Layout @@ -249,7 +249,7 @@ lib/ snap_diff.rb # SnapDiff module: compare/start/configure/config snap_diff/ # Canonical implementation (v2) dsl.rb # screenshot(), screenshot_group(), etc. - config.rb # SnapDiff::Config — THE storage for all 27 settings + config.rb # SnapDiff::Config — THE storage for all 28 settings errors.rb # Error / ExpectationNotMet / UnstableImage / WindowSizeMismatchError region.rb # SnapDiff::Region — bounding box (+ eager top-level ::Region alias) deprecation.rb # Warn-once-per-constant machinery diff --git a/docs/ci-integration.md b/docs/ci-integration.md index ac296b4c..873a0137 100644 --- a/docs/ci-integration.md +++ b/docs/ci-integration.md @@ -253,15 +253,23 @@ jobs: -> **`CI: ""` is what makes this job able to record a *new* baseline.** `fail_if_new` -> defaults to `true` whenever `ENV["CI"]` is set and non-empty, and the check runs -> **before** the capture ([`screenshot_matcher.rb`](https://github.com/snap-diff/snap_diff-capybara/blob/master/lib/snap_diff/screenshot_matcher.rb) -> — `check_base_screenshot` precedes `capture_screenshot`). So on a stock GitHub Actions -> runner a screenshot with no committed baseline raises `No existing screenshot found for -> …` and **nothing is written to disk** — the commit step then finds nothing to commit, -> whatever the error message suggests. Clearing `CI` for this one step (or setting -> `SnapDiff.config.fail_if_new = false`) lets both new and changed baselines land. -> *Changed* baselines are rewritten either way; only new ones need this. +> **Set the record mode, or clear `CI`, to let this job record a *new* baseline.** With +> nothing set, a missing baseline fails whenever `ENV["CI"]` is set and non-empty — so on a +> stock GitHub Actions runner a screenshot with no committed baseline raises `No existing +> screenshot found for …`. `SnapDiff.config.record = :once` +> (or clearing `CI` for this one step, or the older `SnapDiff.config.fail_if_new = false`) +> records new baselines instead. *Changed* baselines are rewritten either way; only new ones +> need this. See [Record modes](configuration.md#record-modes--accepting-changes). +> +> The screenshot itself **is** written before the raise ([`screenshot_matcher.rb`](https://github.com/snap-diff/snap_diff-capybara/blob/master/lib/snap_diff/screenshot_matcher.rb) +> — `capture_screenshot` precedes `fail_if_new_screenshot`), so the `git add` the message names +> is a command you can actually run. But the raise still fails the test it happened in, and a red +> job usually never reaches the commit step — which is why a recording job sets the mode rather +> than relying on the files being there. +> +> **`record = :all` is not the mode for this job** — it refuses to run under CI, because it +> would accept every *changed* rendering unreviewed as well. `:once` records what is new and +> keeps comparing everything that already has a baseline. **How it works:** 1. Go to Actions → "Update Screenshot Baselines" → "Run workflow" diff --git a/docs/configuration.md b/docs/configuration.md index c7844813..441ea914 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -48,10 +48,87 @@ exception: `Capybara::Screenshot.enabled` is `SnapDiff.config.screenshot_enabled `SnapDiff.config.enabled` is taken by `Capybara::Screenshot::Diff.enabled`. See [SnapDiff — the canonical API](snapdiff.md) for the full SnapDiff-native surface. -**Note:** `fail_if_new` defaults to `true` in CI environments (when `ENV['CI']` is set to a non-empty value). New screenshots are allowed locally but rejected in CI — no configuration needed. Setting it yourself outranks the environment: `fail_if_new = false` stays `false` under `CI=true`, and `fail_if_new = true` stays `true` off CI. Assign `nil` to hand it back to the environment. - **Note:** Setting `Capybara::Screenshot.enabled = false` is sufficient to disable all screenshots. There is no need to define no-op modules or monkey-patch the gem. +## Record modes — accepting changes + +`record` is the single setting for *what happens when a screenshot has no committed baseline, or +when you want to accept the ones that changed*. It replaces `fail_if_new`, which is removed in 2.1. + +```ruby +SnapDiff.config.record = :once # what a local run already does; see Precedence below for CI +``` + +| Mode | Missing baseline | Baseline present | Reach for it when | +|------|------------------|------------------|-------------------| +| `:once` | recorded, not compared | compared | the default — you want a normal run | +| `:none` | **fails** with the `git add` command attached | compared | every screenshot must already be recorded | +| `:all` | recorded | **re-recorded, not compared** | you changed the UI on purpose and want the new rendering to become the baseline | + +### `:all` — the bulk-accept verb + +After an intentional redesign that changed forty screenshots, re-record them in one run rather +than accepting them one failure at a time: + +```ruby +# test_helper.rb. There is no CLI flag — the gem has no runner to hang one on — +# so gate it on an environment variable of your own if you want one: +SnapDiff.config.record = ENV["ACCEPT_SCREENSHOTS"] ? :all : :once +``` + +```bash +ACCEPT_SCREENSHOTS=1 bin/rails test:system # with the line above in test_helper.rb +``` + +Every screenshot is written to its baseline path and nothing is compared, so `git status` lists +exactly what changed. Review the images, then commit them — see +[Accepting an intentional change](../README.md#accepting-an-intentional-change). + +At the end of the run the gem names what it accepted: + +``` +[snap_diff] record: :all re-recorded 3 screenshots WITHOUT comparing: checkout/cart, checkout/payment, checkout/review. Review the result before committing -- an unintended change is accepted just as silently. +``` + +> **`:all` refuses to run under CI** (when `ENV['CI']` is set to a non-empty value). It accepts +> every rendering by design, so a mode left in a committed config file would buy you a build that +> compares nothing and passes forever, with the "recorded" screenshots discarded when the runner +> is torn down. Re-record locally, where you can look at the result. +> +> A CI job that needs to record screenshots with **no baseline yet** does not need `:all`: +> `record = :once` records those and still compares everything that has a baseline. See +> [CI integration](ci-integration.md). + +### Per screenshot + +```ruby +assert_matches_screenshot "flaky_widget", record: :none # this one must already exist +``` + +### Precedence + +**An explicitly set mode outranks `fail_if_new`; `fail_if_new` decides only when no mode was set.** +That is the same rule `fail_if_new` itself has over the `CI` sniff — explicit outranks implicit, all +the way down. Per screenshot outranks the config; the config outranks `fail_if_new`. + +With **nothing** set, `record` reads back as `:none` under CI and `:once` off it — which is exactly +what `fail_if_new` has always done, so a suite that never mentions `record` behaves as it always +did. The missing-baseline default is deliberately unchanged: failing only under CI is what Jest, +AVA, Vitest, testthat and jest-image-snapshot all chose, and a screenshot baseline recorded on your +laptop is often worthless on another OS. `:none` makes strictness an explicit choice instead. + +| You wrote | `record` reads | Missing baseline | +|-----------|----------------|------------------| +| nothing, off CI | `:once` | recorded | +| nothing, under CI | `:none` | fails | +| `record = :once` | `:once` | recorded, on CI too | +| `record = :none` | `:none` | fails, off CI too | +| `record = :once`, `fail_if_new = true` | `:once` | recorded — the mode wins | +| `record = nil`, `fail_if_new = true` | `:none` | fails — nil hands it back | + +A misspelt mode raises `ArgumentError` at the point you set it, rather than reading back as +"nobody said". + ## Recommended tolerance values | Use Case | VIPS `tolerance` | ChunkyPNG `color_distance_limit` | `stability_time_limit` | @@ -107,7 +184,8 @@ screenshot 'dashboard', color_distance_limit: 15 ## Configuration Tiers **Tier 1 — Zero config (works immediately):** -`blur_active_element`, `hide_caret`, and `fail_if_new` (in CI) are enabled by default. +`blur_active_element` and `hide_caret` are on by default, and `record` behaves as `:none` in CI +(a missing baseline fails) and `:once` off it. Just `require 'snap_diff/integrations/minitest'` (legacy: `capybara_screenshot_diff/minitest`) and call `screenshot`. **Tier 2 — Set when tests are flaky:** @@ -174,6 +252,10 @@ Capybara::Screenshot::Diff.enabled = ENV['COMPARE_SCREENSHOTS'] ### Tolerate screenshot differences +> **Removed in 2.1.** A screenshot that differs from its baseline fails — that is what the gem is +> for. To *accept* a difference, re-record it: [`record = :all`](#record-modes--accepting-changes). +> It keeps working for the whole 2.x line and warns once per process. + To allow screenshot differences, but still fail on functional errors, you can set the following option: ```ruby @@ -185,6 +267,11 @@ report while still reporting functional errors. ### Does not tolerate new screenshots +> **Removed in 2.1, superseded by [`record`](#record-modes--accepting-changes).** +> `record = :none` is `fail_if_new = true`; `record = :once` is `fail_if_new = false`. Unlike the +> boolean, a mode means the same thing on CI and off it. It keeps working for the whole 2.x line and +> warns once per process. + To fail the test if a new screenshot is taken, set the following option: ```ruby @@ -196,8 +283,17 @@ that does not have a corresponding previous image to compare against. This can be useful in situations where you want to ensure that every screenshot taken by your tests corresponds to an expected state of your application. +`fail_if_new` defaults to `true` in CI environments (when `ENV['CI']` is set to a non-empty value). +Setting it yourself outranks the environment: `fail_if_new = false` stays `false` under `CI=true`. +Assign `nil` to hand it back to the environment. Setting `record` outranks it either way. + ### Marks new screenshots as pending +> **Removed in 2.1.** It skips the test instead of saying what to do about the missing baseline. +> [`record = :none`](#record-modes--accepting-changes) fails with the `git add` command attached; +> `record = :once` records the screenshot and names it in the end-of-run summary. It keeps working +> for the whole 2.x line and warns once per process. + To mark tests as pending (skipped) if a new screenshot is taken without a baseline, set: ```ruby diff --git a/docs/snapdiff.md b/docs/snapdiff.md index 02dd9b14..b96cae24 100644 --- a/docs/snapdiff.md +++ b/docs/snapdiff.md @@ -110,7 +110,7 @@ loads the Minitest integration. See ## Configuration -All 27 settings live on one flat object, `SnapDiff.config` (a `SnapDiff::Config`). +All 28 settings live on one flat object, `SnapDiff.config` (a `SnapDiff::Config`). ```ruby # test_helper.rb / rails_helper.rb diff --git a/lib/snap_diff/config.rb b/lib/snap_diff/config.rb index 1491aee8..03d3794f 100644 --- a/lib/snap_diff/config.rb +++ b/lib/snap_diff/config.rb @@ -69,6 +69,7 @@ class Config capybara_screenshot_options delayed area_size_limit + record fail_if_new pending_if_new fail_on_difference @@ -83,14 +84,33 @@ class Config manager ].freeze - # shift_distance_limit and driver are excluded from the generated - # writers and hand written below (they announce their 2.1 removal); - # fail_if_new is excluded from the generated READER (it falls back to - # the environment). Generating them here too would print Ruby's + # The record modes, in the order they escalate: do nothing new, refuse + # to record, record everything. See {#record}. + RECORD_MODES = %i[once none all].freeze + + # @api private + # + # Shared by {#record=} and the per-screenshot `record:` option, which is + # resolved in {SnapDiff::ScreenshotMatcher}. A misspelt mode must not + # read back as "nobody said" and silently mean today's behaviour -- + # that is the exact silent no-op ADR-010 exists to stop. + def self.validate_record_mode!(mode) + return mode if mode.nil? || RECORD_MODES.include?(mode) + + raise ArgumentError, + "unknown record mode #{mode.inspect} -- one of #{RECORD_MODES.map(&:inspect).join(", ")} (or nil)" + end + + # shift_distance_limit, driver and the three older new-screenshot + # booleans are excluded from the generated writers and hand written + # below (they announce their 2.1 removal, or validate); fail_if_new and + # record are excluded from the generated READER (both fall back rather + # than returning storage). Generating them here too would print Ruby's # "method redefined" warning on every load. - attr_accessor(*(SETTINGS - %i[root shift_distance_limit driver fail_if_new])) - attr_reader :root, :shift_distance_limit, :driver - attr_writer :fail_if_new + attr_accessor(*(SETTINGS - %i[ + root shift_distance_limit driver record fail_if_new pending_if_new fail_on_difference + ])) + attr_reader :root, :shift_distance_limit, :driver, :pending_if_new, :fail_on_difference def initialize # Every setting gets its ivar up front (nil-defaulted ones included) @@ -140,6 +160,63 @@ def fail_if_new @fail_if_new.nil? ? !ENV["CI"].to_s.empty? : @fail_if_new end + # Announces the 2.1 removal. The writer, not the reader: {#record} reads + # +fail_if_new+ on every screenshot for everyone, and #initialize stores + # no default at all, so only a user who sets it hears about it. + def fail_if_new=(value) + Removal.warn_once(:fail_if_new, Removal::FAIL_IF_NEW_REMOVED) + @fail_if_new = value + end + + # Same shape, same reason: SnapDiff.pending_screenshots_message reads + # this for everyone, and #initialize seeds the +false+ default straight + # into the ivar. + def pending_if_new=(value) + Removal.warn_once(:pending_if_new, Removal::PENDING_IF_NEW_REMOVED) + @pending_if_new = value + end + + # Same: ScreenshotAssertion.verify_screenshots! reads it once per test + # for everyone, and #initialize seeds the +true+ default. + def fail_on_difference=(value) + Removal.warn_once(:fail_on_difference, Removal::FAIL_ON_DIFFERENCE_REMOVED) + @fail_on_difference = value + end + + # THE ACCEPT WORKFLOW (#259). What to do about a screenshot whose + # baseline is missing -- or, for +:all+, about every screenshot there + # is. VCR-shaped: modes in config, because there is no runner to hang a + # CLI flag on. + # + # +:once+:: (default) record a screenshot that has no committed + # baseline; compare against the baseline when there is one. + # +:none+:: strict. A missing baseline always fails. + # +:all+:: re-record. Every screenshot is written as the new baseline + # and nothing is compared -- the bulk-accept verb, for the + # redesign that changed forty screenshots at once. Refused + # under CI (see {SnapDiff::ScreenshotMatcher}). + # + # PRECEDENCE: an explicitly set mode outranks +fail_if_new+; nil means + # nobody said, and only then does +fail_if_new+ answer -- so a setup + # with no +record+ line behaves EXACTLY as it did before this setting + # existed, CI sniff and all. +:none+ is the mode spelling of + # fail_if_new = true and +:once+ of = false, which is + # why the fallback can express today's behaviour without a special case + # anywhere downstream: the matcher branches on the mode alone. + # + # The missing-baseline DEFAULT is deliberately unchanged -- failing only + # under CI is what Jest, AVA, Vitest, testthat and jest-image-snapshot + # all chose, and a locally recorded screenshot baseline is often + # worthless across OS. +:none+ makes strictness an explicit choice + # instead; that is the whole point of having the mode. + def record + @record || (fail_if_new ? :none : :once) + end + + def record=(mode) + @record = Config.validate_record_mode!(mode) + end + # Overrides the generated accessor above to announce the 2.1 removal # (chunky_png-only, and chunky_png goes too). The writer, not the reader: # the reader runs on every comparison through #default_options, including diff --git a/lib/snap_diff/legacy_shims.rb b/lib/snap_diff/legacy_shims.rb index fe708489..1af39818 100644 --- a/lib/snap_diff/legacy_shims.rb +++ b/lib/snap_diff/legacy_shims.rb @@ -161,6 +161,12 @@ def self.require_unit(target) # Capybara::Screenshot::Diff delayed: [Capybara::Screenshot::Diff, :delayed], area_size_limit: [Capybara::Screenshot::Diff, :area_size_limit], + # New in 2.0 and it has no v1 history, but it is storage on the one + # Config like everything else, and the two-views invariant is + # all-or-nothing: an unmapped setting is storage the v1 surface cannot + # see. Mapping it also means a user still on the old namespace reaches + # the accept workflow without migrating first. + record: [Capybara::Screenshot::Diff, :record], fail_if_new: [Capybara::Screenshot::Diff, :fail_if_new], pending_if_new: [Capybara::Screenshot::Diff, :pending_if_new], fail_on_difference: [Capybara::Screenshot::Diff, :fail_on_difference], diff --git a/lib/snap_diff/removal.rb b/lib/snap_diff/removal.rb index ff756e70..e5add3c2 100644 --- a/lib/snap_diff/removal.rb +++ b/lib/snap_diff/removal.rb @@ -50,6 +50,31 @@ module Removal "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." + # --- the new-screenshot booleans, superseded by `record:` (#259) ----- + # + # Three separate settings, each answering some part of "what happens + # when there is no baseline / when there is a difference", none of them + # naming the action the user actually wants. 2.0 adds the verb -- + # `SnapDiff.config.record` -- and 2.1 removes the booleans. Each message + # names the mode that replaces THAT setting, and nothing more: `record:` + # supersedes the missing-baseline half squarely and the other two only + # obliquely, and a message that overclaims is a message that misroutes. + + FAIL_IF_NEW_REMOVED = + "`fail_if_new` is REMOVED in 2.1: the record modes replace it. `SnapDiff.config.record = :none` " \ + "is `fail_if_new = true`, `= :once` is `fail_if_new = false` -- and unlike the boolean, a mode " \ + "means the same thing on CI and off it. See docs/configuration.md." + + PENDING_IF_NEW_REMOVED = + "`pending_if_new` is REMOVED in 2.1: it skips the test instead of saying what to do about the " \ + "missing baseline. `SnapDiff.config.record = :none` fails with the `git add` command attached; " \ + "`= :once` records it and reports it in the end-of-run summary. See docs/configuration.md." + + FAIL_ON_DIFFERENCE_REMOVED = + "`fail_on_difference` is REMOVED in 2.1: a screenshot that differs from its baseline fails, " \ + "which is what the gem is for. To ACCEPT a difference, re-record it -- " \ + "`SnapDiff.config.record = :all` -- and commit the result. See docs/configuration.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 diff --git a/lib/snap_diff/reporting.rb b/lib/snap_diff/reporting.rb index 0497cd78..39b1915e 100644 --- a/lib/snap_diff/reporting.rb +++ b/lib/snap_diff/reporting.rb @@ -18,6 +18,7 @@ module Reporting @reporters = [] @mutex = Mutex.new @missing_baselines = Set.new + @rerecorded_baselines = Set.new class << self attr_reader :reporters, :mutex @@ -47,6 +48,21 @@ def reset_missing_baselines! @mutex.synchronize { @missing_baselines.clear } end + # Remembers a screenshot re-recorded by `record: :all` -- captured as + # the new baseline with nothing compared against it. A separate tally + # from {record_missing_baseline} on purpose: "there was no baseline" + # and "there was one and we accepted the new rendering over it" are + # different facts, and the summary must not claim the first when the + # second happened. + def record_rerecorded_baseline(name) + @mutex.synchronize { !!@rerecorded_baselines.add?(name) } + end + + # @api private + def reset_rerecorded_baselines! + @mutex.synchronize { @rerecorded_baselines.clear } + end + # Registers a reporter for the rest of the process. The canonical way # in: the append happens under the mutex, so concurrent registrations # cannot lose one (issue #217 item 2). `reporters` stays public and @@ -90,6 +106,10 @@ def finalize! if (msg = missing_baselines_summary) $stdout.puts msg end + + if (msg = rerecorded_baselines_summary) + $stdout.puts msg + end end # --- fork-parallel reports (issue #258) --------------------------- @@ -139,6 +159,7 @@ def parallel_fragments_dir def dump_parallel_fragment payload = { "missing_baselines" => @mutex.synchronize { @missing_baselines.to_a }, + "rerecorded_baselines" => @mutex.synchronize { @rerecorded_baselines.to_a }, "reporters" => @mutex.synchronize { @reporters.dup } .map { |reporter| reporter.dump_state if reporter.respond_to?(:dump_state) } } @@ -162,6 +183,9 @@ def merge_parallel_fragments! payload = JSON.parse(File.read(fragment)) @mutex.synchronize { payload["missing_baselines"].each { |name| @missing_baselines << name } } + # `to_a` on a fresh install predates this key: a fragment written + # by an older worker has no "rerecorded_baselines" at all. + @mutex.synchronize { payload.fetch("rerecorded_baselines", []).each { |name| @rerecorded_baselines << name } } reporters_snapshot = @mutex.synchronize { @reporters.dup } payload["reporters"].each_with_index do |state, index| @@ -187,6 +211,22 @@ def missing_baselines_summary "[snap_diff] #{label} had no committed baseline and #{(names.size == 1) ? "was" : "were"} NOT compared: " \ "#{names.join(", ")}. Commit the captured file(s) to enable comparison." end + + # The other half of "nothing was compared", and the louder one: + # `record: :all` accepts whatever the page rendered as the new + # baseline. Names the screenshots that really went down that path this + # run, so `git add` lands on the right files -- and so nobody commits + # forty accepted regressions without being told they were accepted. + # + # @return [String, nil] nil when nothing was re-recorded + def rerecorded_baselines_summary + names = @mutex.synchronize { @rerecorded_baselines.to_a } + return if names.empty? + + label = (names.size == 1) ? "1 screenshot" : "#{names.size} screenshots" + "[snap_diff] record: :all re-recorded #{label} WITHOUT comparing: #{names.join(", ")}. " \ + "Review the result before committing -- an unintended change is accepted just as silently." + end end end end diff --git a/lib/snap_diff/screenshot_matcher.rb b/lib/snap_diff/screenshot_matcher.rb index 19765a80..9ee17b01 100644 --- a/lib/snap_diff/screenshot_matcher.rb +++ b/lib/snap_diff/screenshot_matcher.rb @@ -12,7 +12,7 @@ module SnapDiff class ScreenshotMatcher - attr_reader :screenshot_full_name, :driver_options, :screenshot_format + attr_reader :screenshot_full_name, :driver_options, :screenshot_format, :record_mode def initialize(screenshot_full_name, options = {}) @screenshot_full_name = screenshot_full_name @@ -21,11 +21,24 @@ def initialize(screenshot_full_name, options = {}) Removal.warn_once(:driver_setting, Removal::DRIVER_REMOVED) if options.key?(:driver) @driver_options = SnapDiff.config.default_options.merge(options) + # `record:` is a workflow mode, not a capture or comparison option, so + # it is carved out here rather than added to Comparison::KNOWN_OPTIONS + # -- a key that hash accepts and nothing downstream reads is the + # silent no-op ADR-010 forbids. Deleted before anything else touches + # the hash, so no later split has to know about it. + @record_mode = resolve_record_mode(@driver_options.delete(:record)) + @screenshot_format = @driver_options[:screenshot_format] @snapshot = SnapDiff::SnapManager.snapshot(screenshot_full_name, @screenshot_format) end def build_screenshot_assertion(skip_stack_frames: 0) + # Here rather than in #initialize, so it covers exactly the path where + # `:all` means anything. #capture never compares against a baseline, so + # the mode has nothing to say about it and refusing there would be a + # failure invented out of a setting that changes nothing. + refuse_bulk_record_under_ci! if record_mode == :all + Capture::Viewport.prepare!(SnapDiff.config.window_size) prepare_screenshot_options check_base_screenshot @@ -41,7 +54,7 @@ def build_screenshot_assertion(skip_stack_frames: 0) # Pre-computation: No need to compare without base screenshot # NOTE: Consider to return PreValid Assertion Value Object with hard coded valid result unless need_to_compare? - SnapDiff.session.record_new_screenshot(screenshot_full_name) + record_uncompared_screenshot return end @@ -61,10 +74,53 @@ def capture private + # The per-screenshot option outranks the configured mode, which in turn + # outranks `fail_if_new` (SnapDiff::Config#record). One resolution, so + # both routes to a mode agree about everything downstream. + def resolve_record_mode(per_screenshot) + SnapDiff::Config.validate_record_mode!(per_screenshot) || SnapDiff.config.record + end + + # `:all` accepts every rendering as correct -- that is the feature. On + # CI there is nobody to review the result and the re-recorded files go + # away with the box, so a mode left in a committed config file buys a + # build that compares nothing and passes, forever. That is precisely how + # Percy goes green on a job that lost its token; refuse it rather than + # shipping our own version. + # + # A CI job that must record NEW baselines does not need `:all` at all: + # `record: :once` records them and still compares everything that has a + # baseline (docs/ci-integration.md). + def refuse_bulk_record_under_ci! + return if ENV["CI"].to_s.empty? + + raise SnapDiff::ExpectationNotMet.new(<<~ERROR.chomp, caller) + `record: :all` re-records every baseline WITHOUT comparing, so it refuses to run under CI (ENV["CI"] is set). + Nothing would be verified and the recorded screenshots would be discarded with the runner. + Re-record locally, review the result, and commit the screenshots. + To let a CI job record screenshots that have no baseline yet: SnapDiff.config.record = :once + ERROR + end + + # No `record_mode != :all` clause here on purpose: mutation testing + # showed one guards nothing. #check_base_screenshot already leaves `:all` + # with no base file at all, so this reads false for it either way, and a + # second condition that cannot fire is a second thing to keep true. def need_to_compare? @snapshot.base_path.exist? end + # `:all` re-records, so nothing was compared and nothing is "new" in the + # missing-baseline sense. Reported through its own channel, which counts + # only the screenshots that really went down this path. + def record_uncompared_screenshot + if record_mode == :all + SnapDiff::Reporting.record_rerecorded_baseline(screenshot_full_name) + else + SnapDiff.session.record_new_screenshot(screenshot_full_name) + end + end + def prepare_screenshot_options area_calculator = AreaCalculator.new(driver_options.delete(:crop), driver_options[:skip_area]) @@ -78,16 +134,26 @@ def prepare_screenshot_options # moment at which `@snapshot.path` still tells us whether the user had a # PNG sitting there already -- the case that confuses people most. def check_base_screenshot + # `:all` does not ask git for a baseline -- there is nothing to + # compare against. A `.base.` left by an earlier failing run + # would otherwise sit beside the re-recorded screenshot and land in + # the user's `git add`. + return discard_base_screenshot if record_mode == :all + @snapshot.checkout_base_screenshot return if @snapshot.base_path.exist? # fail_if_new_screenshot raises after the capture and says the same # thing with the fix attached; two messages for one missing baseline # is one too many. - return if SnapDiff.config.fail_if_new + return if record_mode == :none warn_no_committed_baseline end + def discard_base_screenshot + @snapshot.base_path.delete if @snapshot.base_path.exist? + end + # Runs AFTER the capture, so `@snapshot.path` names a file that is # really there and `git add` on it is a command the user can run on this # very test run (#260). Before, the raise came first and the screenshot @@ -95,12 +161,12 @@ def check_base_screenshot # place fail_if_new is on by default. def fail_if_new_screenshot return if @snapshot.base_path.exist? - return unless SnapDiff.config.fail_if_new + return unless record_mode == :none raise SnapDiff::ExpectationNotMet.new(<<~ERROR.chomp, caller) No existing screenshot found for #{@snapshot.path}! To record it: `git add #{@snapshot.path}` and commit -- baselines are read from git. - To allow new screenshots: SnapDiff.config.fail_if_new = false + To allow new screenshots: SnapDiff.config.record = :once (was: SnapDiff.config.fail_if_new = false) ERROR end diff --git a/test/test_helper.rb b/test/test_helper.rb index 7b261ff6..8d211749 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -117,6 +117,7 @@ class ActiveSupport::TestCase # baseline-less screenshots pile up and get listed in one enormous line # at the end of `rake test`. SnapDiff::Reporting.reset_missing_baselines! + SnapDiff::Reporting.reset_rerecorded_baselines! end def persist_comparisons? diff --git a/test/unit/record_modes_test.rb b/test/unit/record_modes_test.rb new file mode 100644 index 00000000..f943cd9d --- /dev/null +++ b/test/unit/record_modes_test.rb @@ -0,0 +1,444 @@ +# frozen_string_literal: true + +require "test_helper" +require "snap_diff" + +# THE ACCEPT WORKFLOW (#259). +# +# Accepting an intentional UI change is the most frequent action in the +# product and until now it had no verb: users read vcs.rb to work out that +# the answer is `git add` + commit. `record:` is that verb, VCR-shaped -- +# config, not a CLI flag, because there is no runner to hang a flag on. +# +# :once the default. Record when there is no committed baseline; compare +# when there is. A RENAME of what the gem already did -- it adds no +# branch, which is what makes "behaviour-identical to today" a +# structural fact rather than a claim. +# :none strict. A missing baseline always fails, whatever `fail_if_new` +# says. +# :all re-record. Every screenshot becomes the new baseline and nothing +# is compared. THE BULK-ACCEPT VERB. +# +# PRECEDENCE, in one line: an explicitly set mode outranks `fail_if_new`, +# and `fail_if_new` is what decides when nothing was set. Same property +# #267 gave `fail_if_new` itself over the CI sniff -- explicit outranks +# implicit, all the way down. +class RecordModesTest < ActiveSupport::TestCase + include SnapDiff::DSL + include DSLStub + + def before_setup + @original_root = SnapDiff.config.root + @new_root = Dir.mktmpdir + SnapDiff.config.root = Pathname.new(@new_root) + super + end + + def after_teardown + super + SnapDiff.config.root = @original_root + FileUtils.remove_entry(@new_root) if @new_root + SnapDiff::Reporting.reset_rerecorded_baselines! + end + + # --- the default is not touched -------------------------------------- + # + # The missing-baseline default was researched and deliberately NOT + # flipped: community precedent is CI-only failure. `record` reads back as + # a real mode either way, so the matcher has one thing to branch on -- but + # the answer with nothing set is exactly what `fail_if_new` already said. + + test "with nothing set, record mirrors fail_if_new -- :once off CI, :none on it" do + SnapDiff.config.record = nil + + SnapDiff.config.fail_if_new = false + assert_equal :once, SnapDiff.config.record + + SnapDiff.config.fail_if_new = true + assert_equal :none, SnapDiff.config.record + end + + # THE PRECEDENCE GUARD. Both surfaces set and disagreeing: the mode wins. + test "an explicit record mode outranks fail_if_new in both directions" do + SnapDiff.config.fail_if_new = true + SnapDiff.config.record = :once + assert_equal :once, SnapDiff.config.record + + SnapDiff.config.fail_if_new = false + SnapDiff.config.record = :none + assert_equal :none, SnapDiff.config.record + end + + test "assigning nil hands the mode back to fail_if_new" do + SnapDiff.config.fail_if_new = true + SnapDiff.config.record = :once + SnapDiff.config.record = nil + + assert_equal :none, SnapDiff.config.record + end + + # --- :once ------------------------------------------------------------ + + test ":once records a screenshot that has no committed baseline, and does not compare it" do + SnapDiff.config.record = :once + name = "c_#{Time.now.nsec}" + + capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_nil SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + assert_includes SnapDiff.session.new_screenshots, name + assert_predicate SnapDiff::SnapManager.path_for(name).path, :exist? + end + + # `record = :once` is the replacement for `fail_if_new = false`, so it has + # to beat a `fail_if_new` that is on -- including the CI default. + test ":once allows a new screenshot even where fail_if_new would have raised" do + SnapDiff.config.fail_if_new = true + SnapDiff.config.record = :once + name = "c_#{Time.now.nsec}" + + capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_nil SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + assert_predicate SnapDiff::SnapManager.path_for(name).path, :exist? + end + + test ":once compares against a committed baseline like it always did" do + SnapDiff.config.record = :once + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :c) + assertion = SnapDiff::ScreenshotMatcher.new(snap.full_name).build_screenshot_assertion + + assert_instance_of SnapDiff::ScreenshotAssertion, assertion + assert_predicate assertion.compare, :different? + end + end + + # --- :none ------------------------------------------------------------ + + test ":none fails on a missing baseline even with fail_if_new explicitly false" do + SnapDiff.config.fail_if_new = false + SnapDiff.config.record = :none + name = "c_#{Time.now.nsec}" + + error = SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_raises(SnapDiff::ExpectationNotMet) do + SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + assert_includes error.message, SnapDiff::SnapManager.path_for(name).path.to_s + end + + # #260, re-asserted for the mode that raises: the message says `git add + # `, so has to be on disk by the time it is printed. + test "the :none failure names a file that is really on disk and offers the record fix" do + SnapDiff.config.record = :none + name = "c_#{Time.now.nsec}" + path = SnapDiff::SnapManager.path_for(name).path + + error = SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_raises(SnapDiff::ExpectationNotMet) do + SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + assert_predicate path, :exist? + assert_includes error.message, "SnapDiff.config.record = :once" + end + + test ":none does not also warn about the missing baseline" do + SnapDiff.config.record = :none + + _out, err = capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_raises(SnapDiff::ExpectationNotMet) do + SnapDiff::ScreenshotMatcher.new("c_#{Time.now.nsec}").build_screenshot_assertion + end + end + end + + assert_no_match(/No committed baseline/, err) + end + + # --- :all, the bulk-accept verb --------------------------------------- + + # THE ONE THAT MATTERS: a baseline exists AND differs, and :all accepts it. + # Asserted on the BYTES on disk, not on the absence of an exception -- + # "no assertion was built" would also be true if the capture never ran. + test ":all leaves the newly captured bytes on disk without comparing them" do + SnapDiff.config.record = :all + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :c) + + assert_nil SnapDiff::ScreenshotMatcher.new(snap.full_name).build_screenshot_assertion + + assert_captured(:c, snap.path, "the re-recorded screenshot must be the new capture") + assert_not_captured(:a, snap.path, "the old baseline must not survive a re-record") + assert_not_predicate SnapDiff.session, :assertions_present? + end + end + + # :all does not ask git for a baseline -- there is nothing to compare + # against. A `.base.` from an earlier run would otherwise sit beside + # the re-recorded screenshot and land in the user's `git add`. + test ":all asks VCS for nothing and leaves no .base file behind" do + SnapDiff.config.record = :all + asked = [] + + SnapDiff::Vcs.stub(:checkout_vcs, ->(*args) { + asked << args + true + }) do + snap = create_snapshot_for(:a, :c) + SnapDiff::ScreenshotMatcher.new(snap.full_name).build_screenshot_assertion + + assert_empty asked, ":all must not spend a git process on a baseline it will not use" + assert_not_predicate snap.base_path, :exist? + end + end + + test ":all never fails on a missing baseline, whatever fail_if_new says" do + SnapDiff.config.fail_if_new = true + SnapDiff.config.record = :all + name = "c_#{Time.now.nsec}" + + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_nil SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + + assert_predicate SnapDiff::SnapManager.path_for(name).path, :exist? + end + + # What :all reports has to be what actually happened: the names that went + # down the re-record path this run, and no others. + test ":all summarises exactly the screenshots it re-recorded" do + SnapDiff.config.record = :all + names = nil + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + names = 3.times.map { create_snapshot_for(:a, :c).full_name } + names.each { |n| SnapDiff::ScreenshotMatcher.new(n).build_screenshot_assertion } + end + + summary = SnapDiff::Reporting.rerecorded_baselines_summary + + assert_match(/3 screenshots/, summary) + names.each { |n| assert_includes summary, n } + assert_match(/[Rr]eview/, summary, "accepting 3 baselines unreviewed is the thing to say out loud") + end + + test "no re-record summary when nothing was re-recorded" do + SnapDiff.config.record = :once + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + SnapDiff::ScreenshotMatcher.new(create_snapshot_for(:a, :c).full_name).build_screenshot_assertion + end + + assert_nil SnapDiff::Reporting.rerecorded_baselines_summary + end + + # --- :all is hard to trigger by accident ------------------------------ + # + # Percy exits 0 when misconfigured, so a CI job that loses its token goes + # green forever. `:all` accepts every rendering BY DESIGN, so left in a + # committed config file it is exactly that failure: a build that compares + # nothing and passes, with the "recorded" files thrown away with the box. + # Refuse it under CI instead of building our own version of Percy. + + test ":all refuses to run under CI" do + SnapDiff.config.record = :all + + error = with_ci("true") do + assert_raises(SnapDiff::ExpectationNotMet) do + SnapDiff::ScreenshotMatcher.new("c_#{Time.now.nsec}").build_screenshot_assertion + end + end + + assert_match(/record/, error.message) + assert_match(/CI/, error.message) + end + + test ":all runs off CI" do + SnapDiff.config.record = :all + + with_ci("") do + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :c) + assert_nil SnapDiff::ScreenshotMatcher.new(snap.full_name).build_screenshot_assertion + end + end + end + + # #capture never compares against a baseline, so `:all` has nothing to say + # about it. Refusing there would be a failure invented out of a setting + # that changes nothing on that path. + test ":all does not refuse the compare-free #capture path under CI" do + SnapDiff.config.record = :all + name = "b_#{Time.now.nsec}" + + with_ci("true") do + SnapDiff::ScreenshotMatcher.new(name).capture + end + + assert_predicate SnapDiff::SnapManager.path_for(name).path, :exist? + end + + # The refusal is about :all only -- a CI run on any other mode is the + # normal case and must be untouched. + test ":once and :none are unaffected by CI" do + with_ci("true") do + SnapDiff.config.record = :once + + capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_nil SnapDiff::ScreenshotMatcher.new("c_#{Time.now.nsec}").build_screenshot_assertion + end + end + end + end + + # --- the per-screenshot option ---------------------------------------- + + test "a per-screenshot record: outranks the configured mode" do + SnapDiff.config.record = :none + + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_nil SnapDiff::ScreenshotMatcher.new("c_#{Time.now.nsec}", record: :all).build_screenshot_assertion + end + end + + test "a per-screenshot record: :none raises where the configured :once would not" do + SnapDiff.config.record = :once + + SnapDiff::Vcs.stub(:checkout_vcs, false) do + assert_raises(SnapDiff::ExpectationNotMet) do + SnapDiff::ScreenshotMatcher.new("c_#{Time.now.nsec}", record: :none).build_screenshot_assertion + end + end + end + + # `record:` is a workflow mode, not a capture or comparison option. It has + # to be carved out before the options hash reaches Comparison, or ADR-010's + # unknown-key check would warn at every user who sets it. + test "record: never reaches the capture or comparison options" do + seen = [] + screenshoter = Class.new do + define_method(:initialize) { |capture, comparison| seen << [capture, comparison] } + + def take_comparison_screenshot(snapshot) + snapshot.path.dirname.mkpath + FileUtils.cp(File.expand_path("a.png", TEST_IMAGES_DIR), snapshot.path) + end + end + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + SnapDiff.config.stub(:screenshoter, screenshoter) do + snap = create_snapshot_for(:a, :c) + SnapDiff::ScreenshotMatcher.new(snap.full_name, record: :once).build_screenshot_assertion + end + end + + capture_options, comparison_options = seen.fetch(0) + assert_not_includes capture_options, :record + assert_not_includes comparison_options, :record + assert_not_includes SnapDiff::Comparison::KNOWN_OPTIONS, :record, + ":record is carved out upstream, so accepting it at Comparison would be a silent no-op (ADR-010)" + end + + # --- a typo must not silently mean "not set" -------------------------- + + test "an unknown mode raises at config time" do + error = assert_raises(ArgumentError) { SnapDiff.config.record = :non } + + assert_match(/:non/, error.message) + assert_match(/:once/, error.message) + end + + test "an unknown per-screenshot mode raises" do + assert_raises(ArgumentError) do + SnapDiff::ScreenshotMatcher.new("c_#{Time.now.nsec}", record: "all") + end + end + + # --- THE USER'S CODE -------------------------------------------------- + # + # Their config, their assertion, through the documented DSL -- not the + # matcher internals every other example here pokes at. This is the shape + # of the accept workflow as it is documented, and if it stops working the + # feature is gone whatever the unit tests say. + + test "a user accepts an intentional change with SnapDiff.config.record = :all" do + snap = nil + + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :c) + + SnapDiff.configure { |config| config.record = :all } + + # false is what every "nothing was compared" path already returns -- + # the same answer a screenshot with no baseline gives today. + assert_equal false, assert_matches_screenshot(snap.full_name) + end + + SnapDiff.session.verify + + assert_captured(:c, snap.path, "the accepted rendering is what the user is left to `git add`") + end + + test "the same user assertion fails on the default mode" do + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :c) + + SnapDiff.configure { |config| config.record = :once } + + assert assert_matches_screenshot(snap.full_name) + + assert_raises(SnapDiff::ExpectationNotMet) { SnapDiff.session.verify } + end + end + + private + + # Compares by IMAGE, not by bytes: the screenshoter re-encodes what it + # captures, so a byte match against the source fixture proves nothing + # either way. + def assert_captured(fixture, path, message) + assert_predicate SnapDiff::Comparison.new(path, fixture_image_path_from(fixture)), :quick_equal?, message + end + + def assert_not_captured(fixture, path, message) + assert_not_predicate SnapDiff::Comparison.new(path, fixture_image_path_from(fixture)), :quick_equal?, message + end + + # This class exercises record modes; `:all` deliberately refuses to run under + # CI, so the whole class runs as a developer's laptop by default and the + # CI-refusal cases opt IN via `with_ci`. Without this, every `:all` example + # fails on CI for the very reason it is testing -- and passes locally, where + # ENV["CI"] is unset. Local green was half the bar here. + def setup + super + @original_ci = ENV.delete("CI") + end + + def teardown + ENV["CI"] = @original_ci + super + end + + def with_ci(value) + original = ENV["CI"] + ENV["CI"] = value + yield + ensure + ENV["CI"] = original + 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 b1748e78..a4e737c5 100644 --- a/test/unit/removed_in_2_1_deprecation_test.rb +++ b/test/unit/removed_in_2_1_deprecation_test.rb @@ -186,6 +186,90 @@ class MyOtherDriver assert_empty out.lines.grep(/`driver`/), "config's own default must not warn at the user" end + # --- the new-screenshot booleans, superseded by `record:` (#259) ------ + # + # These are the settings the accept workflow replaces. They keep working + # for the whole 2.x line (ADR-010: 2.0 deletes nothing) and each says so + # once, naming the mode that takes over. + + test "setting fail_if_new warns once and names the record mode that replaces it" do + lines = probe(<<~RUBY) + require "snap_diff" + 3.times { SnapDiff.config.fail_if_new = true } + RUBY + + assert_equal 1, lines.size, lines.join + assert_match(/`fail_if_new`/, lines.first) + assert_match(/REMOVED in 2\.1/, lines.first) + assert_match(/record = :none/, lines.first) + end + + test "setting pending_if_new warns once, naming 2.1" do + lines = probe(<<~RUBY) + require "snap_diff" + 3.times { SnapDiff.config.pending_if_new = true } + RUBY + + assert_equal 1, lines.size, lines.join + assert_match(/`pending_if_new`/, lines.first) + assert_match(/REMOVED in 2\.1/, lines.first) + assert_match(/record/, lines.first) + end + + test "setting fail_on_difference warns once, naming 2.1" do + lines = probe(<<~RUBY) + require "snap_diff" + 3.times { SnapDiff.config.fail_on_difference = false } + RUBY + + assert_equal 1, lines.size, lines.join + assert_match(/`fail_on_difference`/, lines.first) + assert_match(/REMOVED in 2\.1/, lines.first) + assert_match(/record = :all/, lines.first) + end + + # ADR-010: 2.0 deletes NOTHING. Warned is not removed -- the boolean has + # to keep deciding what it always decided, or the warning is a lie and the + # upgrade is a break. + test "the warned booleans still work exactly as they did" do + out = probe_stderr(<<~RUBY, "CI" => "") + require "snap_diff" + SnapDiff.silence_deprecations = true + SnapDiff.config.fail_if_new = true + raise "fail_if_new lost its effect" unless SnapDiff.config.fail_if_new + raise "fail_if_new must still drive the mode" unless SnapDiff.config.record == :none + SnapDiff.config.pending_if_new = true + raise "pending_if_new lost its effect" unless SnapDiff.config.pending_if_new + SnapDiff.config.fail_on_difference = false + raise "fail_on_difference lost its effect" if SnapDiff.config.fail_on_difference + RUBY + + assert_equal "", out + end + + # THE UPGRADE GUARD: a setup with no `record` line and no boolean must be + # byte-for-byte silent AND behave exactly as it did -- CI sniff included. + test "a setup that sets none of them stays silent and keeps the CI-only default" do + out = probe_stderr(<<~RUBY, "CI" => "1") + require "snap_diff" + raise "the CI default moved" unless SnapDiff.config.fail_if_new + raise "the CI default moved" unless SnapDiff.config.record == :none + RUBY + + assert_equal "", out, "reading the settings must not warn -- only setting them does" + end + + test "record itself is not deprecated" do + out = probe_stderr(<<~RUBY) + require "snap_diff" + SnapDiff.config.record = :none + SnapDiff.config.record = :all + SnapDiff.config.record = :once + RUBY + + assert_equal "", out + end + # --- what must stay silent ------------------------------------------- # The mutation that matters for everyone who is NOT affected: a plain vips