Provide better feedback when call matcher throws

[Mertsch]

Please consider the following code and comments on it. As you can see in the examples, an exception in rule building makes finding the issue very hard, since you have no debug info at all, that is usually very rich with FIE

using System;
using FakeItEasy;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace FakeItEasyBugs
{
    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void EverythingOkTest()
        {
            ICanDoSomething fake = A.Fake<ICanDoSomething>();
            this.myApiCallGetsDataFromSomewhere(fake);
            A.CallTo(() => fake.CallMe(A<JObject>.That.Matches(o =>
                o["SomeName"].ToObject<string>() == "TestValue"))).MustHaveHappenedOnceExactly();
        }

        /// <summary>
        /// Test method FakeItEasyBugs.UnitTest1.SlightChanceToFindTheErrorTest threw exception: 
        /// FakeItEasy.ExpectationException: 
        /// 
        /// Assertion failed for the following call:
        /// FakeItEasyBugs.ICanDoSomething.CallMe(o: &lt;o =&gt; (o.get_Item("SomeName").ToObject() == "WrongValue")&gt;)
        /// Expected to find it once exactly but didn't find it among the calls:
        /// 1: FakeItEasyBugs.ICanDoSomething.CallMe(o: [[[]]])
        /// 
        /// 
        /// at FakeItEasy.Core.FakeAsserter.AssertWasCalled(Func`2 callPredicate, Action`1 callDescriber, CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Core\FakeAsserter.cs:line 41
        /// at FakeItEasy.Configuration.RuleBuilder.MustHaveHappened(CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Configuration\RuleBuilder.cs:line 174
        /// at FakeItEasyBugs.UnitTest1.SlightChanceToFindTheErrorTest()
        /// </summary>
        [TestMethod]
        public void SlightChanceToFindTheErrorTest()
        {
            ICanDoSomething fake = A.Fake<ICanDoSomething>();
            this.myApiCallGetsDataFromSomewhere(fake);
            A.CallTo(() => fake.CallMe(A<JObject>.That.Matches(o =>
                    o["SomeName"].ToObject<string>() == "WrongValue")))
                .MustHaveHappenedOnceExactly();
        }

        /// <summary>
        /// Test method FakeItEasyBugs.UnitTest1.HaveNoIdeaWhatGoesWrongTest threw exception: 
        /// System.NullReferenceException: Object reference not set to an instance of an object.
        /// at lambda_method(Closure , JObject )
        /// at System.Linq.Enumerable.All[TSource](IEnumerable`1 source, Func`2 predicate)
        /// at FakeItEasy.Configuration.RuleBuilder.RuleMatcher.Matches(IFakeObjectCall call) in C:\projects\fakeiteasy\src\FakeItEasy\Configuration\RuleBuilder.cs:line 322
        /// at System.Linq.Enumerable.Count[TSource](IEnumerable`1 source, Func`2 predicate)
        /// at FakeItEasy.Core.FakeAsserter.AssertWasCalled(Func`2 callPredicate, Action`1 callDescriber, CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Core\FakeAsserter.cs:line 34
        /// at FakeItEasy.Configuration.RuleBuilder.MustHaveHappened(CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Configuration\RuleBuilder.cs:line 172
        /// at FakeItEasyBugs.UnitTest1.HaveNoIdeaWhatGoesWrongTest()
        /// </summary>
        [TestMethod]
        public void HaveNoIdeaWhatGoesWrongTest()
        {
            ICanDoSomething fake = A.Fake<ICanDoSomething>();
            this.myApiCallGetsDataFromSomewhere(fake);
            A.CallTo(() => fake.CallMe(A<JObject>.That.Matches(o =>
                    o["Misspelled"].ToObject<string>() == "TestValue")))
                .MustHaveHappenedOnceExactly();
        }

        /// <summary>
        /// Test method FakeItEasyBugs.UnitTest1.TryingToHackSomeDebugInfoTest threw exception: 
        /// FakeItEasy.ExpectationException: 
        /// 
        /// Assertion failed for the following call:
        /// FakeItEasyBugs.ICanDoSomething.CallMe(o: &lt;{
        /// "SomeName": "TestValue"
        /// }&gt;)
        /// Expected to find it once exactly but didn't find it among the calls:
        /// 1: FakeItEasyBugs.ICanDoSomething.CallMe(o: [[[]]])
        /// 
        /// 
        /// at FakeItEasy.Core.FakeAsserter.AssertWasCalled(Func`2 callPredicate, Action`1 callDescriber, CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Core\FakeAsserter.cs:line 41
        /// at FakeItEasy.Configuration.RuleBuilder.MustHaveHappened(CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Configuration\RuleBuilder.cs:line 174
        /// at FakeItEasyBugs.UnitTest1.TryingToHackSomeDebugInfoTest()
        /// </summary>
        [TestMethod]
        public void TryingToHackSomeDebugInfoTest()
        {
            ICanDoSomething fake = A.Fake<ICanDoSomething>();
            JObject fakeParamRecieved = null;
            A.CallTo(() => fake.CallMe(A<JObject>.Ignored)).Invokes((JObject o) => fakeParamRecieved = o);
            this.myApiCallGetsDataFromSomewhere(fake);
            A.CallTo(() => fake.CallMe(A<JObject>.That.Matches(o =>
                    o["SomeName"].ToObject<string>() == "WrongValue", io => io.Write(JsonConvert.SerializeObject(fakeParamRecieved, Formatting.Indented)))))
                .MustHaveHappenedOnceExactly();
        }

        /// <summary>
        /// Test method FakeItEasyBugs.UnitTest1.MyDebugInfoIsCompletelyLostTest threw exception: 
        /// System.NullReferenceException: Object reference not set to an instance of an object.
        /// at lambda_method(Closure , JObject )
        /// at System.Linq.Enumerable.All[TSource](IEnumerable`1 source, Func`2 predicate)
        /// at FakeItEasy.Configuration.RuleBuilder.RuleMatcher.Matches(IFakeObjectCall call) in C:\projects\fakeiteasy\src\FakeItEasy\Configuration\RuleBuilder.cs:line 322
        /// at System.Linq.Enumerable.Count[TSource](IEnumerable`1 source, Func`2 predicate)
        /// at FakeItEasy.Core.FakeAsserter.AssertWasCalled(Func`2 callPredicate, Action`1 callDescriber, CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Core\FakeAsserter.cs:line 34
        /// at FakeItEasy.Configuration.RuleBuilder.MustHaveHappened(CallCountConstraint callCountConstraint) in C:\projects\fakeiteasy\src\FakeItEasy\Configuration\RuleBuilder.cs:line 172
        /// at FakeItEasyBugs.UnitTest1.MyDebugInfoIsCompletelyLostTest()
        /// </summary>
        [TestMethod]
        public void MyDebugInfoIsCompletelyLostTest()
        {
            ICanDoSomething fake = A.Fake<ICanDoSomething>();
            JObject fakeParamRecieved = null;
            A.CallTo(() => fake.CallMe(A<JObject>.Ignored)).Invokes((JObject o) => fakeParamRecieved = o);
            this.myApiCallGetsDataFromSomewhere(fake);
            A.CallTo(() => fake.CallMe(A<JObject>.That.Matches(o =>
                    o["Misspelled"].ToObject<string>() == "WrongValue", io => io.Write(JsonConvert.SerializeObject(fakeParamRecieved, Formatting.Indented))))) // IO is ignored comepletely
                .MustHaveHappenedOnceExactly();
        }

        [TestMethod]
        public void WhatIWhishForTest()
        {
            ICanDoSomething fake = A.Fake<ICanDoSomething>();
            this.myApiCallGetsDataFromSomewhere(fake);
            A.CallTo(() => fake.CallMe(A<JObject>.That.Matches(o => o["Misspelled"].ToObject<string>() == "TestValue"
                    // Here I can print any kind of info about the call I expected with the actual value
                    //, (IFakeObjectCall c, IOutputWriter io) => io.Write(JsonConvert.SerializeObject(c.Arguments, Formatting.Indented))
                )))
                .MustHaveHappenedOnceExactly();
        }

        /// <summary>
        /// This method gives me data I have no clue about!
        /// </summary>
        private void myApiCallGetsDataFromSomewhere(ICanDoSomething fake)
        {
            JObject testObject = new JObject();
            testObject.Add("SomeName", JToken.FromObject("TestValue"));
            fake.CallMe(testObject);
        }
    }

    public interface ICanDoSomething
    {
        void CallMe(JObject o);
    }
}

