Assert.That methods should autogenerate message, if null

[FrankDzaebel]

if we use something like: Assert.That(actualBool, Is.EqualTo(expectedBool)); in NUnit version: 3.13.3.0
when they do not match we get the output:

  Expected: True
  But was:  False

With the new CallerArgumentExpressionAttribute it would be easy to get:

  actualBool
  Expected: True
  But was:  False

This would be easier to understand in unit tests. ... and extremly easy to implement.

[FrankDzaebel]

"easy to implement" for example:

    public static void That<TActual>(
        TActual actual,
        IResolveConstraint expression,
        string? message,
        [CallerArgumentExpression(nameof(actual))] string? actualExpression = null,
        params object?[]? args)
    {
        message ??= actualExpression;
        IConstraint constraint = expression.Resolve();
        Assert.IncrementAssertCount();
        ConstraintResult result = constraint.ApplyTo<TActual>(actual);
        if (result.IsSuccess)
            return;
        Assert.ReportFailure(result, message, args);
    }

[manfred-brands]

That way you still need to supply a message. We could put the attribute on the message parameter instead.
See discussions at #3936

[OsirisTerje]

I just had to play around with this, and it actually looks pretty good!
Just to play around I made a new class Verify, with the That method, a bit modified, as I wanted both the actual value and the constraint to be displayed, otherwise it looked like it was missing something. I also used a bit more complex constraints in order to check out if this actually worked or not.

Test code:

[Test]
    public void SingleTest()
    {
        int nunitAge = 23;
        Verify.That(nunitAge, Is.GreaterThan(42).And.LessThan(85));
    }

I got results like:

  Message: 
  Assert.That(nunitAge, Is.GreaterThan(42).And.LessThan(85))
  Expected: greater than 42 and less than 85
  But was:  23

NUnit constraints doers a great job of making a meaningful message, but it is nice to get the complete expression out too.
And I really like that it actually gets the complete constraint out.

If I change the test to use double values, then it makes even more sense:

[Test]
    public void SingleTest2()
    {
        double nunitAge = 23.4;
        Verify.That(nunitAge, Is.GreaterThan(42.0).Within(0.2).And.LessThan(85.4), null);
    }

Result:

  Message: 
  Assert.That(nunitAge, Is.GreaterThan(42.0).Within(0.2).And.LessThan(85.4))
  Expected: greater than 42.0d within 0.20000000000000001d and less than 85.400000000000006d
  But was:  23.399999999999999d

Now it is much more easy to read than what the internal code did, although we could possibly fix that a bit, but with doubles you never know.

Then adding a message to this it works nice too: (Some linefeed/spacing issue but...)

  [Test]
    public void SingleTest3()
    {
        int nunitAge = 23;
        Verify.That(nunitAge, Is.GreaterThan(42).And.LessThan(85),$"The age {nunitAge} fails to meet the objective");
    }

Result:

  Message: 
  The age 23 fails to meet the objective
Assert.That(nunitAge, Is.GreaterThan(42).And.LessThan(85)) 
  Expected: greater than 42 and less than 85
  But was:  23

But where this really shines is when you have a multiple asserts block:

[Test]
    public void Test1()
    {
        int age = 42;
        int employment = 5;
        Assert.That(age, Is.GreaterThan(2));
        Assert.Multiple(() =>
        {
            Verify.That(age, Is.EqualTo(2));
            Verify.That(age, Is.EqualTo(3));
            Verify.That(age,Is.EqualTo(42));
            Verify.That(employment, Is.GreaterThan(55));
            Verify.That(age, Is.LessThan(42));
            Verify.That(age, Is.EqualTo(4));
            Verify.That(employment, Is.EqualTo(55));
        });
    }

Without the CallerArgumentExpression it becomes quite unreadable:

Multiple failures or warnings in test:
  1)   Expected: 2
  But was:  42

  2)   Expected: 3
  But was:  42

  3)   Expected: greater than 55
  But was:  5

  4)   Expected: less than 42
  But was:  42

  5)   Expected: 4
  But was:  42

  6)   Expected: 55
  But was:  5

And this is with only two variables, imagine how this would look with more. Notice also that there are ONE assert that actually works, but it is hard to see that, and the numbers in front doesn't help you, 3-6 is actually asserts 4-7.

Now adding the CallerArgumentExpression :

Multiple failures or warnings in test:
  1)   Assert.That(age, Is.EqualTo(2))
  Expected: 2
  But was:  42

  2)   Assert.That(age, Is.EqualTo(3))
  Expected: 3
  But was:  42

  3)   Assert.That(employment, Is.GreaterThan(55))
  Expected: greater than 55
  But was:  5

  4)   Assert.That(age, Is.LessThan(42))
  Expected: less than 42
  But was:  42

  5)   Assert.That(age, Is.EqualTo(4))
  Expected: 4
  But was:  42

  6)   Assert.That(employment, Is.EqualTo(55))
  Expected: 55
  But was:  5

