diff --git a/docs/architecture.md b/docs/architecture.md index 84521b1c..87661b89 100644 --- a/docs/architecture.md +++ b/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 diff --git a/docs/configuration.md b/docs/configuration.md index b8af96d1..c7844813 100644 --- a/docs/configuration.md +++ b/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. diff --git a/lib/capybara/screenshot/diff/config_legacy.rb b/lib/capybara/screenshot/diff/config_legacy.rb index 0da6e61a..a2a3ab2b 100644 --- a/lib/capybara/screenshot/diff/config_legacy.rb +++ b/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" diff --git a/lib/snap_diff/config.rb b/lib/snap_diff/config.rb index 1fd97d80..1491aee8 100644 --- a/lib/snap_diff/config.rb +++ b/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 ENV["CI"]) 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 ENV["CI"] 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 diff --git a/lib/snap_diff/screenshot_matcher.rb b/lib/snap_diff/screenshot_matcher.rb index 6cc21eb1..19765a80 100644 --- a/lib/snap_diff/screenshot_matcher.rb +++ b/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 diff --git a/test/legacy/legacy_config_default_timing_test.rb b/test/legacy/legacy_config_default_timing_test.rb index 72592857..a7e77a7f 100644 --- a/test/legacy/legacy_config_default_timing_test.rb +++ b/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 diff --git a/test/unit/config_default_timing_test.rb b/test/unit/config_default_timing_test.rb index 1667dbb1..c99fd171 100644 --- a/test/unit/config_default_timing_test.rb +++ b/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 diff --git a/test/unit/dsl_test.rb b/test/unit/dsl_test.rb index a243881a..ec4b71d7 100644 --- a/test/unit/dsl_test.rb +++ b/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_" + # 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 diff --git a/test/unit/screenshot_matcher_test.rb b/test/unit/screenshot_matcher_test.rb index 4bf4db31..8bd903dc 100644 --- a/test/unit/screenshot_matcher_test.rb +++ b/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 ` and commit. That is only + # followable if 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 diff --git a/test/unit/snap_diff_config_test.rb b/test/unit/snap_diff_config_test.rb index 4608d373..ecb991a3 100644 --- a/test/unit/snap_diff_config_test.rb +++ b/test/unit/snap_diff_config_test.rb @@ -162,6 +162,47 @@ def config end end + # --- fail_if_new precedence ------------------------------------------ + # + # NOT about what the default IS: failing only under CI stays, deliberately + # (a locally recorded baseline is often worthless across OS). This is only + # about who wins. An explicit setting outranks the environment sniff -- + # the ordering insta narrowed to in insta#924 after Ruff hit it ("normally, + # CLI flags take precedence over environment variables"), and the one Jest + # got backwards for a whole major version in jest#12288, where reading + # argv.ci instead of detected CI state made `CI=1 jest` and `jest --ci` + # disagree. + # + # For "explicit wins" to mean anything the fallback has to be live: while + # the CI sniff was frozen into the ivar at require time there was no way to + # tell "the user asked for false" from "CI was absent when we loaded". + def with_env_ci(value) + original = ENV["CI"] + ENV["CI"] = value + yield + ensure + ENV["CI"] = original + end + + test "an unset fail_if_new reads ENV['CI'] live, not once at require time" do + config.fail_if_new = nil + + with_env_ci(nil) { assert_equal false, config.fail_if_new } + with_env_ci("") { assert_equal false, config.fail_if_new, "an empty CI is not CI" } + with_env_ci("true") { assert_equal true, config.fail_if_new } + end + + test "an explicit fail_if_new outranks ENV['CI'] in both directions" do + config.fail_if_new = false + with_env_ci("true") { assert_equal false, config.fail_if_new, "the user said false under CI=true" } + + config.fail_if_new = true + with_env_ci(nil) { assert_equal true, config.fail_if_new, "the user said true off CI" } + + config.fail_if_new = nil + with_env_ci("true") { assert_equal true, config.fail_if_new, "nil hands it back to the environment" } + end + test "SnapDiff.configure yields the SnapDiff.config object" do yielded = nil SnapDiff.configure { |c| yielded = c }