Confusing empty diff behavior with classes that implement equally-confusing inspect

[ivoanjo]

While writing specs for a project using an older version of the moped gem, I've got bit by how one of its classes implements the inspect method (source).

The following testcase reproduces the issue:

require 'spec_helper'

class Foo
  def self.something(arg); end
  def to_s; 'foobar'; end
  def inspect; to_s.inspect; end
end

RSpec.describe Foo do
  it 'confuses users with an empty diff' do
    expect(Foo).to receive(:something).with('foobar')

    Foo.something(Foo.new)
  end
end

The problem here is that when the expectation fails, rspec will try to diff which will result in

Failures:

  1) Foo confuses users with an empty diff
     Failure/Error: Foo.something(Foo.new)

       #<Foo (class)> received :something with unexpected arguments
         expected: ("foobar")
              got: ("foobar")
       Diff:

     # ./spec/foo_spec.rb:13:in `block (2 levels) in <top (required)>'

which is mighty confusing.

May I suggest adding some visual indication that although both inspects are the same, they are different instances? E.g. by adding a note that although Foo.new.inspect == 'foobar'.inspect' that 'Foo.new != 'foobar'.

I know this is a rather corner case, but in the spirit of "let's not make newcomers tear out their hair" I think it would be awesome if rspec could provide some guidance.

[JonRowe]

The only object that won't satisfy Foo.new.inspect == 'foobar'.inspect' are strings...

[JonRowe]

I can't reliably think of a way to detect this, I mean we could print the class of everything we inspect, but our convention is not to rescue people from broken Ruby semantics...

[ivoanjo]

Perhaps just add a quick tip/warning when the diff comes up empty? e.g. Tip: You diff is empty -- some of your objects have similar inspect strings but are not ==

[ivoanjo]

Edit: I know that not doing stupid stuff like this is better, but given that there is code out there that does it, I'm just thinking of the number of people that will still get bitten by this.

[benoittgt]

Perhaps just add a quick tip/warning when the diff comes up empty? e.g. Tip: You diff is empty -- some of your objects have similar inspect strings but are not ==

Is is something we would like to do @JonRowe?

Otherwise I think we can close the issue.

[JonRowe]

I'm open to the idea.

[benoittgt]

I look at a quick and naive approach for this but I am not convinced that matchers are the right place to do this. Maybe at the diff level?

Happy to get few feedbacks @JonRowe

--- a/lib/rspec/matchers/built_in/eq.rb
+++ b/lib/rspec/matchers/built_in/eq.rb
@@ -8,7 +8,12 @@ module RSpec
         # @api private
         # @return [String]
         def failure_message
-          "\nexpected: #{expected_formatted}\n     got: #{actual_formatted}\n\n(compared using ==)\n"
+          msg = "\nexpected: #{expected_formatted}\n     got: #{actual_formatted}\n\n(compared using ==)\n"
+
+          if actual.class != expected.class
+            msg += "\nYou diff is empty. #{actual.class} implement inspect method but strings are not ==\n"
+          end
+          msg
         end

         # @api private
@@ -26,7 +31,11 @@ module RSpec
         # @api private
         # @return [Boolean]
         def diffable?
-          true
+          if actual.class != expected.class
+            false
+          else
+            true
+          end
         end

require 'spec_helper'

class Foo
  def to_s; 'foobar'; end
  def inspect; to_s.inspect; end
end

RSpec.describe Foo do
  it do
    expect(Foo.new).to eq('foobar')
  end
end

Failures:

  1) Foo confuse users
     Failure/Error: expect(Foo.new).to eq('foobar')

       expected: "foobar"
            got: "foobar"

       (compared using ==)

       You diff is empty. Foo implement inspect method but strings are not ==
     # ./spec/repro_spec.rb:11:in `block (2 levels) in <top (required)>'

---

Another idea will be to be in ExpectedsForMultipleDiffs but to get the class of expected we have to do ugly code.

--- a/lib/rspec/matchers/expecteds_for_multiple_diffs.rb
+++ b/lib/rspec/matchers/expecteds_for_multiple_diffs.rb
@@ -46,7 +46,12 @@ module RSpec
       # @return [String]
       def message_with_diff(message, differ, actual)
         diff = diffs(differ, actual)
-        message = "#{message}\n#{diff}" unless diff.empty?
+        if @expected_list[0].first.class != actual.class
+          message += "\nYou diff is empty. #{actual.class} implement " \
+            "inspect method but strings are not ==\n"
+        elsif !diff.empty?
+          message = "#{message}\n#{diff}" unless diff.empty?
+        end
         message
       end

[JonRowe]

When we produce the diff and it's empty, we should insert the warning then.
Something like diff is empty, do your objects produce identical inspect output??

Failures:

  1) Foo confuses users with an empty diff
     Failure/Error: Foo.something(Foo.new)

       #<Foo (class)> received :something with unexpected arguments
         expected: ("foobar")
              got: ("foobar")
       Diff:
          <diff is empty, do your objects produce identical inspect output?>

     # ./spec/foo_spec.rb:13:in `block (2 levels) in <top (required)>'

[benoittgt]

Seems to be a good idea. I will implement it.