fix: skip_area wait, an invisible summary line, and a phantom assertion (#272, #269, #270)

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:** 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.
+**Note:** Setting `SnapDiff.config.screenshot_enabled = false` is sufficient to disable all screenshots. There is no need to define no-op modules or monkey-patch the gem.
 
 ## Record modes — accepting changes
 
@@ -227,15 +227,18 @@ unless the desired window size can be achieved.
 If you want to skip taking screen shots, set
 
 ```ruby
-Capybara::Screenshot.enabled = false
+SnapDiff.config.screenshot_enabled = false
 ```
 
 You can of course set this by an environment variable
 
 ```ruby
-Capybara::Screenshot.enabled = ENV['TAKE_SCREENSHOTS']
+SnapDiff.config.screenshot_enabled = ENV['TAKE_SCREENSHOTS']
 ```
 
+A disabled screenshot is not an assertion, and Minitest is told so: a test whose only assertion
+was a screenshot reports as missing assertions rather than as a pass over nothing.
+
 ### Disabling diff
 
 If you want to skip the assertion for change in the screen shot, set

docs/reporters.md

@@ -20,6 +20,30 @@ The report includes a sidebar with thumbnails, side-by-side comparison with diff
 
 **Note:** The report is not generated when all screenshots match.
 
+## The end-of-run summary
+
+Every run ends with what it actually did, whether or not you require a reporter:
+
+```
+[snap_diff] 12 verified, 1 changed, 2 new (not verified).
+```
+
+- **verified** — a committed baseline existed and was compared
+- **changed** — of those, the ones that differed
+- **new** — captured but *not* compared, for want of a committed baseline: neither a pass nor a
+  failure. Commit the files to turn them into baselines.
+
+`0 verified` is printed as `NOTHING WAS VERIFIED`, because it is the only signal for the failures
+no per-assertion rule can see: a `rake test` that ran zero system tests, or an inherited `GIT_DIR`
+sending every baseline lookup to the wrong repository. Both leave a green suite that compared
+nothing.
+
+Requiring `snap_diff/reporters/html` adds one more line, naming the file it wrote:
+
+```
+[snap_diff] Report: doc/screenshots/snap_diff_report.html
+```
+
 ## Parallel test runs
 
 `finalize` — the hook that writes the report — runs from the framework's end-of-suite hook. Whether

lib/snap_diff/browser_helpers.rb

@@ -98,8 +98,19 @@ def self.blur_from_focused_element
       ]
     JS
 
+    # `minimum: 0` is load-bearing (issue #272). Capybara's `all` defaults to
+    # `minimum: 1` and blocks in `synchronize` until the count is satisfied,
+    # so every `skip_area`/`crop` selector matching nothing burned a full
+    # `Capybara.default_max_wait_time` -- 5s by default, per selector, per
+    # screenshot. One project measured `%w[picture img]` on an image-less
+    # page at 10s per screenshot, 44% of their suite.
+    #
+    # Waiting is the wrong semantic here regardless of the cost: these
+    # selectors describe a MASK over whatever is currently on the page. A
+    # selector that matches nothing has nothing to mask, and that answer is
+    # available immediately.
     def self.all_visible_regions_for(selector)
-      BrowserHelpers.session.all(selector, visible: true).map { |el| region_for(el) }
+      BrowserHelpers.session.all(selector, visible: true, minimum: 0).map { |el| region_for(el) }
     end
 
     def self.region_for(element)

lib/snap_diff/integrations/minitest.rb

@@ -31,8 +31,20 @@ module Minitest
     module Assertions
       include ::SnapDiff::DSL
 
+      # The `if` is the whole point (issue #270). `super` returns false
+      # immediately when screenshots are disabled -- nothing captured,
+      # nothing compared -- so counting unconditionally reported
+      # `1 runs, 1 assertions, 0 failures` over a test that asserted
+      # nothing at all.
+      #
+      # Getting the count right hands the alarm to Rails for free: it
+      # prepends ActiveSupport::Testing::TestsWithoutAssertions into every
+      # ActiveSupport::TestCase (test_case.rb:205), which warns
+      # "Test is missing assertions: `test_x`" on exactly the tests whose
+      # sole assertion was a disabled screenshot -- and stays quiet for
+      # tests that assert something else.
       def assert_matches_screenshot(*args, skip_stack_frames: 0, **opts)
