nunit: Catching AssertionException fails tests since 3.10.

@fluffynuts A long time ago, when I first wrote Assert.Catch it merely caught any exception and didn’t generate an error if no exception was thrown. Unfortunately, that changed and we no longer have a “neutral” equivalent of Assert.Throws. You could write one, however. Here’s an untested version based on the code in Assert.Throws. It only handles synchronous code but you could extend it easily for async using addional code from Assert.Throws.

public static void Exception SafelyCatchAnNUnitException(TestDelegate code)
{
    Exception caughtException = null;

    using (new TestExecutionContext.IsolatedContext())
    {
        try
        {
            code();
        }
        catch (Exception ex)
        {
            caughtException = ex;
        }

        return caughtException;
    }
}

The method will return any exception that is thrown, cleaning up any remnants of the error left behind by NUnit’s assert methods. If no exception is thrown, it returns null.

It’s also possible to use IsolatedContext() at a higher level in your code so as to retrieve the actual test result from it. That would allow you to test entirely at the level of test outcome, without any reference to exceptions. I’ve given some thought to writing a library for this type of testing, which I think would be superior to the way NUnit currently does its own tests.

The simplest approach is to use Assert.Throws to catch the exception rather than doing it yourself. Assert.Throws understands NUnit internals and ensures that the failure is not reported.

Your test is testing two propositions about how NUnit works:

1. That all assertion failures throw an AssertionException.
2. That catching the exception prevents the error from being reported.

As it happens, the first statement is mostly true but doesn’t hold within multiple assert blocks or when issuing warnings. The second is not true at all. By the time you catch the exception, the failure has already been recorded. This is a change in the internal behavior of NUnit, only observable if you are catching these exceptions. I, for one, have been telling people not to rely on that behavior for years, since it’s merely an accident of implementation.

I realize that your sample code is only intended to demonstrate the problem you see. Can you provide an example of what you are actually trying to do in catching the exception? I’m pretty sure we can help you to find a better solution.

@jnm2.

I agree with you on the importance of the XML docs but fact is that those docs are not how we have been communicating part of the contract up to now. I actually think you may be the first person to suggest it and I think it’s a good idea.

But I think you have to recognize that starting to think of them that way will involve changing a lot of erroneous comments! I do not think we can treat each of these doc changes as a breaking change. On a case by case basis, we can reasonably treat any underlying behavior change as breaking, but that’s different. The docs changes should not be breaking.

This brings up the question of what should be in the list of breaking changes. If I have understood your past comments, I think you tend to want to document anything that might break any user. I prefer to document things we believe would break a lot of users.

Here’s how I see the difference - others may disagree… Documenting everything that might break smells like CYA to me, like the kind of CYA that management sometimes indulges in when they focus on who gets blamed for failure rather than preventing the failure itself. IF we document everything, then we can say… “See, we’re covered.” If we only document what we see as important, then we run the risk of being wrong and having to apologize but we give the average user a much easier to understand set of changes. I prefer the latter.

There are times when it’s safe to catch an assertion exception. For example, I made a change a while back so that it would remain safe when running tests directly, without a TestExecutionContext involved. If you make the exception internal, then you break that. Actually, I was happy to break it, but the team agreed we should not so I fixed it. You just have to decide what you are willing to break. That fix did not handle the current issue, which deals with catching the exception inside the test itself, rather than in the code that invokes the method.

Catching AssertionException in a test is what I would call a classic “smell.” It’s probably wrong, but you have to look at the specific case to know. I would tell users “Catching AssertionException is most likely an error, unless you have detailed knowledge of NUnit internals. To ensure proper behavior, you should not try to change the outcome of the test and should rethrow the exception after doing whatever you want to do with it.” However, I still don’t think such info belongs in the XML docs or the general user docs. I’d put it as a page in the internals section.

BTW, my past experience has been that documenting something and then telling people not to use it leads to increased usage. 😄

So many of our methods document that the API throws AssertionException on failure:

https://github.com/nunit/nunit/blob/d03e93c8f25170a8ff48e80863a3fe6fd294613a/src/NUnitFramework/framework/Assert.That.cs#L40-L43

Doesn’t this documentation, plus the longtime behavior of NUnit, constitute a breaking change in 3.10? Unless I’m missing something, we should at least remove the now-incorrect docs.

@svanimpelen have you tried using the Retry attribute for your flaky tests? It will allow you to run them several times to get them to pass.

As for warn resulting in skipped tests in the explorer, we’ve talked to the visual studio team about allowing us to specify more result states than the limited number from MSTest that they currently support. They like the idea, so hopefully we will see improvements there in the future. Until then you’ll need to suffer with skipped. That said, skipped results in warnings. I would assume you wanted warnings to be very obvious 😄