The code for the Verify.That, modified from @FrankDzaebel code above:

public class Verify
{
    public static void That<TActual>(
            TActual actual,
            IResolveConstraint expression,
            string? message=null,
            [CallerArgumentExpression("actual")] string? actualExpression = null,
            [CallerArgumentExpression("expression")] string? constraintExpression = null,
            params object?[]? args)
    {
        var expressionMessage = "Assert.That(" + actualExpression + ", " + constraintExpression + ")";
        string msg = message != null ? $"{message}\n{expressionMessage} " : expressionMessage;
        var constraint = expression.Resolve();
        // Assert.IncrementAssertCount();
        var result = constraint.ApplyTo(actual);
        if (result.IsSuccess)
            return;
        MessageWriter writer = new TextMessageWriter(msg, args);
        result.WriteMessageTo(writer);
        Assert.Fail(writer.ToString(), args);
    }
}

Based on this and the discussions in #3936 I think this idea has merit. There are surely something that might cause issues, but it should be possible to work around.

The sequence of the different messages needs to be changed though, as seen here: Only the first test use the Assert, the other use the Verify, and the Verify ones also needs to output the "real message" first, so that is displayed first.

Code for the issue is here: https://github.com/nunit/nunit3-vs-adapter.issues/tree/master/Issue4413And3936
(I know, wrong repo, I'll move it later)

@nunit/framework-team : What do you guys think?

[manfred-brands]

That attribute is great, but as mentioned in #3936 it is the backward compatibility we have an issue with.

A slightly different example would no longer compile:

  [Test]
    public void SingleTest3()
    {
        int nunitAge = 23;
        Assert.That(nunitAge, Is.GreaterThan(42).And.LessThan(85), "The age {0} fails to meet the objective", nunitAge);
    }

This is because the two extra arguments before the params args and I'm not sure that specifying null would result in the caller's expression. The spec isn't clear here and I don't have access to a PC to verify.

If the argument for {0} was a string that would be assigned to the actual parameter resulting in a runtime error when formatting the message.

Lots of potential, but not straightforward.

[OsirisTerje]

You're right this is incompatible. Adding nulls to the CAE results in nothing.
But, using the string with args is very old, and replacing with interpolated strings should be done. I assume some still use it, but version 4 will have breaking changes, so this could be one of those.
I tried added an overload to cover it, and that works, but then the CAE dont return anything useful.

[stevenaw]

I haven't been able to experiment in front of a computer either, but I like this idea myself, it feels like a nice usability improvement. I agree that it might be ok to introduce a small breaking change to make this happen, but I feel like we should try to minimize it. It would be nice if it "just worked" and we could avoid having to explicitly pass a null value here, but without having a computer in front of me it is tough to understand how feasible this is

[OsirisTerje]

I tried with a separate method for the params, and that works, but then you don't have the CAE there. That would work for backwards compatibility I think.

public static void That<TActual>(
        TActual actual,
        IResolveConstraint expression,
        string? message = null,
        params object?[]? args)
    {
        var msg = args is not null && args.Length > 0 && message != null ? string.Format(message, args) : message;
        That(actual, expression, msg);
    }

It compiles and it works as it should, only picking up the ones with param, even if the signature looks equal to me, so the CAE seems to have some preference. Could possibly also remove the null defaults for both the message and the params, since we only want this when there actually IS a params there.

[OsirisTerje]

There are two params issues which are troublesome:

1. When having a Assert.That(somebool,"My message of {0}",somebool) , this will have two possible overloads, so it will be a compile error
2. When having the params as a string
   
   The param object will be interpreted as the value of the actualExpression, and we will see the name and not the value of that.

No.1 here is breaking badly, no.2 will raise confusions.

But these two are exceptions really, and outside these two, the params may work. We can let them be there, and mark the explicit methods for them as obsolete warnings.

[manfred-brands]

The params will never work as it requires explicitly specifying the expression parameters.
If we say that all newer code will use the interpolated string syntax we don't need them anymore. Potentially we can make a rule in the analyzer to convert code that uses {0} syntax to $"{argument}".

[OsirisTerje]

If we say that all newer code will use the interpolated string syntax we don't need them anymore.

Agree

Potentially we can make a rule in the analyzer to convert code that uses {0} syntax to $"{argument}"

Agree

By creating an overload that takes params and no CAE, it will actually cover everything that is NOT a string, but, it is useless, as using strings will fail, so, I agree with @manfred-brands - let us drop the params.

[OsirisTerje]

@FrankDzaebel Do you agree the solution outlined also solves your issue?