-        self.assertions += 1
+        self.assertions += 1 if SnapDiff.config.active?
 
         super(*args, skip_stack_frames: skip_stack_frames + 1, **opts)
       rescue ::SnapDiff::ExpectationNotMet => e

lib/snap_diff/integrations/rspec.rb

@@ -9,6 +9,17 @@
 RSpec::Matchers.define :match_screenshot do |name, **options|
   description { "match screenshot '#{name}'" }
 
+  # The literal `true` is deliberate, not the Minitest miscount of issue
+  # #270. `assert_matches_screenshot` returns false when screenshots are
+  # disabled, and returning that here would FAIL the example for a config
+  # switch the user set on purpose. A real mismatch does not come back as
+  # false either -- it raises SnapDiff::ExpectationNotMet, or is deferred
+  # to the append_after hook below.
+  #
+  # There is nothing to hand off to the way Minitest hands off to Rails'
+  # TestsWithoutAssertions: RSpec has no assertion count, so a disabled
+  # screenshot leaves an example that passed having checked nothing, and
+  # only the end-of-run `0 verified` line can see it.
   match do |_page|
     assert_matches_screenshot(name, **options)
     true

lib/snap_diff/reporters/html.rb

@@ -83,31 +83,17 @@ def merge_state!(state)
       def passed = total - failures.size
       def failed = failures.size
 
-      # The last line of the run, and the only place it says what it
-      # actually did:
+      # Both customer personas named this path as the best output in the
+      # product, so it gets its own line -- but only when a report was
+      # actually written.
       #
-      #   verified -- a committed baseline existed and was compared
-      #   changed  -- of those, the ones that differed
-      #   new      -- captured but NOT compared, for want of a committed
-      #               baseline: neither a pass nor a failure
+      # The counts this used to carry moved to SnapDiff::Reporting (issue
+      # #269): they are printed for every user, and this file is not. See
+      # Reporting.counts_summary.
       #
-      # Printed on every run, passing or failing, and never nil. "N
-      # screenshots compared" counted only what it compared, so it was
-      # silent about exactly the screenshots it did not -- and silent
-      # altogether when it compared nothing, which is the one case worth
-      # shouting about: no assertion runs, so nothing else in the output
-      # can notice a suite that ran no tests, or a baseline lookup pointed
-      # at the wrong repository.
+      # @return [String, nil] nil when no report was written
       def summary
-        line = "[snap_diff] #{total} verified, #{failed} changed, " \
-          "#{Reporting.missing_baselines_count} new (not verified)."
-
-        return "#{line} NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline." if total.zero?
-        return line if failures.empty?
-
-        # Both customer personas named this path as the best output in the
-        # product. It is only here when a report was actually written.
-        "#{line} Report: #{output_path}"
+        "[snap_diff] Report: #{output_path}" if @finalized
       end
 
       def render

lib/snap_diff/reporting.rb

@@ -19,10 +19,25 @@ module Reporting
     @mutex = Mutex.new
     @missing_baselines = Set.new
     @rerecorded_baselines = Set.new
+    @verified = 0
+    @changed = 0
 
     class << self
       attr_reader :reporters, :mutex
 
+      # How many screenshots were compared to a committed baseline, and how
+      # many of those differed.
+      #
+      # These counters live HERE, not in a reporter (issue #269). Counting
+      # is core honesty; writing an HTML file is a feature. The summary
+      # exists to catch the failure modes no per-assertion rule can see -- a
+      # run where zero system tests executed, or where an inherited GIT_DIR
+      # redirected every baseline lookup -- and `0 verified` is the only
+      # signal for either. It shipped inside Reporters::HTML, the gem's one
+      # and only `register` call site, so the documented Rails setup (which
+      # requires just the Minitest integration) printed nothing at all.
+      attr_reader :verified, :changed
+
       # Remembers a screenshot that had no COMMITTED baseline and was
       # therefore never compared.
       #
