Check.ThatCode for Task-returning methods does not await result in 3.0.0

[bacar]

Bug Type

Please pick one:

• [ ] a check failed to detect an error (false negative), i.e. a test is green while it should be red.
• [X] a check raised un existing error (false positive), i.e a test is red while it should be green.
• [ ] an error message is invalid/incomplete (please provide samples)
• [ ] a ran into an exception.
• [ ] other.

Describe the bug

I'm upgrading from 2.7.2 to 3.0.0 and have found I had to migrate .ThatAsyncCode to .ThatCode since the former is Obsolete and I treat warnings as errors. https://www.nuget.org/packages/NFluent/3.0.0.351#readme-body-tab advises that this should replace it ("ThatAsyncCode: you can now use ThatCode even for async methods")

However, unlike .ThatAsyncCode in the earlier version, .ThatCode does not await the Task for non-generic Task-returning, non-async methods. (However it works fine for Task<T>-returning non-async methods, and also for async methods returning Task).

To Reproduce

    [Test]
    public void Async_issue()
    {
        // simulates the code under test
        async Task<bool> PleaseThrowAsync()
        { 
            await Task.Yield();
            throw new InvalidOperationException("Oh dear!)");
        }
        async Task PleaseThrowAsync2()
        { 
            await Task.Yield();
            throw new InvalidOperationException("Oh dear!)");
        }

        // assert

        // #1 lambda is async method returning Task<T>:
        Check.ThatCode(async () => await PleaseThrowAsync()).Throws<InvalidOperationException>();

        // #2 lambda is non-async method returning Task<T>:
        Check.ThatCode(() => PleaseThrowAsync()).Throws<InvalidOperationException>();

        // #3 lambda is async method returning Task:
        Check.ThatCode(async () => await PleaseThrowAsync2()).Throws<InvalidOperationException>();

        // #4 lambda is non-async method returning Task:
        Check.ThatCode(() => PleaseThrowAsync2()).Throws<InvalidOperationException>();
    }

This throws on the 4th and final Check.ThatCode. For now I can explicitly make the lambda async (ie transform code like check 4 to code like check 3), but I don't think this is expected to be required.

The equivalent code in 2.7.2 (using ThatAsyncCode instead of ThatCode) does not fail. I encountered this problem when I did a global replace of ThatAsyncCode with ThatCode upon upgrading.

I note that ThatCode<TU>(Func<Task<TU>> awaitableFunc) calls RunTrace.GetAsyncTrace, while ThatCode(Func<Task> awaitableFunc) does not (it calls RunTrace.GetTrace. which then explicitly checks whether the method is async by inspecting whether it has an AsyncStateMachineAttribute). This explains why it works for check 2 but not check 4. Perhaps the fix is simply to change this method to use GetAsyncTrace?

Under the earlier version, ThatAsyncCode(Func<Task> awaitableMethod) called GetAsyncTrace, which explains why it used to work.

Expected behavior Test code above should not fail, and certainly cases 2 and 4 should not be inconsistent.

Desktop (please complete the following information):

• NFluent version: 3.0.0
• Net Version: fwk 4.8.2
• IDE: Rider

[dupdob]

thanks for reporting this I am looking into it right away

[dupdob]

oh oh this one is tricky to fix. I need to understand why it worked previously.

[bacar]

Thanks - let me know if you have any further detail on the regressions - will be helpful to decide if I should hold off upgrading at this point or not.

[dupdob]

nah, there is not much regression. Got fooled by async void method.

The problem is that one cannot wait for async void method, no matter how hard you try.

And the magic of return type determination for lambda.

[dupdob]

Long story short, I may have to reintroduce Check.ThatAsyncCode

[dupdob]

By the way, actually, you should write your tests this way

   [Test]
    public void Async_issue()
    {
        ...
        // assert

        // #1 lambda is async method returning Task<T>:
        Check.ThatCode(PleaseThrowAsync).Throws<InvalidOperationException>();

        // #3 lambda is async method returning Task:
        Check.ThatCode(PleaseThrowAsync2).Throws<InvalidOperationException>();
    }

Method groups are a forgotten feature, but clean for this kind of code.

[bacar]

