DelegatingConstraintResult does not use its _innerResult for WriteMessageTo

[metoule]

The DelegatingConstraintResult should override the WriteMessageTo to call the _innerResult.WriteMessageTo method, otherwise it's not writing the expected message to the output.

WriteActualValueTo and WriteAdditionalLinesTo are already properly forwarded to the _innerResult.

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

Lines 369 to 382 in 6a89737

	private class DelegatingConstraintResult : ConstraintResult
	{
	private readonly ConstraintResult _innerResult;
	public DelegatingConstraintResult(IConstraint constraint, ConstraintResult innerResult)
	: base(constraint, innerResult.ActualValue, innerResult.Status)
	{
	_innerResult = innerResult;
	}
	public override void WriteActualValueTo(MessageWriter writer) => _innerResult.WriteActualValueTo(writer);
	public override void WriteAdditionalLinesTo(MessageWriter writer) => _innerResult.WriteAdditionalLinesTo(writer);
	}

[metoule]

Repro steps:

[TestFixture]
public class MyTests
{
    [Test]
    public void ShouldWriteSameMessage()
    {
        var actualValue = false;
        Assert.Multiple(() =>
        {
            Assert.That(actualValue, new MyConstraint());
            Assert.That(actualValue, new MyConstraint().After(20));
        });
    }

    public class MyConstraint : Constraint
    {
        public MyConstraint()
        {
            Description = "Custom description";
        }

        public override ConstraintResult ApplyTo<TActual>(TActual actual)
        {
            return new MyConstraintResult(this);
        }
    }

    public class MyConstraintResult : ConstraintResult
    {
        public override void WriteMessageTo(MessageWriter writer)
        {
            writer.Write("Custom message");
        }

        public MyConstraintResult(MyConstraint myConstraint) : base(myConstraint, actualValue: null, ConstraintStatus.Failure)
        {
        }
    }
}

The output is not what I'd expect:

Message: 
Multiple failures or warnings in test:
  1) Custom message 
  2)   Expected: Custom description after 20 milliseconds delay
  But was:  null

The output of the delayed constraint uses the constraint's description, but ignores the custom WriteMessageTo of the constraint result.

[stevenaw]

Thanks for the clear repro @metoule . I've uploaded a sample of it to our issues repo: https://github.com/nunit/nunit.issues/tree/main/Issue4545

I can confirm as well that it happens for both NUnit 3.14 and 4.0 beta1

[stevenaw]

@metoule Is this something you'd be interested in submitting a PR to fix?

[metoule]

I originally wanted to open a PR, because that looked like a quick fix, but it's surprisingly tricky. You can't simply modify the DelegatingConstraintResult to call the inner result's WriteMessageTo method, because if there's no WriteMessageTo override, then the delayed constraint's description won't be used and the delay information is lost.

One possible solution, but that would completely change the display of a delayed constraint, is to always output the delay before writing the message. Something like:

Before:

Expected: True after 500 milliseconds delay
But was:  0

After:

After 500 milliseconds delay:
Expected: True 
But was:  0

[stevenaw]

Thanks for the investigation and analysis @metoule
Looking at the original PR, the DelayedConstraintResult was added and seems to have overridden all methods except WriteMessageTo in order to preserve the original result (#3547).

Are you able to explain a bit more about how you came across this issue, and if you've only seen it when used in conjunction with other custom constraints?

[metoule]

My goal is to wrap the GregFinzer/Compare-Net-Objects library in a NUnit constraint. That library performs a deep equality check, and returns a user friendly output that looks like this:

Displaying 1 differences out of max 1
      Types [Int32,Int32], Item Expected.Age != Actual.Age, Values (42,45)

It makes it easy to compare complex objects in one go, instead of one property at a time, but it's doesn't really match the NUnit assumption that there's an expected and an actual value, because everything is compared at once.

To use it in NUnit, I use the ContraintResult.WriteMessageTo to output the library's output. The code looks like this:

using KellermanSoftware.CompareNetObjects;
using NUnit.Framework.Constraints;

public class CompareNetObjectsConstraint<TExpected> : EqualConstraint
{
    private readonly TExpected _expected;

    public CompareNetObjectsConstraint(TExpected expected) : base(expected)
    {
        _expected = expected;
    }

    public override ConstraintResult ApplyTo<TActual>(TActual actual)
    {
        var comparisonResult = new CompareLogic().Compare(_expected, actual);
        return new CompareNetObjectsConstraintResult<TExpected>(this, actual, comparisonResult);
    }
}

public class CompareNetObjectsConstraintResult<TExpected> : EqualConstraintResult
{
    private readonly ComparisonResult _comparisonResult;

    public CompareNetObjectsConstraintResult(CompareNetObjectsConstraint<TExpected> constraint, object? actual, ComparisonResult comparisonResult)
        : base(constraint, actual, comparisonResult.AreEqual)
    {
        _comparisonResult = comparisonResult;
    }

    public override void WriteMessageTo(MessageWriter writer)
    {
        writer.WriteMessageLine($"Showing {_comparisonResult.Differences.Count} differences (max: {_comparisonResult.Config.MaxDifferences})");
        foreach (var difference in _comparisonResult.Differences)
        {
            writer.WriteMessageLine(difference.ToString());
        }
    }
}

For my own purpose, I found a way to make it work though:

1. Override the constraint's Description to "No differences"
2. Keep overriding the WriteMessageTo method, for the normal Assert
3. Add an override for WriteActualValueTo, that calls the WriteMessageTo method, for the DelayedContraint

That way, I have:

# normal Assert
Showing 1 differences (max: 1)
  Types [Int32,Int32], Item Expected.Age != Actual.Age, Values (42,45)

# Delayed constraint
Expected: No differences after 10 milliseconds delay
But was:    Showing 1 differences (max: 1)
  Types [Int32,Int32], Item Expected.Age != Actual.Age, Values (42,45)

[stevenaw]

Thank you for the thorough repro and information @metoule ! I haven't been able to get to a computer for an extended time to dig into it with the latest info, but I'm glad to hear you've been able to make this work on your end.
I'll try to follow up again once I can investigate a bit more

[OsirisTerje]

@metoule

My goal is to wrap the GregFinzer/Compare-Net-Objects library in a NUnit constraint.

If you intend this for a PR for NUnit, please note that we are not likely to accept 3rd party dependencies, except in a very few cases, and after a deep discussion on the necessity of it.

[metoule]

@OsirisTerje, thanks for the information. Rest assured that I only want to create a new constraint for my own use: this is quite niche and certainly not something I'd even consider being included in NUnit. I just wanted to illustrate how I use the WriteMessageTo method.

[stevenaw]

I've had a bit less time than expected lately but I've tried taking a look at this a few times now to try to understand it. I think the issue here may be a general extensibility one. Most of NUnit's built-in constraints operate or compose together by appending lines to the output but the DelegatingConstraintResult seems to append to an existing line of output making it a bit harder to compose with custom constraints if they must override WriteMessageTo() themselves.

I think @metoule 's idea here to change the output to have the "delay" information appear on its own line is a good solution to the problem if we wanted to pursue it. @nunit/framework-team what are your thoughts?

[manfred-brands]

@stevenaw I agree that @metoule idea is sound. First write out the result from the DelayConstraint, followed by the result of whatever constraints was used.

[mikkelbu]

Sounds good to me