Improve error message when verification fails

[bfriesen]

I would like to improve the verification error message for these two specific scenarios:

1. Currently, if you have a variable of type LambdaExpression, and you pass it in to the It.Is<TValue> static method, and verification fails, then the formatting of the lambda expression in the exception message isn't as good as it should be. Specifically, the representation of the lambda expression in the message is poor because ToString() has been called on it instead of the friendly expression formatting routine.

   var mock = new Mock<IFoo>();
   
   // Declare a variable to hold the lambda expression.
   Expression<Func<string, bool>> nullOrEmpty = s => string.IsNullOrEmpty(s);
   
   // Pass the lambda expression variable to `It.Is`.
   mock.Setup(x => x.Execute(It.Is(nullOrEmpty)))
       .Returns("ack")
       .Verifiable();
   
   var mex = Assert.Throws<MockException>(() => mock.Verify());
   Assert.Contains(@".Execute(It.Is<string>(s => string.IsNullOrEmpty(s)))", mex.Message);

   The second assertion fails with the message (nobody wants to see "<>c__DisplayClass" - yuck!):

   This mock failed verification due to the following:
   
         VerifyFixture.IFoo x => x.Execute(It.Is<string>(s => IsNullOrEmpty(s))):
         This setup was not matched.
   

   Note the lambda expression s => IsNullOrEmpty(s), when it should be s => string.IsNullOrEmpty(s). There seems to be a difference in how a lambda expression is constructed depending on whether it is captured in a variable or not. When captured in a variable, the overall expression contain a ConstantExpression which has a value of type LambdaExpression - this is the actual lambda expression that we want. However, currently, the value of all constant expressions is basically formatted by calling ToString() on them.

2. Assuming scenario 1 (where we have a variable of type lambda expression passed to It.Is), if a lambda expression captures a local variable as a closure, then we should not include the ugly name of the closure class (beginning with "<>c__DisplayClass") in the verification failed message.

   var mock = new Mock<IFoo>();
   
   string password = "abc123";
   
   Expression<Func<string, bool>> matchingPassword = s => s == password;
   
   mock.Setup(x => x.Execute(It.Is(matchingPassword)))
       .Returns("ack")
       .Verifiable();
   
   var mex = Assert.Throws<MockException>(() => mock.Verify());
   Assert.Contains(@".Execute(It.Is<string>(s => s == password)", mex.Message);

   The second assertion fails with the message (nobody wants to see "<>c__DisplayClass" - yuck!):

   This mock failed verification due to the following:
   
      VerifyFixture.IFoo x => x.Execute(It.Is<string>(s => s == VerifyFixture.<>c__DisplayClass5_0.password)):
      This setup was not matched.
   

I have a branch containing these tests, along with a fix for each. If this is something you're interested in, I'll make a pull request. I do have one question though: for the purpose of the updating the changelog, is this an 'Added', 'Changed' or 'Fixed'?

[stakx]

If this is something you're interested in, I'll make a pull request.

Your contribution would be welcome @bfriesen!

I haven't yet had time to study your code in detail, but I expect that some further simplifications would be possible. To give you a few pointers:

1. We have a helper extension method stringBuilder.AppendNameOf for appending type names to a StringBuilder. (That in turn is based on the extension methods in the TypeNameFormatter namespace.)

2. We have an expression tree rewriter EvaluateCaptures in the Moq.Expressions.Visitors namespace for evaluating captured variables to their values (i.e. the "display class" bit). I suppose you might be able to hook that up at the appropriate places inside the appropriate stringBuilder.AppendExpression method(s). (Something like horribleLookingExpression.Apply(EvaluateCaptures.Rewriter), IIRC.)

3. Regarding static members, it may be good to distinguish between (a) regular static methods, where the class name should be included; (b) extension methods, where extension method syntax should be used, and (c) enum fields, where the enum type name should perhaps be included (but it's probably not as important as with case (a)).

Do you think you can simplify your code further using these hints? I'll do an in-depth code review on your PR (if you choose to submit one).

I do have one question though: for the purpose of the updating the changelog, is this an 'Added', 'Changed' or 'Fixed'?

I'd probably go with "changed" and provide a high-level description like "improved error message formatting of ...".

[bfriesen]

1. We have a helper extension method stringBuilder.AppendNameOf for appending type names to a StringBuilder. (That in turn is based on the extension methods in the TypeNameFormatter namespace.)

I saw that extension method, but I don't think it's relevant here, since I'm trying to omit the name of the closure type, not format it better.

2. We have an expression tree rewriter EvaluateCaptures in the Moq.Expressions.Visitors namespace for evaluating captured variables to their values (i.e. the "display class" bit). I suppose you might be able to hook that up at the appropriate places inside the appropriate stringBuilder.AppendExpression method(s). (Something like horribleLookingExpression.Apply(EvaluateCaptures.Rewriter), IIRC.)

This looks similar to what I'm trying to accomplish. I'll look into this in greater detail later tonight/tomorrow and see if it could simplify my changes.

3. Regarding static members, it may be good to distinguish between (a) regular static methods, where the class name should be included; (b) extension methods, where extension method syntax should be used, and (c) enum fields, where the enum type name should perhaps be included (but it's probably not as important as with case (a)).

I don't think this is relevant either, since the captured local variables appear as instance fields of the compiler-generated closure class.

[bfriesen]

I think actually using EvaluateCaptures isn't necessary and would be overkill. However, the way it detects closure member access is definitely superior to the way I'd done it (ce.Type.IsDefined(typeof(CompilerGeneratedAttribute)) vs ce.Type.Name.StartsWith("<>c__DisplayClass")). So I've updated my branch accordingly. I'll update the changelog and make a PR shortly...

[stakx]

Great work! Thanks for your contribution!

[stakx]

P.S.:

However, the way it detects closure member access is definitely superior to the way I'd done it (ce.Type.IsDefined(typeof(CompilerGeneratedAttribute)) vs ce.Type.Name.StartsWith("<>c__DisplayClass")).

I agree that the former check is nicer and means we won't need to implement a parser for those generated names. 🙂 And that particular check has worked well in practice, so far. That being said, here's some trivia you may find of interest:

While the former check may look nicer, the latter is potentially more accurate. As you may know, those horrible-looking generated names actually have a specific format (see this comment in the Roslyn compiler source) and the >c__DisplayClass bit is used specifically with lambdas (see this enum). (Eric Lippert, who used to be on the C# compiler team, stated here that "display classes" should probably have been called "closure classes"; that's what they really are.)

On the other hand, [CompilerGenerated], is used in several situations, and it doesn't carry any additional metadata explaining what source code pattern led to it being present.