Raise does not trigger event handler when CallBase is true

[ScotSalmon]

This code:

    public class TestEventRaiserBase
    {
        public virtual event EventHandler<TestEventArgs> TestEventHappened;
    }

    public class TestEventRaiserDerived : TestEventRaiserBase
    {
    }

    public class TestEventArgs
    {
    }

    public class MoqRaiseTest
    {
        private bool raiseHappened = false;

        private void HandleTestEvent(object sender, TestEventArgs args)
        {
            raiseHappened = true;
        }

        [Fact]
        public void TestMoqRaise()
        {
            var mock = new Mock<TestEventRaiserDerived>() { CallBase = true };
            var raiser = mock.Object;
            raiser.TestEventHappened += HandleTestEvent;
            Mock.Get(raiser).Raise(x => x.TestEventHappened += null, raiser, new TestEventArgs());
            Assert.True(raiseHappened);
        }
    }

Fails the test (raiseHappened is false) and does not hit a breakpoint set in HandleTestEvent.

If I comment out CallBase = true then the handler is called as expected and the test passes.

We have a guess that setting CallBase is causing Moq to remove the necessary infrastructure to handle Raise, which would be okay if we could use an explicit Setup to put it back, but we can't find a way to do that for event handlers. We can work around it by not setting CallBase and then explicitly using Setup on all the other methods on our class to make them act like we'd set CallBase but that would be a lot of methods in our actual code (not shown in the snippet above). Another workaround we might use is to perform the Invoke from a method. That's clunky though.

Interestingly, this code using Raise with CallBase = true works as expected with an ancient build of Moq.dll that we have from before we were using the NuGet. The properties on that DLL show version 4.0.0.0. I tried the closest NuGet version to that (4.0.10827) as well as the one older NuGet (3.1.416.3) and spot checked several newer versions up to the latest, and none of them worked.

A possibly related quirk is that you can see the parameters to Raise in my snippet don't match the docs at http://www.nudoq.org/#!/Packages/Moq/Moq/Mock(T)/M/Raise -- specifically I had to provide something as the sender (I used raiser here but null also works). If I don't do that, the CallBase = true behavior is the same (still does not raise the event) but I get a parameter count mismatch exception at run time if I don't set CallBase.

[stakx]

@ScotSalmon: Thanks for reporting. I'm currently away until the end of the week, but will take a look at this then. In the meantime, could you perhaps track down the latest 4.x version where your scenario was still working?

[ScotSalmon]

There's no version in the NuGet repo that has it working that I can find, and I don't actually know the provenance of the 4.0.0.0 that does work (it predates my involvement in this source base). If that 4.0.0.0 is accurate and on the same versioning scheme as the NuGets, that's the latest version I know of that worked, but note that 3.1.416.3 from the NuGet didn't work, so either the issue was fixed briefly at 4.0.0.0 and then broken again, or there's something special about that 4.0.0.0 build. I'll ask around about the origin of that working version in case it ends up being useful, and post here if I learn anything.

[stakx]

It's actually good enough for the moment to know that your scenario hasn't worked for a good while (i.e. nothing recent), or perhaps only ever worked due to a regression. I'll look at this in detail over the weekend. Thanks for the moment! 👍

[stakx]

@ScotSalmon, I think I've found the problem. In short, there's a inconsistency between what happens when subscribing to events and what happens when raising events:

When subscribing to a mock's event:

raiser.TestEventHappened += HandleTestEvent;

due to the following lines, the event handler will be addd to the base class' TestEventHappened delegate instead of to a mock's own internal list of event handlers when CallBase is set:

https://github.com/moq/moq4/blob/dce0164b74402847582c56581764e473a03f6928/Source/Interception/InterceptionAspects.cs#L215-L233

(invocation.ReturnBase basically has the effect of deferring to the base class' event add accessor.)

However, when an event is raised:

Mock.Get(raiser).Raise(x => x.TestEventHappened += null, raiser, new TestEventArgs());

the base class' delegate is never considered—it will always be assumed that the handler was added to the mock's own event handler list—there is no special case for CallBase:

https://github.com/moq/moq4/blob/dce0164b74402847582c56581764e473a03f6928/Source/Mock.cs#L1104-L1115

Interestingly, there's currently no test coverage for the CallBase scenario, so even if we remove the above if (mock.CallBase ...) clause all tests will happily succeed.

For these reasons, I'd say the current situation qualifies as a bug.

On the other hand, Moq has worked like this for basically forever, and the exact semantics of CallBase in relation to events isn't quite clear to me. What should the effect of CallBase = true be when subscribing to / unsubscribing from / raising an event?

I'd love to fix this, but I am not sure whether it's a good idea to do so, as we might break more scenarios than we're fixing.

I'd be interested in your opinion!

[stakx]

Btw. after thinking about this a bit, there is no way to reliably trigger a base class' event from the outside. Let's say that base class implements its event's add and remove accessors explicitly:

public class Base
{
    public virtual event SomeEvent
    {
        add { ... }
        remove { ... }
    }
}

then there's no general way to know how to trigger SomeEvent from the outside. We don't know what the event's backing field (of some delegate type) is called; we don't even know if there is such a backing field at all!

Thus the best we could do (when CallBase == true) is to check whether the event in question has [CompilerGenerated] accessors; if so, we can assume that there's a backing field with the same name as the event, and we can trigger the event by invoking that delegate via reflection. If, however, the event accessors are not compiler-generated, then all bets are off and we throw an exception right whenever an attempt is made to either subscribe to / unsubscribe from / raising the event.

That kind of support for raising a base class' event is quite spotty, perhaps it would be better to not support this at all...?

Again, I'm interested in your opinion on this, too.

[ScotSalmon]

Can you make it so CallBase == true causes the registration to happen on both the base and the mock? If as you say the base class's delegate is never considered when Raiseing in the mock case, it should make my case work without breaking anyone else or changing how the base behaves currently.

[stakx]

Yes, that should be easily possible. But is it the right think to do?

Imagine the following class:

class Base
{
    public virtual event EventHandler Event
    {
        add { }
        remove { }
    }
}

Note that there is no way this class could raise this event.

Given your proposal, new Mock<Base> { CallBase = true }.Raise(b => b.Event += null) would raise the event anyway, even though it shouldn't.

I realise that this particular example is extreme and mostly academic. But it points out that we probably don't know what the exact semantics should be when Raise is used together with CallBase.

In fact, there is no "base" when it comes to raising an event. Does that mean that we're free to let Raise do whatever we want for CallBase, or does it mean we should do nothing, or should Raise simply throw a usage exception?

[ScotSalmon]

After discussing around here, I think we agree, there's no right way to use this. We'll work around it for now and change the tests not to need it at some point. Thanks for the detailed advice -- I'll close the issue.