Allow Is.Ordered to Compare Null Values

[lundmikkel]

If you try to assert that an enumerable containing a null value is ordered:

Assert.That(new[] { null, "Hello", "World" }, Is.Ordered);

An exception is thrown:

System.ArgumentNullException: Null value at index 0

This is because the CollectionOrderedConstraint explicitly checks for null values:

if (obj == null)
    throw new ArgumentNullException("actual", "Null value at index " + index.ToString());

This seems like an unnecessary strict assertion. In the MSDN documentation, it states that:

By definition, any object compares greater than (or follows) null, and two null references compare equal to each other.

This explicitly says that null is before anything else, and implicitly that null values are comparable and allowed.

I understand that null values are often not desired, but they are a part of C#'s type system and it should be allowed to check if null references are ordered – at least using a setting if the default behavior is to disallow them:

Assert.That(new[] { null, "Hello", "World" }, Is.Ordered.AllowingNull); // Should not thrown an exception

(this is merely the best name I could come up with on the fly 😉)

[rprouse]

It looks like the original purpose of the null check was to handle the case where we are comparing properties of the objects within the collection. If that is the case, why not just apply the constraint in that situation? Or, if the object is null, then the property is null too?

It would be good to dig back into the history of that null check to see why it was originally added.

[CharliePoole]

If memory serves, I simply decided that a null value would be an error in any reasonable use case. Of course, I may have been wrong, but I'd want to actually see the use case.

There are lots of instances where NUnit doesn't do the typical or standard .NET thing. That's because we're a test framework and we support scenarios that make sense in writing test code. I'm not sure there's a useful test to write here, but I'm willing to be shown.

[lundmikkel]

@CharliePoole I get what you mean, but I do think that there are actual use cases for this. In fact, I found the problem, because I had this very problem:

I'm working on a collection library, where items can be null (if a user set property AllowsNull is true). The library's IList<T> interface allows users to sort the list's items. Even when the collection allows null values, I would still very much like to ensure that the list was properly sorted.

This was my test:

[Test]
public void Sort_RandomCollectionWithNull_Sorted()
{
    // Arrange
    var items = GetStrings(Random);
    var index = Random.Next(1, items.Length);
    items[index] = null;
    var collection = GetList(items, allowsNull: true);

    // Act
    collection.Sort();

    // Assert
    Assert.That(collection, Is.Ordered);
}

[CharliePoole]

It would be a breaking change for someone who expects nulls to cause a failure but probably one we could live with. Such a user would have to assert

Has.None.Null.And.Ordered

[CharliePoole]

@lundmikkel It has been a long time since we talked about this. What do you think should happen with a null item in the collection if the assertion is...

Assert.That(collection, Is.Ordered.By("X");

Personally, I'd be inclined to keep the error in that case.

[jnm2]

I have real-world code that relies on null being sortable.

Assert.That(collection, Is.Ordered.By("X")); should totally fail (it's a NullReferenceException) if the collection item is null but should not fail if the collection item's X property has the value null.

[CharliePoole]

Yes, the distinction is clear. I was specifically referring to the case of a null item. I a, the agree that for null properties, a null value should be acceptable.

@lundmikkel This is your issue - do you agree?

[mikkelbu]

@CharliePoole This issue has been underway a long time, and I'm unsure whether we will hear from @lundmikkel. I agree with @jnm2 that it should fail if the collection item is null, and that it should work if the value of the property is null. I propose that we move the issue to the backlog and mark the issue up-for-grabs.

[ChrisMaddock]

I agree. I would have uses for this too.

[CharliePoole]

Me too!

[ChrisMaddock]

Up-for-grabs - good find @mikkelbu 😄

[lundmikkel]

Sorry, guys; I totally missed you comments!

So, I think the assertion should work if the item being checked is null. That would mean that checking null items should be fine, just like checking properties on non-null items should be fine. If you order by a property, it should fail if the object is null, since that is a regular null pointer exception.

I haven't done any kind of work on this, nor will I be doing anything. So if @mikkelbu is up for it, be my guest.

[pknouff]

I can take this one.

Just so I'm clear on the intent here,

Assert.That(new[] { null, "Hello", "World" }, Is.Ordered);

should not thrown an exception. Or is the intent to explicitly control this via something like

Assert.That(new[] { null, "Hello", "World" }, Is.Ordered.AllowingNull); 

as originally suggested in the issue description?

[jnm2]

I see several people in favor of the first and no one set against it. I also am in favor of not requiring AllowingNull. AllowingNull is basically admitting that there was no reason to block it in the first place.

We need to make a note of the behavioral change in the release notes so that people can do Has.None.Null.And.Ordered like @CharliePoole said if they relied on Is.Ordered to implicitly check for null.

[pknouff]

Alright. I take it the docs should be updated as well to explain this new behavior?

[jnm2]

They won't be incorrect after the change but being explicit always improves docs. @rprouse can give you access to edit the docs.

I'm removing the up-for-grabs label to indicate that the issue is yours.

[jnm2]

Oh wait, it's called help wanted now. 😄