feat: make the failure message legible

lib/snap_diff/reporters/default.rb

@@ -1,6 +1,10 @@
 # frozen_string_literal: true
 
+require "json"
+
 require "snap_diff/annotation_service"
+# For SnapDiff.config.root, the base for the relative artifact paths below.
+require "snap_diff/config"
 
 module SnapDiff
   module Reporters
@@ -57,20 +61,95 @@ def build_error_for_different_dimensions
 
       NEW_LINE = "\n"
 
+      # The thresholds a comparison is judged against, in the order they read
+      # best. Only the ones actually set are printed -- see #thresholds.
+      THRESHOLDS = [
+        :tolerance,
+        :area_size_limit,
+        :color_distance_limit,
+        :shift_distance_limit,
+        :perceptual_threshold
+      ].freeze
+
+      # The five artifacts a comparison can leave on disk, in the order a
+      # reader wants them: the two inputs first, then what we drew on them.
+      ARTIFACT_LABELS = {
+        "baseline" => :base_image_path,
+        "actual" => :image_path,
+        "baseline annotated" => :annotated_base_image_path,
+        "actual annotated" => :annotated_image_path,
+        "heatmap" => :heatmap_diff_path
+      }.freeze
+
       def build_error_message
-        [
-          "(#{difference.to_h.to_json})",
-          image_path.to_path,
-          annotated_base_image_path.to_path,
-          annotated_image_path.to_path,
-          heatmap_diff_path.to_path
-        ].join(NEW_LINE)
+        [headline, *metric_lines, *artifact_lines].join(NEW_LINE)
       end
 
       private
 
       attr_reader :annotation_service
 
