Broken `char` comparison in v3.7 and higher

[dgg]

Hi,

I just found out that there is a problem comparing chars that happened from 3.7.

Take this assertion:

Assert.That('2', Is.GreaterThan('3'));

When run in 3.6.1, the assertion will (correctly) fail:

Test 'whatever' failed:   Expected: greater than '3'
  But was:  '2'

When the same assertion is run in 3.7.0 or higher, it will not fail, which, in my opinion, is wrong as '2' is definitely not greater than '3' and neither are their numeric values (50 and 51).

[rprouse]

The following also passes, so it seems like GreaterThan silently always passes with chars now.

Assert.That('3', Is.GreaterThan('2'));

[mikkelbu]

The problem is here (we always return 1 if the two chars are different instead of using the comparison)

nunit/src/NUnitFramework/framework/Constraints/NUnitComparer.cs

Lines 58 to 59 in d03e93c

	if (x is char && y is char)
	return (char)x == (char)y ? 0 : 1;

[dgg]

@mikkelbu that line surely looks suspicious, but it was the way it is already in 3.6.1:

https://github.com/nunit/nunit/blob/3.6.1/src/NUnitFramework/framework/Constraints/NUnitComparer.cs#L58

And it worked back then, so maybe, just maybe, there is more to it 🤔

[CharliePoole]

I think the code has been broken like this for as long as it has existed. In the past, it was only used for equality comparisons. GreaterThan and LessThan comparisons were made using operators.

Now this code is also used for GreaterThan and LessThan, so we have to return -1 as well when it's appropriate, not just 0 or 1.

[jnm2]

Would it make sense to remove those two lines so that it falls through to these lines?

nunit/src/NUnitFramework/framework/Constraints/NUnitComparer.cs

Lines 64 to 65 in d03e93c

	if (x is IComparable)
	return ((IComparable)x).CompareTo(y);

[CharliePoole]

It won't fall through but will be treated as numeric. That's why the code is where it is. Numeric order is wrong for some cultures.

[jnm2]

Would it make sense to never consider it numeric, i.e. remove this line also?

nunit/src/NUnitFramework/framework/Constraints/Numerics.cs

Line 80 in d03e93c

if (obj is System.Char) return true;

[CharliePoole]

@jnm2 It's fairly common, especially in older code to compare characters to numbers...

if (c > 127)
    ...
Assert.That(c, Is.GreaterThan(127);

The code we have in NUnit is an outgrowth of people trying to do that sort of thing over the years.

At some point, numerics were added ahead of everything else due to an issue. At some other point, chars were added to numerics due to an issue report. At yet another point, the char comparison was added due to an issue.

We can rethink the whole approach, but it's hard to duplicate years of both thinking and error reporting by users without a great deal of analysis.

Ideally, stuff like this should have been added with an explanatory comment. I guess we figured that somebody would always be around to remember the reason. 😞

[jnm2]

@CharliePoole Ah, that makes sense. We need to consider it numeric so that we do numeric type coercion between the expected and actual types.

If we remove only the two lines which I originally asked about, I think that (plus tests) would resolve the issue. Looks like it would fall through to this line which would do the right thing:

nunit/src/NUnitFramework/framework/Constraints/Numerics.cs

Line 384 in d03e93c

return Convert.ToInt32(expected).CompareTo(Convert.ToInt32(actual));

[CharliePoole]

No, I'm afraid it wouldn't, although it would take me a while to find the original example that caused this. We've been through several reorganizations of the code and at least two changes of where we host and record our issues since then.

IIRC the problem only arises when using certain cultures and or character sets. The upshot is that the desired "order" of two characters doesn't necessarily match the numeric order of their representation.

Any reason you can see not to make the obvious fix?

return (char)x == (char)y ? 0 : (char)x > (char)y ? 1 : -1; 

[jnm2]

The reason I don't want to special-case char there is that return (char)x == (char)y ? 0 : (char)x > (char)y ? 1 : -1; and return x.CompareTo(y); both behave as ordinal (numeric) comparisons no matter what the culture settings are, just like the int comparison that happens on fallthrough:

nunit/src/NUnitFramework/framework/Constraints/Numerics.cs

Line 384 in d03e93c

return Convert.ToInt32(expected).CompareTo(Convert.ToInt32(actual));

Other than performance, all three behave the same in all circumstances so far as I can tell. So my hesitation with the manual calculation is that deleting code is awesome when we can get away with it, and that it seems inconsistent to special-case char and not, for example, byte.