change.rb in rspec-expectations-3.8.0

- old
+ new

@@ -41,20 +41,17 @@
           ChangeFromValue.new(change_details, value)
         end
 
         # @private
         def matches?(event_proc)
-          @event_proc = event_proc
-          return false unless Proc === event_proc
           raise_block_syntax_error if block_given?
-          change_details.perform_change(event_proc)
-          change_details.changed?
+          perform_change(event_proc) && change_details.changed?
         end
 
         def does_not_match?(event_proc)
           raise_block_syntax_error if block_given?
-          !matches?(event_proc) && Proc === event_proc
+          perform_change(event_proc) && !change_details.changed?
         end
 
         # @api private
         # @return [String]
         def failure_message
@@ -90,23 +87,35 @@
 
         def change_details
           @change_details ||= ChangeDetails.new(matcher_name, @receiver, @message, &@block)
         end
 
+        def perform_change(event_proc)
+          @event_proc = event_proc
+          change_details.perform_change(event_proc) do |actual_before|
+            # pre-compute values derived from the `before` value before the
+            # mutation is applied, in case the specified mutation is mutation
+            # of a single object (rather than a changing what object a method
+            # returns). We need to cache these values before the `before` value
+            # they are based on potentially gets mutated.
+            @actual_before_description = description_of(actual_before)
+          end
+        end
+
         def raise_block_syntax_error
           raise SyntaxError, "Block not received by the `change` matcher. " \
           "Perhaps you want to use `{ ... }` instead of do/end?"
         end
 
         def positive_failure_reason
           return "was not given a block" unless Proc === @event_proc
-          "is still #{description_of change_details.actual_before}"
+          "is still #{@actual_before_description}"
         end
 
         def negative_failure_reason
           return "was not given a block" unless Proc === @event_proc
-          "did change from #{description_of change_details.actual_before} " \
+          "did change from #{@actual_before_description} " \
           "to #{description_of change_details.actual_after}"
         end
       end
 
       # Used to specify a relative change.
@@ -127,13 +136,11 @@
         end
 
         # @private
         def matches?(event_proc)
           @event_proc = event_proc
-          return false unless Proc === event_proc
-          @change_details.perform_change(event_proc)
-          @comparer.call(@change_details.actual_delta)
+          @change_details.perform_change(event_proc) && @comparer.call(@change_details.actual_delta)
         end
 
         # @private
         def does_not_match?(_event_proc)
           raise NotImplementedError, "`expect { }.not_to change " \
@@ -171,25 +178,22 @@
           @expected_after  = to
         end
 
         # @private
         def matches?(event_proc)
-          @event_proc = event_proc
-          return false unless Proc === event_proc
-          @change_details.perform_change(event_proc)
-          @change_details.changed? && matches_before? && matches_after?
+          perform_change(event_proc) && @change_details.changed? && @matches_before && matches_after?
         end
 
         # @private
         def description
           "change #{@change_details.value_representation} #{change_description}"
         end
 
         # @private
         def failure_message
           return not_given_a_block_failure unless Proc === @event_proc
-          return before_value_failure      unless matches_before?
+          return before_value_failure      unless @matches_before
           return did_not_change_failure    unless @change_details.changed?
           after_value_failure
         end
 
         # @private
@@ -197,22 +201,31 @@
           true
         end
 
       private
 
-        def matches_before?
-          values_match?(@expected_before, @change_details.actual_before)
+        def perform_change(event_proc)
+          @event_proc = event_proc
+          @change_details.perform_change(event_proc) do |actual_before|
+            # pre-compute values derived from the `before` value before the
+            # mutation is applied, in case the specified mutation is mutation
+            # of a single object (rather than a changing what object a method
+            # returns). We need to cache these values before the `before` value
+            # they are based on potentially gets mutated.
+            @matches_before = values_match?(@expected_before, actual_before)
+            @actual_before_description = description_of(actual_before)
+          end
         end
 
         def matches_after?
           values_match?(@expected_after, @change_details.actual_after)
         end
 
         def before_value_failure
           "expected #{@change_details.value_representation} " \
           "to have initially been #{description_of @expected_before}, " \
