Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't raise non-virtual async event #977

Closed
TaffarelJr opened this issue Mar 18, 2020 · 13 comments
Closed

Can't raise non-virtual async event #977

TaffarelJr opened this issue Mar 18, 2020 · 13 comments

Comments

@TaffarelJr
Copy link

TaffarelJr commented Mar 18, 2020

I need Moq to raise an async event on my mock object, so I can test how my code handles it. But I can't seem to get Moq to do it - either it's not supported, or I can't find the syntax anywhere.

The setup:

I'm using a 3rd-party class (can't change it) with an async event defined on it:

public class BaseClass
{
    public event Func<CustomEventArgs, Task> SomethingHappenedAsync;
}

I have my own class which encapsulates a BaseClass and attaches to its events:

public class MyClass
{
    private readonly BaseClass _base;

    public MyClass(BaseClass base)
    {
        _base = base ?? throw new ArgumentNullException(nameof(base));
        _base.SomethingHappenedAsync += DoSomething;
    }

    private Task DoSomething(CustomEventArgs args)
    {
        ...
    }
}

In my tests, I'm mocking the BaseClass, passing it to my constructor, and now I want to raise the event to test how my code handles it:

// Arrange
var eventArgs = new CustomEventArgs(...);
var mockBase = new Mock<BaseClass>(MockBehavior.Strict);
var subject = new MyClass(mockBase.Object);

// Act
mockBase.Raise(b => b.SomethingHappenedAsync += null, new object[] { eventArgs });

Instead, the lambda inside the call to Raise(...) throws an ArgumentNullException:

Value cannot be null. Parameter name: SomethingHappenedAsync

Stack trace:
    at BaseClass.add_SomethingHappenedAsync(Func`2 value)
    at MyClassTests.<>c.<Test1>b__3_1(BaseClass c) in C:\Code\TestProject\MyClassTests.cs:line 30
    at Moq.ActionObserver.ReconstructExpression[T](Action`1 action, Object[] ctorArgs)

Is this a supported feature?

@TaffarelJr
Copy link
Author

I assume it has something to do with the fact the event needs a Task returned. I tried:

mockBase.Raise(b => b.SomethingHappenedAsync += a => Task.CompletedTask, new object[] { eventArgs });

... but that fails with an ArgumentException:

Unsupported expression: c => c.ToString()

Stack trace:
    at Moq.Mock.RaiseEvent(Mock mock, LambdaExpression expression, Stack`1 parts, Object[] arguments)
    at Moq.Mock.RaiseEvent[T](Mock mock, Action`1 action, Object[] arguments)
    at Moq.Mock`1.Raise(Action`1 eventExpression, Object[] args)
    at MyClassTests.Test1() in C:\Code\TestProject\MyClassTests.cs:line 30

@TaffarelJr
Copy link
Author

Also, I tried making the event hander method public as well - but no luck.
I'd prefer it to be private if possible ...

@stakx
Copy link
Contributor

stakx commented Mar 18, 2020

Not much time to study your issue in details tonight, just this for now:

"Async events" probably make just as little sense as "async properties". While it is technically possible for a property to return a Task, that's not a pattern usually seen in .NET world. Properties are supposed to model a value that's already computed, or that can be computed & cached quickly. Properties that return a task pretty much violate this design principle.

The patterns for events are even stricter than those for regular properties: The event delegate signature should have a void return type. Otherwise, if there are several subscribers, all return values except that of just one subscriber (the last one invoked, IIRC) will be silently discarded. Not what you usually want with tasks.

So it should come as no surprise that Moq doesn't cater for these scenarios.

@TaffarelJr
Copy link
Author

TaffarelJr commented Mar 18, 2020

I agree that it's not a common pattern -- but the 3rd-party class I'm dealing with is actually the latest (v5) client for Azure EventHubs from Microsoft:

(decompiled)

