Is the ordering in AssertEqualityComparer correct?

[bradwilson]

When looking at AssertEqualityComparer.Equals, clearly the ordering of the checks is important in terms of setting predictable priority.

As of the writing of this issue, the current priority list (from highest to lowest is):

1. Implementing IEquatable<T>
2. Implementing IComparable<T>
3. Implementing IComparable
4. Implementing IDictionary
5. Implementing ISet<T>
6. Implementing IEnumerable
7. Implementing IStructuralEquatable
8. Implementing IEquatable<T2> where T2 == the concrete type of second parameter y
9. Implementing IComparable<T2> where T2 == the concrete type of second parameter y
10. When all else fails, call object.Equals()

You can group these into roughly 3 camps right now:

• 1-3, which are about testing for equality
• 4-6, which are about handling containers
• 7-9, which is... everything else.

I see there are (at least) three potential issues right now:

1. Checking whether things are containers and should be treated like containers should be done fairly late in the list. The grouping of 7-9 is really much more similar to the grouping of 1-3, and should probably be moved up there. It's also worth considering what the interleaving of that should be; for example, should the order be more like 1/8/2/9/3 or 1/2/3/8/9? And where does 7 fit into the list?

2. The logic for 8/9 isn't exactly correct. It currently assumes that the second parameter (y) will potentially be the derived-from-T type, but there's no guarantee of that. The type T could be chosen as the base type based on either of x or y being the more derived type. That should probably be fixed.

3. Step 10 is that we fall back to object.Equals() (which itself will call overridden Equals methods on the first parameter when both are non-null). Should we try to determine if the user has implemented a custom Equals method, and move that logic higher up the list (in particular, before the collections)? This was inspired by a user tripping over their custom implementation of Equals being ignored on their type, because it also was IEnumerable.

Also, I haven't verified this, but I'm guessing there are few (or maybe no) explicit ordering tests that validate the order that things happen in.

Lastly, this should be probably also become a documentation issue so that we have a page on xunit.net that describes the default ordering behavior of checks in v2 (and the differences for v3, if we decide to make any changes).

[trivalik]

Why IEnumerable? This lead to strange effects if you forget that your complex object implements IEnumerable. See #2485.

[bradwilson]

Why IEnumerable?

To support equality for collections.

[bradwilson]

The work to prevent double enumeration, which resulted in an overhaul of some of the assertion logic (especially as it relates to enumerables), means that 4-5-6 are now completely gone.

That just leaves ordering, and I've decided to go with:

1. IEquatable<typeof(x)>
2. IEquatable<typeof(y)>
3. IStructuralEquatable
4. IComparable<typeof(x)>
5. IComparable<typeof(y)>
6. IComparable