Breaking change: Is.SupersetOf with ValueTuple requires IComparable in NUnit 3.13.2

[ghelyar]

The following test passed in 3.13.1 and earlier, but fails in 3.13.2.

This is a simplified repro, of course. ValueTuple equality is useful for simplifying more complex comparisons.

[Test]
public void Test()
{
    var a = new S { C = 'a' };
    var b = new S { C = 'b' };
    var c = new S { C = 'c' };
    var d = new S { C = 'd' };

    var actual = new[] { (a, 1), (b, 2), (c, 3), (d, 4) };
    var expected = new[] { (b, 2), (c, 3) };
    Assert.That(actual, Is.SupersetOf(expected));
}

class S
{
    public char C;
}

Here is the stack trace:

System.InvalidOperationException: Failed to compare two elements in the array.
 ---> System.ArgumentException: At least one object must implement IComparable.
   at System.Collections.Comparer.Compare(Object a, Object b)
   at System.Collections.Generic.ObjectComparer`1.Compare(T x, T y)
   at System.ValueTuple`2.CompareTo(ValueTuple`2 other)
   at System.ValueTuple`2.System.IComparable.CompareTo(Object other)
   at System.Collections.Comparer.Compare(Object a, Object b)
   at System.Array.SorterObjectArray.InsertionSort(Int32 lo, Int32 hi)
   at System.Array.SorterObjectArray.IntroSort(Int32 lo, Int32 hi, Int32 depthLimit)
   at System.Array.SorterObjectArray.IntrospectiveSort(Int32 left, Int32 length)
   --- End of inner exception stack trace ---
   at System.Array.SorterObjectArray.IntrospectiveSort(Int32 left, Int32 length)
   at System.Array.Sort(Array keys, Array items, Int32 index, Int32 length, IComparer comparer)
   at System.Collections.ArrayList.Sort(Int32 index, Int32 count, IComparer comparer)
   at System.Collections.ArrayList.Sort()
   at NUnit.Framework.Constraints.CollectionTally..ctor(NUnitEqualityComparer comparer, IEnumerable c)
   at NUnit.Framework.Constraints.CollectionItemsEqualConstraint.Tally(IEnumerable c)
   at NUnit.Framework.Constraints.CollectionSupersetConstraint.Matches(IEnumerable actual)
   at NUnit.Framework.Constraints.CollectionSupersetConstraint.ApplyTo[TActual](TActual actual)
   at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression, String message, Object[] args)
   at NUnit.Framework.Assert.That[TActual](TActual actual, IResolveConstraint expression)

Note that ValueTuple's CompareTo works as expected:

(a, 1).CompareTo((a, 1)) // 0

but trying to use this on the assertion does not help. This still fails, presumably because it's not using this delegate as the comparer for the sorting.

Assert.That(actual, Is.SupersetOf(expected).Using<(S, int)>((t1, t2) => t1.CompareTo(t2)));

Implementing IComparable on the inner type works, but having to implement this all over the place just to make an assertion in the unit tests work, when that assertion used to work fine until a patch change in NUnit is not very nice. The objects are already reference equal.

class S : IComparable
{
    public char C;
    public int CompareTo(object s) => C.CompareTo(((S)s).C);
}

Also note that HashSet<T>.IsSupersetOf still works as expected, so this works:

Assert.That(new HashSet<(S, int)>(actual).IsSupersetOf(new HashSet<(S, int)>(expected)));

but one of the reasons NUnit is so good is that Assert.That(actual, Is.SupersetOf(expected)) generates more useful messages when the assertion fails than "expected true but was false".

Using anonymous types rather than ValueTuples also still works as expected.

[mikkelbu]

Note that ValueTuple's CompareTo works as expected:
(a, 1).CompareTo((a, 1)) // 0

It "only" works as expected, since a is the same reference, so it does not need to compare the values. E.g. (a, 1).CompareTo((b, 1) fails with At least one object must implement IComparable.

So the logic we use to determine if we can sort the collection (i.e. whether it implements System.IComparable) is not good enough, as ValueTuple<T1,T2> implements IComparable, IComparable<(T1,T2)>, but then fail at runtime. Perhaps we also need to examine the type parameters to see whether they also support comparison (and perform this check recursively). Then in the case above we would not try to sort the lists, as the class is not comparable.

Alternatively, and perhaps more safe, we should only sort the collection if it consists of primitive types (plus some selected types - like strings ...).

[mikkelbu]

A possible quick-and-dirty approach could be to have a fallback to the old approach if ArrayList.Sort fails with a InvalidOperationException. Alternatively, we could be more strict about which types we could sider sortable by introducing a whitelist.

[stevenaw]

It looks like this will fail only if one of a ValueTuple's contained item's types don't implement IComparable. I like the idea of catching InvalidOperation here and falling back to the old approach rather than to special-case this particular scenario for value tuples.

I'm going to assign this to myself. I hope to get a PR up in the next few days

[MichaelKetting]

@stevenaw please note that this also affected Tuple, not just ValueTuple.
#3877

I don't mind a quick fix to get this back into working order, but I would not call the lack of IComparable in a tuple's item an edge-case.

[stevenaw]

Thanks @MichaelKetting I've added a few tests to ensure it works with tuples.

I want to clarify that my "special case" comment only refers to an internal optimization made possible when all items are sortable. Tuples and ValueTuples which reference a non-IComparable type should also work after this change in the same manner as in 3.13.1 and earlier

[mikkelbu]

Closed by #3881