Properties on System.Type cannot be used with either property constraint

[jnm2]

Given:

Assert.That(
    new[] { typeof(TopLevelType), typeof(TopLevelType.NestedType) },
    Has.None.With.Property("IsNested").EqualTo(true));

public static class TopLevelType
{
    public static class NestedType
    {
    }
}

I get:

System.ArgumentException : Property IsNested was not found

Just as though the .None.With was not there!

But it gets weirder. If I use any type other than System.Type, it behaves:

Assert.That(
    new[] { new FakeTypeForTesting(false), new FakeTypeForTesting(true) },
    Has.None.With.Property("IsNested").EqualTo(true));

public sealed class FakeTypeForTesting
{
    public bool IsNested { get; }

    public FakeTypeForTesting(bool isNested)
    {
        IsNested = isNested;
    }
}

Gives what it should:

Expected: no item property IsNested equal to True
But was: < <FakeTypeForTesting>, <FakeTypeForTesting> >

[jnm2]

Affected versions: 2.5.7–3.8.1. (Didn't test < 2.5.7.)

[jnm2]

So this is what's going on:

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

Lines 61 to 63 in dba87e5

	Type actualType = actual as Type;
	if (actualType == null)
	actualType = actual.GetType();

I think when actual is typeof(System.Type), rather than throwing ArgumentException when the property is not found, it should at least look for a property with that name on System.Type.

[jnm2]

I guess this a design question now. Do you agree that my fallback suggestion is more intuitive than the current ArgumentException and uselessness of PropertyValueConstraint on System.Type objects?

[jnm2]

Hang on. I understand examining a type rather than an instance for the property existence constraint, but how does it make sense to examine a type rather than an instance for the property value constraint?

You can't have a value without an instance anyway.

[CharliePoole]

I'd say you should separate this from the collection issue in order to zero in on any problem. What happens if you do

Assert.That(typeof(TopLevelType, Has.Property("IsNested"));
Assert.That(typeof(TopLevelType.NestedType, Has.Property("IsNested"));

I can't think of a common use for this sort of assertion, but it seems like it should be possible. You might use it in a library for accessing type information.

[jnm2]

@CharliePoole Sure, I can see that for the property existence constraint, but not for the property value constraint. I also can't see why the property existence constraint can't fall back to examining properties on the passed object instance if it fails to find the properties via the passed object's GetProperty method.

In order of preference:

1. Can we just remove the ability to skip calling .GetType() on the instance?

   • Con: It's a breaking change (for the property existence constraint, not the property value constraint).
   • Pro: It makes things so straightforward.
   • With Has.Property there's no way to pass in binding flags specifying visibility, static vs instance, declaredonly, etc. This means calling .GetProperty manually is preferable anyway (or creating your own PropertyReflectionConstraint with a DSL that exposes these things.)

2. a: If not, can we remove the ability to skip calling .GetType() on the instance, just for the property value constraint?

   • It's not a breaking change.
     No one has ever used the property value constraint on a System.Type instance before. Due to the as Type special-casing, there is no way for this code to succeed if you pass in a System.Type instance:

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

     Lines 61 to 72 in dba87e5

     	Type actualType = actual as Type;
     	if (actualType == null)
     	actualType = actual.GetType();
     	PropertyInfo property = actualType.GetProperty(name,
     	BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
     	// TODO: Use an error result here
     	if (property == null)
     	throw new ArgumentException(string.Format("Property {0} was not found", name), "name");
     	propValue = property.GetValue(actual, null);

     We are using actual as both the type and the instance. There is no scenario in which that makes sense. It's equivalent to trying to compile the literal C# code typeof(Dog).IsHouseTrained. I've verified that you can only get a System.Reflection.TargetException : Object does not match target type..

   b: And enable the property existence constraint to fall back to calling .GetType() if the property is not found on the instance as Type, for consistency in behavior with a?

   • Con: It'll be ambiguous anytime someone says Has.Property("IsNested") if they pass a typeof(ObjectWithAnIsNestedProperty) and probably still surprise people that System.Type is special-cased.
   • Pro: Since we didn't go with option 1, it's the only remaining way to ever use the property existence constraint on System.Type properties without adding new API to the property constraint DSL.

3. My least favorite: add an OnInstance DSL to allow people to force the .GetType() call on the instance they pass: Assert.That(someType, Has.Property("IsNested").OnInstance.EqualTo(false))

   • Con: It's a whole new API to address an ambiguity that only occurs with System.Type instances (because we didn't go with options 1 or 2).
   • Con: It's awkward and unintuitive and there's a chance that the people that need it may not realize it.
   • Pro: Since we didn't go with options 1 or 2, it's the only remaining way to ever use the property existence constraint or the property value constraint on properties defined in System.Type.

[CharliePoole]

@jnm2 We're missing each other here. I'm still back at trying to confirm there is a problem. Your original example seems to me to confuse the issue by involving a collection constraint, which could be the source of further problems. Therefore I was suggesting you verify there is a problem using a plain property constraint (i.e. try oth PropertyConstraint and PropertyExistsConstraint)

An instance of System.Type is an instance like any other. Obviously, it can be confusing for some users to know whether they are supposed to use the instance or a Type in any syntactic expression and we want to make that easy.

So... what happens if you do the following...

Assert.That(typeof(TopLevelType, Has.Property("IsNested"));
Assert.That(typeof(TopLevelType.NestedType, Has.Property("IsNested"));

The question was not rhetorical. 😄

Also, what happens with

Assert.That(typeof(TopLevelType, Has.Property("IsNested").EqualTo(false));
Assert.That(typeof(TopLevelType.NestedType, Has.Property("IsNested").EqualTo(true));

If those don't all succeed, I'll agree there is an issue.

[jnm2]

@CharliePoole I'm sorry, I must not have answered directly. It's inherent to the property constraints and unrelated to the collection constraint.

Assert.That(typeof(TopLevelType, Has.Property("IsNested"));
Assert.That(typeof(TopLevelType.NestedType, Has.Property("IsNested"));

^ Allows you to check the existence of properties defined on TopLevelType and NestedType.
^ Prevents you from checking the existence of properties defined on System.Type. (Which people almost certainly never do anyway.)

Assert.That(typeof(TopLevelType, Has.Property("IsNested").EqualTo(true));
Assert.That(typeof(TopLevelType.NestedType, Has.Property("IsNested").EqualTo(true));

^ Currently does not work at all for any purpose if the instance you pass in is a System.Type.
See 2a above.

[CharliePoole]

So, has it always been that way or is it a consequence of the recent changes that were intended to bring the two constraints more in line with one another WRT messages?

[CharliePoole]

My usual sequence...

1. Define what the problem is - expected versus actual behavior
2. Figure out if it's a regression - i.e. did it once work and we broke it
3. If it's a regression, back out or fix up the change that broke it
4. If not, think about alternatives

😃

[CharliePoole]

There does seem to be a bug here, as indicated by the last two assertions.

[jnm2]

I've verified that this is the behavior as far back into v2 as we have on NUnit. This is not a regression.

As to 4, that is this comment. 😄

[CharliePoole]

Understood, you were just getting a bit ahead of me there.

[jnm2]

My guess would be that it was an organically overlooked scenario when making property constraints composable to assert against property values.

[CharliePoole]

I think I like your option 1 best, but we should get other comments on it. Can you post an example of an assertion that would be broken by the change?