dotnet / reactive

The Reactive Extensions for .NET
http://reactivex.io
MIT License
6.73k stars 751 forks source link

TestScheduler WithAsync swallows all exceptions making all tests pass #2096

Closed MarshMello0 closed 6 months ago

MarshMello0 commented 8 months ago

Bug

In Unit testing, when doing TestScheduler().WithAsync any expection doesn't fail the test.

I did find this stackoverflow question, which is super close to the same senario but not fully

Which library version?

6.0.0

What are the platform(s), environment(s) and related component version(s)?

.NET 6

What is the use case or problem?

All tests passing, even though they are failing by throwing exceptions.

What is the expected outcome?

Tests that throw exceptions, fail

What is the actual outcome?

Tests that throw exceptions, pass

Do you have a code snippet or project that reproduces the problem?

[Test]
public void Test() => new TestScheduler().WithAsync(async scheduler =>
{
    await Task.Delay(1);
    throw new Exception("This should fail the test");
});
MarshMello0 commented 8 months ago

Workaround is to GetAwaiter GetResult

[Test]
public void Test() => new TestScheduler().With(scheduler =>
{
    Task.Delay(1).GetAwaiter().GetResult();
    throw new Exception("This should fail the test");
});
idg10 commented 7 months ago

What's this WithAsync method?

I've searched the whole Rx.NET source repo, and we do not define any such method.

Right now if you search: https://github.com/search?q=repo%3Adotnet%2Freactive%20WithAsync&type=code the only result for WithAsync is this issue that you opened.

I'm going to hazard a guess that you're using ReactiveUI (which uses Rx, but is a separate thing from Rx).

If that's correct, there's an obvious problem with your test: ReactiveUI's WithAsync method returns a Task, but your test returns void. That means that you've given the unit test framework no way of knowing when your test actually finished.

Since WithAsync more or less immediately returns a Task, which you then fail to return from your test, the test runner thinks your test is done whether or not it has even begun to execute the task.

If we simply change the return type of your test from void to Task:

[Test]
public Task Test() => new TestScheduler().WithAsync(async scheduler =>
{
    await Task.Delay(1);
    throw new Exception("This should fail the test");
});

then the test fails in exactly the way you want.

This works because by returning the Task, you've made it possible for the test runner to wait for your test to finish, and to discover whether it threw any exceptions. By returning void you were making it impossible for the test runner to find out what the outcome of your code was.

I would strongly advise against using your 'workaround', because GetResult is an example of 'sync over async`, which is well known to be a good way of causing deadlocks.

Any test that does anything async should return a Task, so that the test runner can properly wait for it to finish, and report the results.

MarshMello0 commented 6 months ago

doh, aplogies. Looking back on this and the code, yes it is from the namespace ReactiveUI.Testing.

Thank you for explaining the solution as well in great detail! That makes sense now