allow inspectors to be registered for different objects

[b-loyola]

Subject of the issue

When comparing objects nested in diffable objects (e.g. Hash), it can be difficult to differentiate
between objects which are failing the comparison and objects which are considered equal but
have different outputs for inspect. One notable example is ActiveSupport::Duration where
60.minutes is considered equivalent to 1.hour, but produces different output for inspect
which can cause confusion and seem like it is failing the assertion when it is in fact just failing the
inspect comparison.

The proposal is to allow inspectors to be registered in ObjectFormatter so that custom
inspectors can be added for these classes, to improve the signal to noise ratio in diffs
when looking at failed tests.

At the moment it is possible to 'hack' this by manually prepending a custom inspector class
to RSpec::Support::ObjectFormatter::INSPECTOR_CLASSES, but here we create a more
friendly interface to allow this through something like:

RSpec::Support::ObjectFormatter.inspectors.register(CustomInspector)

If this gains enough traction this could be expanded to allow for configuring this in
RSpec's configuration, perhaps in a similar way to what is mentioned here.

Related to rspec/rspec-expectations#97

Your environment

• Ruby version: ruby 2.7.3p183
• rspec-support version: 3.11.0.pre

Steps to reproduce

To see an example of failing specs which provide misleading diffs, run this example and
note how in the 'does not show in the diff when objects are equivalent' example it seems
that both object1 and object2 are failing the assertion, when in fact only object2
is failing the assertion.

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"

  gem "rspec", "3.10.0" # Activate the gem and version you are reporting the issue against.
end

puts "Ruby version is: #{RUBY_VERSION}"
require 'rspec/autorun'

class TimeParts
  SECONDS_PER_MINUTE = 60

  def initialize(minutes, seconds)
    @minutes = minutes
    @seconds = seconds
  end

  def to_i
    (@minutes * SECONDS_PER_MINUTE) + @seconds
  end

  def inspect
    "#{@minutes} minutes and #{@seconds} seconds"
  end

  def ==(other)
    self.to_i == other.to_i
  end

  alias_method :eql?, :==

  def hash
    [@minutes, @seconds].hash
  end
end

RSpec.describe TimeParts do
  it 'considers equavilent objects equal' do
    object1 = TimeParts.new(1, 30)
    object2 = TimeParts.new(0, 90)
    expect(object1).to eq(object2)
  end

  it 'does not show in the diff when objects have the same result from inspect' do
    hash1 = {
      object1: TimeParts.new(0, 90),
      object2: TimeParts.new(0, 90)
    }
    hash2 = {
      object1: TimeParts.new(0, 90),
      object2: TimeParts.new(1, 90)
    }
    expect(hash1).to eq(hash2)
  end

  it 'does not show in the diff when objects are equivalent' do
    hash1 = {
      object1: TimeParts.new(1, 30),
      object2: TimeParts.new(0, 90)
    }
    hash2 = {
      object1: TimeParts.new(0, 90),
      object2: TimeParts.new(1, 90)
    }
    expect(hash1).to eq(hash2)
  end
end

Expected behavior

We should be able to register a custom inspector, to be able to modify the output of inspect
for specific objects, so that it becomes easier to tell what is actually failing the assertion:

class TimePartsInspector < ObjectFormatter::BaseInspector
  def self.can_inspect?(object)
    TimeParts === object
  end

  def inspect
    object.to_i.inspect
  end
end

RSpec::Support::ObjectFormatter.inspectors.register(TimePartsInspector)

This would mean that after registering the custom inspector above, the diff would look like this:

Failure/Error: expect(hash1).to eq(hash2)

  expected: {:object1=>90, :object2=>150}
       got: {:object1=>90, :object2=>90}

  (compared using ==)

  Diff:
  @@ -1,3 +1,3 @@
   :object1 => 90,
  -:object2 => 150,
  +:object2 => 90,

Actual behavior

