fix: a baseline that disappears mid-test raises instead of passing green (#217)

docs/reporters.md

@@ -52,7 +52,7 @@ that hook fires in the process holding the results depends on how your runner pa
 | How the suite runs | Report |
 | --- | --- |
 | Serial | Written, complete. |
-| `parallelize(with: :threads)` (also the default on JRuby) | Written, complete — same failures and counts as a serial run; only the order of the entries differs. |
+| `parallelize(with: :threads)` (also the default on JRuby) | Written, complete — same failures and counts as a serial run; only the order of the entries differs. Verified over repeated runs, provided [screenshot names are unique across tests](thread_safety.md#screenshot-names-must-be-unique-across-tests). |
 | `parallelize(workers: N)` (Rails' default, forks) | Written, complete — one report at the usual path, merged from every worker. No configuration needed. |
 | One process per worker (`parallel_tests`, RSpec, CI sharding) | Written, but only the **last** process to finish is in it; the others are overwritten. |
 

docs/thread_safety.md

@@ -42,12 +42,19 @@ every file involved in the comparison.
 Serially that is merely wasteful: the tests overwrite each other in order. In
 parallel it is dangerous. When two concurrently running tests share a name, one
 test's post-pass baseline archiving moves the baseline that the other just
-checked out, and the second test then finds no baseline — so it records the
-screenshot as *new* and **returns without comparing anything**. The test passes
-green having verified nothing. A run of 64 concurrent assertions sharing 8 names
-measured between 18 and 34 comparisons silently skipped this way, alongside a
-scatter of loud errors from the same collisions (truncated PNG reads, `mv`
-failures).
+checked out, and the second test then finds no baseline. A run of 64 concurrent
+assertions sharing 8 names measured between 18 and 34 comparisons lost this way,
+alongside a scatter of loud errors from the same collisions (truncated PNG reads,
+`mv` failures).
+
+Losing the baseline used to be **silent**: the screenshot was recorded as *new*
+and the test passed green having compared nothing. It is now an error — "no
+baseline was ever committed" (legitimate, warned about) is told apart from "the
+baseline I just checked out has disappeared" (impossible in a correct run):
+
+```
+The baseline for 'dashboard' was checked out and then disappeared before it could be compared -- nothing was verified.
+```
 
 Use `screenshot_section` / `screenshot_group`, or name screenshots after the test,
 so no two tests can collide.

lib/snap_diff/screenshot_assertion.rb

@@ -114,9 +114,18 @@ class AssertionRegistry
     def initialize
       @assertions = []
       @new_screenshots = []
+      @checked_out_baselines = Set.new
       @screenshot_namer = SnapDiff::ScreenshotNamer.new
     end
 
+    # Called by Snap#checkout_base_screenshot when git really handed us a
+    # baseline. Same thread as the reading below -- a test's checkout and
+    # its comparison happen in one call stack -- so no synchronization is
+    # needed or wanted here.
+    def record_baseline_checkout(name)
+      @checked_out_baselines << name
+    end
+
     def add_assertion(assertion)
       return unless assertion&.compare
 
@@ -129,7 +138,21 @@ def assertions_present?
       !@assertions.empty?
     end
 
+    # "No baseline exists" reaches here for two very different reasons. One
+    # is legitimate and warned about: nothing was ever committed for this
+    # name. The other is impossible in a correct run -- git gave us a
+    # baseline moments ago and it is gone now -- and used to be recorded as
+    # if it were the first, leaving the test green having compared nothing.
+    # That is the silently-wrong case measured in #217: two concurrent
+    # tests asserting the SAME name, where one's archive_baseline! moves
+    # the baseline the other just checked out.
     def record_new_screenshot(name)
+      raise SnapDiff::Error.new(<<~ERROR.chomp, caller) if @checked_out_baselines.include?(name)
+        The baseline for '#{name}' was checked out and then disappeared before it could be compared -- nothing was verified.
+        Every artifact path derives from the screenshot name alone, so two tests asserting '#{name}' at the same time race on one set of files: the one that finishes first archives the baseline the other is still using.
+        Give those screenshots distinct names, or do not run them concurrently.
+      ERROR
+
       @new_screenshots.push(name)
     end
 
@@ -151,6 +174,7 @@ def failed_assertions
     def reset
       @assertions.clear
       @new_screenshots.clear
+      @checked_out_baselines.clear
       @screenshot_namer = SnapDiff::ScreenshotNamer.new
     end
   end

lib/snap_diff/snap.rb

@@ -20,8 +20,13 @@ def delete!
       cleanup_attempts!
     end
 
+    # Records the successful checkout on the session, so that a LATER
+    # "there is no baseline" reading can be told apart from "there never
+    # was one". Only the second is a legitimate state (#217).
     def checkout_base_screenshot
-      @manager.checkout_file(path, base_path)
+      @manager.checkout_file(path, base_path).tap do |checked_out|
+        SnapDiff.session.record_baseline_checkout(full_name) if checked_out
+      end
     end
 
     def path_for(version = :actual)

test/unit/baseline_disappeared_test.rb

@@ -0,0 +1,147 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "snap_diff"
+
+# Two concurrently-running tests asserting the SAME screenshot name race on
+# one set of files: every artifact path derives from the name alone. One
+# test's `archive_baseline!` moves the baseline the other test just checked
+# out, and the loser's `need_to_compare?` then sees no baseline, records the
+# screenshot as new, and returns without comparing. Green, having compared
+# nothing (#217).
+#
+# "No baseline was ever committed" is a legitimate state with its own
+# warning. "The baseline I just checked out has disappeared" is impossible
+# in a correct run, and must be loud.
+class BaselineDisappearedTest < ActiveSupport::TestCase
+  include DSLStub
+
+  # Stands in for the git checkout: writes a real baseline file the way a
+  # successful `git show` would, so the disappearance is a real file going
+  # away rather than a stubbed boolean flipping.
+  def checking_out(fixture = "a.png")
+    lambda do |_root, _screenshot_path, checkout_path|
+      checkout_path.dirname.mkpath
+      FileUtils.cp(File.expand_path(fixture, TEST_IMAGES_DIR), checkout_path)
+      true
+    end
+  end
+
+  # Deterministic version of the race: the baseline vanishes while the
+  # screenshot is being captured, exactly as a concurrent
+  # `archive_baseline!` would take it.
+  def stealing_screenshoter
+    Class.new(ScreenshoterStub) do
+      define_method(:take_comparison_screenshot) do |snap|
+        super(snap)
+        snap.base_path.delete if snap.base_path.exist?
+      end
+    end
+  end
+
+  test "raises when a checked-out baseline disappears before the comparison" do
+    name = "a_#{Time.now.nsec}"
+
+    SnapDiff::Vcs.stub(:checkout_vcs, checking_out) do
+      SnapDiff.config.stub(:screenshoter, stealing_screenshoter) do
+        error = assert_raises(SnapDiff::Error) do
+          SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion
+        end
+
+        assert_match(/#{name}/, error.message)
+        assert_match(/disappeared/i, error.message)
+      end
+    end
+
+    assert_empty SnapDiff.session.new_screenshots,
+      "a vanished baseline must not be recorded as a never-committed one"
+  end
+
+  # The legitimate half of the distinction stays legitimate: no checkout
+  # succeeded, so there is nothing to have disappeared.
+  test "still records a new screenshot when no baseline was ever committed" do
+    name = "a_#{Time.now.nsec}"
+
+    SnapDiff::Vcs.stub(:checkout_vcs, false) do
+      assert_nil SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion
+    end
+
+    assert_includes SnapDiff.session.new_screenshots, name
+  end
+
+  # The session outlives the test -- Thread.current[] memoizes it for the
+  # whole thread -- so the checkout record must not. Otherwise a name whose
+  # baseline was read in one test raises in the next one, where having no
+  # baseline is perfectly legitimate.
+  test "SnapDiff.reset forgets which baselines were checked out" do
+    name = "a_#{Time.now.nsec}"
+
+    SnapDiff.session.record_baseline_checkout(name)
+    SnapDiff.reset
+
+    SnapDiff.session.record_new_screenshot(name)
+
+    assert_includes SnapDiff.session.new_screenshots, name
+  end
+
+  # A capture that takes a little time, the way a real browser screenshot
+  # does. The window this bug lives in is checkout -> capture ->
+  # need_to_compare?, so an instant capture hides it.
+  def slow_screenshoter
+    Class.new(ScreenshoterStub) do
+      define_method(:take_comparison_screenshot) do |snap|
+        sleep(0.01)
+        super(snap)
+      end
+    end
+  end
+
+  # The real thing: two threads, one name, no injected file deletion. The
+  # loser's baseline is taken by the winner's archive_baseline!. Repeated
+  # because it is a race -- the audit found it by running the harness ten
+  # times. Any single silently-skipped comparison is the bug.
+  test "concurrent tests on one screenshot name never skip a comparison silently" do
+    rounds = Integer(ENV.fetch("RACE_ROUNDS", 20))
+    name = "a_#{Time.now.nsec}"
+    outcomes = Queue.new
+
+    SnapDiff.config.stub(:screenshoter, slow_screenshoter) do
+      SnapDiff::Vcs.stub(:checkout_vcs, checking_out) do
+        # Free-running rather than barriered: the two tests must meet at
+        # every phase offset, and it is the offset where one is archiving
+        # while the other is still capturing that loses a comparison.
+        threads = 2.times.map do |i|
+          Thread.new do
+            sleep(0.005 * i)
+            rounds.times do
+              outcomes << begin
+                assertion = SnapDiff::ScreenshotMatcher.new(name).build_screenshot_assertion
+                if assertion
+                  assertion.validate
+                  :compared
+                else
+                  :skipped
+                end
+              rescue SnapDiff::Error
+                :raised
+              rescue => e
+                e.class
+              ensure
+                SnapDiff.reset
+              end
+            end
+          end
+        end
+
+        threads.each(&:join)
+      end
+    end
+
+    results = Array.new(outcomes.size) { outcomes.pop }
+    tally = results.tally
+    puts "[race] #{tally.inspect}"
+
+    assert_equal 0, results.count(:skipped),
+      "#{results.count(:skipped)} of #{results.size} comparisons were silently skipped (#{tally.inspect})"
+  end
+end