Incomplete stack trace when exceptions when raising events

[MutatedTomato]

When raising an event which throws an exception, the stack trace is terminated at Raise. In Mock.Generic.cs around line 571

	[SuppressMessage("Microsoft.Usage", "CA2200:RethrowToPreserveStackDetails", Justification = "We want to reset the stack trace to avoid Moq noise in it.")]
	public void Raise(Action<T> eventExpression, EventArgs args)
	{
		var (ev, target) = eventExpression.GetEventWithTarget(this.Object);

		try
		{
			target.DoRaise(ev, args);
		}
		catch (Exception e)
		{
			// Reset stack trace so user gets this call site only.
			throw e;
		}
	}

Removing the try catch should give a full stack trace. There will be noise but there will also be useful information. It is much better than no information.

[stakx]

This should remove information that is valuable only when Moq is malfunctioning. The general idea here is to give you a short stack trace pointing at your code, and to spare you stack frames that represent purely Moq internals, which are going to be uninteresting for most users.

(I do think the original exception should be preserved e.g. as an InnerException.)

What additional information are you hoping for? What scenario made you say that your stack trace contains no information?

[MutatedTomato]

It is quite clumsy to debug user code code executed by Moq. If an exception is thrown in the following code, the stack trace is

at Moq.Mock`1.Raise(Action`1 eventExpression, EventArgs args) in C:\projects\moq4\src\Moq\Mock.Generic.cs:line 587
at TestSomething.TestMethod() in File: line <TestMothod + 4>

public void TestMethod()
{
    var dependency = new Mock<D>();
    var objectBeingTested = new S(dependency);
    dependency.Raise(d => d.EventX += null, EventArgs.Empty);
}

private void EventXHappened(object sender, EventArgs e)
{
    try
    {
        a.X1();
        a.Broken();
        if (b == 0)
        {
            c.Y();
        }
    }
    catch (Exception e) when (NotSwallowExceptionDuringUnitTest)
    {
        // should not but one day will happen in production
        // try-catch log
    }

    bool NotSwallowExceptionDuringUnitTest { get; set; } // https://www.thomaslevesque.com/2015/06/21/exception-filters-in-c-6/
}

The site throwing the exeption is lost. The debugger does not break on the exception by default because it is not an uncaught exception. Putting the exception into an inner exception seems bad too unless the enclosing exception is a smart exception which prints the all user code stack trace.

Setup.().CallBack() and I suspect other places also suffers from this symptom too.

[stakx]

@MutatedTomato: Could you please post a complete code example? I'm not sure I understand the above example correctly.

[stakx]

Closing due to a lack of response. Until we get a clearer idea how the stack trace would be more helpful if it showed Moq internal method calls instead of being truncated at Moq's public API boundary, the main takeaway for me here will be that instead of just throwing away the full stack trace, it should at least be preserved via InnerException.

[MutatedTomato]

All test cases below allow you to click one the stacktrace to locate the exception quickly but not TestEventException. There is noise in the TestCallbackException but it is better than no information.

I cannot reproduce the problem of no automatic breaking on unhandled exceptions. Unfortunately, I am too busy to investigate now.

using System;
using System.Collections;
using System.Threading;
using System.Threading.Tasks;
using Moq;
using NUnit.Framework;
using TestCS;

namespace UnitTestProject1
{
    [TestFixture]
    public class UnitTest1
    {
        public interface IA
        {
            void F();
            event EventHandler<EventArgs> E;
        }

        [Test]
        public void TestCallbackAssert()
        {
            var mock = new Mock<IA>();
            mock.Setup(x => x.F()).Callback(() => AssertSomething());
            mock.Object.F();
        }

        [Test]
        public void TestCallbackException()
        {
            var mock = new Mock<IA>();
            mock.Setup(x => x.F()).Callback(() => Change());
            mock.Object.F();
        }

        [Test]
        public void TestEventAssert()
        {
            var mock = new Mock<IA>();
            mock.Object.E += (o, e) => AssertSomething();
            mock.Raise(x => x.E += null, EventArgs.Empty);
        }

        [Test]
        public void TestEventException()
        {
            var mock = new Mock<IA>();
            mock.Object.E += Handler;
            mock.Raise(x => x.E += null, EventArgs.Empty);
        }
        
        private void Handler(object sender, EventArgs e)
        {
            throw new Exception("Where is it thrown?");            
        }
        
        private void Change()
        {
            throw new Exception("Where is it thrown?");
        }

        private void AssertSomething()
        {
            Assert.Fail("Where is the problem?");
        }
    }
}

[stakx]

Thanks for posting these tests, I'll check them out soon.

[stakx]

@MutatedTomato - I see. I'd say we remove the catches and throws, just like you said in your initial post.

-	try
-	{
 		target.DoRaise(ev, args);
-	}
-	catch (Exception e)
-	{
-		// Reset stack trace so user gets this call site only.
-		throw e;
-	}

We've internally moved to use ExceptionDispatchInfo when invoking a delegate through reflection, so the above "fence" is no longer necessary; we should get reasonably well-structured exception stack traces even without it.

Feel free to submit a PR for this, if you wish. Since you're currently busy, I'll wait for a few days, but if I don't hear from you, I'll take the liberty of applying the changes.

[MutatedTomato]

Nice and thx. Please apply it yourself. Don't wait for me.