Diffs are based on the raw outoput of inspect, so it is hard to tell that the value in object1
is not actually failing the assertion, and that only the value in object2 is the issue:

Failure/Error: expect(hash1).to eq(hash2)

  expected: {:object1=>0 minutes and 90 seconds, :object2=>1 minutes and 90 seconds}
       got: {:object1=>1 minutes and 30 seconds, :object2=>0 minutes and 90 seconds}

  (compared using ==)

  Diff:
  @@ -1,3 +1,3 @@
  -:object1 => 0 minutes and 90 seconds,
  -:object2 => 1 minutes and 90 seconds,
  +:object1 => 1 minutes and 30 seconds,
  +:object2 => 0 minutes and 90 seconds,

[pirj]

What will diff output after this change?

  Diff:
  :object1 => 0 minutes and 90 seconds,
  -:object2 => 1 minutes and 90 seconds,
  +:object2 => 0 minutes and 90 seconds,

Related: #365

[benoittgt]

I love the idea.

Maybe we could specify in the failing output the custom inspector used?

[b-loyola]

@pirj , I've updated the expected behaviour section to clarify what the diff would look like after registering the new inspector. Hopefully that answers your question but let me know if it doesn't.
Note that the change here would not affect any diffs unless a new inspector is registered. It only makes it easier to add custom inspectors to allow for customization of diff output for certain objects.

[b-loyola]

@benoittgt can you clarify in terms of implementation what you have in mind? Are you thinking we should differentiate between custom inspectors and default ones (the ones currently in INSPECTOR_CLASSES) and concatenate something in the inspect itself to denote that the object was inspected with a custom inspector? E.g.:

  Diff:
  @@ -1,3 +1,3 @@
   :object1 => 90 (inspected with TimePartsInspector),
  -:object2 => 150 (inspected with TimePartsInspector),
  +:object2 => 90 (inspected with TimePartsInspector),

From what I understand of the code (and I could definitely be misunderstanding this), with the exception of arrays and hashes, currently it will always use an inspector to inspect any object, falling back to UninspectableObjectInspector and ultimately to InspectableObjectInspector if all else fails. So I believe we would need to differentiate between the default ones custom ones to avoid adding too much noise in the failing output?

[benoittgt]

@b-loyola yes that was exactly what I was thinking when I saw your example.

So I believe we would need to differentiate between the default ones custom ones to avoid adding too much noise in the failing output?

Yes completely. Only custom one could be "noisy" by default. I am worried about having future issues on RSpec with people that use invalid custom inspector but doesn't know it is used in diff.

Also I think this PR should be added in RSpec 4 only. :)

But I think that first we should wait for @JonRowe feedbacks on your existing PR. ;)

[pirj]

There are two problems here.

1. Output is confusingly similar for equal objects
   Would you consider different output for TimeParts#to_s and TimeParts#inspect? Like e.g. the beforementioned ActiveSupport::Duration does?

1.minute.to_s => "60"
60.seconds.to_s # => "60"
1.minute.inspect # => "1 minute"
60.seconds.inspect # => "60 seconds"

This wouldn't be a problem if:

• see problem 2 below (for simple data structures)
• we were showing good diffs for complicated structures (and using super_diff is an option)

2. Failure messages for Hashes and Arrays
   Both are sub-par.

2.1 Hashes

     Failure/Error: it { expect({foo: 1.minute, bar: 1.minute}).to include(foo: 60.seconds, bar: 59.seconds) }

       expected {:bar => 1 minute, :foo => 1 minute} to include {:bar => 59 seconds}
       Diff:
       @@ -1,3 +1,3 @@
       -:bar => 59 seconds,
       -:foo => 60 seconds,
       +:bar => 1 minute,
       +:foo => 1 minute,

as you already mentioned, it's the diff that has a deficiency. Should have been:

{
  :foo => 60 seconds,
- :bar => 59 seconds,
+ :bar => 1 minute,
}

