Merge pull request #2862 from rspec/remove-deprecated-results-hash-compatibility

Remove deprecated Hash-like behavior from example execution result

Author: pirj

Changelog.md

@@ -15,6 +15,8 @@ Breaking Changes:
   an unsupported value. (Phil Pirozhkov, #2849)
 * Remove deprecated access to an example group's metadata through the example.
   (Phil Pirozhkov, #2851)
+* Remove deprecated Hash-like behavior from example
+  execution result. (Phil Pirozhkov, #2862)
 
 Enhancements:
 

lib/rspec/core/example.rb

@@ -551,10 +551,7 @@ def location_description
       end
 
       # Represents the result of executing an example.
-      # Behaves like a hash for backwards compatibility.
       class ExecutionResult
-        include HashImitatable
-
         # @return [Symbol] `:passed`, `:failed` or `:pending`.
         attr_accessor :status
 
@@ -616,32 +613,6 @@ def calculate_run_time(finished_at)
           self.finished_at = finished_at
           self.run_time    = (finished_at - started_at).to_f
         end
-
-        # For backwards compatibility we present `status` as a string
-        # when presenting the legacy hash interface.
-        def hash_for_delegation
-          super.tap do |hash|
-            hash[:status] &&= status.to_s
-          end
-        end
-
-        def set_value(name, value)
-          value &&= value.to_sym if name == :status
-          super(name, value)
-        end
-
-        def get_value(name)
-          if name == :status
-            status.to_s if status
-          else
-            super
-          end
-        end
-
-        def issue_deprecation(_method_name, *_args)
-          RSpec.deprecate("Treating `metadata[:execution_result]` as a hash",
-                          :replacement => "the attributes methods to access the data")
-        end
       end
     end
 

lib/rspec/core/metadata.rb

@@ -290,107 +290,5 @@ def full_description
         :shared_group_inclusion_backtrace
       ]
     end
-
-    # Mixin that makes the including class imitate a hash for backwards
-    # compatibility. The including class should use `attr_accessor` to
-    # declare attributes.
-    # @private
-    module HashImitatable
-      def self.included(klass)
-        klass.extend ClassMethods
-      end
-
-      def to_h
-        hash = extra_hash_attributes.dup
-
-        self.class.hash_attribute_names.each do |name|
-          hash[name] = __send__(name)
-        end
-
-        hash
-      end
-
-      (Hash.public_instance_methods - Object.public_instance_methods).each do |method_name|
-        next if [:[], :[]=, :to_h].include?(method_name.to_sym)
-
-        define_method(method_name) do |*args, &block|
-          issue_deprecation(method_name, *args)
-
-          hash = hash_for_delegation
-          self.class.hash_attribute_names.each do |name|
-            hash.delete(name) unless instance_variable_defined?(:"@#{name}")
-          end
-
-          hash.__send__(method_name, *args, &block).tap do
-            # apply mutations back to the object
-            hash.each do |name, value|
-              if directly_supports_attribute?(name)
-                set_value(name, value)
-              else
-                extra_hash_attributes[name] = value
-              end
-            end
-          end
-        end
-      end
-
-      def [](key)
-        issue_deprecation(:[], key)
-
-        if directly_supports_attribute?(key)
-          get_value(key)
-        else
-          extra_hash_attributes[key]
-        end
-      end
-
-      def []=(key, value)
-        issue_deprecation(:[]=, key, value)
-
-        if directly_supports_attribute?(key)
-          set_value(key, value)
-        else
-          extra_hash_attributes[key] = value
-        end
-      end
-
-    private
-
-      def extra_hash_attributes
-        @extra_hash_attributes ||= {}
-      end
-
-      def directly_supports_attribute?(name)
-        self.class.hash_attribute_names.include?(name)
-      end
-
-      def get_value(name)
-        __send__(name)
-      end
-
-      def set_value(name, value)
-        __send__(:"#{name}=", value)
-      end
-
-      def hash_for_delegation
-        to_h
-      end
-
-      def issue_deprecation(_method_name, *_args)
-        # no-op by default: subclasses can override
-      end
-
-      # @private
-      module ClassMethods
-        def hash_attribute_names
-          @hash_attribute_names ||= []
-        end
-
-        def attr_accessor(*names)
-          hash_attribute_names.concat(names)
-          super
-        end
-      end
-    end
   end
 end

spec/rspec/core/example_execution_result_spec.rb

@@ -2,19 +2,8 @@ module RSpec
   module Core
     class Example
       RSpec.describe ExecutionResult do
-        it "supports ruby 2.1's `to_h` protocol" do
-          er = ExecutionResult.new
-          er.run_time = 17
-          er.pending_message = "just because"
-
-          expect(er.to_h).to include(
-            :run_time => 17,
-            :pending_message => "just because"
-          )
-        end
-
-        it 'includes all defined attributes in the `to_h` hash even if not set' do
-          expect(ExecutionResult.new.to_h).to include(
+        it 'has `status` and `pending_message` attributes' do
+          expect(ExecutionResult.new).to have_attributes(
             :status => nil,
             :pending_message => nil
           )
@@ -25,111 +14,12 @@ class Example
           expect { er.pending_fixed = true }.to change(er, :pending_fixed?).from(false).to(true)
         end
 
-        describe "backwards compatibility" do
-          it 'supports indexed access like a hash' do
-            er = ExecutionResult.new
-            er.started_at = (started_at = ::Time.utc(2014, 3, 1, 12, 30))
-            expect_deprecation_with_call_site(__FILE__, __LINE__ + 1, /execution_result/)
-            expect(er[:started_at]).to eq(started_at)
-          end
-
-          it 'supports indexed updates like a hash' do
-            er = ExecutionResult.new
-            expect_deprecation_with_call_site(__FILE__, __LINE__ + 1, /execution_result/)
-            er[:started_at] = (started_at = ::Time.utc(2014, 3, 1, 12, 30))
-            expect(er.started_at).to eq(started_at)
-          end
-
-          it 'can get and set user defined attributes like with a hash' do
-            er = ExecutionResult.new
-            allow_deprecation
-            expect { er[:foo] = 3 }.to change { er[:foo] }.from(nil).to(3)
-            expect(er.to_h).to include(:foo => 3)
-          end
-
-          it 'supports `update` like a hash' do
-            er = ExecutionResult.new
-            expect_deprecation_with_call_site(__FILE__, __LINE__ + 1, /execution_result/)
-            er.update(:pending_message => "some message", :exception => ArgumentError.new)
-            expect(er.pending_message).to eq("some message")
-            expect(er.exception).to be_a(ArgumentError)
-          end
-
-          it 'can set undefined attribute keys through any hash mutation method' do
-            allow_deprecation
-            er = ExecutionResult.new
-            er.update(:pending_message => "msg", :foo => 3)
-            expect(er.to_h).to include(:pending_message => "msg", :foo => 3)
-          end
-
-          it 'supports `merge` like a hash' do
-            er = ExecutionResult.new
-            er.exception = ArgumentError.new
-            er.pending_message = "just because"
-
-            expect_deprecation_with_call_site(__FILE__, __LINE__ + 1, /execution_result/)
-            merged = er.merge(:exception => NotImplementedError.new, :foo => 3)
-
-            expect(merged).to include(
-              :exception => an_instance_of(NotImplementedError),
-              :pending_message => "just because",
-              :foo => 3
-            )
-
-            expect(er.exception).to be_an(ArgumentError)
-          end
-
-          it 'supports blocks for hash methods that support one' do
-            er = ExecutionResult.new
-            expect_deprecation_with_call_site(__FILE__, __LINE__ + 1, /execution_result/)
-            expect(er.fetch(:foo) { 3 }).to eq(3)
-          end
-
-          specify '#fetch treats unset properties the same as a hash does' do
-            allow_deprecation
-            er = ExecutionResult.new
-            expect { er.fetch(:pending_message) }.to raise_error(KeyError)
-            er.pending_message = "some msg"
-            expect(er.fetch(:pending_message)).to eq("some msg")
-          end
-
-          describe "status" do
-            it 'returns a string when accessed like a hash' do
-              er = ExecutionResult.new
-              expect(er[:status]).to eq(nil)
-              er.status = :failed
-              expect(er[:status]).to eq("failed")
-            end
-
-            it "sets the status to a symbol when assigned as a string via the hash interface" do
-              er = ExecutionResult.new
-              er[:status] = "failed"
-              expect(er.status).to eq(:failed)
-              er[:status] = nil
-              expect(er.status).to eq(nil)
-            end
-
-            it "is presented as a string when included in returned hashes" do
-              er = ExecutionResult.new
-              er.status = :failed
-              expect(er.merge(:foo => 3)).to include(:status => "failed", :foo => 3)
-
-              er.status = nil
-              expect(er.merge(:foo => 3)).to include(:status => nil, :foo => 3)
-            end
-
-            it "is updated to a symbol when updated as a string via `update`" do
-              er = ExecutionResult.new
-              er.update(:status => "passed")
-              expect(er.status).to eq(:passed)
-            end
+        it 'does not support `to_h`' do
+          expect(ExecutionResult.new.respond_to?(:to_h)).to be false
+        end
 
-            it 'is presented as a symbol in `to_h`' do
-              er = ExecutionResult.new
-              er.status = :failed
-              expect(er.to_h).to include(:status => :failed)
-            end
-          end
+        it 'does not behave like a hash' do
+          expect(ExecutionResult.new.respond_to?(:[])).to be false
         end
       end
     end

spec/rspec/core/example_group_spec.rb

@@ -1138,8 +1138,8 @@ def extract_execution_results(group)
           end
           group.run
 
-          expect(extract_execution_results(group).map(&:to_h)).to match([
-            a_hash_including(
+          expect(extract_execution_results(group)).to match([
+            have_attributes(
               :status => :pending,
               :pending_message => "Temporarily skipped with #{method_name}"
             )
@@ -1190,12 +1190,12 @@ def extract_execution_results(group)
         end
         group.run
 
-        expect(extract_execution_results(group).map(&:to_h)).to match([
-          a_hash_including(
+        expect(extract_execution_results(group)).to match([
+          have_attributes(
             :status => :failed,
             :pending_message => "No reason given"
           ),
-          a_hash_including(
+          have_attributes(
             :status => :pending,
             :pending_message => "unimplemented"
           )

spec/rspec/core/metadata_spec.rb

@@ -102,7 +102,9 @@ def metadata_for(*args)
         end
 
         it "creates an empty execution result" do
-          expect(example_metadata[:execution_result].to_h.reject { |_, v| v.nil? } ).to eq({})
+          expect(example_metadata[:execution_result])
+            .to be_an(Example::ExecutionResult)
+            .and have_attributes(:status => nil)
         end
 
         it "extracts file path from caller" do