CollectionTally (EquivalentTo) should throw for non-transitive comparisons

[jnm2]

As @CharliePoole and I were discussing, we think this should probably be merged before @ggeurts' work at #2830. I marked it as high priority with respect to that PR.

The NUnit team made a design decision in the past to ignore tolerance for EquivalentTo. It's such a tough problem to solve and there isn't demand for it, so I don't think we should change that decision right now. But here is an example of code that implements tolerance manually via a non-transitive custom comparer:

Assert.That(
    new[] { 2f, 1f },
    Is.EquivalentTo(new[] { 3f, 2f })
        .Using<float>((a, b) => Math.Abs(a - b) <= 1));

It returns an incorrect answer. If you swap the order of the array elements, it'll pass. But rather than passing or failing, I think the test should error with InvalidOperationException in the CollectionTally constructor (since CollectionTally can be used directly) if the comparer being used is non-transitive.

In addition, maybe we could hard-obsolete the Using overloads which enable non-transitivity: the ones that will cause InvalidOperationException to be thrown no matter what. This would have to be done carefully because the Using overloads are defined in the a base class, but the base class (CollectionItemsEqualConstraint) works just fine with non-transitivity, so we can't block that.

Assert.That(
    new[] { 2f, 1f },
    Is.EquivalentTo(new[] { 3f, 2f })
        .Using<float>((a, b) => Math.Abs(a - b) <= 1));

The overloads which allow non-transitivity are:

• CollectionItemsEqualConstraint.Using(Comparison<T>)
• CollectionItemsEqualConstraint.Using(Func<T, T, bool>)

What are your preferences? Anything we should consider?

[jnm2]

Actually, perhaps the it should be ArgumentException in case the user is using the constructor directly and not via Assert.That.

[CharliePoole]

Sounds good to me. A few comments...

1. Title seems wrong. Don't we have to ban all Using statements, since we know nothing of their transitivity?

2. What if we refactor so that CollectionItemsEqualConstraint derived from a base without Using?

[jnm2]

1. IEqualityComparer and IEqualityComparer<> are contractually transitive-only. I don't see anything about that for IComparer or IComparer<>. I know from experience that BCL sort methods all rely upon IComparer to be transitive, so I think we're safe to allow these four.

2. I think all possible Using methods should stay available for CollectionItemsEqualConstraint because there we're just doing pairwise comparisons. Anything that works for non-collection Is.EqualTo will also work for CollectionItemsEqualConstraint comparisons. No transitivity needed.
   In that sense, CollectionEquivalentConstraint breaks the encapsulation by not supporting all methods of its base class. Nothing we can do about it now though except shadowing with obsoleted methods, or just throwing an exception.

[jnm2]

In other words, an end user can break the interface's documented contract and build a non-transitive implementation of IEqualityComparer, but that's a .NET BCL training issue for the end user, not an NUnit issue. They would also have to implement GetHashCode which should make it obvious to such a user that they are building an inherent contradiction.

[OmicronPersei]

if the comparer being used is non-transitive

How is this something that can be determined programatically? Or are we just saying that any comparer specified with .Using that can allow non-transitivity, should be disallowed?

[jnm2]

@OmicronPersei This:

any comparer specified with .Using that can allow non-transitivity, should be disallowed

We should shadow and obsolete these Using methods in CollectionEquivalentConstraint:

• CollectionItemsEqualConstraint.Using(Comparison<T>)
• CollectionItemsEqualConstraint.Using(Func<T, T, bool>)

[OmicronPersei]

Oh alright, sounds much easier than I initially thought!

[OmicronPersei]

I'm a little confused, I add the new keyword to the methods to shadow them but then I get compiler warnings saying that they don't hide anything.

[OmicronPersei]

Guys, any thoughts? I'd like to work on this but don't really know what to do.

[mikkelbu]

I have not looked at this before, but my understanding of #2870 (comment) is to add the following to CollectionEquivalentConstraint (probably with better summary and description 😄)

        /// <summary>
        /// TODO
        /// </summary>
        [Obsolete("Don't")]
        public new CollectionItemsEqualConstraint Using<T>(Comparison<T> comparison)
        {
            return base.Using<T>(comparison);
        }

        /// <summary>
        /// TODO
        /// </summary>
        [Obsolete("Don't")]
        public new CollectionItemsEqualConstraint Using<T>(Func<T, T, bool> comparer)
        {
            return base.Using<T>(comparer);
        }

and then do some consequential corrections in CollectionEquivalentConstraintTests.cs.