For comparison, super_diff's output:

         {
           foo: #<ActiveSupport::Duration:0x00007fd7a3e3d2f8 {
             @parts=nil,
             @value=60
           }>,
       -   bar: #<ActiveSupport::Duration:0x00007fd7a3e3cfd8 {
       -     @parts=nil,
       -     @value=59
       -   }>
       +   bar: #<ActiveSupport::Duration:0x00007fd7a3e3d208 {
       +     @parts=nil,
       +     @value=60
       +   }>
         }

A bit too verbose for my taste, but it doesn't suffer from either problems.
With or without require "super_diff/active_support" doesn't make a difference.

2.2 Arrays

     Failure/Error: it { expect([1.minute, 1.minute]).to contain_exactly(60.seconds, 59.seconds) }

       expected collection contained:  [59 seconds, 60 seconds]
       actual collection contained:    [1 minute, 1 minute]
       the missing elements were:      [59 seconds]
       the extra elements were:        [1 minute]

Surely, it would better look as

[
- 59 seconds,
+ 1 minute,
  60 seconds
]

For comparison, super_diff's output:

         [
           #<ActiveSupport::Duration:0x00007fd7a31c6c68 {
             @parts=nil,
             @value=60
           }>,
       +   #<ActiveSupport::Duration:0x00007fd7a31c6ba0 {
       +     @parts=nil,
       +     @value=60
       +   }>,
       -   #<ActiveSupport::Duration:0x00007fd7a31c6808 {
       -     @parts=nil,
       -     @value=59
       -   }>,
         ]

Again, quite verbose.

Speaking of custom inspectors, I'd love to highlight @benoittgt's concern:

use invalid custom inspector but doesn't know it is used in diff

The failure message output for eq/eql/equal/be matchers is identical, but their comparison is different. It might be surprising to see:

     Failure/Error: it { expect(1.minute).to equal(60.seconds) }

Diff:
- 60
+ 60

super_diff for example:

     Failure/Error: it { expect(1.minute).to equal 60.seconds }

       Expected #<ActiveSupport::Duration:0x00007fa69a55a030 @parts=nil, @value=60>
       to equal #<ActiveSupport::Duration:0x00007fa69a559f40 @parts=nil, @value=60>
     # /Users/pirj/.rvm/gems/ruby-2.7.2@cowboy-web/gems/super_diff-0.8.0/lib/super_diff/rspec/monkey_patches.rb:43:in `handle_failure'

And stock RSpec wins the race here:

       expected #<ActiveSupport::Duration:60840> => 60 seconds
            got #<ActiveSupport::Duration:60940> => 1 minute

       Compared using equal?, which compares object identity, but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about object identity in this example.

       Diff:
       @@ -1 +1 @@
       -60 seconds
       +1 minute

For those issues, custom differs could be an option, but it's quite a long way to catch up with super_diff [[1], [2]].

[b-loyola]

@benoittgt that makes sense. But I think a potential pitfall with appending a (inspected with CustomInspector) to custom inspector diffs directly in ObjectFormatter is that it a makes it more difficult to customize diff output when the objective is to make two objects from different classes which are considered equivalent to be displayed in the same way in diffs.
For example, let's say I want ActiveSupport::Duration to be inspected just like Integer since for most intents and purposes it behaves like an integer . That is, 1.hour == 3600 returns true so it would be nice to be able to customize durations to be inspected the same way as integers for failing specs output only. For example, currently if I write the following assertion:

hash1 = {
  travel_duration: 1.hour,
  meal_duration: 2.hours
}
hash2 = {
  travel_duration: 3600,
  meal_duration: 7000
}
expect(hash1).to eq(hash2)

it fails with:

     Failure/Error: expect(hash1).to eq(hash2)

       expected: {:meal_duration=>7000, :travel_duration=>3600}
            got: {:meal_duration=>2 hours, :travel_duration=>1 hour}

       (compared using ==)

       Diff:
       @@ -1,3 +1,3 @@
       -:meal_duration => 7000,
       -:travel_duration => 3600,
       +:meal_duration => 2 hours,
       +:travel_duration => 1 hour,