[blairconrad]

Thanks for writing, @Mertsch. I can see how an exception being thrown in the matcher is really causing some pain. I feel for you.
It would indeed be nice if FakeItEasy provided more help in the face of user-supplied call matchers that throw exceptions.

In the meantime, note that you can get better results in the face of some failures by providing an argument value formatter for JObject. Currently FakeItEasy sees JObject as "something that is enumerable" and tries to provide nice output based on that, even though that's not the best way to represent a JObject.

If you include a class like this:

public class JObjectFormatter : ArgumentValueFormatter<JObject>
{
    protected override string GetStringValue(JObject argumentValue)
    {
        return argumentValue.ToString();
    }
}

anywhere in your test project, you'll see output like this:

Test 'FakeItEasyBugs.UnitTest1.SlightChanceToFindTheErrorTest' failed: FakeItEasy.ExpectationException : 

  Assertion failed for the following call:
    FakeItEasyBugs.ICanDoSomething.CallMe(o: <o => (o.get_Item("SomeName").ToObject() == "WrongValue")>)
  Expected to find it once exactly but didn't find it among the calls:
    1: FakeItEasyBugs.ICanDoSomething.CallMe(o: {
        "SomeName": "TestValue"
      })

instead of

  Expected to find it once exactly but didn't find it among the calls:
    1: FakeItEasyBugs.ICanDoSomething.CallMe(o: [[[]]])

