MSTEST0058 false positive

[Mertsch]

Describe the bug

When using a [DoesNotReturn] Assertion like Assert.Fail, MSTEST0058 is still triggered.

Version used

MSTest.Sdk 4.1

Steps To Reproduce

try
{
    (await Assert.ThrowsExactlyAsync<UnauthorizedAccessException>(() => System.IO.File.ReadAllTextAsync(share.Combine("Test.txt"), ct)))
        .Message.ShouldBe($@"Access to the path '{share.Combine("Test.txt")}' is denied.");
}
catch (AssertFailedException ex) when (ex.Message.Contains("no exception was thrown.", StringComparison.Ordinal))
{
#pragma warning disable MSTEST0058 // Avoid assertions in catch blocks
    Assert.Inconclusive("We should not have access to this path. This suggests the share is still mounted and does not disconnect (until a system reboot).");
#pragma warning restore MSTEST0058
}

Expected behavior

no warning when catch always throws assertion exception.

Actual behavior

warning MSTEST0058: Do not use asserts in catch blocks because they may not fail the test if no exception is thrown (https://learn.microsoft.com/dotnet/core/testing/mstest-analyzers/mstest0058)

[Youssef1313]

For the above code snippet specifically, if the catch is hit, the test won't be seen as failed (because it's Assert.Inconclusive). And if the catch is not hit, the test will be seen as passed. What's the point of having this catch block altogether? Maybe the intent is something like this?

try
{
    (await Assert.ThrowsExactlyAsync<UnauthorizedAccessException>(() => System.IO.File.ReadAllTextAsync(share.Combine("Test.txt"), ct)))
        .Message.ShouldBe($@"Access to the path '{share.Combine("Test.txt")}' is denied.");
}
finally
{
    share.SignOut();
}

[Mertsch]

@Youssef1313 You are right with share.SignOut();, it's redundant, share is in a using statement anyway.

But I still would like to customize my test result for this "negative" test. A failure for this very specific case is non-critical and can be numbed down to Assert.Inconclusive. As the message states, a reboot of the test server is need, which will happen at some point, just now now.

Another example is this test helper method

    [MustUseReturnValue]
    private static string[] getPdfContent(string filePath)
    {
        try
        {
            using FileStream fs = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.Read);
            using PdfLoadedDocument document = new PdfLoadedDocument(fs);
            return document.Pages.OfType<PdfPageBase>().Select(page => page.ExtractText()).ToArray();
        }
        catch (Exception ex)
        {
#pragma warning disable MSTEST0058 // Avoid assertions in catch blocks - https://github.com/microsoft/testfx/issues/7340
            Assert.Fail($"Failed to get PDF content: {ex.ToStringRecursive()}");
#pragma warning restore MSTEST0058
            throw;
        }
    }

Of course you could say "Just let the PDF exception bubble up.". But I specifically want to throw an Assert.Fail to make the error clear and not have some 50 layers deep stack trace with PDF specifics that I would need to understand and in the end don't really care about.

[Youssef1313]

We will consider discussing about special casing Assert.Fail and Assert.Inconclusive so that they are not considered by that analyzer.