-          "but was #{description_of @change_details.actual_before}"
+          "but was #{@actual_before_description}"
         end
 
         def after_value_failure
           "expected #{@change_details.value_representation} " \
           "to have changed to #{description_of @expected_after}, " \
@@ -224,11 +237,11 @@
           "to have changed #{change_description}, but did not change"
         end
 
         def did_change_failure
           "expected #{@change_details.value_representation} not to have changed, but " \
-          "did change from #{description_of @change_details.actual_before} " \
+          "did change from #{@actual_before_description} " \
           "to #{description_of @change_details.actual_after}"
         end
 
         def not_given_a_block_failure
           "expected #{@change_details.value_representation} to have changed " \
@@ -258,20 +271,17 @@
           if @description_suffix
             raise NotImplementedError, "`expect { }.not_to change { }.to()` " \
               "is not supported"
           end
 
-          @event_proc = event_proc
-          return false unless Proc === event_proc
-          @change_details.perform_change(event_proc)
-          !@change_details.changed? && matches_before?
+          perform_change(event_proc) && !@change_details.changed? && @matches_before
         end
 
         # @private
         def failure_message_when_negated
           return not_given_a_block_failure unless Proc === @event_proc
-          return before_value_failure unless matches_before?
+          return before_value_failure unless @matches_before
           did_change_failure
         end
 
       private
 
@@ -310,12 +320,24 @@
         end
       end
 
       # @private
       # Encapsulates the details of the before/after values.
+      #
+      # Note that this class exposes the `actual_after` value, to allow the
+      # matchers above to derive failure messages, etc from the value on demand
+      # as needed, but it intentionally does _not_ expose the `actual_before`
+      # value. Some usages of the `change` matcher mutate a specific object
+      # returned by the value proc, which means that failure message snippets,
+      # etc, which are derived from the `before` value may not be accurate if
+      # they are lazily computed as needed. We must pre-compute them before
+      # applying the change in the `expect` block. To ensure that all `change`
+      # matchers do that properly, we do not expose the `actual_before` value.
+      # Instead, matchers must pass a block to `perform_change`, which yields
+      # the `actual_before` value before applying the change.
       class ChangeDetails
-        attr_reader :actual_before, :actual_after
+        attr_reader :actual_after
 
         def initialize(matcher_name, receiver=nil, message=nil, &block)
           if receiver && !message
             raise(
               ArgumentError,
@@ -332,43 +354,60 @@
         end
 
         def value_representation
           @value_representation ||=
             if @message
-              "##{@message}"
+              "`#{message_notation(@receiver, @message)}`"
             elsif (value_block_snippet = extract_value_block_snippet)
               "`#{value_block_snippet}`"
             else
               'result'
             end
         end
 
         def perform_change(event_proc)
           @actual_before = evaluate_value_proc
+          @before_hash = @actual_before.hash
+          yield @actual_before if block_given?
+
+          return false unless Proc === event_proc
           event_proc.call
+
           @actual_after = evaluate_value_proc
+          true
         end
 
         def changed?
-          @actual_before != @actual_after
+          # Consider it changed if either:
+          #
+          # - The before/after values are unequal
+          # - The before/after values have different hash values
+          #
+          # The latter case specifically handles the case when the value proc
+          # returns the exact same object, but it has been mutated.
+          #
+          # Note that it is not sufficient to only check the hashes; it is
+          # possible for two values to be unequal (and of different classes)
+          # but to return the same hash value.
+          @actual_before != @actual_after || @before_hash != @actual_after.hash
         end
 
         def actual_delta
           @actual_after - @actual_before
         end
 
       private
 
         def evaluate_value_proc
-          value_proc = @value_proc || lambda { @receiver.__send__(@message) }
+          @value_proc ? @value_proc.call : @receiver.__send__(@message)
+        end
 
-          case val = value_proc.call
-          when IO # enumerable, but we don't want to dup it.
-            val
-          when Enumerable, String
-            val.dup
+        def message_notation(receiver, message)
+          case receiver
+          when Module
+            "#{receiver}.#{message}"
           else
-            val
+            "#{Support.class_of(receiver)}##{message}"
           end
         end
 
         if RSpec::Support::RubyFeatures.ripper_supported?
           def extract_value_block_snippet