namespace Azure.Messaging.EventHubs
{
    public class EventProcessorClient
    {
        ...
        //
        // Summary:
        //     The event to be raised once event processing stops for a given partition.
        public event Func<PartitionClosingEventArgs, Task> PartitionClosingAsync;
        //
        // Summary:
        //     The event to be raised just before event processing starts for a given partition.
        public event Func<PartitionInitializingEventArgs, Task> PartitionInitializingAsync;
        //
        // Summary:
        //     The event responsible for processing unhandled exceptions thrown while this processor
        //     is running. Implementation is mandatory.
        public event Func<ProcessErrorEventArgs, Task> ProcessErrorAsync;
        //
        // Summary:
        //     The event responsible for processing events received from the Event Hubs service.
        //     Implementation is mandatory.
        public event Func<ProcessEventArgs, Task> ProcessEventAsync;
        ...
    }
}

I'd really like to be able to unit test my event handlers (in addition to integration tests). Anything you can give me would be helpful.

@stakx
Copy link
Contributor

stakx commented Mar 18, 2020

Oh boy... either my .NET-fu is getting out of date, or that's a terrible API. I wonder how one is supposed to use it safely.

In the short term, I believe you'd be best off testing this particular case without Moq, i.e. by writing a test double manually.

Longer term, it might make sense to at least fix the misleading error message that you got. If we can easily get this scenario working as a side effect of that, all the better. But beyond that, this seems like too much of an edge case to me to warrant any new APIs (but I could be wrong).

@stakx
Copy link
Contributor

stakx commented Mar 18, 2020

As it turns out, this scenario already works just fine (except for the fact that there's no way to await the Tasks returned by the event handlers). I cannot reproduce your error. Here's the code I'm running, using the latest version of Moq:

[Fact]
public void Test()
{
	var eventArgs = new CustomEventArgs();
	var mockBase = new Mock<BaseClass>(MockBehavior.Strict);
	var subject = new MyClass(mockBase.Object);

	mockBase.Raise(b => b.SomethingHappenedAsync += null, new object[] { eventArgs });

	Assert.True(eventArgs.Used);
}

public class CustomEventArgs : EventArgs
{
	public bool Used { get; set; }
}

public class BaseClass
{
	public virtual event Func<CustomEventArgs, Task> SomethingHappenedAsync;
}

public class MyClass
{
	private readonly BaseClass _base;

	public MyClass(BaseClass @base)
	{
		_base = @base;
		_base.SomethingHappenedAsync += DoSomething;
	}

	private Task DoSomething(CustomEventArgs args)
	{
		args.Used = true;
		return Task.CompletedTask;
	}
}

That test passes just fine.

@TaffarelJr
Copy link
Author

TaffarelJr commented Mar 18, 2020

You know what I think it is, SomethingHappenedAsync is declared as virtual in your test. It's not that way in Microsoft's class, and when I take it out I get an ArgumentException:

Unsupported expression: b => b.ToString()

   at Moq.Mock.RaiseEvent(Mock mock, LambdaExpression expression, Stack`1 parts, Object[] arguments)
   at Moq.Mock.RaiseEvent[T](Mock mock, Action`1 action, Object[] arguments)
   at Moq.Mock`1.Raise(Action`1 eventExpression, Object[] args)
   at MyClassTests.Test1() in C:\Code\TestProject\MyTests.cs:line 30

Which, interestingly, isn't the same as the first error I was getting - which I would have expected here. Instead, this is similiar to the second error I got, when I tried plugging in a dummy event handler in the call to Raise(...).

@TaffarelJr
Copy link
Author

Been a while since I've worked very heavily with events much, but it's all coming screaming back to me that you can't actually declare an event in a base class, and then raise it from a derived class. Microsoft says not to use the virtual keyword; instead, you should create a virtual On<eventname>(EventArgs args) method in the base class that raises the event, and then call/override it in the derived class.

Since we can't add that method to Microsoft's class, we can't actually raise the event at all. Maybe a better error message is the best Moq can do.

Thanks for your help, I guess I'll have to find a different solution.

@stakx stakx closed this as completed Mar 18, 2020
@stakx stakx removed the needs-repro label Mar 18, 2020
@jeremy7nelson
Copy link

jeremy7nelson commented May 10, 2021

I used SetupProperty and then Invoke on the Object:

public delegate Task AwaitableAction();

public interface IClassToMock
{
    AwaitableAction SomethingChanged { get; set; } // async event
}

