nunit: Regression in 3.6.0 when catching AssertionException

@pdpurcell commented on Thu Feb 16 2017

In version 3.5.0, catching a failed assertion would allow the test to continue and the test would then pass. Version 3.6.0 continues running the code, but the final state will be set to failed. Is this a regression? I see no mention of this sort of behaviour change in the release notes.
I have attached an example solution that contains two projects that have identical tests; one using 3.5.0 and one using 3.6.0.
eg:

try
{
    Assert.AreEqual(0, 1);
}
catch(AssertionException)
{
    // The failed assertion is caught but in 3.6.0 the test still fails
}

---

@CharliePoole commented on Thu Feb 16 2017

The bad news is that this has never actually been supported even though it’s a technique promulgated on many blogs and forums. That is…

NUnit makes no guarantee that failures cause an AssertionException that catching the exception will prevent the failure from happening. This is certainly not a guarantee in NUnit 3 and - to my memory - never was in NUnit V2 either.

When this technique has come up, I have frequently tried to warn people off using it. There are lots of reasons to avoid it:

• It implies that your test knows something about the framework that is executing it, which is a serious violation of encapsulation - that is, unless you are actually testing the internals of framework itself.

• There are usually supported ways to accomplish whatever you are trying to do, without the try / catch.

• Where there is an unsupported need, it makes more sense to request a feature, which will be supported now and in the future.

• It’s an implementation detail of NUnit, which could change at any time.

Of course, the last point has come true in the latest release.

In NUnit 3.6, the details of the implementation have changed. We still use an exception to terminate execution of your test. It’s the only way we have to unwind any nested calls you have made. However, we no longer use the exception to report that the test failed. That info is recorded before the exception is thrown. This allows us to use a consistent approach for single assertion failures, multiple assertion failures and warnings.

As you say, there is no mention of this in the release notes. That’s because we felt that there was no need to document an internal change that should never be used by users. For the same reason, we didn’t list this as a breaking change. From some of the feedback, it’s clear that there are users who did not consider this a detail of implementation and relied on it. There are already several issues.

Most of the issues - like yours - illustrate the “problem” without motivating the need to be able to intercept a failure. In each case, I have asked the submitter of the issue to give a motivating example that would help us meet the need. IOW we need to know why you want to be able to prevent a failure by catching the AssertionException. In the long term, we may want to add features that make it unnecessary for you to do such a thing!

Since you’re the fourth or fifth person to raise the issue, it is clear that we need to do something, in spite of my continued belief that you should not really be relying on how NUnit is implemented. I’ll close this comment and start a fresh one with a proposal to partially role back the change.

---

@CharliePoole commented on Thu Feb 16 2017

@nunit/framework-team @nunit/core-team As stated in my first comment here and on several other issues, I continue to believe that folks who rely on AssertionException and particularly on catching it are making a mistake. “NUnit implements failures via an assertion exception and you can stop the failure by catching it” is not a sentence in our documentation and not part of the API we provide to programmers.

Several people have pointed out reasons why it should work, however:

• Most other frameworks work that way
• It has worked in the past
• Exceptions thrown is part of the API of many methods

I could argue against all of those points, but why bother? It seems that a number of users are affected and I think there’s a change that can be made. Here’s what I propose…

1. Stop creating the failure message in the result before throwing an AssertionException outside of a Multiple Assert block.

2. Continue to create the failure message without throwing when inside a Multiple Assert block.

3. Upon exit from a MultipleAssertBlock in which failures have occured, throw a (new) MultipleAssertionException rather than an AssertionException, so that the framework knows that one or more failures have already been recorded and need to be reported.

4. Continue to handle warnings as we do, recording them without throwing an exception.

This will ensure that where we do throw an AssertionException it works exactly as before. Those places where we don’t throw one are new features so no compatibility issue arises.

I think this is critical enough to warrant a hot fix release. Please let me know if you agree with that as well as with the fix itself. I can do the fix myself pretty quickly once we agree on it.