From 94aad616316df06141977bda9c132c5cfd273bb5 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 08:17:26 +0200 Subject: [PATCH 1/2] fix: stop passing green when a screenshot has no committed baseline Baselines are read from git (`git show HEAD:`), never from disk, and `fail_if_new` is false off CI by design. So a screenshot with no COMMITTED baseline was not compared at all: no assertion registered, the test passed whatever the page looked like, and the capture silently overwrote the PNG on disk. Recording a page, changing it, and re-running gave `1 runs, 1 assertions, 0 failures` with the file rewritten -- the product's core promise failing quietly, in the default local configuration, for every new user. `fail_if_new = false` stays a supported choice, so this warns rather than raises: once per screenshot, naming the real screenshot path and what to do about it, and calling out the confusing case where a PNG is already sitting there (it is not a baseline until it is committed). The end-of-run summary gets a matching line -- the reporters count what WAS compared, so their "N screenshots compared, no failures" was silent about exactly the screenshots that passed without being looked at. Three smaller lies fixed in the same messages: * The new-screenshot error promised `RECORD_SCREENSHOTS=1 bundle exec rake test`. Nothing in lib/ has ever read that variable -- it is this repo's own test-suite convention. Not implemented, deleted: a run already captures every changed screenshot to its real path (verified for several screenshots in one test, first one differing), so `git add && git commit` IS the bulk-record path, and an env var would only add a way to accept regressions without looking at them. * That error named `.base.png`, a generated temp file nobody creates or commits, and pointed at the v1 `Capybara::Screenshot::Diff.fail_if_new` in the release whose headline is SnapDiff. Both corrected. * The Minitest activation warning keyed off lib/capybara-screenshot-diff.rb, the gem-NAME file. `Bundler.require` requires the gem's own name, so it fired for everyone with the gem in their Gemfile no matter what they required explicitly, with no action available to silence it. Only the v1 namespace entry (capybara/screenshot/diff) is a deprecated choice, and the remedy now names the canonical require. --- lib/snap_diff/integrations/minitest.rb | 17 ++-- lib/snap_diff/reporting.rb | 38 ++++++++ lib/snap_diff/screenshot_matcher.rb | 26 +++++- .../minitest_activation_warning_test.rb | 51 ++++++++++ test/test_helper.rb | 4 + test/unit/reporter_interplay_test.rb | 27 ++++++ test/unit/screenshot_matcher_test.rb | 93 +++++++++++++++++++ 7 files changed, 246 insertions(+), 10 deletions(-) create mode 100644 test/legacy/minitest_activation_warning_test.rb diff --git a/lib/snap_diff/integrations/minitest.rb b/lib/snap_diff/integrations/minitest.rb index baadf30c..82c85a67 100644 --- a/lib/snap_diff/integrations/minitest.rb +++ b/lib/snap_diff/integrations/minitest.rb @@ -8,14 +8,17 @@ require "snap_diff/screenshot_assertion" require "snap_diff/reporting" -used_deprecated_entrypoint = caller.any? do |path| - path.include?("capybara-screenshot-diff.rb") || path.include?("capybara/screenshot/diff.rb") -end - -if used_deprecated_entrypoint +# Only the v1 NAMESPACE entry is a deprecated choice: requiring +# "capybara/screenshot/diff" is a line in the user's own file, and changing +# it is the fix. The gem-NAME file (lib/capybara-screenshot-diff.rb) is not: +# `Bundler.require` requires the gem's own name, so it loads for everyone +# with the gem in their Gemfile whatever they require explicitly -- keying +# the warning off it shouted at every user on every run, with no action +# available to silence it. See test/legacy/minitest_activation_warning_test.rb. +if caller.any? { |path| path.include?("capybara/screenshot/diff.rb") } warn <<~MSG - [DEPRECATION] The default activation of `capybara_screenshot_diff/minitest` will be removed. - Please `require "capybara_screenshot_diff/minitest"` explicitly. + [DEPRECATION] `require "capybara/screenshot/diff"` activates the Minitest assertions for you; that will be removed. + Please `require "snap_diff/integrations/minitest"` explicitly. MSG end diff --git a/lib/snap_diff/reporting.rb b/lib/snap_diff/reporting.rb index 3f3096a4..8dbff728 100644 --- a/lib/snap_diff/reporting.rb +++ b/lib/snap_diff/reporting.rb @@ -13,10 +13,28 @@ module SnapDiff module Reporting @reporters = [] @mutex = Mutex.new + @missing_baselines = Set.new class << self attr_reader :reporters, :mutex + # Remembers a screenshot that had no COMMITTED baseline and was + # therefore never compared. + # + # @return [Boolean] true the first time this name is seen -- the + # "warn once per screenshot" gate for ScreenshotMatcher, and the + # tally behind {finalize!}'s summary line. Kept here rather than in + # the matcher because the end-of-run summary is this module's job. + def record_missing_baseline(name) + @mutex.synchronize { !!@missing_baselines.add?(name) } + end + + # @api private + # Per-test isolation for this gem's own suite. + def reset_missing_baselines! + @mutex.synchronize { @missing_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 @@ -56,6 +74,26 @@ def finalize! rescue => e warn "[snap_diff] Reporter #{reporter.class} failed (#{e.class}: #{e.message})" end + + if (msg = missing_baselines_summary) + $stdout.puts msg + end + end + + # The reporters' own summary counts what WAS compared ("N screenshots + # compared, no failures") and so says nothing about the screenshots + # that were skipped for want of a committed baseline -- the very ones + # that passed without being looked at. The last line of the run is the + # best chance to correct that impression. + # + # @return [String, nil] nil when every screenshot had a baseline + def missing_baselines_summary + names = @mutex.synchronize { @missing_baselines.to_a } + return if names.empty? + + label = (names.size == 1) ? "1 screenshot" : "#{names.size} screenshots" + "[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 end end diff --git a/lib/snap_diff/screenshot_matcher.rb b/lib/snap_diff/screenshot_matcher.rb index 2dcf0054..c3865596 100644 --- a/lib/snap_diff/screenshot_matcher.rb +++ b/lib/snap_diff/screenshot_matcher.rb @@ -7,6 +7,7 @@ require_relative "capture/viewport" require_relative "vcs" require_relative "area_calculator" +require_relative "reporting" module SnapDiff class ScreenshotMatcher @@ -66,14 +67,33 @@ def prepare_screenshot_options def check_base_screenshot @snapshot.checkout_base_screenshot + return if @snapshot.base_path.exist? - if SnapDiff.config.fail_if_new && !@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.base_path}! - To record baselines: RECORD_SCREENSHOTS=1 bundle exec rake test + 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 + + warn_no_committed_baseline + 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 + # overwrites the file on disk -- a green run that proves nothing. Say so + # once per screenshot, and name what to do about it. + def warn_no_committed_baseline + return unless SnapDiff::Reporting.record_missing_baseline(screenshot_full_name) + + already_there = @snapshot.path.exist? ? " (the file already there is not a baseline until it is committed)" : "" + warn "[snap_diff] No committed baseline for #{@snapshot.path}#{already_there} -- nothing was compared. " \ + "Commit it to enable comparison." end def capture_screenshot(capture_options, comparison_options) diff --git a/test/legacy/minitest_activation_warning_test.rb b/test/legacy/minitest_activation_warning_test.rb new file mode 100644 index 00000000..9874033b --- /dev/null +++ b/test/legacy/minitest_activation_warning_test.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "test_helper" +require "open3" + +# LEGACY SURFACE (test/legacy/, see the Rakefile): both entry points under +# test are deleted with lib/capybara* in 2.1. +# +# `snap_diff/integrations/minitest` warns when it was activated as a side +# effect of an entry point rather than required on purpose. WHICH entries +# count is the whole question, and it has a false-positive direction and a +# false-negative one -- hence a test for both: +# +# * lib/capybara-screenshot-diff.rb is the gem-NAME file. `Bundler.require` +# requires the gem's own name, so it loads for everyone with +# `gem "capybara-screenshot-diff"` in their Gemfile no matter what they +# require explicitly. Keying the warning off it shouted at every user on +# every run with no action available to silence it. +# * lib/capybara/screenshot/diff.rb is the v1 NAMESPACE entry. Requiring it +# IS the deprecated choice, and the fix is to change that one line. +class MinitestActivationWarningTest < ActiveSupport::TestCase + test "the gem-name entry point (what Bundler.require loads) does not warn" do + assert_equal "", warnings_from(<<~RUBY) + require "capybara-screenshot-diff" + require "snap_diff/integrations/minitest" + RUBY + end + + test "the v1 namespace entry point still warns" do + warnings = warnings_from(%(require "capybara/screenshot/diff")) + + assert_match(/\[DEPRECATION\]/, warnings) + assert_match(%r{snap_diff/integrations/minitest}, warnings, + "the remedy must name the canonical require, not another deprecated one") + end + + private + + # Runs +script+ in a fresh process with only lib/ on the load path and + # returns its stderr if the activation warning is in there (the message is + # multi-line, so grepping for the marker line would drop the remedy), "" if + # it is not. The v1-namespace channel is silenced so nothing else can match. + def warnings_from(script) + project_root = File.expand_path("../..", __dir__) + _out, err, status = Open3.capture3( + {"SNAP_DIFF_SILENCE_DEPRECATIONS" => "1"}, RbConfig.ruby, "-Ilib", "-e", script, chdir: project_root + ) + assert_predicate status, :success?, err + err.include?("[DEPRECATION]") ? err : "" + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 191afe33..7b261ff6 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -113,6 +113,10 @@ class ActiveSupport::TestCase Dir.chdir(@_orig_cwd) if @_orig_cwd && Dir.pwd != @_orig_cwd Capybara.app = @_orig_capybara_app if @_orig_capybara_app SnapDiff::SnapManager.cleanup! unless persist_comparisons? + # Process-global, like the reporter list: without this the whole suite's + # baseline-less screenshots pile up and get listed in one enormous line + # at the end of `rake test`. + SnapDiff::Reporting.reset_missing_baselines! end def persist_comparisons? diff --git a/test/unit/reporter_interplay_test.rb b/test/unit/reporter_interplay_test.rb index d4667992..74056b2d 100644 --- a/test/unit/reporter_interplay_test.rb +++ b/test/unit/reporter_interplay_test.rb @@ -75,6 +75,33 @@ def summary assert_empty @spy.recorded end + # The reporters count what WAS compared, so their summary is silent about + # the screenshots that passed without being looked at. The last line of the + # run has to name them, or a run full of false greens ends on + # "no failures". + test "finalize! names the screenshots that had no committed baseline" do + # ScreenshoterStub resolves "c_" to the c.png fixture. + name = "c_#{Time.now.nsec}" + + capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + out, _err = capture_io { SnapDiff::Reporting.finalize! } + + assert_match(/no committed baseline/i, out) + assert_match(/NOT compared/, out) + assert_includes out, name + end + + test "finalize! says nothing about baselines when every screenshot had one" do + out, _err = capture_io { SnapDiff::Reporting.finalize! } + + assert_no_match(/no committed baseline/i, out) + end + test "finalize_reporters! finalizes each reporter and prints its summary" do assert_output(/spy reporter summary/) do SnapDiff::Reporting.finalize! diff --git a/test/unit/screenshot_matcher_test.rb b/test/unit/screenshot_matcher_test.rb index 90ccebb9..4bf4db31 100644 --- a/test/unit/screenshot_matcher_test.rb +++ b/test/unit/screenshot_matcher_test.rb @@ -214,4 +214,97 @@ def fake_stable.take_comparison_screenshot(snapshot) assert_empty SnapDiff.session.new_screenshots end end + + # --- THE FALSE GREEN ------------------------------------------------- + # + # Baselines are read from git (`git show HEAD:`), never from disk, + # and `fail_if_new` is false off CI by design. So a screenshot with no + # COMMITTED baseline is not compared at all: no assertion is registered, + # the test passes whatever the page looks like, and the capture silently + # overwrites the PNG that was sitting on disk. That is the product's core + # promise failing quietly, in the default local configuration, for every + # new user -- it has to be said out loud. + + test "#build_screenshot_assertion warns when nothing was compared for lack of a committed baseline" do + name = "c_#{Time.now.nsec}" + path = SnapDiff::SnapManager.path_for(name).path + + _out, err = capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + assert_match(/No committed baseline/, err) + assert_includes err, path.to_s, "the warning must name the real screenshot, not the .base temp file" + assert_no_match(/\.base\.png/, err) + assert_match(/[Cc]ommit/, err, "the warning must say what to do about it") + # The check has to run BEFORE the capture, or every screenshot looks + # like it already had a file sitting there. + assert_no_match(/already there/, err) + end + + test "#build_screenshot_assertion warns once per screenshot, not once per run" do + name = "c_#{Time.now.nsec}" + + _out, err = capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + 2.times { SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion } + end + end + + assert_equal 1, err.scan("No committed baseline").size, err + end + + # The genuinely confusing case: the user SEES a PNG in doc/screenshots and + # assumes it is the baseline. It is not one until it is committed. + test "#build_screenshot_assertion flags an uncommitted screenshot already on disk" do + name = "c_#{Time.now.nsec}" + path = SnapDiff::SnapManager.path_for(name).path + path.dirname.mkpath + FileUtils.cp(File.expand_path("a.png", TEST_IMAGES_DIR), path) + + _out, err = capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, false) do + SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion + end + end + + assert_match(/not a baseline until it is committed/, err) + end + + test "#build_screenshot_assertion stays quiet when a committed baseline exists" do + _out, err = capture_io do + SnapDiff::Vcs.stub(:checkout_vcs, true) do + snap = create_snapshot_for(:a, :c) + SnapDiff::ScreenshotMatcher.new(snap.full_name).build_screenshot_assertion + end + end + + assert_no_match(/No committed baseline/, err) + end + + # The other half of the same message. It used to name `<...>.base.png` (a + # generated temp file nobody creates or commits), promise + # `RECORD_SCREENSHOTS=1`, which nothing in lib/ has ever read, and point at + # the legacy namespace in the release whose headline is SnapDiff. + test "the fail_if_new error tells the truth about the file and the fix" 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_no_match(/\.base\.png/, error.message) + assert_no_match(/RECORD_SCREENSHOTS/, error.message) + # The canonical name, not the v1 namespace it used to print in the + # release whose headline is SnapDiff. + assert_includes error.message, "SnapDiff.config.fail_if_new = false" + end end From dd5e3b0ed98dc0b3e66c03feaadbbc4345d7ca6b Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Mon, 24 Aug 2026 08:38:33 +0200 Subject: [PATCH 2/2] fix: the gem no longer crashes at boot in an RSpec-only bundle `Bundler.require` -- the default -- requires the gem's own name, so lib/capybara-screenshot-diff.rb and lib/snap_diff-capybara.rb load for every consumer. Both hard-required capybara_screenshot_diff/minitest, which requires "minitest", which is NOT a declared runtime dependency (the gemspec declares capybara only). An RSpec-only bundle died at boot: There was an error while trying to load the gem 'capybara-screenshot-diff'. Gem Load Error is: cannot load such file -- minitest from a gem that ships a first-class RSpec integration. minitest is not added as a runtime dependency -- that would force it on RSpec users to fix a problem caused by assuming it. The gem-name entry now loads the gem, then feature-detects: minitest present keeps the documented zero-require Rails path (assert_matches_screenshot with no explicit require), minitest absent loads fine and says so, naming the integration to require and the `require: false` that silences the line. A gem that loads and then does nothing, silently, is its own bug report. The v1 gem-name file now forwards to the surviving one, so there is one copy of the detection rather than two that drift. Explicit requires are untouched: `require "snap_diff/integrations/minitest"` (and its legacy alias) still hard-require minitest and still fail loudly. That is a line the user wrote, at a path they chose. `snap_diff/static` does the same and is left alone for the same reason -- worth a docs note, not a code change. Also, from the release lane: the activation deprecation was a bare Kernel#warn, so SnapDiff.silence_deprecations / SNAP_DIFF_SILENCE_DEPRECATIONS did not reach it. It now honours the documented switch. Guards: subprocess probes that chdir OUT of the project (inside it, RubyGems re-adds -rbundler/setup and the gemspec unshifts the real lib/) and shadow "minitest" with a raising stub on the load path, so `require "minitest"` fails exactly as it does in a bundle without it. Every probe opens with a gate line asserting the environment really is what it claims; both gate lines were shown to abort when it is not. A real `bundle install` with rspec and no minitest was used to confirm the crash and the fix by hand. test/unit/legacy_deletion_test.rb's EDITS and the canonical-refs allowlist track the new shape of snap_diff-capybara.rb -- both are deletion-simulation fixtures whose stated contract is to go red when those lines change. --- lib/capybara-screenshot-diff.rb | 5 +- lib/snap_diff-capybara.rb | 30 ++++- lib/snap_diff/integrations/minitest.rb | 6 +- test/legacy/legacy_entry_point_probe_test.rb | 28 +++++ .../minitest_activation_warning_test.rb | 15 ++- ...canonical_suite_has_no_legacy_refs_test.rb | 3 +- test/unit/gem_name_entry_point_test.rb | 112 ++++++++++++++++++ test/unit/legacy_deletion_test.rb | 13 +- 8 files changed, 199 insertions(+), 13 deletions(-) create mode 100644 test/unit/gem_name_entry_point_test.rb diff --git a/lib/capybara-screenshot-diff.rb b/lib/capybara-screenshot-diff.rb index dff8d102..eb46ea11 100644 --- a/lib/capybara-screenshot-diff.rb +++ b/lib/capybara-screenshot-diff.rb @@ -1,3 +1,6 @@ # frozen_string_literal: true -require "capybara_screenshot_diff/minitest" +# Bundler.require entry point for `gem "capybara-screenshot-diff"` -- the v1 +# gem name, deleted in 3.0. The surviving name owns the logic (including the +# minitest feature detection this door needs just as much). +require "snap_diff-capybara" diff --git a/lib/snap_diff-capybara.rb b/lib/snap_diff-capybara.rb index 2a35777b..87bf04ac 100644 --- a/lib/snap_diff-capybara.rb +++ b/lib/snap_diff-capybara.rb @@ -3,6 +3,30 @@ # Bundler.require entry point for `gem "snap_diff-capybara"`: Bundler # requires the gem's own name, and its dash->slash fallback ("snap_diff/ # capybara") misses too, so without this file a Rails user gets a silent -# no-op and a confusing NameError later. Loads what the sibling -# capybara-screenshot-diff.rb loads. -require "capybara_screenshot_diff/minitest" +# no-op and a confusing NameError later. The sibling +# capybara-screenshot-diff.rb is the same door under the v1 gem name and +# forwards here. +# +# Everything below therefore loads for EVERY consumer, RSpec and Cucumber +# users included, so nothing outside the gem's declared runtime dependencies +# may be hard-required. minitest is not one of them -- the gemspec declares +# capybara only -- and requiring it here killed an RSpec-only bundle at boot +# with `cannot load such file -- minitest`, from a gem that ships a +# first-class RSpec integration. +# +# So: load the gem, then feature-detect minitest. Present is the documented +# zero-require Rails path and still activates the assertions. Absent gets a +# line saying so -- a gem that loads and then does nothing, silently, is its +# own bug report. +require "capybara_screenshot_diff" + +begin + require "minitest" +rescue LoadError + warn "[snap_diff] minitest is not in this bundle, so `Bundler.require` activated no test-framework " \ + "integration. Require the one you use -- `require \"snap_diff/integrations/rspec\"` or " \ + "`require \"snap_diff/integrations/cucumber\"` -- and set `require: false` on the gem in your " \ + "Gemfile to silence this. See docs/framework-setup.md." +end + +require "capybara_screenshot_diff/minitest" if defined?(::Minitest) diff --git a/lib/snap_diff/integrations/minitest.rb b/lib/snap_diff/integrations/minitest.rb index 82c85a67..ea7c2dce 100644 --- a/lib/snap_diff/integrations/minitest.rb +++ b/lib/snap_diff/integrations/minitest.rb @@ -15,7 +15,11 @@ # with the gem in their Gemfile whatever they require explicitly -- keying # the warning off it shouted at every user on every run, with no action # available to silence it. See test/legacy/minitest_activation_warning_test.rb. -if caller.any? { |path| path.include?("capybara/screenshot/diff.rb") } +# +# Silenceable through the documented switch (SnapDiff.silence_deprecations / +# SNAP_DIFF_SILENCE_DEPRECATIONS): it was a bare Kernel#warn, so the one knob +# the docs offer did not reach it. +if !SnapDiff.silence_deprecations? && caller.any? { |path| path.include?("capybara/screenshot/diff.rb") } warn <<~MSG [DEPRECATION] `require "capybara/screenshot/diff"` activates the Minitest assertions for you; that will be removed. Please `require "snap_diff/integrations/minitest"` explicitly. diff --git a/test/legacy/legacy_entry_point_probe_test.rb b/test/legacy/legacy_entry_point_probe_test.rb index f9dc189d..649d8de2 100644 --- a/test/legacy/legacy_entry_point_probe_test.rb +++ b/test/legacy/legacy_entry_point_probe_test.rb @@ -2,6 +2,7 @@ require "test_helper" require "unit/support_load_probe_test" # single source of truth for the subprocess probe +require "unit/gem_name_entry_point_test" # single source of truth for the minitest-less probe # LEGACY SURFACE (test/legacy/, see the Rakefile). # @@ -278,6 +279,33 @@ class LegacyEntryPointProbeTest < ActiveSupport::TestCase "must not advise referencing a name we just failed to load" end + # The v1 gem-name file is the same `Bundler.require` door under the old + # name, and it had the same crash: an RSpec-only bundle died at boot on + # `cannot load such file -- minitest`, which is not a runtime dependency. + # Canonical half (and the probe itself) in + # test/unit/gem_name_entry_point_test.rb. + test "the v1 gem-name entry point loads in a bundle without minitest" do + out, err, status = GemNameEntryPointTest.probe(<<~'RUBY', minitest: false) + require "capybara-screenshot-diff" + puts "DSL:#{!defined?(CapybaraScreenshotDiff::DSL).nil?}" + puts "ASSERTIONS:#{!defined?(CapybaraScreenshotDiff::Minitest::Assertions).nil?}" + RUBY + + assert_predicate status, :success?, "boot must not fail without minitest:\n#{out}\n#{err}" + assert_includes out, "DSL:true", "the v1 surface must still load" + assert_includes out, "ASSERTIONS:false", "the Minitest assertions cannot be live without minitest" + end + + test "the v1 gem-name entry point still auto-activates the Minitest assertions when minitest is present" do + out, err, status = GemNameEntryPointTest.probe(<<~'RUBY') + require "capybara-screenshot-diff" + puts "ASSERTIONS:#{!defined?(CapybaraScreenshotDiff::Minitest::Assertions).nil?}" + RUBY + + assert_predicate status, :success?, "#{out}\n#{err}" + assert_equal "ASSERTIONS:true\n", out + end + private def probe(entry, script) diff --git a/test/legacy/minitest_activation_warning_test.rb b/test/legacy/minitest_activation_warning_test.rb index 9874033b..21ce6a43 100644 --- a/test/legacy/minitest_activation_warning_test.rb +++ b/test/legacy/minitest_activation_warning_test.rb @@ -34,17 +34,22 @@ class MinitestActivationWarningTest < ActiveSupport::TestCase "the remedy must name the canonical require, not another deprecated one") end + # It was a bare Kernel#warn, so the documented switch did not reach it -- + # a deprecation you cannot silence is one more thing users learn to ignore. + test "the warning is silenced by the documented deprecation switch" do + assert_equal "", warnings_from(%(require "capybara/screenshot/diff"), + "SNAP_DIFF_SILENCE_DEPRECATIONS" => "1") + end + private # Runs +script+ in a fresh process with only lib/ on the load path and # returns its stderr if the activation warning is in there (the message is # multi-line, so grepping for the marker line would drop the remedy), "" if - # it is not. The v1-namespace channel is silenced so nothing else can match. - def warnings_from(script) + # it is not. + def warnings_from(script, env = {}) project_root = File.expand_path("../..", __dir__) - _out, err, status = Open3.capture3( - {"SNAP_DIFF_SILENCE_DEPRECATIONS" => "1"}, RbConfig.ruby, "-Ilib", "-e", script, chdir: project_root - ) + _out, err, status = Open3.capture3(env, RbConfig.ruby, "-Ilib", "-e", script, chdir: project_root) assert_predicate status, :success?, err err.include?("[DEPRECATION]") ? err : "" end diff --git a/test/unit/canonical_suite_has_no_legacy_refs_test.rb b/test/unit/canonical_suite_has_no_legacy_refs_test.rb index 6d81df19..a5e220dd 100644 --- a/test/unit/canonical_suite_has_no_legacy_refs_test.rb +++ b/test/unit/canonical_suite_has_no_legacy_refs_test.rb @@ -88,7 +88,8 @@ class CanonicalSuiteHasNoLegacyRefsTest < ActiveSupport::TestCase # tree -- it cannot do that without spelling the doomed names. "unit/legacy_deletion_test.rb" => [ '["snap_diff.rb", %(require "snap_diff/legacy_shims"), nil],', - '%(require "capybara_screenshot_diff/minitest"),', + '%(require "capybara_screenshot_diff/minitest" if defined?(::Minitest)),', + '%(require "capybara_screenshot_diff"),', 'gate << "SnapDiff.start is still defined" if SnapDiff.respond_to?(:start)', 'gate << "CapybaraScreenshotDiff is still defined" if defined?(CapybaraScreenshotDiff)', 'assert_includes failure, "SnapDiff.start is still defined"' diff --git a/test/unit/gem_name_entry_point_test.rb b/test/unit/gem_name_entry_point_test.rb new file mode 100644 index 00000000..45ae35dc --- /dev/null +++ b/test/unit/gem_name_entry_point_test.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require "test_helper" +require "open3" +require "tmpdir" + +# `Bundler.require` -- the default for Rails, and for anyone who does not +# write `require: false` -- requires the gem's OWN NAME, so lib/.rb +# loads for every consumer whatever test framework they use. It may therefore +# hard-require nothing outside the gem's declared runtime dependencies, and +# minitest is not one of them: the gemspec declares capybara only. +# +# It used to. An RSpec-only bundle died at boot with +# +# There was an error while trying to load the gem 'snap_diff-capybara'. +# Gem Load Error is: cannot load such file -- minitest +# +# from a gem that ships a first-class RSpec integration. Both directions +# matter: minitest absent must load, minitest present must still auto- +# activate the assertions, which is the documented zero-require Rails path. +# +# The v1 gem-name entry (deleted in 3.0) is probed the same way from +# test/legacy/legacy_entry_point_probe_test.rb, which reuses .probe below. +class GemNameEntryPointTest < ActiveSupport::TestCase + LIB = File.expand_path("../../lib", __dir__) + + # Opens every probe: a probe that quietly tested the wrong environment + # would "prove" whatever we hoped it would. + GATE = { + false => <<~RUBY, + begin + require "minitest" + abort("GATE: minitest is loadable in this probe, so it proves nothing") + rescue LoadError + end + RUBY + true => <<~RUBY + begin + require "minitest" + rescue LoadError + abort("GATE: minitest is NOT loadable in this probe, so it proves nothing") + end + RUBY + }.freeze + + # Runs +script+ in a fresh process with the gem's lib/ on the load path and + # the cwd OUTSIDE the project: inside it, RubyGems re-adds + # `-rbundler/setup` and the gemspec unshifts the real lib/, so the probe + # would silently exercise the development bundle instead of what it set up. + # + # With minitest: false a shim directory shadowing "minitest" goes + # FIRST on the load path, so `require "minitest"` raises LoadError exactly + # as it does in a bundle without the gem. (A real `bundle install` in a + # scratch Gemfile reproduces the same thing and was used to confirm this + # one, but it needs the network and has no place in the unit suite.) + # + # @return [Array(String, String, Process::Status)] stdout, stderr, status + def self.probe(script, minitest: true) + Dir.mktmpdir do |dir| + load_paths = ["-I#{LIB}"] + + unless minitest + shim = File.join(dir, "shim") + Dir.mkdir(shim) + # No LoadError#path: RubyGems retries a require whose LoadError names + # the same path, and would then activate the real minitest gem. + File.write(File.join(shim, "minitest.rb"), %(raise LoadError, "cannot load such file -- minitest"\n)) + load_paths.unshift("-I#{shim}") + end + + Open3.capture3(RbConfig.ruby, *load_paths, "-e", GATE.fetch(minitest) + script, chdir: dir) + end + end + + test "the gem-name entry point loads in a bundle without minitest" do + # Non-interpolating heredoc: `#{}` in a probe script belongs to the + # CHILD, and this suite has SnapDiff loaded, so interpolating here would + # answer every question with the parent's own state. + out, err, status = self.class.probe(<<~'RUBY', minitest: false) + require "snap_diff-capybara" + puts "VERSION:#{SnapDiff::VERSION}" + puts "DSL:#{!defined?(SnapDiff::DSL).nil?}" + puts "ASSERTIONS:#{!defined?(SnapDiff::Minitest::Assertions).nil?}" + RUBY + + assert_predicate status, :success?, "boot must not fail without minitest:\n#{out}\n#{err}" + assert_match(/VERSION:\d+\./, out, "the gem's own surface must still load") + assert_includes out, "DSL:true" + assert_includes out, "ASSERTIONS:false", "the Minitest assertions cannot be live without minitest" + end + + # A gem that loads but does nothing, silently, is its own bug report. + test "the gem-name entry point says where to go when it activated nothing" do + out, err, status = self.class.probe(%(require "snap_diff-capybara"), minitest: false) + + assert_predicate status, :success?, "#{out}\n#{err}" + assert_match(%r{snap_diff/integrations/rspec}, err) + assert_match(/require: false/, err, "the message must name the way to silence it") + end + + # The other direction: the documented zero-require Rails path. + test "the gem-name entry point still auto-activates the Minitest assertions when minitest is present" do + out, err, status = self.class.probe(<<~'RUBY') + require "snap_diff-capybara" + puts "ASSERTIONS:#{!defined?(SnapDiff::Minitest::Assertions).nil?}" + RUBY + + assert_predicate status, :success?, "#{out}\n#{err}" + assert_equal "ASSERTIONS:true\n", out + assert_no_match(%r{snap_diff/integrations/rspec}, err, "nothing to say when the assertions are live") + end +end diff --git a/test/unit/legacy_deletion_test.rb b/test/unit/legacy_deletion_test.rb index 0a79bd5e..1795e221 100644 --- a/test/unit/legacy_deletion_test.rb +++ b/test/unit/legacy_deletion_test.rb @@ -48,9 +48,18 @@ class LegacyDeletionTest < ActiveSupport::TestCase # The new gem name's Bundler entry point is KEPT, repointed off the v1 # umbrella. It matches neither gate's file glob, so this is the only # thing that checks its post-2.1 shape at all. + # + # Two lines since the minitest feature detection landed: the file loads + # the gem unconditionally (minitest is not a runtime dependency, so + # `Bundler.require` must not die in an RSpec-only bundle) and activates + # the Minitest integration only when minitest is really there. The + # conditional line goes first -- it contains the shorter one. + ["snap_diff-capybara.rb", + %(require "capybara_screenshot_diff/minitest" if defined?(::Minitest)), + %(require "snap_diff/integrations/minitest" if defined?(::Minitest))], ["snap_diff-capybara.rb", - %(require "capybara_screenshot_diff/minitest"), - %(require "snap_diff/integrations/minitest")] + %(require "capybara_screenshot_diff"), + %(require "snap_diff")] ].freeze ENTRY_POINTS = SupportLoadProbeTest::CANONICAL_ENTRY_POINTS