@@ -43,9 +58,16 @@ def missing_baselines_count
       end
 
       # @api private
-      # Per-test isolation for this gem's own suite.
-      def reset_missing_baselines!
-        @mutex.synchronize { @missing_baselines.clear }
+      # Per-test isolation for this gem's own suite: everything {finalize!}
+      # reports, cleared in one call. One surface rather than one reset per
+      # tally, so a tally added later cannot be forgotten at the call site.
+      def reset_run_totals!
+        @mutex.synchronize do
+          @missing_baselines.clear
+          @rerecorded_baselines.clear
+          @verified = 0
+          @changed = 0
+        end
       end
 
       # Remembers a screenshot re-recorded by `record: :all` -- captured as
@@ -58,11 +80,6 @@ def record_rerecorded_baseline(name)
         @mutex.synchronize { !!@rerecorded_baselines.add?(name) }
       end
 
-      # @api private
-      def reset_rerecorded_baselines!
-        @mutex.synchronize { @rerecorded_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
@@ -80,6 +97,18 @@ def register(reporter)
       def notify(assertions)
         return if assertions.nil? || assertions.empty?
 
+        # Warned about and skipped, never raised: `notify` runs inside every
+        # test's teardown (SnapDiff.reset), and a raise here would abort the
+        # reset before it clears the registry -- leaking one test's
+        # assertions into the next. A tally must not be able to take a
+        # user's suite down. Same contract the reporter loop below applies,
+        # and just as loud: unconditional, not DEBUG-gated.
+        begin
+          count(assertions)
+        rescue => e
+          warn "[snap_diff] Could not tally the run (#{e.class}: #{e.message})"
+        end
+
         reporters_snapshot = @mutex.synchronize { @reporters.dup }
         return if reporters_snapshot.empty?
 
@@ -90,10 +119,70 @@ def notify(assertions)
         end
       end
 
-      # End-of-suite hook: finalizes each reporter and prints its summary.
-      # A raising reporter is warned about and skipped; the rest are still
-      # finalized.
+      # Tallies a finished test's assertions. An assertion with no
+      # `compare` never reached a baseline, so it is neither verified nor
+      # changed -- it is counted, if at all, by {record_missing_baseline}.
+      def count(assertions)
+        verified = 0
+        changed = 0
+
+        assertions.each do |assertion|
+          compare = assertion.compare
+          next unless compare
+
+          verified += 1
+          changed += 1 if compare.difference&.different?
+        end
+
+        @mutex.synchronize do
+          @verified += verified
+          @changed += changed
+        end
+      end
+
+      # The last line of the run, and the only place it says what it
+      # actually did:
+      #
+      #   verified -- a committed baseline existed and was compared
+      #   changed  -- of those, the ones that differed
+      #   new      -- captured but NOT compared, for want of a committed
+      #               baseline: neither a pass nor a failure
+      #
+      # Printed on every run, passing or failing, reporter or no reporter,
+      # and never nil. "N screenshots compared" counted only what it
+      # compared, so it was silent about exactly the screenshots it did not
+      # -- and silent altogether when it compared nothing, which is the one
+      # case worth shouting about.
+      def counts_summary
+        verified, changed, new_count, rerecorded = @mutex.synchronize {
+          [@verified, @changed, @missing_baselines.size, @rerecorded_baselines.size]
+        }
+        line = "[snap_diff] #{verified} verified, #{changed} changed, #{new_count} new (not verified)."
+
+        # `record: :all` (#274) accepts the rendering as the new baseline
+        # without comparing, so those are neither verified nor changed --
+        # and not "new" either, which is a different fact. Only shown when
+        # it happened; the names are on their own line below.
+        line += " #{rerecorded} re-recorded (not verified)." if rerecorded.positive?
+
+        # The shout is for an UNEXPLAINED zero -- a suite that ran no system
+        # tests, a GIT_DIR pointed at the wrong repository. Re-recording
+        # explains it, and the user asked for it: shouting there is a false
+        # alarm, and false alarms are how the real one stops being read.
+        if verified.zero? && rerecorded.zero?
+          return "#{line} NOTHING WAS VERIFIED -- no screenshot was compared to a committed baseline."
+        end
+
+        line
+      end
+
+      # End-of-suite hook: prints the counts, then finalizes each reporter
+      # and prints its summary. A raising reporter is warned about and
+      # skipped; the rest are still finalized -- and the counts line is
+      # already out, so no reporter can take it down with it.
       def finalize!
+        $stdout.puts counts_summary
+
         @mutex.synchronize { @reporters.dup }.each do |reporter|
           reporter.finalize
           if (msg = reporter.summary)
@@ -160,6 +249,8 @@ def dump_parallel_fragment
         payload = {
           "missing_baselines" => @mutex.synchronize { @missing_baselines.to_a },
           "rerecorded_baselines" => @mutex.synchronize { @rerecorded_baselines.to_a },
+          "verified" => @verified,
+          "changed" => @changed,
           "reporters" => @mutex.synchronize { @reporters.dup }
             .map { |reporter| reporter.dump_state if reporter.respond_to?(:dump_state) }
         }
@@ -182,10 +273,19 @@ def merge_parallel_fragments!
         Dir[File.join(parallel_fragments_dir, "*.json")].sort.each do |fragment|
           payload = JSON.parse(File.read(fragment))
 
-          @mutex.synchronize { payload["missing_baselines"].each { |name| @missing_baselines << name } }
-          # `to_a` on a fresh install predates this key: a fragment written
-          # by an older worker has no "rerecorded_baselines" at all.
-          @mutex.synchronize { payload.fetch("rerecorded_baselines", []).each { |name| @rerecorded_baselines << name } }
+          # Only "missing_baselines" is read without a default: it is the
+          # one key every version of this fragment has ever written. Every
+          # key added since is `fetch`ed with one, because the fragments
+          # directory is keyed by pid under the system temp dir -- a
+          # recycled pid can hand this merge a fragment left behind by an
+          # older version of the gem, and a partial payload must not take
+          # the run down.
+          @mutex.synchronize do
+            payload["missing_baselines"].each { |name| @missing_baselines << name }
+            payload.fetch("rerecorded_baselines", []).each { |name| @rerecorded_baselines << name }
+            @verified += payload.fetch("verified", 0)
+            @changed += payload.fetch("changed", 0)
+          end
 
           reporters_snapshot = @mutex.synchronize { @reporters.dup }
           payload["reporters"].each_with_index do |state, index|

test/fixtures/summary_line_case.rb

@@ -13,7 +13,10 @@
 #   SNAP_CASES  -- comma-separated subset of verified,changed,new (may be empty)
 require "minitest/autorun"
 require "snap_diff/integrations/minitest"
-require "snap_diff/reporters/html"
+# The HTML report is a FEATURE and stays opt-in; the summary line is core
+# honesty and must print either way. SNAP_NO_REPORTER runs the documented
+# Rails setup, which registers no reporter at all.
+require "snap_diff/reporters/html" unless ENV["SNAP_NO_REPORTER"]
 require "fileutils"
 require "pathname"
 
@@ -31,7 +34,8 @@ class FileCopyScreenshoter < SnapDiff::Screenshoter
   CAPTURES = {
     "verified" => "a.png",
     "changed" => "b.png",
-    "new" => "a.png"
+    "new" => "a.png",
+    "rerecorded" => "b.png"
   }.freeze
 
   def take_screenshot(screenshot_path)
@@ -49,7 +53,18 @@ def take_screenshot(screenshot_path)
 class SummaryLineCase < Minitest::Test
   include SnapDiff::Minitest::Assertions
 
+  # `record: :all` accepts whatever rendered as the new baseline. This one
+  # has a COMMITTED baseline that differs, and the mode ignores it -- the
+  # interaction worth asserting on a finished process.
+  RECORD_ALL = %w[rerecorded].freeze
+
   CASES.each do |name|
-    define_method(:"test_#{name}") { assert_matches_screenshot(name) }
+    define_method(:"test_#{name}") do
+      if RECORD_ALL.include?(name)
+        assert_matches_screenshot(name, record: :all)
+      else
+        assert_matches_screenshot(name)
+      end
+    end
   end
 end