Is.Ordered.By() with a field throws NullReferenceException

[rprouse]

The following code throws a NullReferenceException because Name is a field, not a property, This is unintuitive for users, so we should probably handle public fields. At the very least, we should give a better error message.

void Main()
{
	Skill[] skills = new[]
	{
		new Skill("aaaa"),
		new Skill("kkkk"),
		new Skill("zzzz")
	};

	var result = Is.Ordered.By("Name").ApplyTo(skills);
}

public class Skill
{
	public string Name;

	public Skill(string name)
	{
		Name = name;
	}
}

[jnm2]

I agree. NullReferenceException should never be thrown in any NUnit framework method.
Do you want to put on the backlog as up-for-grabs?

[jnm2]

Wait, I guess we have the design question of whether to support public fields in By. I see no reason why not.

[rprouse]

I was already looking at this answering the related StackOverflow question, so I may as well fix it.

[CharliePoole]

I'm in favor of a better error message. The way to get it is to stop letting exceptions get out of the framework. That will solve a lot of other problems.

I'm not in favor of adding fields as an option. There are a number of other features that limit you to properties and wouldn't want to start on a slippery slope.

[jnm2]

The way to get it is to stop letting exceptions get out of the framework. That will solve a lot of other problems.

If we fix this one, I'm not sure that will impact any of the rest of NUnit's potential bare errors. Seems like a separate review effort.

The reason I'm slightly in favor of that slope is that it fits the C# and VB.NET mental model of “a value I can refer to by writing .Name.” The only time it makes sense to me to draw the line is in the case of Property("Name") which is restricted to properties.

[CharliePoole]

@jnm2 Well, it depends how you fix it. All errors that escape can be captured at a single point and reported as some kind of internal framework error by the runner. That would be helpful in telling users that the error is in the framework and should give a stacktrace that we can use to fix it.

At some point, we have to say - let's leave the error in and fix the reporting first. Alternatively, you could inject a failure in a private copy of the framework and see how well you can catch and report it.

The Property("Name") feature is one of those things I'm thinking of. Somebody will next want Field("Name"). 😄

[jnm2]

So, only catching bare errors thrown during calculation of a constraint. That sounds good.

Somebody will next want Field("Name"). 😄

To which we can say no if it's a genuinely bad idea. 😄

[rprouse]

I am just going to switch this to throwing an ArgumentException if the property is not found. Looking at the code, that looks like it was the intention.

[jnm2]

Actually no. Some constraints execute user delegates and which could NRE, making it look like the Framework NRE'd. It's a matter of widespread whitelisting or widespread blacklisting. 😄

[CharliePoole]

Any code a user executes in a test, including constructing constraints and asserting on them, generates a result, not an exception. That is, the code may throw, but it's captured and packaged as a result.

If an exception escapes, it's one of two things:

1. Non-user code is throwing (i.e. our code)
2. We are not properly capturing a test exception and converting it to a result

Both of those things are errors in the framework.

[jnm2]

Okay. So we already have the whitelisting going. I should have known that from my await PR.