search

xunit / visualstudio.xunit

VSTest runner for xUnit.net (for Visual Studio Test Explorer and dotnet test)
https://xunit.net/
Other
146 stars 81 forks source link

False positives with ValueTasks #347

Closed Yeah69 closed 1 year ago

Yeah69 commented 2 years ago

In order to quickly demonstrate the issue and compare with other unit testing frameworks, I've created example tests: https://github.com/Yeah69/AsyncTesting However, this issue caused me confusion in unit test that I created for a concrete project. So it isn't just a theoretical issue for me.

The false positive tests are: https://github.com/Yeah69/AsyncTesting/blob/e371f20073035f267f95c0f4a708186ef38f70e0/AsyncTesting.xUnit/Tests.cs#L38-L43 https://github.com/Yeah69/AsyncTesting/blob/e371f20073035f267f95c0f4a708186ef38f70e0/AsyncTesting.xUnit/Tests.cs#L98-L104

Here you can see the results and comparison with the other unit testing frameworks: image

MSTest doesn't seem to support the ValueTask-Tests at all. NUnit seems to do everything right. Unfortunately xUnit has the false positives.

bradwilson commented 1 year ago

async ValueTask tests are not supported on xUnit.net v2. This is expected behavior. Switch to either async Task or async void.

Yeah69 commented 1 year ago

Okay. Will it be supported in future versions? E.g. xUnit.net v3?

Now that I know how xUnit.net v2 behaves I switched to async Task already almost a year ago. But thank you for the advice.

If async ValueTask officially isn't supported, then I would suggest maybe throw a NotSupportedException instead of running green. In my case I've had the false assumption that everything was green, although there were actually semantically failing tests. Then when I coincidentally found out that something isn't right, I did took some some time to investigate and prepare the minimal example so the issue could be understood quickly (and compared with the behavior of other unit testing frameworks). Additionally, it took time to refactor all the tests to async Task and fix the red tests (which would have been more efficient if they would be red right away).

Other users of xUnit.net v2, who are yet unknowing about this, might be in a similar situation, which causes false assumptions, confusion and time cost. So if it is technically not too much effort at least a NotSupportedException in such cases would be nice. Or another idea would be to let the test runner ignore such tests, so effectively the way that MSTest handles such cases. Would still probably cause some confusion, but no false assumptions and very little time cost if any.

I am writing this a happy xUnit.net user. Despite this issue I still like to use it. And this is also the single issue for many years of usage.

bradwilson commented 1 year ago

Yes, the main branch (for v3) has support for ValueTask.

The problem with "just throw" is that the code you're using pre-dates ValueTask. It's hard to throw when you don't know something exists. 😁