[Test]
public async Task ClassTest()
{
    var mock = new Mock<IClassToMock>();
    mock.SetupProperty(x => x.SomethingChanged);

    var testClass = new TestClass(mock.Object);
    await mock.Object.SomethingChanged.Invoke().ConfigureAwait(false);
    Assert.That(testClass.SomethingHappened, Is.True);
}

@JimHume
Copy link

JimHume commented Jan 6, 2022

I stumbled upon this issue (here in github) because I was facing something similar. RabbitMQ has its own async events: https://rabbitmq.github.io/rabbitmq-dotnet-client/api/RabbitMQ.Client.Events.AsyncEventHandler-1.html

Most of the Rabbit API is consumed via async events and it makes it very easy to adopt it into a typical async/await ecosystem.

In the world of async/await, it makes sense that async events are becoming a thing--too bad Microsoft hasn't made them a first-class-citizen yet. :/

In the interim I had to revert back to synchronous events so that I could test things appropriately.

@stakx stakx reopened this Jan 23, 2022
@stakx
Copy link
Contributor

stakx commented Jan 23, 2022

I'm reopening this as a reminder to myself to look into RabbitMQ's async events. My current thinking is still that async events don't make sense in principle, but perhaps things have moved on and I am now mistaken. Will take a look as soon as I find some spare time.

@calledude
Copy link

FWIW;
The workaround I currently use for this is registering a callback to the setter of the event

I.e.

        var myMock = new Mock<IInterface>();
        Func<Task> ev = null;
        myMock
            .SetupAdd(x => x.SomethingHappenedAsync += It.IsAny<Func<Task>>())
            .Callback((Func<Task> evt) => ev = evt);
            
       // ...
       await ev.Invoke();

Hope this helps

@stakx
Copy link
Contributor

stakx commented Jul 24, 2022

OK, I finally took a closer look. There's several different things we could talk about. Let's look at one after the other.

[...] it's all coming screaming back to me that you can't actually declare an event in a base class, and then raise it from a derived class.

Moq goes to some lengths to allow raising an event from the outside. It does so by intercepting the mock.E += h and mock.E -= h accessors and keeping track of the (un-) subscribed event handlers h in a separate delegate list for the E event, which it can then invoke when one calls mock.Raise(m => m.E, ...). But Moq can only do all of that when E's accessors are interceptable (i.e. when E is declared virtual, which N.B. is implicitly the case for events of an interface).

You know what I think it is, SomethingHappenedAsync is declared as virtual in your test. It's not that way in Microsoft's class [...].

virtual is simply necessary if you want to setup or raise that event with Moq. Moq v4 can only deal with inheritable types and overridable members... that's pretty much set in stone, nothing we can do to change that. If Microsoft's library declares a non-virtual event, there'll be no way to set it up, or raise it, using Moq.

So what could be done to improve Moq?

Since we can't add that method to Microsoft's class, we can't actually raise the event at all. Maybe a better error message is the best Moq can do.

I don't think we can. Say you call mock.SetupAdd(m => m.NonOverridableEvent += null) or mock.Raise(m => m.NonOverridableEvent += null, ...), you should currently see an ArgumentException with a message like the following:

Unsupported expression: m => m...
The next member after the last one shown above is non-virtual, sealed, or not visible to the proxy factory.
  at ...
  at Moq.Mock`1.SetupAdd(Action`1 addExpression) in ...
  at ...

We cannot improve that error message because NonOverridableEvent is simply not "visible" to Moq due to being non-overridable. For that reason, we cannot even mention its name in the error message. That's why the message refers to it indirectly.

All of that being said, I retract my earlier statement that async events are a horrible design. Given that the event delegates used have a return type of Task or ValueTask, and not Task<> or ValueTask<>, I can see now how those can actually make good sense.

this scenario already works just fine (except for the fact that there's no way to await the Tasks returned by the event handlers)

We could in theory add a mock.RaiseAsync method specifically for events whose handlers return a Task. Raise would then invoke each handler, gather its Task, and either await them in series or in parallel (using Task.WhenAll). But the events would still have to be overridable / virtual.

In conclusion, there's still nothing we can do to enable RaiseEvent or SetupAdd/SetupRemove for non-virtual events, which is what this issue was originally about, so I'm going to close this. If someone needs the mock.RaiseAsync mentioned in the previous paragraph, please request it in a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants