diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e33905d..5e634b44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -363,6 +363,17 @@ One removal 2.0 cannot warn about, written down instead: the legacy `LOADED_DRIV and omitted the README ### Fixed +- **`CI: ""` means "not CI" for the HTML report too.** The report embedded every image + whenever the `CI` variable merely existed, so the documented opt-out (`CI: ""` in the + update-baselines job) still produced a report with inlined images while the record mode + read the same value as "not CI". One predicate, `SnapDiff::Config.ci?`, now serves every + reader: unset or empty is not CI +- **The documented `ACCEPT_SCREENSHOTS` snippet no longer switches off the CI missing-baseline + check.** README and `docs/configuration.md` assigned + `record = ENV["ACCEPT_SCREENSHOTS"] ? :all : :once`, which sets an explicit `:once` on every + ordinary run. An explicit mode outranks the `:none`-under-CI default, so a CI that copied it + stopped failing on a screenshot with no committed baseline. Both now assign `:all` only when + the variable is non-empty and leave the default alone otherwise - **A screenshot with no committed baseline no longer passes silently** — it warns, names the file, and is counted as `new (not verified)` rather than as a pass - **An inherited `GIT_DIR` / `GIT_WORK_TREE` / `GIT_INDEX_FILE` no longer redirects diff --git a/README.md b/README.md index 3c5e6a71..c7ff066c 100644 --- a/README.md +++ b/README.md @@ -204,7 +204,9 @@ One failure at a time is fine for one screenshot. After a redesign that changed ```ruby # test_helper.rb -SnapDiff.config.record = ENV["ACCEPT_SCREENSHOTS"] ? :all : :once +# Set the mode only when asked: an explicit mode, even :once, outranks the +# :none-under-CI default and would stop CI from failing on a missing baseline. +SnapDiff.config.record = :all unless ENV["ACCEPT_SCREENSHOTS"].to_s.empty? ``` ```bash diff --git a/docs/configuration.md b/docs/configuration.md index 49d756a0..bbb65a65 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -72,8 +72,10 @@ 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 +# so gate it on an environment variable of your own if you want one. Set the +# mode only when asked: an explicit mode, even :once, outranks the +# :none-under-CI default and would stop CI from failing on a missing baseline. +SnapDiff.config.record = :all unless ENV["ACCEPT_SCREENSHOTS"].to_s.empty? ``` ```bash diff --git a/lib/snap_diff/config.rb b/lib/snap_diff/config.rb index 03d3794f..8c4442e8 100644 --- a/lib/snap_diff/config.rb +++ b/lib/snap_diff/config.rb @@ -101,6 +101,12 @@ def self.validate_record_mode!(mode) "unknown record mode #{mode.inspect} -- one of #{RECORD_MODES.map(&:inspect).join(", ")} (or nil)" end + # Are we on CI? Read live, and an empty CI is not CI: `CI: ""` is how a + # job opts out (docs/ci-integration.md), so every reader must agree. + def self.ci? + !ENV["CI"].to_s.empty? + 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 @@ -157,7 +163,7 @@ def root=(path) # ("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 + @fail_if_new.nil? ? Config.ci? : @fail_if_new end # Announces the 2.1 removal. The writer, not the reader: {#record} reads diff --git a/lib/snap_diff/reporters/html.rb b/lib/snap_diff/reporters/html.rb index d2b990e3..8621fc90 100644 --- a/lib/snap_diff/reporters/html.rb +++ b/lib/snap_diff/reporters/html.rb @@ -220,5 +220,5 @@ def write_report # Framework adapters (Minitest, RSpec, Cucumber) call SnapDiff::Reporting.finalize! via native hooks. # For custom frameworks, call SnapDiff::Reporting.finalize! manually. unless SnapDiff::Reporting.reporters.any?(SnapDiff::Reporters::HTML) - SnapDiff::Reporting.register(SnapDiff::Reporters::HTML.new(embed_images: !!ENV["CI"])) + SnapDiff::Reporting.register(SnapDiff::Reporters::HTML.new(embed_images: SnapDiff::Config.ci?)) end diff --git a/lib/snap_diff/screenshot_matcher.rb b/lib/snap_diff/screenshot_matcher.rb index ec242c32..439631ef 100644 --- a/lib/snap_diff/screenshot_matcher.rb +++ b/lib/snap_diff/screenshot_matcher.rb @@ -92,7 +92,7 @@ def resolve_record_mode(per_screenshot) # `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? + return unless SnapDiff::Config.ci? 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). diff --git a/scripts/generate_sample_report.rb b/scripts/generate_sample_report.rb index af716ff0..870abb23 100644 --- a/scripts/generate_sample_report.rb +++ b/scripts/generate_sample_report.rb @@ -20,7 +20,7 @@ {name: "header-resized", base: "a", new: "a_cropped"} # dimensions change: 80x80 => 80x60 ] -embed = ARGV.include?("--embed") || !!ENV["CI"] +embed = ARGV.include?("--embed") || SnapDiff::Config.ci? reporter = CapybaraScreenshotDiff::Reporters::HTML.new(output_path: output_path, embed_images: embed) fixtures = File.expand_path("../test/fixtures/images", __dir__) tmp_dir = File.expand_path("../tmp/sample_images", __dir__) diff --git a/test/unit/snap_diff_config_test.rb b/test/unit/snap_diff_config_test.rb index ecb991a3..c98a21d2 100644 --- a/test/unit/snap_diff_config_test.rb +++ b/test/unit/snap_diff_config_test.rb @@ -184,6 +184,15 @@ def with_env_ci(value) ENV["CI"] = original end + # One predicate for "are we on CI". The HTML reporter used `!!ENV["CI"]`, + # so the documented `CI: ""` recipe (docs/ci-integration.md) embedded every + # image into the report while the record mode read the same value as "not CI". + test "Config.ci? is false for an unset or empty CI, true otherwise" do + with_env_ci(nil) { assert_equal false, SnapDiff::Config.ci? } + with_env_ci("") { assert_equal false, SnapDiff::Config.ci?, "an empty CI is not CI" } + with_env_ci("true") { assert_equal true, SnapDiff::Config.ci? } + end + test "an unset fail_if_new reads ENV['CI'] live, not once at require time" do config.fail_if_new = nil