`include` diff can wrongly indicate some substrings are missing that are not

[myronmarston]

Consider this spec:

specify "the diff indicates the substrings that are included are not" do
  words = "  foo\nbar\nbazz"
  expect(words).to include("foo", "bar") # passes
  expect(words).to include("foo", "bar", "gaz")
end

The 2nd expectation fails with:

     Failure/Error: expect(words).to include("foo", "bar", "gaz")
       expected "  foo\nbar\nbazz" to include "foo", "bar", and "gaz"
       Diff:

       @@ -1,4 +1,4 @@
       -foo
       +  foo
        bar
       -gaz
       +bazz

Notice that foo is shown in the diff to not match, and yet the string clearly includes foo, and as the first expectation shows, it is considered to match. It's just the last include argument (gaz) that does not match. The diff is confusing here.

[imtayadeway]

started to look into this last night. in rspec-support, i can verify the following:

# spec/rspec/support/differ_spec.rb
it "outputs unified diff of a string and a collection of strings" do
  expected = ["foo", "bar", "gaz"]
  actual = " foo\nbar\nbazz"
  expected_diff = <<-'EOD'

@@ -1,4 +1,4 @@
-foo
+ foo
 bar
-gaz
+bazz
EOD

  diff = differ.diff(actual, expected)
  expect(diff).to be_diffed_as(expected_diff)
end

are we saying that the differ is behaving as expected here, given the above input, and that this is definitely an issue in rspec-expectations? if we can just clarify this, i'm happy to start tackling this :)

[myronmarston]

@imtayadeway -- First off, I apologize for taking so long to get back to you. The notification email has been sitting in my inbox for a while and I only now got around to responding. Sorry about that.

Anyhow, I would say that it's not clear where the fault with this lies. A line of "foo" is different than a line of " foo", so it's correct that the differ highlights it as a difference. And with a matcher like eq, it's proper that it be considered a difference. The problem is that the differ doesn't understand the semantics of the matcher's fuzzy matching logic. I have no ideas of what a good solution for this would be.

[imtayadeway]

@myronmarston no apologies necessary! thanks for your thoughts - i'll try to dig into this this weekend.

[imtayadeway]

@myronmarston having given this some more thought.... I'm coming back with more problems :)

given the above example, it might be reasonable to instruct the differ to ignore whitespace. but what if it's more complicated?

for example, expect("foobar\nbaz\nqux").to include("foo", "bar", "baz", "qux") will pass. expect("foobar\nbaz\nqux").to include("foo", "bar", "gaz"), however, produces:

Diff:
       @@ -1,4 +1,4 @@
       -foo
       -bar
       -gaz
       +foobar
       +baz
       +qux

Does this seem like a reasonable diff for this expectation? I wonder if diff is the right tool for the job here?

[myronmarston]

given the above example, it might be reasonable to instruct the differ to ignore whitespace.

Interesting idea. I hadn't considered that!

I think it would solve some of the common cases here, but it would get messy if the snippets the user was expecting included whitespace, e.g.:

expect(string).to include(" foo ", "    bar")

For an expectation like that whitespace matters very much and telling the differ to ignore it would be confusing.

I wonder if diff is the right tool for the job here?

Good question...and that actually gives me a good idea for what to do!

I think that a line-by-line diff is not the right tool for certain cases of the include matcher, as we've seen. Given that, I think it's better to disable the diff when it's going to be confusing rather than trying to magically make the differ work well for all cases. So maybe you can come up with some logic for the include matcher's diffable? method to have it return false in cases where it will just be confusing?

[imtayadeway]

@myronmarston that sounds like an agreeable solution to me. i'll try to get something done over the weekend!

[myronmarston]

Closing in favor of #763.