In this case, only meal_duration is failing the assertion, but travel_duration is appearing in the diff as well. If we appended a (inspected with DurationInspector) which is an inspector which can_inspect? only returns true to instances of ActiveSupport::Duration, then we still end up with a red herring for travel_duration in the diff:

       Diff:
       @@ -1,3 +1,3 @@
       -:meal_duration => 7000,
       -:travel_duration => 3600,
       +:meal_duration => 7200 (inspected with DurationInspector),
       +:travel_duration => 3600 (inspected with DurationInspector),

I think this means that to cover this case without bigger changes we'd need to use the same inspector for both ActiveSupport::Duration and Integer in order for these to now show up in the diff?
With that in mind I'd say that my opinion is that a note about which inspector was used probably doesn't belong in ObjectFormatter. It could probably be added somewhere else (maybe in the Differ?) so that it lists all custom inspectors used, but I'm not sure how much work that would be and how much scope it would add here.

[b-loyola]

@pirj thanks for the detailed feedback and all of the examples. I definitely agree and I wasn't really thinking of the object identity case here so I do see how that could be a potential pitfall.
That being said though, the objective here was just to provide a more friendly interface to customize object inspection for spec purposes only.
For instance, right now if I have an assertion which compares hashes where all the values are instances of ActiveSupport::Duration, it can become difficult to tell what is failing the assertion. This is because inspect for these objects really doesn't convey the true nature of the information which they represent (number of seconds as an integer):

(2.hours - 60.minutes) == 1.hour
#=> true
(2.hours - 60.minutes) == 3600
#=> true
(2.hours - 60.minutes).inspect
#=> "2 hours and -60 minutes"
1.hour.inspect
#=> "1 hour"
3600.inspect
#=> "3600"

