Allow graceful fallback by call_both when new code path raises

lib/suture/error/result_mismatch.rb

@@ -16,11 +16,11 @@ def message
         Arguments: ```
           #{@plan.args.inspect}
         ```
-        The new code path returned: ```
-          #{@new_result.inspect}
+        The new code path #{@new_result.errored? ? "raised error" : "returned value"}: ```
+          #{@new_result.value.inspect}
         ```
-        The old code path returned: ```
-          #{@old_result.inspect}
+        The old code path #{@old_result.errored? ? "raised error" : "returned value"}: ```
+          #{@old_result.value.inspect}
         ```
 
         Here's what we recommend you do next:

lib/suture/surgeon/auditor.rb

@@ -1,42 +1,73 @@
 require "suture/error/result_mismatch"
+require "suture/value/result"
+require "suture/util/compares_results"
 require "suture/util/scalpel"
 
 module Suture::Surgeon
   class Auditor
     include Suture::Adapter::Log
 
+    def initialize
+      @scalpel = Suture::Util::Scalpel.new
+    end
+
     def operate(plan)
-      scalpel = Suture::Util::Scalpel.new
-      new_result = scalpel.cut(plan, :new)
-      old_result = scalpel.cut(plan, :old)
-      if !plan.comparator.call(old_result, new_result)
+      new_result = result_for(plan, :new)
+      old_result = result_for(plan, :old)
+      if !comparable?(plan, old_result, new_result)
         handle_mismatch(plan, old_result, new_result)
       else
-        new_result
+        return_result(new_result)
       end
     end
 
     private
 
+    def result_for(plan, path)
+      begin
+        Suture::Value::Result.returned(@scalpel.cut(plan, path))
+      rescue StandardError => error
+        Suture::Value::Result.errored(error)
+      end
+    end
+
+    def comparable?(plan, old_result, new_result)
+      Suture::Util::ComparesResults.new(plan.comparator).compare(
+        old_result,
+        new_result
+      )
+    end
+
     def handle_mismatch(plan, old_result, new_result)
       log_warning(plan, old_result, new_result)
       if plan.raise_on_result_mismatch
         raise Suture::Error::ResultMismatch.new(plan, new_result, old_result)
       elsif plan.return_old_on_result_mismatch
-        old_result
+        return_result(old_result)
       else
-        new_result
+        return_result(new_result)
+      end
+    end
+
+    def return_result(result)
+      if result.errored?
+        raise result.value
+      else
+        return result.value
       end
     end
 
     def log_warning(plan, old_result, new_result)
       log_warn <<-MSG.gsub(/^ {8}/,'')
         Seam #{plan.name.inspect} is set to :call_both the :new and :old code
-        paths, but they did not match. The new result was: ```
-          #{new_result.inspect}
+        paths, but they did not match.
+
+        The new result #{new_result.errored? ? "raised" : "returned"}: ```
+          #{new_result.value.inspect}
         ```
-        The old result was: ```
-          #{old_result.inspect}
+
+        The old result #{old_result.errored? ? "raised" : "returned"}: ```
+          #{old_result.value.inspect}
         ```
       MSG
     end

test/support/assertions.rb

@@ -15,7 +15,20 @@ def assert_not_match matcher, obj, msg = nil
     # Flips expected & actual so it's easier to use heredocs
 
     def assert_spacey_match obj, matcher, msg = nil
-      msg = message(msg) { "Expected #{mu_pp matcher} to match #{mu_pp obj} (with relaxed whitespace)" }
+      og_matcher = matcher
+      msg = message(msg) {
+        <<-MSG.gsub(/^ {10}/,'')
+          Expected this #{obj.class}:
+          ```
+          #{obj}
+          ```
+
+          To match this #{og_matcher.class}:
+          ```
+          #{og_matcher}
+          ```
+        MSG
+      }
       assert_respond_to matcher, :"=~"
       matcher = Regexp.new(Regexp.escape(matcher.gsub(/\s+/, ''))) if String === matcher
       assert(matcher =~ obj.gsub(/\s+/, ''), msg)

test/suture/error/result_mismatch_test.rb

@@ -0,0 +1,58 @@
+module Suture::Error
+  class ResultMismatchTest < UnitTest
+    def test_simple_mismatch
+      plan = Suture::Value::Plan.new(:name => :SEAM, :args => [1,2,3])
+      new = Suture::Value::Result.returned('A')
+      old = Suture::Value::Result.errored(ZeroDivisionError.new('B'))
+
+      subject = ResultMismatch.new(plan, new, old)
+
+      assert_spacey_match subject.message, <<-MSG.gsub(/^ {8}/,'')
+        The results from the old & new code paths did not match for the seam
+        :SEAM and Suture is raising this error because the `:call_both`
+        option is enabled, because both code paths are expected to return the
+        same result.
+
+        Arguments: ```
+          [1, 2, 3]
+        ```
+        The new code path returned value: ```
+          "A"
+        ```
+        The old code path raised error: ```
+          #<ZeroDivisionError: B>
+        ```
+
+        Here's what we recommend you do next:
+
+        1. Verify that this mismatch does not represent a missed requirement in
+           the new code path. If it does, implement it!
+
+        2. If either (or both) code path has a side effect that impacts the
+           return value of the other, consider passing an `:after_old` and/or
+           `:after_new` hook to clean up your application's state well enough to
+           run both paths one-after-the-other safely.
+
+        3. If the two return values above are sufficiently similar for the
+           purpose of your application, consider writing your own custom
+           comparator that relaxes the comparison (e.g. only checks equivalence
+           of the attributes that matter). See the README for more info on custom
+           comparators.
+
+        4. If the new code path is working as desired (i.e. the old code path had
+           a bug for this argument and you don't want to reimplement it just to
+           make them perfectly in sync with one another), consider writing a
+           one-off comparator for this seam that will ignore the affected range
+           of arguments. See the README for more info on custom comparators.
+
+        By default, Suture's :call_both mode will log a warning and raise an
+        error when the results of each code path don't match. It is intended for
+        use in any pre-production environment to "try out" the new code path
+        before pushing it to production. If, for whatever reason, this error is
+        too disruptive and logging is sufficient for monitoring results, you may
+        disable this error by setting `:raise_on_result_mismatch` to false.
+      MSG
+    end
+
+  end
+end

test/suture/surgeon/auditor_test.rb

@@ -11,8 +11,8 @@ def setup
     def test_new_and_old_returns_new_result_when_equivalent
       new_result = [:woo]
       plan = Suture::BuildsPlan.new.build(:lol,
-        :old => lambda { |a| [:woo] if a == :yay },
-        :new => lambda { |a| new_result unless a != :yay },
+        :old => lambda { |a| [:woo] },
+        :new => lambda { |a| new_result },
         :args => [:yay]
       )
 
@@ -31,52 +31,7 @@ def test_raises_when_new_and_old_differ
 
       error = assert_raises(Suture::Error::ResultMismatch) { @subject.operate(plan) }
 
-      expected_message = <<-MSG.gsub(/^ {8}/,'')
-        The results from the old & new code paths did not match for the seam
-        :face_swap and Suture is raising this error because the `:call_both`
-        option is enabled, because both code paths are expected to return the
-        same result.
-
-        Arguments: ```
-          [:face]
-        ```
-        The new code path returned: ```
-          :shrugface
-        ```
-        The old code path returned: ```
-          :trollface
-        ```
-
-        Here's what we recommend you do next:
-
-        1. Verify that this mismatch does not represent a missed requirement in
-           the new code path. If it does, implement it!
-
-        2. If either (or both) code path has a side effect that impacts the
-           return value of the other, consider passing an `:after_old` and/or
-           `:after_new` hook to clean up your application's state well enough to
-           run both paths one-after-the-other safely.
-
-        3. If the two return values above are sufficiently similar for the
-           purpose of your application, consider writing your own custom
-           comparator that relaxes the comparison (e.g. only checks equivalence
-           of the attributes that matter). See the README for more info on custom
-           comparators.
-
-        4. If the new code path is working as desired (i.e. the old code path had
-           a bug for this argument and you don't want to reimplement it just to
-           make them perfectly in sync with one another), consider writing a
-           one-off comparator for this seam that will ignore the affected range
-           of arguments. See the README for more info on custom comparators.
-
-        By default, Suture's :call_both mode will log a warning and raise an
-        error when the results of each code path don't match. It is intended for
-        use in any pre-production environment to "try out" the new code path
-        before pushing it to production. If, for whatever reason, this error is
-        too disruptive and logging is sufficient for monitoring results, you may
-        disable this error by setting `:raise_on_result_mismatch` to false.
-      MSG
-      assert_equal expected_message, error.message
+      assert_spacey_match error.message, ":trollface"
     end
 
     def test_does_not_raise_when_raise_is_disabled
@@ -92,6 +47,68 @@ def test_does_not_raise_when_raise_is_disabled
       assert_equal :shrugface, result
     end
 
+    def test_raises_mismatch_when_new_raises_and_old_does_not
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| :trollface },
+        :new => lambda { |type| raise ZeroDivisionError.new("shrugface") },
+        :args => [:face]
+      )
+
+      error = assert_raises(Suture::Error::ResultMismatch) { @subject.operate(plan) }
+
+      assert_spacey_match error.message, "new code path raised"
+      assert_spacey_match error.message, "old code path returned"
+    end
+
+    def test_raises_mismatch_when_old_raises_and_new_does_not
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| raise ZeroDivisionError.new("trollface") },
+        :new => lambda { |type| :shrugface },
+        :args => [:face]
+      )
+
+      error = assert_raises(Suture::Error::ResultMismatch) { @subject.operate(plan) }
+
+      assert_spacey_match error.message, "new code path returned"
+      assert_spacey_match error.message, "old code path raised"
+    end
+
+    def test_raises_mismatch_when_new_raises_and_old_raises_different
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| raise ZeroDivisionError.new("trollface") },
+        :new => lambda { |type| raise ZeroDivisionError.new("shrugface") },
+        :args => [:face]
+      )
+
+      assert_raises(Suture::Error::ResultMismatch) { @subject.operate(plan) }
+    end
+
+    def test_raises_actual_when_new_raises_and_old_raises_same
+      new_error = RuntimeError.new("trollface")
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| raise "trollface" },
+        :new => lambda { |type| raise new_error },
+        :args => [:face]
+      )
+
+      error = assert_raises { @subject.operate(plan) }
+
+      assert_equal error.__id__, new_error.__id__
+    end
+
+    def test_raises_actual_when_raise_mismatch_disabled_and_new_raises_and_old_does_not
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| :trollface },
+        :new => lambda { |type| raise ZeroDivisionError.new("shrugface") },
+        :args => [:face],
+        :raise_on_result_mismatch => false
+      )
+
+      error = assert_raises(ZeroDivisionError) { @subject.operate(plan) }
+
+      assert_equal "shrugface", error.message
+    end
+
     def test_returns_old_when_toggled_and_raise_disabled
       plan = Suture::BuildsPlan.new.build(:face_swap,
         :old => lambda { |type| :trollface },
@@ -118,5 +135,33 @@ def test_passes_when_comparator_bails_them_out
 
       assert_equal 7, result
     end
+
+    def test_raise_disabled_old_return_and_new_raises_return_old
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| :trollface },
+        :new => lambda { |type| raise ZeroDivisionError.new("shrugface") },
+        :args => [:face],
+        :raise_on_result_mismatch => false,
+        :return_old_on_result_mismatch => true
+      )
+
+      result = @subject.operate(plan)
+
+      assert_equal :trollface, result
+    end
+
+    def test_raise_disabled_old_return_and_new_raises_and_old_raises_raise_old
+      plan = Suture::BuildsPlan.new.build(:face_swap,
+        :old => lambda { |type| raise ZeroDivisionError.new("trollface") },
+        :new => lambda { |type| raise ZeroDivisionError.new("shrugface") },
+        :args => [:face],
+        :raise_on_result_mismatch => false,
+        :return_old_on_result_mismatch => true
+      )
+
+      error = assert_raises(ZeroDivisionError) { @subject.operate(plan) }
+
+      assert_equal "trollface", error.message
+    end
   end
 end