Bad error message if collections have different types

[Dreamescaper]

While framework supports checking equality of collections of different type, error message is not very informative in case of failure:

Expected is <System.String[1]>, actual is <System.Linq.Enumerable+SelectListIterator`2[NamespaceA.SomeType,System.String]>
  Values differ at index [0]

Possible options to fix:
a) always log collections as items (e.g. "< 1,2,3... >"), ignoring type
b) log actual and expected item where failed

(Personally I prefer option a) )

[ChrisMaddock]

@Dreamescaper - could you provide a test which demonstrates this problem? We'd welcome a PR to resolve this, if you'd be interested. 🙂

[Dreamescaper]

Ok, so seems like I haven't provided all details (because I missed them myself).
Such uninformative message occurs when there are non-collection arguments, and quantity differs:

        [Test]
        public void DifferentCollectionType()
        {
            var expected = new int[] { 1, 2, 3 };
            var actual = expected.Take(2);

            Assert.That(actual, Is.EqualTo(expected));
        }

---->

Expected is <System.Int32[3]>, actual is <System.Linq.Enumerable+ListPartition`1[System.Int32]>
  Values differ at index [2]

Would it make sense to log something like:

Values differ at index [2]
Expected: 3
Actual: (none)

?

[ChrisMaddock]

Thanks for adding that! That makes it a lot easier for someone to pick up - if you'd be interested in contributing a fix yourself, it would be very welcome! 😄

[Dreamescaper]

Yeah, will take a look!

[ChrisMaddock]

Great - thanks!

[Dreamescaper]

I just thought that it might be more sense to log it like

Values differ at index [2]
Missing: 3

(To have it similar to existing Missing/Extra messages, but with single element).

Unlike currently suggested in PR fix, that would be couple of lines.

@ChrisMaddock WDYT?

[Dreamescaper]

(Updated PR with suggested)

[CharliePoole]

What message do you currently get in the same situation with two arrays? My suspicion is that the problem arises only when one item is an IEnumerable but not an ICollection.

[Dreamescaper]

Problem occured when there's at least one IEnumerable argument, and quantity of elements is diffferent.
For array in this case non-matching items are logged (Missing/Extra items), but that is disabled for IEnumerable - I suppose to avoid possible double evaluation.
That's why I suggest to log single Missing/Extra item - that doesn't require second evaluation.

[CharliePoole]

Exactly. In that case they are both handled as IEnumerable. I imagine the same problem will arise if both are IEnumerable.

I'm on my phone so can't easily look at the code. Don't we have an internal setting for how many missing or extra items to show?

[CharliePoole]

"I'm not sure you can say if the item is missing or extra without evaluating the entire enumerable can you? It could just be the collection out of order - which would make your original solution the better version in my eyes. 🙂"

@ChrisMaddock Except that all the NUnit equality tests on collections and enumerables consider order as part of the match. I'd agree with you in the case of an equivalence test, but when the the actual runs out of items, then you have missing stuff and when the expected runs out first, you have extras.

(Equivalence, by the way, is where we really have problems with error messages.)

[ChrisMaddock]

@ChrisMaddock Except that all the NUnit equality tests on collections and enumerables consider order as part of the match.

To be clear - all I had meant was that saying an item is 'missing' is not necessarily correct, just that it is "not in the expected position". I had deleted that comment however, as I realised I'd originally misread this issue...unfortunately email notifications live for ever. 😉

Will be back to this issue with my actual thoughts shortly...

[Dreamescaper]

No, it's defined directly in EqualConstraintResult (3 items).
I can't reuse existing method (as it will re-evaluate enumerable), but I can log item on which assertion failed (which I did in suggested PR).

[Dreamescaper]

@ChrisMaddock
I just thought that approach with Extra/Missing is more consistent with existent behavior, and more elegant to implement.
I can submit PR original implementation as alternative to consider (I have it in separate branch).

[Dreamescaper]

(for my purposes I would always log it same way as for array - with expected/actual items, full Extra/Missing items etc, as more readable failure message is worth performance costs of double evaluation or iterating to array, but I suppose it's not the case for everyone)

[ChrisMaddock]

@Dreamescaper Apologies, my fault, I'd mis-read the original issue.

Can I just clarify that this issue is specific to the case where a) one or more items are not ICollections AND b) the items differ in length?