+      # Reads as the tail of "Screenshot does not match for 'name': ".
+      def headline
+        width, height = driver.dimension(comparison.base_image)
+        total_pixels = width * height
+        area = difference.region_area_size
+
+        "the change spans #{area.round} of #{total_pixels} px " \
+          "(#{percent(area.to_f / total_pixels)} of the #{width}x#{height} image)"
+      end
+
+      def metric_lines
+        lines = ["  changed region: #{difference.coordinates.to_json} (left,top,right,bottom edges)"]
+        # difference_level is the changed share of the image area -- the
+        # number `tolerance` is compared against. Only computed when a
+        # tolerance is set, so only printed then.
+        if difference.ratio
+          lines << "  difference level: #{difference.ratio} (#{percent(difference.ratio)} of the image area)"
+        end
+        max_color_distance = difference.meta[:max_color_distance]
+        lines << "  max color distance: #{max_color_distance}" if max_color_distance&.positive?
+        max_shift_distance = difference.meta[:max_shift_distance]
+        lines << "  max shift distance: #{max_shift_distance} px" if max_shift_distance&.positive?
+        lines << "  judged against: #{thresholds}"
+      end
+
+      def thresholds
+        applied = THRESHOLDS.filter_map do |name|
+          value = difference.options[name]
+          "#{name} #{value}" if value
+        end
+
+        applied.empty? ? "no tolerance thresholds configured (any difference fails)" : applied.join(", ")
+      end
+
+      # Only what is on disk gets a line: the heatmap exists solely for
+      # drivers that produce a diff mask, and a comparison can be reported
+      # before either input has been written out.
+      def artifact_lines
+        present = ARTIFACT_LABELS.filter_map do |label, path_method|
+          path = send(path_method)
+          [label, path] if path.exist?
+        end
+        width = present.map { |label, _path| label.length }.max.to_i
+
+        present.map { |label, path| "  #{"#{label}:".ljust(width + 1)} #{display_path(path)}" }
+      end
+
+      # Relative to the configured root: shorter to read, and still
+      # click-through-able in terminals that resolve paths against the
+      # working directory. Anything outside the root stays absolute, because
+      # a "../../.." path is neither.
+      def display_path(path)
+        relative = path.expand_path.relative_path_from(SnapDiff.config.root).to_path
+        relative.start_with?("..") ? path.to_path : relative
+      end
+
+      def percent(fraction)
+        value = fraction * 100
+        (value.positive? && value < 0.01) ? "<0.01%" : format("%.2f%%", value)
+      end
+
       def base_image_path
         comparison.base_image_path
       end

test/unit/dsl_test.rb

@@ -34,12 +34,18 @@ def after_teardown
   test "#assert_image_not_changed generates correct error message for image mismatch" do
     message = assert_image_not_changed(["my_test.rb:42"], "name", make_comparison(:a, :c, destination: "screenshot.png"))
     value = (RUBY_VERSION >= "2.4") ? 187.4 : 188
+    # Paths are relative to SnapDiff.config.root, and only the artifacts
+    # that exist are listed: chunky_png produces no diff mask, so there is
+    # no heatmap on disk for this comparison.
     assert_equal <<~MSG.chomp, message
-      Screenshot does not match for 'name': ({"area_size":629,"region":[11,3,48,20],"max_color_distance":#{value}})
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.png
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.base.diff.png
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.diff.png
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.heatmap.diff.png
+      Screenshot does not match for 'name': the change spans 629 of 6400 px (9.83% of the 80x80 image)
+        changed region: [11,3,48,20] (left,top,right,bottom edges)
+        max color distance: #{value}
+        judged against: no tolerance thresholds configured (any difference fails)
+        baseline:           doc/screenshots/screenshot.base.png
+        actual:             doc/screenshots/screenshot.png
+        baseline annotated: doc/screenshots/screenshot.base.diff.png
+        actual annotated:   doc/screenshots/screenshot.diff.png
       my_test.rb:42
     MSG
   end
@@ -52,11 +58,15 @@ def after_teardown
     )
     value = (RUBY_VERSION >= "2.4") ? 5.0 : 5
     assert_equal <<~MSG.chomp, message
-      Screenshot does not match for 'name': ({"area_size":629,"region":[11,3,48,20],"max_color_distance":#{value},"max_shift_distance":15})
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.png
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.base.diff.png
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.diff.png
-      #{SnapDiff.config.root}/doc/screenshots/screenshot.heatmap.diff.png
+      Screenshot does not match for 'name': the change spans 629 of 6400 px (9.83% of the 80x80 image)
+        changed region: [11,3,48,20] (left,top,right,bottom edges)
+        max color distance: #{value}
+        max shift distance: 15 px
+        judged against: shift_distance_limit 1
+        baseline:           doc/screenshots/screenshot.base.png
+        actual:             doc/screenshots/screenshot.png
+        baseline annotated: doc/screenshots/screenshot.base.diff.png
+        actual annotated:   doc/screenshots/screenshot.diff.png
       my_test.rb:42
     MSG
   end

test/unit/reporters/default_test.rb

@@ -74,21 +74,107 @@ class DefaultReporterTest < ActiveSupport::TestCase
     difference.meta[:difference_level] = 0.42
 
     message = SnapDiff::Reporters::Default.new(difference).generate
-    metrics = message.lines.first
 
-    assert_includes metrics, "area_size"
-    assert_includes metrics, "region"
-    assert_includes metrics, "difference_level"
-    assert_not_includes metrics, "Vips::Image"
-    assert_not_includes metrics, "0x"
+    assert_includes message, "changed region"
+    assert_includes message, "difference level"
+    assert_not_includes message, "Vips::Image"
+    # The signature of a leaked Ruby object in a message: `#<Vips::Image 0x...>`.
+    assert_not_includes message, "#<"
+  end
+
+  # Every number in the message needs a denominator or a stated unit: an
+  # `area_size` on its own and a bare four-number `region` were the two
+  # things readers could not interpret.
+  test "failure message gives the changed area a denominator and the region its interpretation" do
+    driver = SnapDiff::Drivers::VipsDriver.new
+    difference = driver.find_difference_region(build_comparison_for(driver, "a.png", "b.png", tolerance: 0.001))
+
+    message = SnapDiff::Reporters::Default.new(difference).generate
+
+    assert_includes message, "100 of 6400 px (1.56% of the 80x80 image)"
+    assert_includes message, "changed region: [20.0,15.0,30.0,25.0] (left,top,right,bottom edges)"
+    assert_includes message, "difference level: 0.00765625 (0.77% of the image area)"
+  end
+
+  # VCR's error restates the config it applied; half of "why did this fail?"
+  # is answered by the threshold the comparison was judged against.
+  test "failure message states the thresholds that were applied" do
+    driver = SnapDiff::Drivers::VipsDriver.new
+    difference = driver.find_difference_region(
+      build_comparison_for(driver, "a.png", "b.png", tolerance: 0.001, color_distance_limit: 20)
+    )
+
+    message = SnapDiff::Reporters::Default.new(difference).generate
+
+    assert_includes message, "judged against: tolerance 0.001, color_distance_limit 20"
+  end
+
+  test "failure message says so when no threshold was configured" do
+    driver = SnapDiff::Drivers::VipsDriver.new
+    difference = driver.find_difference_region(build_comparison_for(driver, "a.png", "b.png"))
+
+    message = SnapDiff::Reporters::Default.new(difference).generate
+
+    assert_includes message, "judged against: no tolerance thresholds configured (any difference fails)"
+  end
+
+  test "failure message labels every artifact it lists" do
+    driver = SnapDiff::Drivers::VipsDriver.new
+    comparison = build_comparison_for(driver, "a.png", "b.png")
+    FileUtils.cp(TEST_IMAGES_DIR.join("a.png"), comparison.new_image_path)
+    FileUtils.cp(TEST_IMAGES_DIR.join("b.png"), comparison.base_image_path)
+    reporter = SnapDiff::Reporters::Default.new(driver.find_difference_region(comparison))
+
+    message = reporter.generate.squeeze(" ")
+
+    assert_includes message, "baseline: #{comparison.base_image_path}"
+    assert_includes message, "actual: #{comparison.new_image_path}"
+    assert_includes message, "baseline annotated: #{reporter.annotated_base_image_path}"
+    assert_includes message, "actual annotated: #{reporter.annotated_image_path}"
+    assert_includes message, "heatmap: #{reporter.heatmap_diff_path}"
+  end
+
+  # Issue #260: the message must never name a file that is not on disk.
+  test "failure message omits artifacts that were never written" do
+    driver = SnapDiff::Drivers::VipsDriver.new
+    comparison = build_comparison_for(driver, "a.png", "b.png")
+    reporter = SnapDiff::Reporters::Default.new(driver.find_difference_region(comparison))
+
+    message = reporter.generate.squeeze(" ")
+
+    assert_not_includes message, "baseline: "
+    assert_not_includes message, "actual: "
+    assert_includes message, "heatmap: #{reporter.heatmap_diff_path}"
+  end
+
+  test "artifact paths are printed relative to the configured root, absolute when outside it" do
+    driver = SnapDiff::Drivers::VipsDriver.new
+    comparison = build_comparison_for(driver, "a.png", "b.png")
+    reporter = SnapDiff::Reporters::Default.new(driver.find_difference_region(comparison))
+
+    with_config_root(@_tmpdir) do
+      assert_includes reporter.generate.squeeze(" "), "heatmap: a.heatmap.diff.png"
+    end
+
+    with_config_root(@_tmpdir / "elsewhere") do
+      assert_includes reporter.generate.squeeze(" "), "heatmap: #{reporter.heatmap_diff_path}"
+    end
   end
 
   private
 
-  def build_comparison_for(driver, *images)
+  def with_config_root(root)
+    previous = SnapDiff.config.root
+    SnapDiff.config.root = root
+    yield
+  ensure
+    SnapDiff.config.root = previous
+  end
+
+  def build_comparison_for(driver, *images, **options)
     new_image = driver.from_file(TEST_IMAGES_DIR.join(images.first))
     base_image = driver.from_file(TEST_IMAGES_DIR.join(images.last))
 
-    SnapDiff::Comparison::Images.new(new_image, base_image, {}, driver, @_tmpdir / images.first, @_tmpdir / images.last)
+    SnapDiff::Comparison::Images.new(new_image, base_image, options, driver, @_tmpdir / images.first, @_tmpdir / images.last)
   end
 end