Add ApplyToAsync override to ThrowsExceptionConstraint

[manfred-brands]

Fixes #4907

[stevenaw]

LGTM. Thanks @manfred-brands ! Feel free to merge if you're ready.

[Dreamescaper]

While changes look fine, I don't get why they are needed. The intention was that previous behavior would be preserved if ApplyToAsync is not overridden.

[manfred-brands]

The default behaviour of ApplyAsync does fail in case of exceptions. That is fine for all normal constraints, just not for any Throws one. For the same reason there is an ApplyAsync overload for the other Throw constraint.

[Dreamescaper]

@manfred-brands
So, my intention with default ApplyToAsync was to preserve the existing behavior of ApplyTo method. And only to allow to change this behavior for Constraints that need that.
Obviously, I failed. If I wanted to preserve the behavior, I shoud've simply invoke sync ApplyTo method (which would do sync-over-async) from default ApplyToAsync implementation.

Now, I'm not sure what is the best cource of action here.
Sync-over-async is, well, bad, especially for a designated Async method.
OTOH, currently we have a breaking change. This issue might happen for user-created Contstraints.

[manfred-brands]

@manfred-brands So, my intention with default ApplyToAsync was to preserve the existing behavior of ApplyTo method. And only to allow to change this behavior for Constraints that need that. Obviously, I failed. If I wanted to preserve the behavior, I shoud've simply invoke sync ApplyTo method (which would do sync-over-async) from default ApplyToAsync implementation.

Now, I'm not sure what is the best cource of action here. Sync-over-async is, well, bad, especially for a designated Async method. OTOH, currently we have a breaking change. This issue might happen for user-created Contstraints.

Indeed calling ApplyTo would keep existing behaviour and making the Assert.ThatAsync a fake.
To then use proper async would require overloading ApplyToAsync for every Constraint including user ones.

The approach we have now will convert 99% of the constraints to proper async and only the ones that need it require an override. We forgot one because for some reason there are 3 Throw constraints.

On the other hand, how many people pass AsyncDelegate to constraints that only care about the actual result?
Why call: await Assert.ThatAsync(SomeAsyncMethod, Is.EqualTo(expected))
Instead of: Assert.That(await SomeAsyncMethod(), Is.EqualTo(expected))
There is more chance they have: Assert.That(SomeAsyncMethod, Is.EqualTo(expected)) as that code worked before we had Assert.ThatAsync

But I have been wrong making assumptions about how code is being used.

The only cases where NUnit needs to call the delegate are Throws to be able to inspect the exception and Delay to call it more than once.

Yes, there could be user constraints requiring this behaviour.

I suggest we do:

1. Change the implementation in Constraint to: return Task.FromResult(ApplyTo(async () => await taskDel());
   This will call the ActualValueDelegate overload which will use the AsyncToSyncAdapter.
   This method has overloads in the 3 Throw and in the Delay constraints. The same constraints that have their own ApplyToAsync overload. I tested the above by removing that overload. We should add a special test constraint to verify the behaviour.
   It will fix end-user constraints that override the ApplyTo<TActual>(ActualValueDelegate<TActual> del) method.
   It will cause other constraints in my first example to call async-over-sync, but those should be nudged to change to await the result before calling Assert. Maybe I can create an analyzer rule for this.
2. Document that if a constraints overloads ApplyTo<TActual>(ActualValueDelegate<TActual> del) it must also override ApplyToAsync<TActual>(Func<Task<TActual>> taskDel) to support proper async.