fix: never print a path or command that is not derived from live state (#260)

docs/architecture.md

@@ -240,7 +240,7 @@ The canonical way in is `SnapDiff.configure { |config| ... }` (all 27 settings f
 
 `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 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. `fail_if_new` (from `ENV["CI"]`) and `root` (from `Rails.root`) must never become lazy read-time defaults. The one deliberately live value is `default_options[:wait]`, a method-body read of `Capybara.default_max_wait_time`.
+**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.
 
 ## File Layout
 

docs/configuration.md

@@ -48,7 +48,7 @@ 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). New screenshots are allowed locally but rejected in CI — no configuration needed.
+**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.
 

lib/capybara/screenshot/diff/config_legacy.rb

@@ -13,9 +13,10 @@
 # Diff.compare) keeps working unchanged: one storage, two views.
 #
 # Load order: requiring snap_diff/config first also eagerly evaluates the
-# require-time defaults (ENV["CI"] for fail_if_new, Rails.root/pwd for
-# root) at this same load moment, exactly when the old mattr_accessor
-# default blocks used to run. Neither file requires back here, so the graph
-# stays acyclic.
+# require-time defaults (Rails.root/pwd for root) at this same load moment,
+# exactly when the old mattr_accessor default blocks used to run.
+# fail_if_new is the exception -- its ENV["CI"] fallback is read live, so an
+# explicit setting outranks the environment whenever the variable appears.
+# Neither file requires back here, so the graph stays acyclic.
 require "snap_diff/config"
 require "snap_diff/legacy_shims"

lib/snap_diff/config.rb

@@ -29,14 +29,18 @@ module SnapDiff
   # through the other structurally, not by synchronization.
   #
   # Default timing contract (pinned by config_default_timing_test.rb):
-  # every default below is evaluated ONCE, in #initialize, which runs at
-  # require time of this file (the eager +Config.new+ at the bottom) -- the
-  # same load moment the old +mattr_accessor+ default blocks evaluated at.
-  # In particular +fail_if_new+ (from <tt>ENV["CI"]</tt>) and +root+ (from
-  # +Rails.root+ / pwd) must never become lazy read-time defaults, memoized
-  # or not. The one deliberately LIVE value, +default_options[:wait]+, is
-  # not storage at all: it stays a method-body read of
-  # +Capybara.default_max_wait_time+ in +#default_options+.
+  # every stored default below is evaluated ONCE, in #initialize, which runs
+  # at require time of this file (the eager +Config.new+ at the bottom) --
+  # the same load moment the old +mattr_accessor+ default blocks evaluated
+  # at. In particular +root+ (from +Rails.root+ / pwd) must never become a
+  # lazy read-time default, memoized or not.
+  #
+  # Two values are deliberately LIVE and are not storage at all:
+  # +default_options[:wait]+ stays a method-body read of
+  # +Capybara.default_max_wait_time+ in +#default_options+, and
+  # +fail_if_new+ falls back to <tt>ENV["CI"]</tt> in its reader whenever
+  # nothing explicit was set -- see {#fail_if_new} for why freezing that
+  # sniff into storage let the environment outrank the user.
   class Config
     # Every setting this object stores, in the order the two legacy holders
     # used to declare them.
@@ -81,10 +85,12 @@ class Config
 
     # 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]))
+    # fail_if_new is excluded from the generated READER (it falls back to
+    # the environment). 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
 
     def initialize
       # Every setting gets its ivar up front (nil-defaulted ones included)
@@ -104,7 +110,7 @@ def initialize
       @capybara_screenshot_options = {}
       # Capybara::Screenshot::Diff side.
       @delayed = true
-      @fail_if_new = !ENV["CI"].nil? && !ENV["CI"].empty?
+      # No stored default for fail_if_new on purpose -- see the reader.
       @pending_if_new = false
       @fail_on_difference = true
       @enabled = true
@@ -117,6 +123,23 @@ def root=(path)
       @root = Pathname(path).expand_path
     end
 
+    # An explicit setting outranks the environment. nil means nobody said,
+    # and only then does the CI sniff answer -- read live, so a CI variable
+    # that appears at any point is honoured, not just one that happened to
+    # be exported before the gem was required. Assigning nil hands the
+    # setting back to the environment.
+    #
+    # This is a precedence rule, not a change of default: failing only under
+    # CI stays (a locally recorded baseline is often worthless across OS).
+    # Storing the sniff instead, as this did, made the two indistinguishable
+    # -- `fail_if_new = false` and "CI was absent at require time" were the
+    # same false, so the environment could win. Same fix as insta#924
+    # ("normally, CLI flags take precedence over environment variables") and
+    # the inverse of jest#12288.
+    def fail_if_new
+      @fail_if_new.nil? ? !ENV["CI"].to_s.empty? : @fail_if_new
+    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

lib/snap_diff/screenshot_matcher.rb

@@ -34,6 +34,10 @@ def build_screenshot_assertion(skip_stack_frames: 0)
 
       capture_screenshot(capture_options, comparison_options)
 
+      # AFTER the capture, so the path the message tells the user to
+      # `git add` is one that exists by the time they read it (#260).
+      fail_if_new_screenshot
+
       # 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?
@@ -69,24 +73,37 @@ def prepare_screenshot_options
       driver_options[:driver] = SnapDiff::Drivers.for(driver_options[:driver])
     end
 
+    # The git checkout that drives #need_to_compare?, plus the half of the
+    # no-baseline reporting that MUST run before the capture: it is the only
+    # 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
       @snapshot.checkout_base_screenshot
       return if @snapshot.base_path.exist?
-
-      # Runs BEFORE the capture below, which is the only moment at which
-      # `@snapshot.path` still tells us whether the user had a PNG sitting
-      # there already -- the case that confuses people most.
-      if SnapDiff.config.fail_if_new
-        raise SnapDiff::ExpectationNotMet.new(<<~ERROR.chomp, caller)
-          No existing screenshot found for #{@snapshot.path}!
-          To record it: run the test, then `git add #{@snapshot.path}` and commit -- baselines are read from git.
-          To allow new screenshots: SnapDiff.config.fail_if_new = false
-        ERROR
-      end
+      # 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
 
       warn_no_committed_baseline
     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
+    # was never written -- the instruction was unfollowable in CI, the one
+    # 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
+
+      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
+      ERROR
+    end
+
     # `fail_if_new` defaults to false off CI, deliberately: a new screenshot
     # must not break a local run. The cost is that nothing is compared and
     # the test passes whatever the page looks like, while the capture

test/legacy/legacy_config_default_timing_test.rb

@@ -39,12 +39,12 @@ def run_probe(script, env)
   RUBY
 
   ENTRY_POINTS.each do |entry|
-    test "#{entry}: defaults snapshot matches; ENV/pwd frozen at require, wait live" do
+    test "#{entry}: defaults snapshot matches; pwd frozen at require, wait and fail_if_new live" do
       run_probe(ConfigDefaultTimingTest::SNAPSHOT_SCRIPT, {"PROBE_ENTRY" => entry, "CI" => nil})
     end
 
-    test "#{entry}: CI=1 before require turns fail_if_new on; unset after require does not turn it off" do
-      run_probe(ConfigDefaultTimingTest::CI_SET_SCRIPT, {"PROBE_ENTRY" => entry, "CI" => "1"})
+    test "#{entry}: an explicit fail_if_new outranks ENV['CI'], which is otherwise read live" do
+      run_probe(ConfigDefaultTimingTest::CI_PRECEDENCE_SCRIPT, {"PROBE_ENTRY" => entry, "CI" => "1"})
     end
 
     test "#{entry}: Rails.root defined before require wins; reassigning it after require is not seen" do

test/unit/config_default_timing_test.rb

@@ -9,14 +9,18 @@
 #
 # Current behavior being pinned:
 #
-# - the ENV/pwd-derived defaults (fail_if_new from ENV["CI"], root from
-#   Rails.root/pwd) are evaluated ONCE, when snap_diff/config.rb is first
-#   required. Mutating ENV, cwd, or Rails.root after the require -- even
-#   before the first read -- must NOT change the value. A refactor that
-#   turns any of these into a lazy (read-time) default, memoized or not,
-#   goes red here.
+# - the pwd-derived default (root, from Rails.root/pwd) is evaluated ONCE,
+#   when snap_diff/config.rb is first required. Mutating cwd or Rails.root
+#   after the require -- even before the first read -- must NOT change the
+#   value. A refactor that turns it into a lazy (read-time) default,
+#   memoized or not, goes red here.
 # - default_options[:wait] is the opposite: it reads
 #   Capybara.default_max_wait_time at CALL time, live, every call.
+# - fail_if_new is live too, and deliberately so: it has no stored default,
+#   so an unset one asks ENV["CI"] on every read while an explicit setting
+#   outranks the environment. That is the whole point -- a frozen sniff
+#   cannot tell "the user asked for false" from "CI was absent at require".
+#   See CI_PRECEDENCE_SCRIPT and snap_diff_config_test.rb.
 #
 # Canonical entry points only, read through SnapDiff.config only. The v1
 # entry points and the "both surfaces agree" half re-run these same scripts
@@ -52,13 +56,12 @@ def check(name, expected, actual)
 
     require ENV.fetch("PROBE_ENTRY")
 
-    ENV["CI"] = "1"
     require "tmpdir"
     Dir.chdir(Dir.tmpdir)
 
     # Expected default values (CI unset, no Rails, at require time).
-    # fail_if_new false / root == launch pwd also pin require-time
-    # evaluation: ENV["CI"] and cwd were changed above, pre-first-read.
+    # root == launch pwd also pins require-time evaluation: cwd was changed
+    # above, pre-first-read.
     {
       add_driver_path: nil,
       add_os_path: nil,
@@ -95,15 +98,34 @@ def check(name, expected, actual)
     Capybara.default_max_wait_time = 42.5
     check("default_options[:wait] follows Capybara.default_max_wait_time set after require",
       42.5, SnapDiff.config.default_options[:wait])
+
+    # fail_if_new is live too: CI appearing after the require is seen.
+    ENV["CI"] = "1"
+    check("fail_if_new follows ENV['CI'] set after require", true, SnapDiff.config.fail_if_new)
   RUBY
 
-  # Probe B: ENV["CI"] present (non-empty) BEFORE the require flips the
-  # fail_if_new default on -- and unsetting it after the require does not
-  # flip it back (frozen at require time).
-  CI_SET_SCRIPT = CHECK_HELPER + <<~RUBY
+  # Probe B: fail_if_new precedence. The ENV["CI"] sniff is the FALLBACK,
+  # read live in both directions; an explicit setting outranks it whenever
+  # the variable appears (insta#924, jest#12288). Entered with CI=1 set
+  # before the require, so a require-time freeze is distinguishable.
+  CI_PRECEDENCE_SCRIPT = CHECK_HELPER + <<~RUBY
     require ENV.fetch("PROBE_ENTRY")
+    check("CI=1 at require", true, SnapDiff.config.fail_if_new)
+
+    ENV.delete("CI")
+    check("CI unset after require is seen", false, SnapDiff.config.fail_if_new)
+
+    ENV["CI"] = "1"
+    SnapDiff.config.fail_if_new = false
+    check("explicit false outranks CI=1", false, SnapDiff.config.fail_if_new)
+
     ENV.delete("CI")
-    check(:fail_if_new, true, SnapDiff.config.fail_if_new)
+    SnapDiff.config.fail_if_new = true
+    check("explicit true outranks CI unset", true, SnapDiff.config.fail_if_new)
+
+    ENV["CI"] = "1"
+    SnapDiff.config.fail_if_new = nil
+    check("nil hands it back to the environment", true, SnapDiff.config.fail_if_new)
   RUBY
 
   # Probe C: a Rails module with .root defined BEFORE the require wins over
@@ -125,12 +147,12 @@ class << self
   RUBY
 
   ENTRY_POINTS.each do |entry|
-    test "#{entry}: defaults snapshot matches; ENV/pwd frozen at require, wait live" do
+    test "#{entry}: defaults snapshot matches; pwd frozen at require, wait and fail_if_new live" do
       run_probe(SNAPSHOT_SCRIPT, {"PROBE_ENTRY" => entry, "CI" => nil})
     end
 
-    test "#{entry}: CI=1 before require turns fail_if_new on; unset after require does not turn it off" do
-      run_probe(CI_SET_SCRIPT, {"PROBE_ENTRY" => entry, "CI" => "1"})
+    test "#{entry}: an explicit fail_if_new outranks ENV['CI'], which is otherwise read live" do
+      run_probe(CI_PRECEDENCE_SCRIPT, {"PROBE_ENTRY" => entry, "CI" => "1"})
     end
 
     test "#{entry}: Rails.root defined before require wins; reassigning it after require is not seen" do

test/unit/dsl_test.rb

@@ -21,11 +21,15 @@ def after_teardown
     FileUtils.remove_entry(@new_root) if @new_root
   end
 
+  # "missing" means missing BASELINE (checkout_vcs false), not an unwritable
+  # screenshot: the capture now runs before the raise (#260), so the name has
+  # to be one ScreenshoterStub can actually produce -- it resolves "a_<digits>"
+  # to the a.png fixture.
   test "#screenshot raises error when screenshot is missing and fail_if_new is true" do
     SnapDiff::Vcs.stub(:checkout_vcs, false) do
       SnapDiff.config.stub(:fail_if_new, true) do
         assert_raises SnapDiff::ExpectationNotMet, match: /No existing screenshot found for/ do
-          screenshot "not_existing_screenshot-name"
+          screenshot "a_#{Time.now.nsec}"
         end
       end
     end

test/unit/screenshot_matcher_test.rb

@@ -307,4 +307,67 @@ def fake_stable.take_comparison_screenshot(snapshot)
     # release whose headline is SnapDiff.
     assert_includes error.message, "SnapDiff.config.fail_if_new = false"
   end
+
+  # Found by mutation while moving the raise for #260: deleting
+  # `@snapshot.checkout_base_screenshot` outright left the ENTIRE unit suite
+  # green. Every test stubs Vcs.checkout_vcs, so nothing asserted that the
+  # matcher ever asks git for the baseline -- and need_to_compare?, the
+  # warning and the raise all hang off that one call.
+  test "#build_screenshot_assertion asks VCS for the baseline of the screenshot it is about to take" do
+    name = "a_#{Time.now.nsec}"
+    snap = SnapDiff::SnapManager.path_for(name)
+    asked = []
+    checkout = lambda do |_root, path, as_path|
+      asked << [path, as_path]
+      false
+    end
+
+    capture_io do
+      SnapDiff::Vcs.stub(:checkout_vcs, checkout) do
+        SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion
+      end
+    end
+
+    assert_equal [[snap.path, snap.base_path]], asked
+  end
+
+  # #260. The message says `git add <path>` and commit. That is only
+  # followable if <path> is actually there, and the raise used to run before
+  # the capture -- so on the exact run that printed the instruction, nothing
+  # had been written. Never print a path or command that is not derived from
+  # live state: capture first, then raise.
+  test "the fail_if_new error names a file that is really on disk" do
+    name = "c_#{Time.now.nsec}"
+    path = SnapDiff::SnapManager.path_for(name).path
+
+    error = SnapDiff::Vcs.stub(:checkout_vcs, false) do
+      SnapDiff.config.stub(:fail_if_new, true) do
+        assert_raises(SnapDiff::ExpectationNotMet) do
+          SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion
+        end
+      end
+    end
+
+    assert_includes error.message, path.to_s
+    assert_predicate path, :exist?,
+      "the message says `git add #{path}` -- that file has to exist by then"
+  end
+
+  # The raise pre-empts the warning: one screenshot with no baseline is one
+  # thing to say, and the raise says it with the fix attached.
+  test "#build_screenshot_assertion does not also warn when fail_if_new raises" do
+    name = "c_#{Time.now.nsec}"
+
+    _out, err = capture_io do
+      SnapDiff::Vcs.stub(:checkout_vcs, false) do
+        SnapDiff.config.stub(:fail_if_new, true) do
+          assert_raises(SnapDiff::ExpectationNotMet) do
+            SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion
+          end
+        end
+      end
+    end
+
+    assert_no_match(/No committed baseline/, err)
+  end
 end