rspec / rspec-support

Common code needed by the other RSpec gems. Not intended for direct use.
https://rspec.info
MIT License
95 stars 104 forks source link

Differ does not report differences in string arrays #518

Open dmolesUC opened 2 years ago

dmolesUC commented 2 years ago

Subject of the issue

When comparing two arrays of objects, Differ provides a useful report on which expected objects are missing and which unexpected objects are present. When comparing two arrays of strings, however, the array diff is omitted and only the truncated inspect diff is present.

Looking at the source code, it appears this was sort of intentional; but on the other hand it looks as though the original change back in 2010 was only intended to make sure the multiline differ wasn't applied to single-line strings. Ignoring all single-line string arrays just seems to have been a side effect.

Your environment

Steps to reproduce

Run the following (failing) spec:

module ModuleWithALongName
  class ClassWithALongName
    include Comparable

    attr_reader :v

    def initialize(v)
      @v = v
    end

    def to_s
      "Corge<#{v}>"
    end

    def <=>(o)
      return nil unless o.is_a?(ClassWithALongName)
      self.v <=> o.v
    end

    def inspect
      "#{self.class.name}<value: #{v}>"
    end
  end

  describe ClassWithALongName do
    let(:d1) { Array.new(5) { |i| ClassWithALongName.new(i) } }
    let(:d2) { Array.new(5) { |i| ClassWithALongName.new((i / 2).odd? ? i : i * 2) } }

    context 'as objects' do
      it 'is equal' do
        expect(d1).to eq(d2)
      end
    end

    context 'as strings' do
      it 'is equal' do
        d1s = d1.map(&:inspect)
        d2s = d2.map(&:inspect)
        expect(d1s).to eq(d2s)
      end
    end
  end
end

Expected behavior

Failure messages for both tests should show the array diff.

Actual behavior

Only the failure message for as objects shows the array diff.

Failures:

  1) ModuleWithALongName::ClassWithALongName as objects is equal
     Failure/Error: expect(d1).to eq(d2)

       expected: [ModuleWithALongName::ClassWithALongName<value: 0>, ModuleWithALongName::ClassWithALongName<value: 2>...oduleWithALongName::ClassWithALongName<value: 3>, ModuleWithALongName::ClassWithALongName<value: 8>]
            got: [ModuleWithALongName::ClassWithALongName<value: 0>, ModuleWithALongName::ClassWithALongName<value: 1>...oduleWithALongName::ClassWithALongName<value: 3>, ModuleWithALongName::ClassWithALongName<value: 4>]

       (compared using ==)

       Diff:

       @@ -1,6 +1,6 @@
        [ModuleWithALongName::ClassWithALongName<value: 0>,
       + ModuleWithALongName::ClassWithALongName<value: 1>,
         ModuleWithALongName::ClassWithALongName<value: 2>,
       - ModuleWithALongName::ClassWithALongName<value: 2>,
         ModuleWithALongName::ClassWithALongName<value: 3>,
       - ModuleWithALongName::ClassWithALongName<value: 8>]
       + ModuleWithALongName::ClassWithALongName<value: 4>]

     # ./spec/lib/module_with_a_long_name_spec.rb:33:in `block (3 levels) in <module:ModuleWithALongName>'
     # /Users/david/.rvm/gems/ruby-3.0.2/gems/webmock-3.14.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

  2) ModuleWithALongName::ClassWithALongName as strings is equal
     Failure/Error: expect(d1s).to eq(d2s)

       expected: ["ModuleWithALongName::ClassWithALongName<value: 0>", "ModuleWithALongName::ClassWithALongName<value:...leWithALongName::ClassWithALongName<value: 3>", "ModuleWithALongName::ClassWithALongName<value: 8>"]
            got: ["ModuleWithALongName::ClassWithALongName<value: 0>", "ModuleWithALongName::ClassWithALongName<value:...leWithALongName::ClassWithALongName<value: 3>", "ModuleWithALongName::ClassWithALongName<value: 4>"]

       (compared using ==)
     # ./spec/lib/module_with_a_long_name_spec.rb:41:in `block (3 levels) in <module:ModuleWithALongName>'
     # /Users/david/.rvm/gems/ruby-3.0.2/gems/webmock-3.14.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
2 examples, 2 failures, 0 passed
Finished in 0.037725 seconds
dmolesUC commented 2 years ago

FWIW, the ridiculous workaround I've come up with as a temporary hack to debug failing tests is this:

    context 'as string map values' do
      it 'is equal' do
        d1s = d1.map(&:inspect)
        d2s = d2.map(&:inspect)

        expect(d1s.map { |v| { v: v } }).to eq(d2s.map { |v| { v: v } })
      end
    end

Which produces the at-least-somewhat-readable array diff:

 [{:v=>"ModuleWithALongName::ClassWithALongName<value: 0>"},
+ {:v=>"ModuleWithALongName::ClassWithALongName<value: 1>"},
  {:v=>"ModuleWithALongName::ClassWithALongName<value: 2>"},
- {:v=>"ModuleWithALongName::ClassWithALongName<value: 2>"},
  {:v=>"ModuleWithALongName::ClassWithALongName<value: 3>"},
- {:v=>"ModuleWithALongName::ClassWithALongName<value: 8>"}]
+ {:v=>"ModuleWithALongName::ClassWithALongName<value: 4>"}]
pirj commented 2 years ago

Can you dig why it was done initially? I found "Extract diffing code from expectations to support" commit, and it leads to a different repo.

dmolesUC commented 2 years ago

@pirj See this commit in rspec-expectations (also linked above). Note that the code was in fail_with.rb at that time.

pirj commented 2 years ago

Ah, sorry, I've missed this. The original reasoning of not showing such diffs:

-that
+this

doesn't really apply to string arrays.

Would you like to submit a PR?

dmolesUC commented 2 years ago

@pirj Done.