diff --git a/docs/thread_safety.md b/docs/thread_safety.md new file mode 100644 index 00000000..65731249 --- /dev/null +++ b/docs/thread_safety.md @@ -0,0 +1,97 @@ +# Thread Safety Guide for Parallel Testing + +This document explains how `snap_diff` behaves under Rails parallel tests with the `:thread` strategy. + +## Overview + +`snap_diff` is thread safe for parallel test execution as long as global configuration is set before tests run. Per-thread state is isolated, and shared state is protected where it matters. + +## Architecture Summary + +### Per-thread Assertion Registry + +Each thread gets its own `AssertionRegistry` stored in thread-local storage: + +```ruby +def registry + Thread.current[:capybara_screenshot_diff_registry] ||= AssertionRegistry.new +end +``` + +This prevents cross-thread leakage for assertions and screenshot naming. + +### Reporters Snapshot on Notify + +Reporters are notified using a snapshot protected by an eagerly initialized mutex: + +```ruby +@reporters_mutex = Mutex.new + +def notify_reporters(assertions) + reporters_snapshot = reporters_mutex.synchronize { reporters.dup } + reporters_snapshot.each { |reporter| reporter.record(assertions) } +end +``` + +This ensures a stable list while notifying without forcing a global lock around reporter work. + +### HTML Reporter Internal Lock + +The HTML reporter protects `@failures`, `@total`, and `@finalized` with a mutex so `record` and `finalize` can run safely: + +```ruby +@mutex.synchronize do + return if @finalized + @total += total + @failures.concat(failures) +end +``` + +`@finalized` is set only after `write_report` succeeds, so a failed write can be retried. + +### Screenshot Naming Isolation + +Each thread gets its own `ScreenshotNamer` via the per-thread registry, so counters, sections, and groups do not collide. + +### SnapManager Per Call + +`SnapManager` returns a new instance for each call, avoiding shared mutable state. + +## Global Configuration + +Configuration uses `mattr_accessor` and should be set once before tests run. Do not mutate config during parallel execution. + +## Parallel Test Lifecycle + +- Setup: per-thread registry is created, config is read +- Execution: assertions are added to the thread-local registry +- Teardown: `verify` and `reset` operate on the thread-local registry, reporters are notified +- Exit: reporters finalize once per process (using mutex-protected snapshot) + +## Usage Examples + +```ruby +parallelize(workers: :number_of_processors, with: :threads) + +Capybara::Screenshot::Diff.configure do |screenshot, diff| + screenshot.window_size = [1280, 1024] + screenshot.save_path = "doc/screenshots" + diff.tolerance = 0.001 +end +``` + +## Do and Do Not + +Do: +- Set config once in test helper +- Pass per-screenshot options in the call + +Do not: +- Change global config inside tests +- Manually mutate registry internals + +## File System Notes + +- Paths are unique per screenshot name and counter +- `FileUtils.mv` is atomic on most file systems +- Directory creation uses `mkpath` diff --git a/lib/capybara_screenshot_diff/reporters/html.rb b/lib/capybara_screenshot_diff/reporters/html.rb index 57f910bf..92349327 100644 --- a/lib/capybara_screenshot_diff/reporters/html.rb +++ b/lib/capybara_screenshot_diff/reporters/html.rb @@ -18,30 +18,43 @@ def initialize(output_path: "tmp/snap_diff/index.html", embed_images: false) @failures = [] @total = 0 @finalized = false + @mutex = Mutex.new end def record(assertions) + return if @finalized + + failures = [] + total = 0 + assertions.each do |assertion| compare = assertion.compare next unless compare - @total += 1 + total += 1 next unless compare.difference&.different? - @failures << failure_entry_for(assertion.name, compare) + failures << failure_entry_for(assertion.name, compare) rescue => e warn "[snap_diff] Reporter skipped '#{assertion.name}': #{e.message}" if ENV["DEBUG"] end + + @mutex.synchronize do + return if @finalized + @total += total + @failures.concat(failures) + end end def finalize - return if @finalized + @mutex.synchronize do + return if @finalized + return if failures.empty? - @finalized = true - return if failures.empty? - - write_report - output_path + write_report + @finalized = true + output_path + end end def passed = total - failures.size @@ -108,7 +121,7 @@ def write_report end at_exit do - CapybaraScreenshotDiff.reporters.each do |reporter| + CapybaraScreenshotDiff.reporters_mutex.synchronize { CapybaraScreenshotDiff.reporters.dup }.each do |reporter| result = reporter.finalize $stdout.puts "[snap_diff] HTML report: #{result}" if result.is_a?(Pathname) rescue => e diff --git a/lib/capybara_screenshot_diff/screenshot_assertion.rb b/lib/capybara_screenshot_diff/screenshot_assertion.rb index ba1cb67c..9faffce3 100644 --- a/lib/capybara_screenshot_diff/screenshot_assertion.rb +++ b/lib/capybara_screenshot_diff/screenshot_assertion.rb @@ -133,19 +133,26 @@ def reporters @reporters ||= [] end + attr_reader :reporters_mutex + def_delegator :registry, :screenshot_namer def_delegator :registry, :verify private def notify_reporters(assertions) - return if reporters.empty? || assertions.nil? || assertions.empty? + return if assertions.nil? || assertions.empty? + + reporters_snapshot = reporters_mutex.synchronize { reporters.dup } + return if reporters_snapshot.empty? - reporters.each do |reporter| + reporters_snapshot.each do |reporter| reporter.record(assertions) rescue => e warn "[capybara-screenshot-diff] Reporter failed: #{e.message}" end end end + + @reporters_mutex = Mutex.new end diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb index c3611342..5713296d 100644 --- a/test/unit/reporters/html_reporter_test.rb +++ b/test/unit/reporters/html_reporter_test.rb @@ -98,6 +98,70 @@ class HTMLReporterTest < ActiveSupport::TestCase assert_includes html, "data:image/png;base64," end + test "#record from multiple threads produces correct totals" do + reporter = HTML.new(output_path: @output_path) + threads = 10 + assertions_per_thread = 5 + + workers = threads.times.map do + Thread.new do + batch = assertions_per_thread.times.map { |i| build_passing_assertion("t#{Thread.current.object_id}_#{i}") } + reporter.record(batch) + end + end + workers.each(&:join) + + assert_equal threads * assertions_per_thread, reporter.total + end + + test "#record and #finalize synchronize internal state" do + reporter = HTML.new(output_path: @output_path) + + fake_mutex = Class.new do + attr_reader :synchronize_calls + + def initialize + @synchronize_calls = 0 + end + + def synchronize + @synchronize_calls += 1 + yield + end + end.new + + # Using private state and a fake mutex here is intentional. + # This test guards a tricky synchronization detail; avoid refactors unless behavior changes. + reporter.instance_variable_set(:@mutex, fake_mutex) + + reporter.record([build_passing_assertion("sync")]) + reporter.finalize + + assert_operator fake_mutex.synchronize_calls, :>=, 1 + end + + test "#finalize can retry after write_report failure" do + reporter = HTML.new(output_path: @output_path) + reporter.record([build_failing_assertion("retry")]) + + # Make write_report fail on first attempt + FileUtils.rm_rf(@output_dir) + File.write(@output_dir.to_s, "not a directory") + + begin + reporter.finalize + rescue # rubocop:disable Lint/SuppressedException + end + + # Restore writable directory and retry + File.delete(@output_dir.to_s) + FileUtils.mkdir_p(@output_dir) + + result = reporter.finalize + assert_instance_of Pathname, result + assert @output_path.exist? + end + private def build_passing_assertion(name) diff --git a/test/unit/reporters_mutex_test.rb b/test/unit/reporters_mutex_test.rb new file mode 100644 index 00000000..11daac7e --- /dev/null +++ b/test/unit/reporters_mutex_test.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require "test_helper" + +module CapybaraScreenshotDiff + class ReportersMutexTest < ActiveSupport::TestCase + setup do + @original_reporters = CapybaraScreenshotDiff.reporters.dup + CapybaraScreenshotDiff.reporters.clear + end + + teardown do + CapybaraScreenshotDiff.reporters.clear + CapybaraScreenshotDiff.reporters.concat(@original_reporters) + end + + test "reporters_mutex is eagerly initialized" do + assert_instance_of Mutex, CapybaraScreenshotDiff.reporters_mutex + end + + test "reporters_mutex returns the same instance" do + assert_same CapybaraScreenshotDiff.reporters_mutex, CapybaraScreenshotDiff.reporters_mutex + end + + test "reporters notification iterates over snapshot" do + received = [] + + mutating_reporter = Class.new do + define_method :record do |assertions| + received << [:original, assertions] + CapybaraScreenshotDiff.reporters.clear + CapybaraScreenshotDiff.reporters << Class.new { + define_method(:record) { |a| received << [:added, a] } + }.new + end + end.new + + CapybaraScreenshotDiff.reporters << mutating_reporter + + assertions = [:some, :assertions] + + assert_nothing_raised do + CapybaraScreenshotDiff.send(:notify_reporters, assertions) + end + + assert_equal [[:original, assertions]], received + end + end +end