From 0292fc33f6222f90b3337b9a458583198cbdd320 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 12 Apr 2026 21:17:32 +0200 Subject: [PATCH 1/4] chore: remove dead code and fix test doubles (T5, T7, T8) Co-Authored-By: Claude Opus 4.6 (1M context) --- .../screenshot/diff/drivers/vips_driver.rb | 7 ----- .../screenshot_namer.rb | 7 ----- test/support/test_doubles.rb | 30 ++----------------- test/unit/screenshot_namer_test.rb | 15 ---------- 4 files changed, 2 insertions(+), 57 deletions(-) diff --git a/lib/capybara/screenshot/diff/drivers/vips_driver.rb b/lib/capybara/screenshot/diff/drivers/vips_driver.rb index b23d3909..45ae42d4 100644 --- a/lib/capybara/screenshot/diff/drivers/vips_driver.rb +++ b/lib/capybara/screenshot/diff/drivers/vips_driver.rb @@ -112,13 +112,6 @@ def highlight_mask(diff_mask, merged_image, color: CapybaraScreenshotDiff::RED_R private - def region_covers_entire_image?(region, base_image) - region.x.zero? && - region.y.zero? && - region.height == height_for(base_image) && - region.width == width_for(base_image) - end - class << self def difference_area(old_image, new_image, color_distance: 0) mask = difference_mask(new_image, old_image, color_distance) diff --git a/lib/capybara_screenshot_diff/screenshot_namer.rb b/lib/capybara_screenshot_diff/screenshot_namer.rb index aaa70c92..47dca3f2 100644 --- a/lib/capybara_screenshot_diff/screenshot_namer.rb +++ b/lib/capybara_screenshot_diff/screenshot_namer.rb @@ -72,13 +72,6 @@ def current_group_directory File.join(*([screenshot_area] + directory_parts)) end - # Clears the directory for the current screenshot group. - # This is typically used when starting a new group to remove old screenshots. - def clear_current_group_directory - dir_to_clear = current_group_directory - FileUtils.rm_rf(dir_to_clear) if Dir.exist?(dir_to_clear) - end - private def reset_group_counter diff --git a/test/support/test_doubles.rb b/test/support/test_doubles.rb index aa6cc1a1..a90c62ef 100644 --- a/test/support/test_doubles.rb +++ b/test/support/test_doubles.rb @@ -89,39 +89,13 @@ def supports?(...) @is_vips_driver end - # Return Object to avoid infinite recursion + # Returns Object so warning messages in ImagePreprocessor don't + # couple tests to the TestDriver class name. def class Object end end - # Test double for image preprocessors - class TestPreprocessor - attr_reader :call_called, :call_args, :process_comparison_called, :process_comparison_args, :processed_images - - def initialize(processed_images) - @processed_images = processed_images - @call_called = false - @call_args = nil - @process_comparison_called = false - @process_comparison_args = nil - end - - def call(images) - @call_called = true - @call_args = images - processed_images - end - - # Process a comparison object directly - # Mirrors the implementation in ImagePreprocessor - def process_comparison(comparison) - @process_comparison_called = true - @process_comparison_args = comparison - comparison - end - end - # Test double for difference results class TestDifference attr_reader :different_value diff --git a/test/unit/screenshot_namer_test.rb b/test/unit/screenshot_namer_test.rb index ed65fc03..cfd71baa 100644 --- a/test/unit/screenshot_namer_test.rb +++ b/test/unit/screenshot_namer_test.rb @@ -138,21 +138,6 @@ class ScreenshotNamerTest < ActiveSupport::TestCase assert_equal ["s1", "g1"], @screenshot_namer.directory_parts end - test "#clear_current_group_directory removes group directory" do - @screenshot_namer.group = "to_clear" - dir_path = @screenshot_namer.current_group_directory - FileUtils.mkdir_p(dir_path) - assert Dir.exist?(dir_path) - - @screenshot_namer.clear_current_group_directory - assert_not Dir.exist?(dir_path) - end - - test "#clear_current_group_directory handles non-existent directories" do - @screenshot_namer.group = "nonexistent" - assert_nothing_raised { @screenshot_namer.clear_current_group_directory } - end - test "#current_group_directory constructs full directory path" do @screenshot_namer.section = "section1" @screenshot_namer.group = "group1" From 9df45be7d47eaf8593d3d7dbba329040a364ef17 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 12 Apr 2026 21:17:32 +0200 Subject: [PATCH 2/4] feat: disable_animations helper, lazy report path (P2.3, P3.1, P3.2) Co-Authored-By: Claude Opus 4.6 (1M context) --- README.md | 2 +- .../screenshot/diff/browser_helpers.rb | 13 ++++++++++++ lib/capybara/screenshot/diff/screenshoter.rb | 7 ++++--- lib/capybara_screenshot_diff.rb | 1 + .../reporters/html.rb | 20 +++++++++++++------ scripts/generate_sample_report.rb | 2 +- test/unit/reporters/html_reporter_test.rb | 6 ++++++ 7 files changed, 40 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 6a5b41b8..81dc8e31 100644 --- a/README.md +++ b/README.md @@ -127,7 +127,7 @@ end **Screenshots differ between CI and local** — Use `tolerance: 0.001` or `perceptual_threshold: 2.0`. Set `window_size` for consistent dimensions. -**Animations cause flaky diffs** — `Capybara.disable_animation = true`, or `stability_time_limit: 1`. +**Animations cause flaky diffs** — `Capybara::Screenshot.disable_animations = true` disables CSS animations/transitions before each screenshot. Or use `stability_time_limit: 1` to wait for animations to finish. **Dynamic content always differs** — `screenshot "page", skip_area: [".timestamp", "#ad-banner"]` diff --git a/lib/capybara/screenshot/diff/browser_helpers.rb b/lib/capybara/screenshot/diff/browser_helpers.rb index 88567e00..80ac7289 100644 --- a/lib/capybara/screenshot/diff/browser_helpers.rb +++ b/lib/capybara/screenshot/diff/browser_helpers.rb @@ -60,6 +60,19 @@ def self.hide_caret session.execute_script(HIDE_CARET_SCRIPT) end + DISABLE_ANIMATIONS_SCRIPT = <<~JS + if (!document.getElementById('csdDisableAnimationsStyle')) { + let style = document.createElement('style'); + style.setAttribute('id', 'csdDisableAnimationsStyle'); + style.textContent = '*, *::before, *::after { animation-duration: 0s !important; animation-delay: 0s !important; transition-duration: 0s !important; transition-delay: 0s !important; }'; + document.head.appendChild(style); + } + JS + + def self.disable_animations + session.execute_script(DISABLE_ANIMATIONS_SCRIPT) + end + FIND_ACTIVE_ELEMENT_SCRIPT = <<~JS function activeElement(){ const ae = document.activeElement; diff --git a/lib/capybara/screenshot/diff/screenshoter.rb b/lib/capybara/screenshot/diff/screenshoter.rb index d7cae79e..bfd48860 100644 --- a/lib/capybara/screenshot/diff/screenshoter.rb +++ b/lib/capybara/screenshot/diff/screenshoter.rb @@ -8,6 +8,8 @@ module Screenshot class Screenshoter attr_reader :capture_options, :driver + # @param capture_options [Hash] Options for capturing (window_size, wait, etc.) + # @param comparison_options [Hash] Options for image comparison (driver, tolerance, etc.) def initialize(capture_options, comparison_options = {}) @capture_options = capture_options @driver = Diff::Drivers.for(comparison_options) @@ -70,9 +72,8 @@ def prepare_page_for_screenshot(timeout:) blurred_input = BrowserHelpers.blur_from_focused_element if Screenshot.blur_active_element - if Screenshot.hide_caret - BrowserHelpers.hide_caret - end + BrowserHelpers.hide_caret if Screenshot.hide_caret + BrowserHelpers.disable_animations if Screenshot.disable_animations blurred_input end diff --git a/lib/capybara_screenshot_diff.rb b/lib/capybara_screenshot_diff.rb index a4631525..f275e32b 100644 --- a/lib/capybara_screenshot_diff.rb +++ b/lib/capybara_screenshot_diff.rb @@ -30,6 +30,7 @@ module Screenshot mattr_accessor(:blur_active_element) { true } mattr_accessor :enabled mattr_accessor(:hide_caret) { true } + mattr_accessor :disable_animations mattr_reader(:root) { (defined?(Rails) && defined?(Rails.root) && Rails.root) || Pathname(".").expand_path } mattr_accessor :stability_time_limit mattr_accessor :window_size diff --git a/lib/capybara_screenshot_diff/reporters/html.rb b/lib/capybara_screenshot_diff/reporters/html.rb index 92349327..17b4d233 100644 --- a/lib/capybara_screenshot_diff/reporters/html.rb +++ b/lib/capybara_screenshot_diff/reporters/html.rb @@ -9,11 +9,10 @@ module CapybaraScreenshotDiff module Reporters class HTML - attr_reader :output_path, :failures, :total + attr_reader :failures, :total - def initialize(output_path: "tmp/snap_diff/index.html", embed_images: false) - @output_path = Pathname.new(output_path) - @report_dir = @output_path.dirname + def initialize(output_path: nil, embed_images: false) + @explicit_output_path = output_path @embed_images = embed_images @failures = [] @total = 0 @@ -57,6 +56,10 @@ def finalize end end + def output_path + @output_path ||= Pathname.new(@explicit_output_path || self.class.default_output_path) + end + def passed = total - failures.size def failed = failures.size @@ -73,6 +76,11 @@ def self.template_path File.expand_path("templates/report.html.erb", __dir__) end + def self.default_output_path + root = Capybara::Screenshot.root || Pathname.pwd + root / Capybara::Screenshot.save_path / "snap_diff_report.html" + end + private def failure_entry_for(name, compare) @@ -96,7 +104,7 @@ def resolve_image(path) pathname = Pathname.new(path).expand_path return unless pathname.exist? - @embed_images ? data_uri(pathname) : pathname.relative_path_from(@report_dir.expand_path).to_s + @embed_images ? data_uri(pathname) : pathname.relative_path_from(output_path.dirname.expand_path).to_s end def data_uri(pathname) @@ -106,7 +114,7 @@ def data_uri(pathname) end def write_report - FileUtils.mkdir_p(@report_dir) + FileUtils.mkdir_p(output_path.dirname) File.write(output_path, render) end end diff --git a/scripts/generate_sample_report.rb b/scripts/generate_sample_report.rb index d46d10ec..9b9245e1 100644 --- a/scripts/generate_sample_report.rb +++ b/scripts/generate_sample_report.rb @@ -9,7 +9,7 @@ require "capybara/screenshot/diff" require "capybara_screenshot_diff/reporters/html" -output_path = File.expand_path("../tmp/snap_diff/index.html", __dir__) +output_path = CapybaraScreenshotDiff::Reporters::HTML.default_output_path # Build real comparisons using the gem's own ImageCompare. # Each pair gets a unique copy of the base image to avoid annotation file conflicts. diff --git a/test/unit/reporters/html_reporter_test.rb b/test/unit/reporters/html_reporter_test.rb index 5713296d..293090c0 100644 --- a/test/unit/reporters/html_reporter_test.rb +++ b/test/unit/reporters/html_reporter_test.rb @@ -78,6 +78,12 @@ class HTMLReporterTest < ActiveSupport::TestCase assert_includes @output_path.read, "valid" end + test "HTML reporter defaults to screenshot root path" do + reporter = HTML.new + expected = Capybara::Screenshot.root / Capybara::Screenshot.save_path / "snap_diff_report.html" + assert_equal expected, reporter.output_path + end + test "#record uses relative paths by default" do reporter = HTML.new(output_path: @output_path) From e9368207ae8fc9251732104ac59034ec65b10a84 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 12 Apr 2026 21:17:32 +0200 Subject: [PATCH 3/4] test: extract shared driver contract tests (T1) Co-Authored-By: Claude Opus 4.6 (1M context) --- test/support/driver_contract_tests.rb | 40 +++++++++++++++++++++ test/unit/drivers/chunky_png_driver_test.rb | 2 ++ test/unit/drivers/vips_driver_test.rb | 2 ++ 3 files changed, 44 insertions(+) create mode 100644 test/support/driver_contract_tests.rb diff --git a/test/support/driver_contract_tests.rb b/test/support/driver_contract_tests.rb new file mode 100644 index 00000000..1d861b8d --- /dev/null +++ b/test/support/driver_contract_tests.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +# Shared contract tests for all image processing drivers. +# Include in any driver test class that uses DSLStub (provides make_comparison). +module DriverContractTests + extend ActiveSupport::Concern + + included do + test "[contract] quick_equal? returns true for identical images" do + comp = make_comparison(:a, :a) + assert comp.quick_equal? + end + + test "[contract] different? returns false for identical images" do + comp = make_comparison(:a, :a) + assert_not comp.different? + end + + test "[contract] different? returns true for different images" do + comp = make_comparison(:a, :c) + assert comp.different? + end + + test "[contract] different? generates annotated images for different images" do + comp = make_comparison(:a, :c) + assert comp.different? + + assert File.exist?(comp.reporter.annotated_base_image_path) + assert File.exist?(comp.reporter.annotated_image_path) + end + + test "[contract] different? does not create annotated images for identical images" do + comp = make_comparison(:c, :c) + assert_not comp.different? + + assert_not File.exist?(comp.reporter.annotated_base_image_path) + assert_not File.exist?(comp.reporter.annotated_image_path) + end + end +end diff --git a/test/unit/drivers/chunky_png_driver_test.rb b/test/unit/drivers/chunky_png_driver_test.rb index c99948bd..6ea42b5c 100644 --- a/test/unit/drivers/chunky_png_driver_test.rb +++ b/test/unit/drivers/chunky_png_driver_test.rb @@ -3,6 +3,7 @@ require "test_helper" require "capybara/screenshot/diff/image_compare" require "capybara/screenshot/diff/drivers/chunky_png_driver" +require "support/driver_contract_tests" module Capybara module Screenshot @@ -10,6 +11,7 @@ module Diff module Drivers class ChunkyPNGDriverTest < ActiveSupport::TestCase include CapybaraScreenshotDiff::DSLStub + include DriverContractTests class QuickEqualTest < self test "#quick_equal? returns true when comparing identical images" do diff --git a/test/unit/drivers/vips_driver_test.rb b/test/unit/drivers/vips_driver_test.rb index f0596243..f51affef 100644 --- a/test/unit/drivers/vips_driver_test.rb +++ b/test/unit/drivers/vips_driver_test.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "test_helper" +require "support/driver_contract_tests" unless defined?(Vips) warn "VIPS not present. Skipping VIPS driver tests." @@ -15,6 +16,7 @@ module Diff module Drivers class VipsDriverTest < ActiveSupport::TestCase include CapybaraScreenshotDiff::DSLStub + include DriverContractTests setup do @new_screenshot_result = Tempfile.new(%w[screenshot .png], Rails.root) From 60222d21c38d9e5ef6215a50d0b1824ae34b96d5 Mon Sep 17 00:00:00 2001 From: Paul Keen <125715+pftg@users.noreply.github.com> Date: Sun, 12 Apr 2026 21:17:32 +0200 Subject: [PATCH 4/4] fix: VCS stability, global state guard, bin/ci - VCS uses Open3/array-form system (no Dir.chdir, no shell interpolation) - Global state guard: snapshot/restore all mattr_accessors per test - static_test.rb restores Screenshot.root in teardown - bin/ci: lint + unit tests with auto-fix for pre-push hook Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/ci | 13 +++++++++ lib/capybara/screenshot/diff/vcs.rb | 43 ++++++++++++++--------------- test/test_helper.rb | 26 +++++++++++++---- test/unit/static_test.rb | 5 ++++ test/unit/vcs_test.rb | 19 +++++++------ 5 files changed, 69 insertions(+), 37 deletions(-) create mode 100755 bin/ci diff --git a/bin/ci b/bin/ci new file mode 100755 index 00000000..ab70c0ab --- /dev/null +++ b/bin/ci @@ -0,0 +1,13 @@ +#!/bin/bash +set -e + +echo "Running linter..." +if ! bin/standardrb; then + echo "Lint errors found. Auto-fixing..." + bin/standardrb -a + echo "Lint errors were auto-fixed. Please review and re-commit." + exit 1 +fi + +echo "Running unit tests..." +bin/rake test:unit diff --git a/lib/capybara/screenshot/diff/vcs.rb b/lib/capybara/screenshot/diff/vcs.rb index 1c1aeea4..06f3a769 100644 --- a/lib/capybara/screenshot/diff/vcs.rb +++ b/lib/capybara/screenshot/diff/vcs.rb @@ -1,40 +1,37 @@ # frozen_string_literal: true +require "open3" require_relative "os" module Capybara module Screenshot module Diff module Vcs - SILENCE_ERRORS = Os::ON_WINDOWS ? "2>nul" : "2>/dev/null" - def self.checkout_vcs(root, screenshot_path, checkout_path) - abs_screenshot_path = Pathname.new(screenshot_path).expand_path - redirect_target = "#{checkout_path} #{SILENCE_ERRORS}" - - Dir.chdir(root) do - git_toplevel = `git rev-parse --show-toplevel 2>/dev/null`.chomp - return false if git_toplevel.empty? - - vcs_file_path = abs_screenshot_path.relative_path_from(Pathname.new(git_toplevel)) - show_command = "git show HEAD:#{vcs_file_path}" - if Screenshot.use_lfs - system("#{show_command} > #{checkout_path}.tmp #{SILENCE_ERRORS}", exception: !!ENV["DEBUG"]) - - `git lfs smudge < #{checkout_path}.tmp > #{redirect_target}` if $CHILD_STATUS == 0 - - File.delete "#{checkout_path}.tmp" - else - system("#{show_command} > #{redirect_target}", exception: !!ENV["DEBUG"]) + root_path = root.to_s + git_root, status = Open3.capture2("git", "-C", root_path, "rev-parse", "--show-toplevel") + return false unless status.success? + + git_root = git_root.chomp + vcs_file_path = Pathname.new(screenshot_path).expand_path.relative_path_from(Pathname.new(git_root)).to_s + + if Screenshot.use_lfs + tmp_path = "#{checkout_path}.tmp" + success = system("git", "-C", root_path, "show", "HEAD:#{vcs_file_path}", out: tmp_path, err: File::NULL) + if success + system("git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL) end + File.delete(tmp_path) if File.exist?(tmp_path) + else + success = system("git", "-C", root_path, "show", "HEAD:#{vcs_file_path}", out: checkout_path.to_s, err: File::NULL) end - if $CHILD_STATUS != 0 + unless success checkout_path.delete if checkout_path.exist? - false - else - true + return false end + + true end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 532188e3..0d9abbc0 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -39,20 +39,34 @@ class ActiveSupport::TestCase # Set up fixtures and test helpers self.file_fixture_path = Pathname.new(File.expand_path("fixtures", __dir__)) + # Snapshot ALL global state before each test, restore after. + # Prevents one test from poisoning another via leaked mattr_accessor changes. + GLOBAL_STATE_MODULES = [ + Capybara::Screenshot, + Capybara::Screenshot::Diff + ].freeze + setup do - @_orig_fail_if_new = Capybara::Screenshot::Diff.fail_if_new - @_orig_blur = Capybara::Screenshot.blur_active_element - @_orig_hide_caret = Capybara::Screenshot.hide_caret + @_global_snapshots = GLOBAL_STATE_MODULES.map { |mod| + attrs = mod.class_variables.map { |cv| [cv, mod.class_variable_get(cv)] } + [mod, attrs] + }.to_h + @_orig_cwd = Dir.pwd + @_orig_capybara_app = Capybara.app Capybara::Screenshot::Diff.fail_if_new = false Capybara::Screenshot.blur_active_element = false Capybara::Screenshot.hide_caret = false + Capybara::Screenshot.disable_animations = false end teardown do - Capybara::Screenshot::Diff.fail_if_new = @_orig_fail_if_new - Capybara::Screenshot.blur_active_element = @_orig_blur - Capybara::Screenshot.hide_caret = @_orig_hide_caret + # Restore all global state + @_global_snapshots&.each do |mod, attrs| + attrs.each { |cv, val| mod.class_variable_set(cv, val) } + end + Dir.chdir(@_orig_cwd) if @_orig_cwd && Dir.pwd != @_orig_cwd + Capybara.app = @_orig_capybara_app if @_orig_capybara_app CapybaraScreenshotDiff::SnapManager.cleanup! unless persist_comparisons? end diff --git a/test/unit/static_test.rb b/test/unit/static_test.rb index 2f1d3ee5..e381a173 100644 --- a/test/unit/static_test.rb +++ b/test/unit/static_test.rb @@ -5,8 +5,13 @@ module CapybaraScreenshotDiff class StaticTest < ActiveSupport::TestCase + setup do + @original_root = Capybara::Screenshot.root + end + teardown do Capybara.app = Rails.application + Capybara::Screenshot.root = @original_root end test ".serve sets Capybara.app to serve the directory" do diff --git a/test/unit/vcs_test.rb b/test/unit/vcs_test.rb index e1cf0a11..f85b5159 100644 --- a/test/unit/vcs_test.rb +++ b/test/unit/vcs_test.rb @@ -8,23 +8,26 @@ module Diff class VcsTest < ActiveSupport::TestCase include Vcs + PROJECT_ROOT = Pathname.new(File.expand_path("../..", __dir__)) + setup do - FileUtils.mkdir_p(Screenshot.root) - @base_screenshot = Tempfile.new(%w[vcs_base_screenshot. .attempt.0.png], Screenshot.root) + @tmp_dir = PROJECT_ROOT / "tmp" / "vcs_test_#{Process.pid}" + FileUtils.mkdir_p(@tmp_dir) + @base_screenshot = Tempfile.new(%w[vcs_base. .png], @tmp_dir.to_s) end teardown do - if @base_screenshot.is_a?(Tempfile) - @base_screenshot.close - @base_screenshot.unlink - end + @base_screenshot&.close + @base_screenshot&.unlink + FileUtils.rm_rf(@tmp_dir) end test "#checkout_vcs checks out and verifies the original screenshot" do screenshot_path = file_fixture("images/a.png") - base_screenshot_path = Pathname.new(@base_screenshot.path) - assert Vcs.checkout_vcs(Screenshot.root, screenshot_path, base_screenshot_path) + + assert Vcs.checkout_vcs(@tmp_dir, screenshot_path, base_screenshot_path), + "checkout_vcs failed: root=#{@tmp_dir}" assert base_screenshot_path.exist? assert_equal screenshot_path.size, base_screenshot_path.size