refact: simplify readability for stable screenshot (#121)

lib/capybara/screenshot/diff/screenshot_matcher.rb

@@ -44,9 +44,12 @@ def build_screenshot_matches_job
           return if Capybara::Screenshot::Diff.fail_if_new && !base_screenshot_path.exist?
 
           capture_options = {
+            # screenshot options
             capybara_screenshot_options: driver_options[:capybara_screenshot_options],
             crop: driver_options.delete(:crop),
+            # delivery options
             screenshot_format: driver_options[:screenshot_format],
+            # stability options
             stability_time_limit: driver_options.delete(:stability_time_limit),
             wait: driver_options.delete(:wait)
           }

lib/capybara/screenshot/diff/screenshoter.rb

@@ -43,11 +43,8 @@ def self.cleanup_attempts_screenshots(base_file)
       # On `stability_time_limit` it checks that page stop updating by comparison several screenshot attempts
       # On reaching `wait` limit then it has been failed. On failing we annotate screenshot attempts to help to debug
       def take_comparison_screenshot(screenshot_path)
-        new_screenshot_path = Screenshoter.gen_next_attempt_path(screenshot_path, 0)
-
-        take_screenshot(new_screenshot_path)
+        capture_screenshot_at(screenshot_path)
 
-        FileUtils.mv(new_screenshot_path, screenshot_path, force: true)
         Screenshoter.cleanup_attempts_screenshots(screenshot_path)
       end
 
@@ -61,13 +58,8 @@ def take_screenshot(screenshot_path)
         blurred_input = prepare_page_for_screenshot(timeout: wait)
 
         # Take browser screenshot and save
-        tmpfile = Tempfile.new([screenshot_path.basename.to_s, PNG_EXTENSION])
-        BrowserHelpers.session.save_screenshot(tmpfile.path, **capybara_screenshot_options)
+        save_and_process_screenshot(screenshot_path)
 
-        # Load saved screenshot and pre-process it
-        process_screenshot(tmpfile.path, screenshot_path)
-      ensure
-        File.unlink(tmpfile) if tmpfile
         blurred_input&.click
       end
 
@@ -84,30 +76,18 @@ def process_screenshot(stored_path, screenshot_path)
         driver.save_image_to(screenshot_image, screenshot_path)
       end
 
-      def reduce_retina_image_size(file_name)
-        expected_image_width = Screenshot.window_size[0]
-        saved_image = driver.from_file(file_name.to_s)
-        return if driver.width_for(saved_image) < expected_image_width * 2
-
-        resized_image = resize_if_needed(saved_image)
-
-        driver.save_image_to(resized_image, file_name)
-      end
-
       def notice_how_to_avoid_this
         unless defined?(@_csd_retina_warned)
           warn "Halving retina screenshot.  " \
-                'You should add "force-device-scale-factor=1" to your Chrome chromeOptions args.'
+                 'You should add "force-device-scale-factor=1" to your Chrome chromeOptions args.'
           @_csd_retina_warned = true
         end
       end
 
       def prepare_page_for_screenshot(timeout:)
         wait_images_loaded(timeout: timeout) if timeout
 
-        blurred_input = if Screenshot.blur_active_element
-          BrowserHelpers.blur_from_focused_element
-        end
+        blurred_input = BrowserHelpers.blur_from_focused_element if Screenshot.blur_active_element
 
         if Screenshot.hide_caret
           BrowserHelpers.hide_caret
@@ -119,12 +99,12 @@ def prepare_page_for_screenshot(timeout:)
       def wait_images_loaded(timeout:)
         return unless timeout
 
-        start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+        deadline_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + timeout
         loop do
           pending_image = BrowserHelpers.pending_image_to_load
           break unless pending_image
 
-          if (Process.clock_gettime(Process::CLOCK_MONOTONIC) - start) >= timeout
+          if Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline_at
             raise CapybaraScreenshotDiff::ExpectationNotMet, "Images have not been loaded after #{timeout}s: #{pending_image.inspect}"
           end
 
@@ -134,6 +114,29 @@ def wait_images_loaded(timeout:)
 
       private
 
+      def save_and_process_screenshot(screenshot_path)
+        tmpfile = Tempfile.new([screenshot_path.basename.to_s, PNG_EXTENSION])
+        BrowserHelpers.session.save_screenshot(tmpfile.path, **capybara_screenshot_options)
+        # Load saved screenshot and pre-process it
+        process_screenshot(tmpfile.path, screenshot_path)
+      ensure
+        File.unlink(tmpfile) if tmpfile
+      end
+
+      def capture_screenshot_at(screenshot_path)
+        new_screenshot_path = Screenshoter.gen_next_attempt_path(screenshot_path, 0)
+        take_and_process_screenshot(new_screenshot_path, screenshot_path)
+      end
+
+      def take_and_process_screenshot(new_screenshot_path, screenshot_path)
+        take_screenshot(new_screenshot_path)
+        move_screenshot_to(new_screenshot_path, screenshot_path)
+      end
+
+      def move_screenshot_to(new_screenshot_path, screenshot_path)
+        FileUtils.mv(new_screenshot_path, screenshot_path, force: true)
+      end
+
       def resize_if_needed(saved_image)
         expected_image_width = Screenshot.window_size[0]
         return saved_image if driver.width_for(saved_image) < expected_image_width * 2

lib/capybara/screenshot/diff/stable_screenshoter.rb

@@ -55,43 +55,39 @@ def take_stable_screenshot(screenshot_path)
           screenshot_path = screenshot_path.is_a?(String) ? Pathname.new(screenshot_path) : screenshot_path
           # We try to compare first attempt with checkout version, in order to not run next screenshots
           attempt_path = nil
-          screenshot_started_at = last_attempt_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+          deadline_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + wait
 
           # Cleanup all previous attempts for sure
           Screenshoter.cleanup_attempts_screenshots(screenshot_path)
 
           0.step do |i|
-            # Prevents redundant screenshots generations
             # FIXME: it should be wait, and wait should be replaced with stability_time_limit
             sleep(stability_time_limit) unless i == 0
-
-            elapsed_time = last_attempt_at - screenshot_started_at
-
-            prev_attempt_path = attempt_path
-            attempt_path = Screenshoter.gen_next_attempt_path(screenshot_path, i)
-
-            @screenshoter.take_screenshot(attempt_path)
-            last_attempt_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
-
-            next unless prev_attempt_path
-            stabilization_comparator = build_comparison_for(attempt_path, prev_attempt_path)
-
-            # If previous screenshot is equal to the current, then we are good
-            return attempt_path if prev_attempt_path && does_not_require_next_attempt?(stabilization_comparator)
-
-            # If timeout then we failed to generate valid screenshot
-            return nil if timeout?(elapsed_time)
+            attempt_path, prev_attempt_path = attempt_next_screenshot(attempt_path, i, screenshot_path)
+            return attempt_path if attempt_successful?(attempt_path, prev_attempt_path)
+            return nil if timeout?(deadline_at)
           end
         end
 
         private
 
-        def does_not_require_next_attempt?(stabilization_comparator)
-          stabilization_comparator.quick_equal?
+        def attempt_successful?(attempt_path, prev_attempt_path)
+          return false unless prev_attempt_path
+          build_comparison_for(attempt_path, prev_attempt_path).quick_equal?
         rescue ArgumentError
           false
         end
 
+        def attempt_next_screenshot(prev_attempt_path, i, screenshot_path)
+          new_attempt_path = Screenshoter.gen_next_attempt_path(screenshot_path, i)
+          @screenshoter.take_screenshot(new_attempt_path)
+          [new_attempt_path, prev_attempt_path]
+        end
+
+        def timeout?(deadline_at)
+          Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline_at
+        end
+
         def build_comparison_for(attempt_path, previous_attempt_path)
           ImageCompare.new(attempt_path, previous_attempt_path, @comparison_options)
         end
@@ -117,7 +113,7 @@ def annotate_stabilization_images(attempts_screenshot_paths)
                 FileUtils.mv(attempts_comparison.reporter.annotated_base_image_path, previous_file, force: true)
               else
                 warn "[capybara-screenshot-diff] Some attempts was stable, but mistakenly marked as not: " \
-                  "#{previous_file} and #{file_name} are equal"
+                       "#{previous_file} and #{file_name} are equal"
               end
 
               FileUtils.rm(attempts_comparison.reporter.annotated_image_path, force: true)
@@ -126,10 +122,6 @@ def annotate_stabilization_images(attempts_screenshot_paths)
             previous_file = file_name
           end
         end
-
-        def timeout?(elapsed_time)
-          elapsed_time > wait
-        end
       end
     end
   end

test/capybara/screenshot/diff/stable_screenshoter_test.rb

@@ -47,7 +47,7 @@ class StableScreenshoterTest < ActionDispatch::IntegrationTest
 
           ImageCompare.stub :new, mock do
             StableScreenshoter
-              .new({stability_time_limit: 0.5, wait: 0.5}, image_compare_stub.driver_options)
+              .new({stability_time_limit: 0.5, wait: 1}, image_compare_stub.driver_options)
               .take_comparison_screenshot("tmp/02_a.png")
           end
 
@@ -80,7 +80,7 @@ class StableScreenshoterTest < ActionDispatch::IntegrationTest
             ImageCompare.stub :new, mock do
               # Wait time is less then stability time, which will generate problem
               StableScreenshoter
-                .new({stability_time_limit: 0.5, wait: 0.5}, build_image_compare_stub(equal: false).driver_options)
+                .new({stability_time_limit: 0.5, wait: 1}, build_image_compare_stub(equal: false).driver_options)
                 .take_comparison_screenshot(screenshot_path.to_s)
             end
           end