Assert.ThatAsync fail to catch expected exception

[shlomi-borovitz-greenroad]

Given a simple test to check that an async method throws a certain exception -

namespace TestFailToFail;

public class Tests
{
    [Test]
    public async Task Test1()
    {
        await Assert.ThatAsync(async () => throw new Exception("Ugh"), Throws.Exception);
    }

    [Test]
    public async Task Test2()
    {
        await Assert.ThatAsync(() => Task.FromException(new Exception("Ugh")), Throws.Exception);
    }

    [Test]
    public void Test3()
    {
        Assert.That(() => throw new Exception("Ugh"), Throws.Exception);
    }
}

With NUnit 4.2.2 - all tests pass (as expected)
With NUnit 4.3.0 (and also 4.3.1) - both Test1 and Test2 fail with the thrown exception. Only the sync version - Test3 pass

 Test1
   Source: UnitTest1.cs line 6
   Duration: 8 ms

  Message: 
System.Exception : Ugh

  Stack Trace: 
<<Test1>b__0_0>d.MoveNext() line 8
--- End of stack trace from previous location ---
<<ThatAsync>b__0>d.MoveNext()
--- End of stack trace from previous location ---
Constraint.ApplyToAsync[TActual](Func`1 taskDel)
Assert.ThatAsync(AsyncTestDelegate code, IResolveConstraint constraint, NUnitString message, String actualExpression, String constraintExpression)
Tests.Test1() line 8
GenericAdapter`1.GetResult()
AsyncToSyncAdapter.Await[TResult](TestExecutionContext context, Func`1 invoke)
AsyncToSyncAdapter.Await(TestExecutionContext context, Func`1 invoke)
TestMethodCommand.RunTestMethod(TestExecutionContext context)
TestMethodCommand.Execute(TestExecutionContext context)
<>c__DisplayClass3_0.<PerformWork>b__0()
<>c__DisplayClass1_0`1.<DoIsolated>b__0(Object _)
ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
ContextUtils.DoIsolated(ContextCallback callback, Object state)
ContextUtils.DoIsolated[T](Func`1 func)
SimpleWorkItem.PerformWork()

[manfred-brands]

@shlomi-borovitz-greenroad I can confirm that your tests fail in 4.3.1.

They however pass when I use:

[Test]
public async Task Test1()
{
    await Assert.ThatAsync(async () => throw new Exception("Ugh"), Throws.InstanceOf<Exception>());
}

[Test]
public async Task Test2()
{
    await Assert.ThatAsync(() => Task.FromException(new Exception("Ugh")), Throws.InstanceOf<Exception>());
}

It looks like we forgot to add an ApplyToAsync to ThrowsExceptionConstraint for PR #4813.

[OsirisTerje]

@manfred-brands Should we release 4.3.2 for this ?

[manfred-brands]

@manfred-brands Should we release 4.3.2 for this ?

The standalone Tnrows.Exception is a not recommended constraint according to our documentation

I don't know how much it is used.

The workaround is to specify the type of the exception.

[shlomi-borovitz-greenroad]

@manfred-brands Should we release 4.3.2 for this ?

The standalone Tnrows.Exception is a not recommended constraint according to our documentation

I don't know how much it is used.

The workaround is to specify the type of the exception.

It also not deprecated

And while I usually throw more specific exceptions (and then test for those) - I wasn't aware that it is not recommended (If it was deprecated, I would have been aware - and avoiding it to begin with).

I cannot speak for others of course

[stevenaw]

Is this just an issue with Throws.Exception when combined with the delay constraint, or with either one independently?

FWIW, in terms of Throws.Exception usage, it has seen creeping increased usage in our codebase at work. I suspect it could be because the generality is very easy (broadly usable) to copy/paste around. Would we want to consider a code-level nudge to supplement the suggestion on our hosted docs site? I don't know if we'd actually want to deprecate Throws.Exception but perhaps an xmldoc hint could be a useful guide to improving test robustness.

[OsirisTerje]

When we have a regression from the former version, I normally believe we should fix and release a hotfix.
I found more than 1.7K usages on github, so I'll add this to an upcoming 4.3.2 release.

I also added a repro, which includes checks on the other usages from the documentation @manfred-brands linked to here: https://github.com/nunit/nunit.issues/tree/main/Issue4907

@stevenaw I agree we could add such a comment, either more clearly in the documentation or in the xmldoc hint as you suggest. It should be kindly phrased, as if devs prefer to use it broadly doesn't necessarily mean they are throwing the Exception class itself, which is frowned upon, as stated here: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/exceptions/creating-and-throwing-exceptions, and it raises CA2201 warning too (https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2201)
They might just want a catch all, I assume.

[shlomi-borovitz-greenroad]

[heart] Shlomi Borovitz reacted to your message:

…

________________________________ From: Terje Sandstrom ***@***.***> Sent: Wednesday, December 25, 2024 8:40:32 PM To: nunit/nunit ***@***.***> Cc: Shlomi Borovitz ***@***.***>; Mention ***@***.***> Subject: Re: [nunit/nunit] Assert.ThatAsync fail to catch expected exception (Issue #4907) When we have a regression from the former version, I normally believe we should fix and release a hotfix. I found more than 1.7K usages on github, so I'll add this to an upcoming 4.3.2 release. I also added a repro, which includes checks on the other usages from the documentation @manfred-brands<https://github.com/manfred-brands> linked to here: https://github.com/nunit/nunit.issues/tree/main/Issue4907 — Reply to this email directly, view it on GitHub<#4907 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BE22NP6QSAPAML7OPVMYNA32HMJ4BAVCNFSM6AAAAABUC2T4SKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRRHE4TKOBWG4>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[stevenaw]

Agreed @OsirisTerje we definitely shouldn't be prescriptive there. Just a kind, helpful hint. Perhaps just something along the lines of "Resolves for any thrown constraint. You may consider using Throws.TypeOf" if you wish to target a specific exception type."

Though perhaps even that almost seems heavy handed in retrospect. Anyone viewing the xmldocs in intellisense would also then have access to discover the specific-typed overloads as well.

I suppose I don't have a strong preference either way on the xmldoc approach, but I offer it as an idea here if others like it.