I just thought that approach with Extra/Missing is more consistent with existent behavior, and more elegant to implement.

I think this depends on current behaviour. Does the current behaviour always enumerate the entirity of the enumerable, or does it stop at the first index where there's a difference?

If the former, then it sounds like we could work out the full list of Extra/Missing, and we should display that information.

If the latter, then I don't think we should display a potentially 'incomplete' list under the same Extra/Missing titles that we'd use for ICollection comparison, and would prefer a solution such as the one you suggested above:

Values differ at index [2]
Expected: 3
Actual: (none)

What do you think?

---

(for my purposes I would always log it same way as for array - with expected/actual items, full Extra/Missing items etc, as more readable failure message is worth performance costs of double evaluation or iterating to array, but I suppose it's not the case for everyone)

I remember having a discussion on this when this was implemented, and a concern was raised that the enumerable may return different items when it is enumerated each time. That's the main reason to only do a single enumeration, in my eyes. 🙂

[Dreamescaper]

Does the current behaviour always enumerate the entirity of the enumerable, or does it stop at the first index where there's a difference?

Stops at first.

What do you think?

Both options are fine by me.

enumerable may return different items when it is enumerated each time.

True, but it is possible to save it to array before any actions, so it would be single enumeration. Not sure if it makes sense here though.

[CharliePoole]

I like the Extra/Missing approach because having a different format from two collections makes it seem as if there is a different problem when really the problem is the same in both cases.

However, I think a full list could produce much too much output in some cases. Suppose one collection has 1000 extra items! In the case of string comparisons, there's an option we can use to show the entire string rather than truncating it. Maybe we could do the same here - as a separate issue, however.

For this issue, I'd say stick with max of three.

[ChrisMaddock]

I like the Extra/Missing approach because having a different format from two collections makes it seem as if there is a different problem when really the problem is the same in both cases.

Ah - but I think there is a different problem here! With collections, we show Extra and Missing, as we've worked out all the items in the entire collection which are extra and missing.* With the enumerable - we have stopped enumerating at the first incorrect item, so don't have that information to display it!

*As per my current understanding without looking at the code - @Dreamescaper, please correct me if I'm wrong!

[CharliePoole]

But we don't actually have to stop enumerating!

[ChrisMaddock]

We don't, but I prefer the correct behaviour to stop enumerating as soon as we can - just based on efficiency. I think that's preferable over the enhanced error message, in this case. 🙂

[CharliePoole]

More detail: with two collections, we are also enumerating through an index. At the point of failure, we don't know what the next (extra or missing) items are but we can simply access that as a start point in the collection. At a high level, the same is true with enumerators. In the case of Missing items, we can continue to enumerate the expected result. In the case of Extra items, we can continue to enumerate the actual value. The key decision is whether to do that in the EqualConstraint or in the EqualConstraintResult. Probably better to discuss this right in the code, however.

[Dreamescaper]

@CharliePoole
It's exactly what is done for array - show max of three items.
But again - if I do same with current implementation that would re-evaluate IEnumerable.
So there are several options:
a) Don't display Missing/Extras as collection, but display only item where assertion failed (in one format or another)
b) Rework implemention to re-use same enumerator for assertion and message logging (to do that in one iteration). Could work, but not sure if worth the effort.
c) Save IEnumerable items to array before any actions. Technically could lead to issues where there are a lot or infinite number of items. And IEnumerable evaluation would be non-lazy. Those are edge cases, and don't think it would matter for anyone, but who knows?..

[CharliePoole]

I think (a) is fine as a first step or possibly for good. I was assuming (b) rather than re-evaluating the enumerator. I think (c) is a non-starter for the reasons you state.

I didn't write (b) originally for exactly the reasons you state. I figured we could come back to it if need be. This could be the time to come back, or we could just do (a) and see if it's sufficient for everyone's needs. I think it's up to you as the implementor.

[ChrisMaddock]

Agreed! 😄

[Dreamescaper]

If a) - what display format should be?

Values differ at index [2]
Expected: 3
Actual: (none)

or

Values differ at index [2]
Missing: 3

?

[CharliePoole]

I think Missing is clearest and also allows for Extra when things go the other way. The value (none) could mean some sort of null value, depending on what language you are coding in.

[Dreamescaper]

In this case, that is how it is currently implemented in PR :)