[blairconrad]

If a call matcher throws, it's still appropriate for the assertion to fail (not just not match, but fail with an exception), but it should clearly indicate that the matcher threw (describing it as well as possible) and it should still indicate which calls have been made to the Fake.

[Mertsch]

@blairconrad Thanks for the Tip, nice to know that Formatter thing.
Additionally to your comment:

The User provided detail message should always be shown (independent of how customizable they are)

[blairconrad]

Sorry @Mertsch, which "User provided detail" message is this? I'm a little slow today.

[Mertsch]

@blairconrad Hehe 😀 I mean the things that you write into the Action<IOutputWriter> descriptionWriter or any other methods of writing the "description".

Thank you for your activeness in this project, that is so nice to see as a developer myself 😃

[blairconrad]

Oh, yes. I see! For sure, if we're spewing a description of the failed matcher, we should use the user's description. Very good point.

[thomaslevesque]

So, there are two different issues here:

• Formatting of JObject arguments is less than ideal. Not sure what we can do about it. I guess we could treat it as a special-case (by type name, so we don't reference Newtonsoft.Json), but I don't like the idea; if we start special-casing types from non-standard libraries, where do we stop?
• Handling of exceptions in user predicates: we should wrap the original exception in an exception of our own, with a message that clearly shows the issue.

[Mertsch]

@thomaslevesque Yes, even though I took the JObject just as an example, in my specific production code case, it was a different object containing a JObject and the displayed info was the usual object.ToSting() which wasn't to helpful either, as that was just the class name.
A means to write your own debug info is IMO the more important thing, to get the info you need rather then having FIE provide a converter for every class out there.

But before that it's important to know where the error was 😀

[thomaslevesque]

A means to write your own debug info is IMO the more important thing, to get the info you need rather then having FIE provide a converter for every class out there.

Well, it's what custom formatters are for 😉

[Mertsch]

@thomaslevesque Just tried a formatter, works great 😄

[thomaslevesque]

I'm wondering what exception we should throw when a user-provided delegate throws. None of the existing exceptions in FIE is a good fit, so we should probably introduce a new one.

Also, there are plenty of other places where we invoke user-provided delegates. We should probably improve these too.

[blairconrad]

I think a new exception makes sense. As you say, there are lots of places we could run into this problem. Even in this one situation, the matcher may fail, the repetition count may fail, the description of the matcher may fail, and the description of an argument value may fail. And I'm probably missing some!

[thomaslevesque]

Yeah, lots of different situations. What about UserCallbackException? With details in the message about which callback failed.

[thomaslevesque]

For discussion, here's a spec that documents the desired behavior:

public class UserCallbackExceptionSpecs
{
    public interface IFoo
    {
        int Bar(int x);
    }

    [Scenario]
    public void ExceptionInArgumentMatcher(IFoo fake, Exception exception)
    {
        "Given a fake"
            .x(() => fake = A.Fake<IFoo>());

        "And a call to the fake is configured with a custom argument matcher that throws an exception"
            .x(() => A.CallTo(() => fake.Bar(A<int>.That.Matches(i => ThrowException()))).Returns(42));

        "When the configured method is called"
            .x(() => exception = Record.Exception(() => fake.Bar(0)));

        "Then a UserCallbackException should be thrown"
            .x(() => exception.Should().BeAnExceptionOfType<UserCallbackException>());

        "And its message should describe where the exception was thrown from"
            .x(() => exception.Message.Should().Be("Argument matcher '<i => ThrowException()>' threw an exception. See inner exception for details."));

        "And the inner exception should be the original exception"
            .x(() => exception.InnerException.Should().BeAnExceptionOfType<MyException>().Which.Message.Should().Be("Oops"));
    }

    private static bool ThrowException()
    {
        throw new MyException("Oops");
    }

    public class MyException : Exception
    {
        public MyException(string message, Exception inner = null)
            : base(message, inner)
        {
        }
    }
}

[Mertsch]

Thank you very much @blairconrad and @thomaslevesque . You are awesome.

[blairconrad]

This change has been released as part of FakeItEasy 4.7.0.

Thanks, @Mertsch. Look for your name in the release notes! 🥇