WithResult extension is always waiting for the Task to be completed

[lg2de]

With #1478 I implemented the extension methods WithResult for assertions on asynchronous operations. While playing around with this methods together with failed Should().CompleteWithinAsync I found that it could be unexpected, that these methods explicitly waiting for the completion of the Task. The original intention of the assertions CompleteWithinAsync were ensuring tests will not hang forever. With awaiting the Task the "timeout" is skipped and the test may hang.

Today I think whether it would be better to not await the Task and instead check whether it is already completed. In case the Task is not completed, further the result is not relevant anymore, isn't it?

[jnyrup]

Without having looked at the code and thought more about potential edge cases, it sounds reasonable to me to add check on task.IsCompleted.

[lg2de]

I guess it could be a breaking change. This is why I'm collecting feedback before creating such a PR.

[lg2de]

While looking at the implementation of CompleteWithinAsync, I'm starting to wonder why we call InvokeWithTimer at all.

Are you talking about the new one just introduced? Can you go more in detail, please? Please share your thoughts.

I agree that IsCompleted can be a quick exit strategy, but if it isn't completed, we should always do what we do in CompletesWithinTimeoutAsync.

No, if the task is completed, the previous assertion (CompleteWithinAsync) was successful and we can access (compare) the result immediately. Otherwise the assertion was failed and the result is not relevant anymore. We should do nothing instead of waiting for the result because this could create a hang.

[lg2de]

Currently you are talking about the internals of AsyncFunctionAssertions. These methods are currently going to be private protected, so we can change it any time. I'm a bit confused because I created these methods according to your suggestion...

This issue is focusing on the WithResult extension only. I guess I'll create a PR for my proposal. Maybe it's easier to understand.

[dennisdoomen]

Never mind me. Trying to catch-up with too many topics at the same time.

[dennisdoomen]

Today I think whether it would be better to not await the Task and instead check whether it is already completed. In case the Task is not completed, further the result is not relevant anymore, isn't it?

But in which scenario will this Task not be completed? I assume that CompleteWithinAsync is not waiting on the original task and the task processed by WithResult is just the async task returned by the entire CompleteWithinAsync method.