PropertyConstraint throws away the more helpful message in the base constraint result

[jnm2]

Problem

PropertyConstraint keeps the base constraint description but throws away the actual comparison message and just prints the actual value.

Starting with a familiar example, Is.EquivalentTo:

var actual = new SomeClass { SomeProperty = { 2, 3, 5, 7 } };
Assert.That(actual.SomeProperty, Is.EquivalentTo(new List<int> { 2, 3, 5, 8 }));

Expected: equivalent to < 2, 3, 5, 8 >
But was: < 2, 3, 5, 7 >
Missing (1): < 8 >
Extra (1): < 7 >

But as soon as you compose it with PropertyConstraint:

var actual = new SomeClass { SomeProperty = { 2, 3, 5, 7 } };
Assert.That(actual, Has.Property("SomeProperty").EquivalentTo(new List<int> { 2, 3, 5, 8 }));

Expected: equivalent to < 2, 3, 5, 8 >
But was: < 2, 3, 5, 7 >

In that example, just printing the actual value is still pretty okay.
But when the value is a complex object that doesn't .ToString() to something you care about, you get something like this:

Expected: property SomeProperty [long description from custom constraint]
But was: <Appearance>

Assert.That(new SomeClass(), Has.Property("SomeComplexThing")
    .SomeCustomConstraint(expectedComplexThing));

Proposal

Here you can see the constraint, which understands the semantics of the actual value, getting thrown away. Only the actual value and success bool are retained:

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

Lines 73 to 74 in bf8f2dc

	var baseResult = BaseConstraint.ApplyTo(propValue);
	return new ConstraintResult(this, baseResult.ActualValue, baseResult.IsSuccess);

Rather than the existing code below, I think we should have an internal-for-now ChainedConstraintResult class which delegates all its writing to the base constraint result:

internal class ChainedConstraintResult : ConstraintResult
{
    private readonly ConstraintResult _baseResult;

    public ChainedConstraintResult(IConstraint constraint, ConstraintResult baseResult) : base(constraint, baseResult.ActualValue, baseResult.Status)
    {
        _baseResult = baseResult;
    }

    public override void WriteActualValueTo(MessageWriter writer)
    {
        _baseResult.WriteActualValueTo(writer);
    }
}

PropertyConstraint would then do this:

var baseResult = BaseConstraint.ApplyTo(propValue);
return new ChainedConstraintResult(this, baseResult);

[CharliePoole]

All our other "chained" constraints produce a single result because that's what the Assert methods expect to deal with. Internally, they look at the nested constraint in producing their result. It seems like this was missed in PropertyConstraint.

IMO, a linked chain of constraint results gives Assert too much to do. Note that whatever Assert needs to do, Assume, Warn and any other similar classes also have to do. Having a single ConstraintResult was intended to simplify things for those upstream classes. The idea was that ConstraintResult should be the class that knows how to "flatten" the chain or tree of nested Constraints.

[jnm2]

@CharliePoole I don't understand how this impacts Assert and the rest. They still get handed a single ConstraintResult. They don't know that it is a ChainedConstraintResult, and even if they did, they can't access the chain.

The chain is retained as a private implementation detail because otherwise the logic for writing the message is not retained.

[CharliePoole]

See my edit of the first comment.

Assert etc. need a "flattened" result. ConstraintResult or it's derivative is supposed to provide it. ConstraintResult already has access to a chain of Constraints to allow it to produce that result. Creating a chain of ConstraintResults seems like an unnecessary level of complexity to me, unless there is something we can do with a chain of results that we can't do with a chain of constraints.

Taking this further, would you implement a chain of results for every constraint that uses a base constraint? For binary constraints? I'm for it if it enables some feature we want to implement, although even then I'd prefer to just fix the bug before coming back and rewriting all constraints.

[jnm2]

@CharliePoole I guess I'm just not seeing a simpler way to flatten the results than the one I proposed. But I'm now very interested to see one. 😃

[jnm2]

AndConstraint does the exact same thing I'm proposing for PropertyConstraint!

Hmm, I see that I need to make a correction though. I should only be delegating WriteActualValueTo to the base constraint, not WriteMessage, or otherwise the property's prefix on the message would never be shown. Nor would anything higher up.

Then if I make ChainedConstraintResult nested and private and name it PropertyConstraintResult on the assumption that it won't see any reuse either directly or as a base class, then I’ve exactly duplicated your technique with the binary operators.

But maybe that was your point all along?

[CharliePoole]

Maybe I misunderstood. Don't we already have PropertyConstraintResult?

[CharliePoole]

I did misunderstand...

Currently we have chained constraints but only one result is produced for the chain.

My understanding was that you would create a mesh like this...

ConstraintResult ==> NestedResult1     ==> NestedResult2     ==> etc.
     |                     |                     |
     V                     V                     V
Constraint       ==> NestedConstraint1 ==> NestedConstraint2 ==> etc.

I'm totally in favor of doing it like AndConstraint. See also EqualConstraint.

[jnm2]

Ah, okay. We do not have a PropertyConstraintResult.

So what do you think of keeping it as an internal ChainedConstraintResult class to be directly used or extended for use in constraints with exactly one base constraint? Meh?

[CharliePoole]

Personally I like a nested class that is tailored for exactly the constraint in which it is nested. If you create a reusable class, then you're almost certain to run into cases where it needs to test what it's being used for and do something different. Either way the code is minimal though, so it can be changed later if that happens.

[jnm2]

Okay, let's go that route then:

class PropertyConstraint
{
    private sealed class PropertyConstraintResult : ConstraintResult
    {
        private readonly ConstraintResult _baseResult;

        public PropertyConstraintResult(IConstraint constraint, ConstraintResult baseResult) : base(constraint, baseResult.ActualValue, baseResult.Status)
        {
            _baseResult = baseResult;
        }

        public override void WriteActualValueTo(MessageWriter writer)
        {
            _baseResult.WriteActualValueTo(writer);
        }
    }
}

[jnm2]

Notes to implementors

All we need is the class defined above and to use return new PropertyConstraintResult(this, baseResult); instead of this line:

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

Line 74 in bf8f2dc

return new ConstraintResult(this, baseResult.ActualValue, baseResult.IsSuccess);

And we need a test that fails before and succeeds afterwards, confirming that PropertyConstraint.ApplyTo(...).WriteActualValueTo calls the base constraint's result's WriteActualValueTo.