So adding the ability to register a custom inspector is really just a way to enable a custom representation of an object for spec comparisons only (without the need to monkey-patch a class' #inspect in a spec helper), if there is a wish to do so.

As mentioned in the description this is actually currently possible to do as a "hack", by prepending a custom inspector to INSPECTOR_CLASSES, like this:

RSpec::Support::ObjectFormatter::INSPECTOR_CLASSES.unshift(MyCustomInspector)

So the main goal of this PR is just to make this a better interface and potentially open up a path forward to do something like what is mentioned here, e.g.:

RSpec.configure do |config|
  config.inspectors.register(MyCustomInspectorForDuration)
  config.inspectors.register(MyCustomInspectorForSomeOtherObject)
  # etc
end

[pirj]

From my (rather subjective and opinionated) point of view, objects should adhere to some protocol that consumers can rely on. In this case, it's "return the internal representation for inspect that is helpful in development, return business representation for to_s, for runtime". And ActiveSupport::Duration does its job very well on this front.
Unfortunately, Ruby is not so formalized as other languages, and this statement has no reinforcement I know of.

I'm personally reluctant to decorate objects for diffing purposes, especially if it seems possible to keep the presentation under control by adhering to the above rule. Given that the differ is matcher-aware and doesn't solely rely on the a.inspect == b.inspect.

In any case, the specs are too specific and don't indicate clearly what the difference custom inspectors make. I would ask to add more specs, but don't really want to waste your time.
If you are dedicated to improve differ's output, I'd suggest improving the output for Hash/Array, for cases described above.
It's a sensitive topic, though, since various CI/IDEs relying on the current output format.

[b-loyola]

Thanks @pirj that's great feedback. While I agree with your opinion that objects should adhere to some protocol that consumers can rely on, my opinion is that it's not easy (if at all possible) to keep the presentation under control just by adhering to the protocol mentioned above. In fact, the ActiveSupport::Duration object is a great example where that protocol is followed but the Differ and ObjectFormatter currently don't do a great job in presenting the differences because it's not easy to do so. Even if it was possible though, there are probably countless codebases with with objects that don't follow this pattern out there, and I'd be reluctant to implement something that assumes all objects follow that protocol and end up with a worse presentation than what is currently done.

From my (also very subjective and opinionated) point of view, it's better to allow users to customize the output to be the way they want it for specific objects than to introduce breaking changes in presentation that assume that an object should be sometimes presented with inspect and sometimes presented with some other method (e.g. to_s), which I think could be more confusing.

In a separate point, I checked what the differ does with regards to the object identity and it seems to handle that case pretty well since it shows that they are different objects:

       expected #<ActiveSupport::Duration:16960> => 90 seconds
            got #<ActiveSupport::Duration:16980> => 90 seconds

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.

This would still be pretty readable with the custom inspector:

       expected #<ActiveSupport::Duration:16960> => 90
            got #<ActiveSupport::Duration:16980> => 90

       Compared using equal?, which compares object identity,
       but expected and actual are not the same object. Use
       `expect(actual).to eq(expected)` if you don't care about
       object identity in this example.

Anyway, I'm happy to add more test cases here if that will help push this forward, but also don't want to waste more time if this is not going to be considered. In my opinion, while this is definitely not a complete solution it's a step in the right direction. If there's disagreement there then there's probably no point in continuing to work on this, but if we agree that this can provide value then I can definitely add more tests here. If that's the case just let me know what kind of specs you would like to see here.

[pirj]

@b-loyola One more example I can think of.

expect(state_change_margins).to eq([0.celsius, 100.celsius]) # celsius

   expected collection contained:  [0'C, 100'C] (inspected with TemperatureInspector)
   actual collection contained:    [-10, 100]

Should there only be just one base representation for inspection in diff? What if the object is comparable with an integer (`100`) and a string (`100 F`), should we inspect is as a string, or as an integer?

Personally, I'm not convinced by the idea. However, my opinion counts last, as I have the least experience with differs code.
Let's see what @benoittgt and @JonRowe think.

[b-loyola]

should we inspect is as a string, or as an integer?

@pirj that's kind of my point here. I don't think it's rspec's job to know how to represent every object out there, and allowing different codebases to customize how to represent objects for only for diffing purposes is what this PR is all about (or at least paving way for doing so). If in one codebase there is an object which is usually compared with integers, registering a custom inspector for that object to be displayed as an integer might make sense. If in another codebase there's an object that is usually compared to strings, there might be a desire to customize how that object is inspected for diffing. And in most cases they will probably just want what the default behaviour is, and let the representation just follow what inspect for the object returns - that's fine too, nothing changes in this case - if an inspector is not registered there would be no changes to this behaviour.

And to voice my (again subjective) opinion here, while I know this is possible I kind of disagree with the idea of an object that can be both equal to 100 (integer) and "100'C"(string) because - and maybe javascript developers will disagree with this - to me it doesn't make sense for an object to be equal to both an integer and a string, specially when the string an integer will not equal each other. However, I don't think any of this is in the scope of the change proposed here - these cases are very hard to deal with and I'd say the rspec differ probably doesn't need to support that. If you really wanted to support this though, someone could propose it in a separate PR - the change proposed here is not mutually exclusive with that.

[idrozd]

Is it possible to achieve something similar and file-local via refinements?

Not sure where to put "using" though.

Below did not work

module Inspection 
  refine Whatever do
    def inspect
      "custom inspection"
    end 
  end
end

# ObjectFormatter is the class that send "inspect" to Whatever's – I'm assuming it should pull the refinement?
RSpec::Support::ObjectFormatter.class_exec do
  using Inspection
end

describe Whatever do
  using Inspection
  example do
    puts Whatever.new.inspect # => "custom inspection"
    expect([]).to include(Whatever.new) # => generic ... expected [#<Whatever x: nil>] got [] ...
  end
end

[pirj]

halostatue/diff-lcs#70 semi-related

[b-loyola]

Closing as this is probably stale now, can reopen if there's any interest.