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

Event monitoring is not working properly for asyncronous tests #477

Closed
MichaWresche opened this issue Sep 7, 2016 · 8 comments
Closed
Assignees
Milestone

Comments

@MichaWresche
Copy link

Please consider the following test:

[TestMethod]
public async Task Test()
{
    var sut = new ViewModel();
    sut.MonitorEvents();
    await sut.DoSomethingAsync();
    sut.ShouldRaisePropertyChangeFor(vm => vm.BusyErrorText);
}

Sometimes, this test fails by an InvalidOperationException with description "Object <{0}> is not being monitored for events or has already been garbage collected. Use the MonitorEvents() extension method to start monitoring events."
The reason is the ThreadStatic-Attribute which is used for decorating the EventRecordersMap in the EventMonitor class.
Typically, UnitTests does not have a synchronization context so the code after await will be executed on any idle thread which is not the same thread as the thread that has been started the test. As a result, a new (empty) EventRecordersMap will be created and used when calling sut.ShouldRaisePropertyChangeFor(...).

@dennisdoomen
Copy link
Member

Thanks for reporting.

@informatorius
Copy link

I don't know if it is the same issue but when 2 tasks fire events in a class under test then sometimes MonitorEvents is not catching both. So either in EventMonitor or EventRecordersMap there should be some locking.

@dennisdoomen
Copy link
Member

@informatorius I believe it is the same problem. The current API doesn't simply not work with async/await code.

@informatorius
Copy link

I can reproduce the issue and can fix it with lock(object).
Should I make a pull request?

@dennisdoomen
Copy link
Member

You can't really solve it like that. You might even cause deadlocks.

@informatorius
Copy link

informatorius commented Nov 11, 2016

My issue is with the MonitorEvents() method called from different threads and I get this exception:
System.AggregateException: One or more errors occurred. ---> System.ArgumentException: Destination array is not long enough to copy all the items in the collection. Check array index and length. at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource) at System.Collections.Generic.Dictionary2.CopyTo(KeyValuePair2[] array, Int32 index) at System.Linq.Buffer1..ctor(IEnumerable1 source) at System.Linq.Enumerable.ToArray[TSource](IEnumerable1 source)
at FluentAssertions.Events.EventRecordersMap.ForEach(Object eventSource, Action1 action) at FluentAssertions.Events.EventRecordersMap.Add(Object eventSource, IEnumerable1 recorders)
at System.Threading.Tasks.Task.Execute()
`

I can fix it be using lock(object) at the MonitorEvents() method directly and in the EventRecordersMap.Add method.

@dennisdoomen
Copy link
Member

The problem is caused by the use of [ThreadStatic] which makes it unfit in multi-threaded situations. There's no workaround other than redesigning the API.

@informatorius
Copy link

informatorius commented Nov 11, 2016

OK then I may have a different issue and proceed in a new issue.
=> https://github.com/dennisdoomen/FluentAssertions/issues/505

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

3 participants