fix: stop passing green when a screenshot has no committed baseline

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"

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)

lib/snap_diff/integrations/minitest.rb

@@ -8,14 +8,21 @@
 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.
+#
+# 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] 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
 

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

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)

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)

test/legacy/minitest_activation_warning_test.rb

@@ -0,0 +1,56 @@
+# 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
+
+  # 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.
+  def warnings_from(script, env = {})
+    project_root = File.expand_path("../..", __dir__)
+    _out, err, status = Open3.capture3(env, RbConfig.ruby, "-Ilib", "-e", script, chdir: project_root)
+    assert_predicate status, :success?, err
+    err.include?("[DEPRECATION]") ? err : ""
+  end
+end

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?

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"'