Sure, though it was just an example - in practice the lambda might have a bit more going on in it. Plus I have got into the habit of avoiding method groups since they always incur a method allocation ( before C# 11 - https://github.com/dotnet/roslyn/issues/5835) - we even have a code inspection that reminds us to avoid this.

[bacar]

And to be clear, I think Task PleaseThrowAsync3() => PleaseThrowAsync2() will suffer from the same problem even if accessed through a method group; the problem here I think is the reliance on whether the method has the AsyncStateMachineAttribute or not, which doesn't propagate through wrapping methods or manually crafted "async" methods, nor do I think it would be present on interfaces etc -- I think it would be a reasonable design choice that all Task and Task returning methods are treated specially as async (ie they are awaited), but I'm fine with dedicated ThatAsyncCode that always awaits too.

[dupdob]

Sure, though it was just an example - in practice the lambda might have a bit more going on in it. Plus I have got into the habit of avoiding method groups since they always incur a method allocation ( before C# 11 - https://github.com/dotnet/roslyn/issues/5835) - we even have a code inspection that reminds us to avoid this.

For my own part, I feel it is ok, even good, to accept different standards between tests and production code, but that is not the point here. I was merely pointing out an alternative workaround.

And to be clear, I think Task PleaseThrowAsync3() => PleaseThrowAsync2() will suffer from the same problem even if accessed through a method group;

Not sure what you mean here, just be clear , I have tested my proposal and it works.

the problem here I think is the reliance on whether the method has the AsyncStateMachineAttribute or not, which doesn't propagate through wrapping methods or manually crafted "async" methods, nor do I think it would be present on interfaces etc

Not exactly this, but this is part of the problem

Explanation follows

[dupdob]

Yep, I am still into this. I may have a way to keep signatures this way, but you are right, it definitely makes more sense to assume Task returning methods are async in nature. I am just looking for a way to check for Task Factory methods.

[bacar]

And to be clear, I think Task PleaseThrowAsync3() => PleaseThrowAsync2() will suffer from the same problem even if accessed through a method group;

Not sure what you mean here, just be clear , I have tested my proposal and it works.

To clarify, neither of these pass for me, despite PleaseThrowAsync3 being a trivial pass-through wrapper around PleaseThrowAsync2:

        Task PleaseThrowAsync3() => PleaseThrowAsync2();

        // #5,6 non-async method returning Task:
        Check.ThatCode(PleaseThrowAsync3).Throws<InvalidOperationException>();
        Check.ThatCode(() => PleaseThrowAsync3()).Throws<InvalidOperationException>();

That is, the workaround of using the method group only works if the original method itself has an async state machine implementation, which may not be the case generally for your methods with awaitable return types (which I've remembered is more complex than just matching on Task - some reflection is indeed needed, if you want to support awaitables generally, not just Task / Task<T>).

[dupdob]

Thanks for the clarification. In terms of target API, here is what I have in mind:

• Check.ThatCode will await lambdas with an awaitable return type: this is probably the expected behavior
• Check.ThatCode will not wait on method groups that are not explicitly awaitable: one needs a way to test for factory method. If one needs to await for them, it is just a matter of wrapping them in a lamba.
• Will contemplate what to do for async stream: specific checks probably make sens

using your examples, it means 1,2,3,4 and 6 will await and pass. 5 will not.

What do you think?

[bacar]

I don't understand the second bullet which is maybe driving you to an API in which 5 will not pass? Are you considering example 5 to be one that is "not explicitly awaitable"? PleaseThrowAsync3 returns a Task; it is 100% awaitable!

In both of cases 5 and 6, this current resolves to the same method, ThatCode(Func<Task> awaitableFunc). After that they shouldn't behave differently! Both return a task; that task should be awaited.

Is there some confusion here over what is considered to be 'awaitable' ? Awaitable != async. We unfortunately often say "async method" as a shorthand for a method that returns something that can be await-ed (because we most often write methods with awaitable return types by using the async keyword), but what matters is whether the method result type is awaitable, not whether its implementation is async:

        public int NonAwaitableNonAsync() => 1; // A
        public async void NonAwaitableAsync() { await Task.Delay(1000); Console.WriteLine("Done"); } // B
        public Task AwaitableNonAsync() => Task.Delay(1000); // C
        public async Task AwaitableAsync() => await Task.Delay(1000); // D

The first two are not awaitable, and it does not seem meaningful for ThatCode/ThatAsyncCode to attempt to await them, whether they are async (B) or not (A). I'm certainly not interested in supporting async void methods here. The second two are awaitable, and ThatCode/ThatAsyncCode should attempt to await them, whether they are async (D) or not (C). And currently it does await them both! But only for Task<TResult> not Task, hence this issue.

Awaitable-ness is the only relevant consideration to the issue at hand (IMO!); whether it has an async implementation is not. To be useful ThatAsyncCode is really a shorthand for ThatCodeWhichReturnsAnAwaitableResultAndWeAwaitThatResult.

In addition I think the problem is adequately resolved with having a separate ThatAsyncCode with overloads for Funcs that return the most common awaitable return types such as Task, Task<T>, maybe also ValueTask, ValueTask<T>. Any other kinds of awaitable can be hard to discover and can be trivially converted to a Func by wrapping it in a lambda async () => await MethodReturningAwaitableThatIsNotATask().

[dupdob]

Clarification

First of all, your right about the needed clarification: I kind of mixed async and awaitable, so let me rephrase what I meant.

In terms of target API, here is what I have in mind:

• Check.ThatCode will await lambdas with an awaitable return type (i.e weither async or not): this is probably the expected behavior
• Check.ThatCode will not wait on method groups that are not explicitly async (one needs a way to test for factory method). If one needs to await for them, it is just a matter of wrapping them in a lamba.

• Will contemplate what to do for async stream: specific checks probably make sense.

  Condition for async/awaitable methods

  As said earlier, returning a Task is neither necessary nor sufficient for a method to be awaitable.

  1. This is not necessary, as the requirement is only to return an object implementing GetAwaiter (plus extra requirements). Task are the most used one, of course
  2. This not sufficient, returning a Task does not implies it has been scheduled for execution. Awaiting for this method results in an infinite wait, while it returns a nop task.

     public Task PseudoAsyncMethod()
     {
         return new Task(() => { });
     }

Coverage

Obviously, handling only Task returning method is an acceptable restriction, as one can convert alternative form as you pointed out. I still think IAsyncEnumerable deserves enumeration related API.

Origins of Check.ThatAsyncCode

When async methods were added to .Net, people raised issues with Check.ThatCode because it did not wait for the result and offered extension method related to the Task<> result and not the expected returned types.

As we did not find a way to address both problems directly with Check.ThatCode, an specific method was added. But this was not a satisfactory method, as people have to remind to use it everytime they check an async method, otherwise unexpected results ensue.

Note that one can also trivally convert async method to sync: Check.ThatCode( () => AsyncMethod(...).Result) which is not much harder than using Check.ThatAsyncCode( () => AsyncMethod(...))

In short, both approaches, using ThatAsyncCode or adding .Result, failed one of the core NFluent principles: fluent to write. Ideally, one should be able to check everything with Check.That, that's why we remove alternatives as soon as possible, such as Check.ThatStruct previously.

Conclusion

I had a concern with non-started Task<> returning functions. Indeed, awaiting for them would result in an infinite wait without any possible workaround for the user. Hence my proposal to treat Task returning, but not async flagged method groups not to be awaited. Alas, you do like it, but even worse, it would only work for parameterless methods. This invalidates this approach. But, there is a silverlining: this is a Task specificity (need to check for IAsyncEnumerable). And NFluent can check is a Task is started or not. So the updated proposal is:

1. Taks returning methods will be waited for
2. ...unless the Task they return has not been scheduled for execution.

[bacar]

Firstly - I don't know enough about IAsyncEnumerable to have a view.

On tasks that are not scheduled - is this based on Task.Status? Task statuses change -- maybe it's just not scheduled yet ? Race conditions could result in different behaviour of the assertion, which would be weird.

More generally on tasks that will never complete -- I guess my view is that it should be the same as for synchronous code that never completes, e.g. ends up entering an infinite loop or waiting on an event that never gets set - it is the least surprising behaviour if it awaits that completion, even if it never comes. If you try to wait for it in a test without a timeout that seems like a bug in the code or the test.

I do think there's an edge case that you may want to test the behaviour of the synchronous portion of an async method, e.g. let's say you have this and you want to test that with invalid input it throws in ValidateInputs in the synchronous portion, without awaiting it:

async Task DoSomething(...)
{
    ValidateInputs(...) // Can throw
    await Task.Yield();
    ...implementation, maybe including awaits ... maybe this part can throw...
}

I guess you could do that with sthg like: Check.ThatCode(() => { DoSomething(invalidInput); return; }).Throws(...) to force it into being treated as a synchronous, non awaitable method and not awaited. You could also use a similar construction to check that for some setup it's the opposite, i.e. only throws if you await it but not otherwise. Personally I think it's clearer to have an API where I know clearly and unambiguously whether it will wait for completion of a returned task or not; it is IMO the least surprising approach; but I can see that it's convenient for it to "guess" the common cases and only be awkward for the less common. (Perhaps ThatCode could always try to 'guess' the right behaviour, but you could have ThatAwaitedCode and ThatSynchronousCode which always force one behaviour or the other, or equally I could write my own extension methods that do that by using either of the two wrapper tricks mentioned that coerce it to being awaited or not awaited).

Most important thing though is for Task and Task<T> to behave consistently, I think.

[dupdob]

Let me try to answer this, dealing with what I feel is more important first:

Most important thing though is for Task and Task to behave consistently, I think. Yes, of course. And this is the case now. Note that there is a difference between the two: you can check the result of Task method (which you can't for a Task return method).

On tasks that are not scheduled - is this based on Task.Status? Task statuses change -- maybe it's just not scheduled yet ? Race conditions could result in different behavior of the assertion, which would be weird. No race condition, unless in the extraordinary circumstance the user introduces them with a non-standard approach to tasks. In that case, it would have to add an explicit wait via a lambda, for example.

More generally on tasks that will never complete ... You misread my example: the task I use complete properly: it does nothing. It is just that the wait is eternal because the task is not, and will not be, scheduled. I did nothing specific for non ending tasks. This is another topic You can try for your self: new Task( () => {}).Wait(); is a endless wait; while Task.Run(() => {}).Wait(); end almost instantly.

I do think there's an edge case that you may want to test the behaviour of the synchronous portion of an async method... My view is that whitebox testing of methods/functions is extremely evil and I would do nothing to support that 🤣 .

[dupdob]

Released 3.0.1 wit the fix

[bacar]

Hi - I forgot to say thanks for fixing this so promptly!