Inutile model validation error message

[AlexArchive]

[Test]
public void Post_ReturnsCorrectModel()
{
    const string Slug = "abc";
    repository
       .Setup(repo => repo.Find(Slug))
       .Returns(PostsMother.CreatePost(withSlug: Slug));

    controller
        .WithCallTo(c => c.Post(Slug))
        .ShouldRenderDefaultView()
        .WithModel<Post>(actual => actual.Slug == "something random");
}

The aforementioned code produces the following error message -

Expected view model to pass the given condition, but it failed.

In practice I found this error message to be completely unhelpful. Instead I suggest something along the lines of -

Expected "abc" but instead was "something random".

[robdmoore]

Cool I learnt a new word - inutile!!

[robdmoore]

Doing an error message like that is tricky because it requires us to start parsing and understanding the lambda expression. If a message like that is needed then the overload that takes a lambda expression can be used with your assertion library of choice, e.g.:

 controller
        .WithCallTo(c => c.Post(Slug))
        .ShouldRenderDefaultView()
        .WithModel<Post>(actual => {Assert.That(actual.Slug, Is.EqualTo("something random"); });

In saying that, I think there are a couple of things we can do to make the error message nicer (thoughts?):

• Print out the value of .ToString() or maybe JSONConvert.SerializeObject(x)? on the object in the error message
• Print out the value of .ToString() on the lambda expression

Then you might get an error that looks something like this for the above example:

Expected view model {Slug: "abc", Title: "A post"} to pass the given condition (actual => actual.Slug == "something random"), but it failed.

[AlexArchive]

I must confess that using an assertion library in that way did not occur to me at the time of writing and so cheers for that one :)

---

Personally I am in high favour of making the error messages nicer as I find the approach that you describe to be constructive but also unfluent.

I am probably naïve, but having explored the expression API and considered our requirements - I feel as though outputting the error message that I proposed initially to be feasible.

This is an evenhanded example of what I have in mind :

internal class ExceptionPropagator : ExpressionVisitor
{
    private readonly object model;
    private readonly ICollection<ParameterExpression> parameters;

    internal ExceptionPropagator(object model, ICollection<ParameterExpression> parameters)
    {
        this.model = model;
        this.parameters = parameters;
    }

    protected override Expression VisitBinary(BinaryExpression node)
    {
        var actual = Expression
            .Lambda(node.Left, parameters)
            .Compile()
            .DynamicInvoke(model);

        var expected = Expression
            .Lambda(node.Right, parameters)
            .Compile()
            .DynamicInvoke(model);

        var memberName = ((MemberExpression) node.Left).Member.Name;

        throw new Exception(string.Format(
            "Expected member {0} to be \"{1}\", but instead got \"{2}\".",
            memberName, 
            expected, 
            actual));
    }

    protected override Expression VisitConstant(ConstantExpression node)
    {
        throw new Exception("Expected "False", but instead got "True".");
    }
}
...

public void WithModel(Expression<Func<TModel, bool>> predicate)
{
    if (predicate.Compile()((TModel) model)) return;

    var propagator = new ExceptionPropagator(model, predicate.Parameters);
    propagator.Visit(predicate);
}

There should be much more conditional checking for the sad path (what should happen if the operator is one other than ==?) but this demonstrates that the foundation does not have to be that complicated.

We should at least support binary and constant expressions and then perhaps fall back on to your proposes error message.

I am wondering if you can identify any evident problems that we will encounter?

I also wonder if this is something that you even want for the library?

[robdmoore]

If you have a nicer assertion library like, say, Shoudly then it's a lot nicer with the lambda, e.g.;

controller
    .WithCallTo(c => c.Post(Slug))
    .ShouldRenderDefaultView()
    .WithModel<Post>(actual => actual.Slug.ShouldBe("something random"));

That aside, what if people have the condition as: .WithModel<Post>(actual => actual.Slug == "something random" && actual.Title == "something else random");. That's the problem with expression trees - there are so many different things you can do. We certainly can create an expression tree visitor and be really clever, but then there is a bunch of expression parsing code that doesn't make sense to most people when the rest of the codebase is actually really simple, plus it feels like we are reinventing the wheel.

So what I'm thinking is either:

• Your suggestion of supporting a few well-defined and simple expression types and falling back to my idea of JSON'ing the model and .ToString'ing the lambda for the other cases
  • Possibly we can add a .And method that allows you to perform multiple simple statements, e.g.: .WithModel<Post>(actual => actual.Slug == "something random").And(actual.Title == "something else random"); - wouldn't work for an or statement, but I suspect most of the time people wouldn't need that
• We add methods that chain off WithModel that allow for checking model properties, e.g. .WithModel<Post>().AndModelProperty(m => m.Slug).ThatEquals("something random") - this is a lot more verbose though so I don't think it's a great idea
• We pull an assertion library as a dependency that can take the expression and assert it while outputting a nice error message when it fails

Thoughts?

[mwhelan]

I agree that it would be nice to have a less generic error message. I also like that you draw a boundary around the scope of what this library covers and, at a certain point, hand off to dedicated assertion libraries. Of the suggestions so far, I prefer the simpler ones, such as serializing the model to JSON, and letting users use another assertion library for more complex assertions. I wouldn't fancy you taking a dependency on an assertion library. I also don't think you need to get into chaining model properties.

[AlexArchive]

Cheers for your comments both.

My prefered resolution is now the one that Rob suggested initially:

• Print out the value of .ToString() or maybe JSONConvert.SerializeObject(x)? on the object in the error message
• Print out the value of .ToString() on the lambda expression

If this approach works well, then there will be no need to introduce unnecessary complexity.

If it does not work well in any situation - we can always revisit our options.

Are we in agreement?

[robdmoore]

Sounds like a good initial approach :
)

On 10 Sep 2014, at 12:18 am, ByteBlast [email protected] wrote:

Cheers for your comments both.

My prefered resolution is now the one that Rob suggested initially:

Print out the value of .ToString() or maybe JSONConvert.SerializeObject(x)? on the object in the error message
Print out the value of .ToString() on the lambda expression
If this approach works well, then there will be no need to introduce unnecessary complexity.

If it does not work well in any situation - we can always revisit our options.

Are we in agreement?

—
Reply to this email directly or view it on GitHub.

[AlexArchive]

Since I implemented the basic ExpressionInspector and consistently, improved the error messages, I have no longer observed this problem.

There are still some short-comings when it comes to compiler-generated code (like closure display classes) but personally, I have not found this to be an issue in practice.

The good news is, if the problem does become prevalent in the future

1. We have an extensive test suite in place for the ExpressionInspector to catch regressions
2. I have done some research into possible solutions.

I am happy to close this issue now if you are, Rob?

[robdmoore]

Yep :)

Great work on this - stuff like this makes the library way nicer to use!!

On 18 Jan 2015, at 5:48 am, ByteBlast [email protected] wrote:

Since I implemented the basic ExpressionInspector and consistently, improved the error messages, I have no longer observed this problem.

There are still some short-comings when it comes to compiler-generated code (like closure display classes) but personally, I have not found this to be an issue in practice.

The good news is, if the problem does become prevalent in the future

We have an extensive test suite in place for the ExpressionInspector to catch regressions
I have done some research into possible solutions.
I am happy to close this issue now if you are, Rob?

—
Reply to this email directly or view it on GitHub.