diff --git a/lib/snap_diff/drivers/vips_driver.rb b/lib/snap_diff/drivers/vips_driver.rb index 0cd91f0a..e9e3ec46 100644 --- a/lib/snap_diff/drivers/vips_driver.rb +++ b/lib/snap_diff/drivers/vips_driver.rb @@ -18,6 +18,17 @@ module Drivers class VipsDriver include SnapDiff::Driver + # libvips caches loader operations keyed on filename + mtime, and mtime + # has ONE-SECOND resolution -- so overwriting a path and re-reading it + # within the same second hands back the PREVIOUS image. This gem does + # exactly that: the screenshoter writes `.png`, + # `checkout_base_screenshot` writes `.base.png` from VCS, and the + # comparison then reads both. + # + # `revalidate: true` tells the loader to skip the cached result (libvips + # 8.15+). + REVALIDATE = Vips.at_least_libvips?(8, 15) ? {revalidate: true}.freeze : {}.freeze + def find_difference_region(comparison) new_image, base_image, options = comparison.new_image, comparison.base_image, comparison.options @@ -86,7 +97,7 @@ def load_images(old_file_name, new_file_name) end def from_file(filename) - result = ::Vips::Image.new_from_file(filename.to_s) + result = ::Vips::Image.new_from_file(filename.to_s, **REVALIDATE) result = result.colourspace(:srgb) if result.bands < 3 result = result.bandjoin(255) if result.bands == 3 diff --git a/test/system_test_case.rb b/test/system_test_case.rb index e2e419e1..f96d176c 100644 --- a/test/system_test_case.rb +++ b/test/system_test_case.rb @@ -51,10 +51,11 @@ class SystemTestCase < ActiveSupport::TestCase SnapDiff.config.tolerance = @orig_tolerance Capybara.current_driver = Capybara.default_driver - if SnapDiff.config.driver == :vips - Vips.cache_set_max(0) - Vips.cache_set_max(1000) - end + # The vips cache flush that used to live here (cache_set_max 0 then 1000) + # papered over libvips serving a stale image when a screenshot path was + # rewritten within the same second. VipsDriver#from_file now passes + # `revalidate: true`, so the workaround is gone -- see the regression test + # in test/unit/drivers/vips_driver_test.rb. end private diff --git a/test/unit/drivers/vips_driver_test.rb b/test/unit/drivers/vips_driver_test.rb index abf22f53..1c70cfdb 100644 --- a/test/unit/drivers/vips_driver_test.rb +++ b/test/unit/drivers/vips_driver_test.rb @@ -24,11 +24,36 @@ class VipsDriverTest < ActiveSupport::TestCase @new_screenshot_result.close @new_screenshot_result.unlink end + end - if defined?(Vips) - Vips.cache_set_max(0) - Vips.cache_set_max(1000) - end + # REGRESSION. libvips caches loaders on filename + mtime, and mtime has + # one-second resolution, so rewriting a path and re-reading it within the + # same second used to hand back the PREVIOUS image. #from_file passes + # `revalidate: true` to defeat that. + # + # The gem does exactly this: the screenshoter writes `.png`, + # `checkout_base_screenshot` writes `.base.png` from VCS, and the + # comparison then reads both -- a stale read compares against an image + # that is no longer on disk. + # + # The teardown above used to flush the whole vips cache + # (`Vips.cache_set_max(0); Vips.cache_set_max(1000)`) to paper over this; + # with the driver fixed, that workaround is gone. + test "#from_file re-reads a path that was overwritten within the same second" do + driver = VipsDriver.new + path = Rails.root / "revalidate_probe.png" + + FileUtils.cp(TEST_IMAGES_DIR / "b.png", path) + first_avg = driver.from_file(path).avg # force evaluation BEFORE the overwrite + + FileUtils.cp(TEST_IMAGES_DIR / "a.png", path) + second_avg = driver.from_file(path).avg + + assert_not_equal first_avg, second_avg, + "vips served the cached b.png after the path was overwritten with a.png" + assert_equal driver.from_file(TEST_IMAGES_DIR / "a.png").avg, second_avg + ensure + FileUtils.rm_f(path) end test "#different? returns false when comparing identical images" do