EqualTo().Using() prevents caller from comparing strings to anything else

[tenwit]

Given a suitable DateTimeComparer class that converts IConvertible objects to DateTime, I expect this code to not throw an assertion:

Assert.That(new DateTime(2016, 8, 2), Is.EqualTo("2 August 2016")
                                        .Using(DateTimeComparer.Instance));

But it does, because Using(DateTimeCompare.Instance) returns either a ComparerAdapter or an EqualityComparerAdapter, neither of which override the default implementation of CanCompare.

Since the person writing the test is taking responsibility for comparing the actual and expected values (due to calling Using), CanCompare should always return true for any ExternalComparer added when calling Using.

The original issue and MCVE is described at Stackoverflow.

[rprouse]

Thanks for the detailed report. I've labeled and moved to the backlog.

[tenwit]

I've confirmed that I can build NUnit and have locally added this test to NUnit.Framework.Constraints.UsingModifier:

            public void CanCompareUncomparableTypes()
            {
                Assert.That(2 + 2, Is.Not.EqualTo("4"));
                var comparer = new ConvertibleComparer();;
                Assert.That(2 + 2, Is.EqualTo("4").Using(comparer));
            }

It is failing correctly (that is, the last line is throwing an assertion). I can claim this issue if you like and submit a pull request later today.

[ChrisMaddock]

Have assigned it to you @tenwit, sure it would be welcome!

[tenwit]

I've reviewed all uses of Using(), NUnitEqualityComparer, EqualityAdapter.For and CanCompare. As far as I can tell, CanCompare is used only when the person writing a test calls Using. And I posit that every time that happens, CanCompare should return true.

Reducing that, my solution would be to remove CanCompare and change NUnitEqualityComparer.GetExternalComparer to return the first externalComparer.. which means there should be only one externalComparer. Which means I'm missing something, since there must once have been a need for this functionality.

Are the specs or executable specs explaining the use case for multiple calls to Using? If someone calls Using and provided a comparer that cannot compare two provided items, is there really a need to see if there's another comparer available?

[CharliePoole]

@tenwit Bear in mind that Using may not be applied at the top level of a comparison. For example, if you are testing that a certain collection contains an item equal to an expected value, the equal comparison is performed on the individual items. In theory, this sort of nesting can be to any depth.

As another example, imagine comparing two object arrays for equality. The object arrays contain strings, ints and user defined class insttances. There may be several usings, each one for a particular type.

Admittedly, this is not a very common situation, but it's exactly the situation that caused us to allow adding multiple comparers. The approach you are suggesting would break those cases.

[tenwit]

Ok found the requirement by searching all the .md in nunit/docs.wiki:

Beginning with NUnit 2.6, multiple generic comparers for different types may be specified. NUnit will use the appropriate comparer for any two types being compared. As a result, it is now possible to provide a comparer for an array, a collection type or a dictionary. The user-provided comparer will be used directly, bypassing the default NUnit logic for array, collection or dictionary equality.

Which describes what @CharliePoole said above.

In investigating the comment "multiple generic comparers for different types may be specified", I changed my comparer from this:

    public class ConvertibleComparer : IComparer
    {
        int IComparer.Compare(object x, object y)
        {
            var str1 = Convert.ToString(x, CultureInfo.InvariantCulture);
            var str2 = Convert.ToString(y, CultureInfo.InvariantCulture);
            return string.Compare(str1, str2, StringComparison.Ordinal);
        }
    }

to this:

    public class ConvertibleComparer : IEqualityComparer<IConvertible>
    {
        public bool Equals(IConvertible x, IConvertible y)
        {
            var str1 = Convert.ToString(x, CultureInfo.InvariantCulture);
            var str2 = Convert.ToString(y, CultureInfo.InvariantCulture);
            return str1.Equals(str2);
        }

        public int GetHashCode(IConvertible obj)
        {
            return obj.GetHashCode();
        }
    }

and also to this:

    public class ConvertibleComparer : IComparer<IConvertible>
    {
        public int Compare(IConvertible x, IConvertible y)
        {
            var str1 = Convert.ToString(x, CultureInfo.InvariantCulture);
            var str2 = Convert.ToString(y, CultureInfo.InvariantCulture);
            return string.Compare(str1, str2, StringComparison.Ordinal);
        }
    }

and the generic comparers support my use case (that is, test "CanCompareUncomparableTypes" mentioned earlier changes from failing to passing). So I suggest that the bug, such as it is, is in the form of incomplete documentation, rather than code. I will submit a PR for the new test though, as I think the situation isn't covered by any existing test that I could see.

I'll go add this information to my original stackoverflow question, and fork/PR the wiki. Is that how wiki updates are made? Or is it just by editing the pages directly in GitHub?

[rprouse]

@tenwit you have been added to the @nunit/contributors team, so you can now edit the documents directly.

[jnm2]

Is this good to be closed?

[mikkelbu]

I think so (but I'm unsure whether the wiki was ever updated)

[CharliePoole]

Looks like I merged a commit that was supposed to fix it.

[mikkelbu]

The merged commit contained only some tests illustrating that it is not a problem (if one used the generic version of the comparers.