`include` matcher failure message does not specify which items were not included

[myronmarston]

     Failure/Error: expect("abc").to include("a", "d", "c")
       expected "abc" to include "a", "d", and "c"

In this example, "a" and "c" were included but not "d". The failure message doesn't specify which were included and which weren't, though. In this case, that's not a huge deal since you can see the letters in the string, but if the subject's inspect output didn't indicate what it includes, you would have no way of knowing.

We should improve the failure message.

[chrisarcand]

What do you propose the improved message should look like?

[myronmarston]

What do you propose the improved message should look like?

Two possibilities I can think of:

expected "abc" to include "d"

...in other words, only mention the items that are not included. This is similar to how respond_to works.

Or:

expected "abc" to include "a", "d" and "c" but did not include "d"

I think I like the first way slightly better, though, as the latter message seems slightly noisier.

[chrisarcand]

I was thinking the first one as well. I'll look into this.

[myronmarston]

Sounds good. Thanks, @ChrisArcand!

[chrisarcand]

@myronmarston After a good look at this, I'm on the fence about which direction this should go.

The include matcher's current implementation uses simple methods like #all? for it's predicates; it's beautifully built IMO, but comes with the side effect that all of the items in the array will necessarily be evaluated (if something is not included, the spec will fail immediately because you need not continue). To get around this and be able to provide failure output that details what all is and is not included, I made some changes to the matcher which includes mapping over all elements.

A diff with complete changes can be found here. It works well functionally, and you can see what the output looks like from the edited (and fully passing) spec.

However, the matcher did need to be edited and the fact that both expected and actual results need to be iterated over didn't sit well with me, so I did some [very unscientific] benchmarking:

RSpec.describe "include benchmarks" do
  1000.times do
    it "does stuff" do
      expect([*1..1000]).to include(*[*25..500])
    end
  end

  # Before changes: 1.18s
  # After changes: 2.36s
  # 2x slower

  1000.times do
    it "does stuff" do
      expect([*1..10]).to include(*[*5..8])
    end
  end

  # Before changes: .129s
  # After changes: .165s
  # 27.9% slower

  1000.times do
    it "does stuff" do
      expect([*1..5]).to include(4).and include(5)
    end
  end

  # Before changes: .175s
  # After changes: .180s
  # 3% slower (nominal, IMO)
end

I'm unsure what to call 'typical' for the array length people use when using this matcher, so I don't know if expecting things like 1,000 element long arrays is very fair, but based on these numbers it actually makes me against this change. My benchmarking is pretty rough, so I'd love to see anything you write to further compare if you're curious.

I don't think the difference in error output is worth changing the matcher to not short circuit and taking such a hit in performance, so I propose that instead we simply show the first element that causes the failure to occur. That is:

# Current output
Failure/Error: expect("abc").to include("a", "d", "c", "e")
       expected "abc" to include "a", "d", "c" and "e"

# Output implemented with my changes from above
Failure/Error: expect("abc").to include("a", "d", "c", "e")
       expected "abc" to include "d" and "e"

# What I now suggest
Failure/Error: expect("abc").to include("a", "d", "c", "e")
       expected "abc" to include "d"

It's still helpful as you can see what an issue specifically (doesn't just show expected), and once you address the issue the next message you'd see is expected "abcd" to include "e"

What do you think? I see validity in all three scenarios.

[chrisarcand]

Interestingly enough, as I look through other issues I see that there others related to include that may merit evaluating all elements (actual and expected) in the future anyway ¯_(ツ)_/¯. I think it really just depends on if the matcher is really often used with large arrays or not and how much performance is valued.

[myronmarston]

I don't think the difference in error output is worth changing the matcher to not short circuit and taking such a hit in performance, so I propose that instead we simply show the first element that causes the failure to occur.

I'm a bit confused by this -- I understand why not short circuiting would make it slower, but wouldn't the existing implementation only short circuit if the matcher fails? All expected elements must be checked for the matcher to pass, so it seems like when it passes there should be no change. And yet your benchmarks -- unless I'm misreading them -- are only for passing cases, which should not be effected by whether or not we short circuit. What am I missing?

Anyhow, here's my two cents:

• We care primarily about the perf in the passing case. That's the "common" case since in a day-to-day TDD workflow you're generally only dealing with a handful of failures at once while the vast majority of specs pass.
• When an expectation fails, we care about the failure output a lot. I think top-notch failure output is a big reason users choose to use RSpec.
• Short circuiting, unless I'm misunderstanding what you mean by it, should only affect the perf of a failing expectation. And If we do not short circuit, the negative case won't be slower than the passing case, it's just that we won't be able to leverage short circuiting to make it faster than the passing case, right?
• If we choose to short circuit and only show the first non-included item, there are a few consequences that I think are problematic:
  • I think it would be confusing for users -- expected array to include "a" suggests that "a" is the only one not included since it is the only one listed, so users would (rightfully) expect that all others are included. That's not a good user experience.
  • To fully fix the failing expectation, users would generally need to know all non-included items, so if we list only the first, they're likely to run the example repeatedly to get the next non-included item, fix it, get the next, etc. Any perf gains from short circuiting are going to be greatly offset by having to run the example many times to get the full list.
• We may be able to find a better performing algorithm that would provide the better output while not having nearly as big of a perf impact.

[chrisarcand]

So, I had a lightbulb go off in my head away from my computer and came back to reply with it...

...And I see you already nailed it on the head. You're right, if what I'm saying is true nothing should really change with the passing spec.

Thanks for all your input! I'll clean things up as well as do some better comparisons and get back to you.