MultipleAssertException doesn't provide proper details on failures

[julealgon]

When using Assert.Multiple and assertion failures happen inside the lambda, a MultipleAssertException is thrown by the framework.

This exception has absolutely no details on the actual failures that happened, which drastically impacts the debugging experience: users have to wait for the run to finish and then inspect the Test Explorer window to see what errors have occurred.

This is where it is being thrown currently:
https://github.com/nunit/nunit/blob/efd115b26c8f5d4ebe6d52f5108097ceb0c77ffb/src/NUnitFramework/framework/Assert.cs#L340

            if (context.MultipleAssertLevel == 0 && context.CurrentResult.PendingFailures > 0)
                throw new MultipleAssertException();

This results in the following message:

"One or more failures in Multiple Assert block:"

This gives no hint to what happened and leaves the developer wondering how to see the actual failures. I think this is a really bad practice with exceptions in general: they should provide as much detail as humanly possible.

I'm aware that the display on Test Explorer has full details, but NUnit shouldn't assume that all developers run tests instead of debugging them.

I propose the following:

1. That failures be integrated as inner exceptions
   This would be similar to how an AggregateException works: have all failures as child exceptions so that people debugging the application can see what errors occurred. Maybe using an AggregateException would be ideal, but I heard you had some issues with that because the type doesn't exist in all versions of the framework. Instead of just inspecting the PendingFailures counter like above, we could convert any AssertionResult into AssertionExceptions and add them to the parent MultipleAssertException before throwing it. I'd even suggest ditching the AssertionResult model itself in favor of directly handling AssertionException objects, but that's probably a bigger discussion that I don't want to get into right now.

2. Create an user-friedly .ToString implementation on MultipleAssertException
   In combination with # 1, the exception's ToString should provide a summary of failures. That would allow developers to see what errors were reported easily, and only have to inspect the inner exceptions in case further details are required. Maybe ommiting the stacktrace for each error on this summary would be a good idea, for example.

This lack of debugging support had a very significant impact when migrating to Assert.Multiple on our team: we used to have a custom method to perform this task which reported a much cleaner error in debug mode showing all assertions that failed, and when we switched to Assert.Multiple there was justified backlash.

[CharliePoole]

@anitokj @rafadestri Pleae don't respond with just thumbs up or me too. It serves no purpose except to annoy folks. You can add thumbs up to the post itself if that makes you feel better. 😄

@julealgon That's a well-thought-out and well-explained issue. I'll try to respond in equal detail. Anyone who gets bored may scroll to the end for what I think we should do about it.!

It's well-known and often discussed that use of exceptions in the normal processing of events is an anti-pattern. Of course, for a test framework, it is normal for tests to fail. Unfortunately, unlike C for example, C# doesn't give us an easy way to terminate execution of a test method except by throwing an exception, so that's what NUnit has always done in the past.

Prior to the introduction of warnings and multiple assertions, the AssertionException was used for two purposes: to terminate execution of the test and to report information about its failure. It's a subtle point, but important to realize that these are two distinct functions. As soon as we introduced test result reporting that did not terminate execution (i.e. warnings) we needed to separate these functions.

My ideal implementation would have been to use AssertionResults to report all information and exceptions for termination only. In fact, the initial implementation did just that. Even individual failures outside of multiple assertion blocks did not throw an exception. They merely reported the result and the NUnit component to which it was reported threw the exception, thereby terminating execution. This broke a use patterns for which we had not planned, but which appears to be somewhat common: folks who call their tests directly from a console application, catching the exception themselves. (Who knew?) We "fixed" this problem by handling assertion failures inside and outside of multiple assertion blocks differently. IMO it's a pretty ugly solution but a necessary compromise.

Your own use pattern is another example of something we didn't think of, by the way. That's probably because when we are developing NUnit we are usually running a debug version of the framework code, so we get to see everything. You of course, don't. The saved assertion results are available to you but it takes some understanding of internals and a few clicks to get there.

I don't think we can abandon the AssertionResult model. It's necessary when reporting any information that does not cause termination: warnings now and success in a future enhancement. We need instead to make it work in your situation.

The problem with using exceptions to report the result when you don't want to terminate is that a useful exception, with stack trace, cannot be be created on all platforms except by throwing and immediately catching it. I assume that's what you did in your own custom methods.

PROPOSAL

All the info you want is available at the time the MultipleAssertionException is thrown. It's just not there for you to look at easily. Therefore, my suggestion is that we simply add a Result field to the exception. This would be a reference to the result in the current TestExecutionContext, which is eventually used by NUnit to report the results.

Ultimately, it would be possible for all NUnit exceptions that terminate test execution to work this way, but I think that's a matter for another issue.

[julealgon]

Thanks for the thorough reply @CharliePoole , really appreciate that.

It's well-known and often discussed that use of exceptions in the normal processing of events is an anti-pattern. Of course, for a test framework, it is normal for tests to fail. Unfortunately, unlike C for example, C# doesn't give us an easy way to terminate execution of a test method except by throwing an exception, so that's what NUnit has always done in the past.

Prior to the introduction of warnings and multiple assertions, the AssertionException was used for two purposes: to terminate execution of the test and to report information about its failure. It's a subtle point, but important to realize that these are two distinct functions. As soon as we introduced test result reporting that did not terminate execution (i.e. warnings) we needed to separate these functions.
...
I don't think we can abandon the AssertionResult model. It's necessary when reporting any information that does not cause termination: warnings now and success in a future enhancement...

This makes a lot of sense to me and I totally agree that throwing exceptions for control flow like this is a bad practice, but a necessary evil inside .Net. Now I get why you have a separate, non-exception abstraction of a test result in AssertionResult: I've not used the warning mechanism at all (even though I heard about it, I completely forgot it existed). My suggestion to ditch AssertionResult in favor of raw exceptions wouldn't make too much sense in that design, so please ignore it.

My ideal implementation would have been to use AssertionResults to report all information and exceptions for termination only. In fact, the initial implementation did just that. Even individual failures outside of multiple assertion blocks did not throw an exception. They merely reported the result and the NUnit component to which it was reported threw the exception, thereby terminating execution. This broke a use patterns for which we had not planned, but which appears to be somewhat common: folks who call their tests directly from a console application, catching the exception themselves. (Who knew?) We "fixed" this problem by handling assertion failures inside and outside of multiple assertion blocks differently. IMO it's a pretty ugly solution but a necessary compromise.

I think that's a bold goal (as I said, "exception as control flow" is a known bad practice after all), but unfortunately goes against what people expect of test frameworks these days. Unfortunately by doing that (not returning assertion failures as exceptions with full detail) you create unnecessary confusion among the consumers of the library.

This is exactly what happened to one of our devs. When I proposed to change our custom method into Assert.Multiple (in a PR), he immediately set out to test how it would work (in fear of breaking changes and such) and then raged about the lack of information on the thrown exception during debug. It was a fair concern to me: when creating the test initially, it is very common to debug it, as is the case when you have a broken test that you want to diagnose.

When he sent me the screenshot showing the barebones exception I was so surprised that I almost sent him a reply like "hey, just check the exception itself, there must be an InnerExceptions kind of property in there that clearly shows the errors" but then I stopped for a minute and decided to do the test myself, "just to make sure...": I was extremely surprised when I couldn't find it too. That's when I took a look at the implementation and everything became clear.

Your own use pattern is another example of something we didn't think of, by the way. That's probably because when we are developing NUnit we are usually running a debug version of the framework code, so we get to see everything.

That makes sense, but still intrigues me no one else complained about it. I figure debugging a test is a common enough activity and that folks would quickly realize that details were missing, or that it was wildly different to something like MSTest would show on a failed assertion.

The saved assertion results are available to you but it takes some understanding of internals and a few clicks to get there.

Yeah, that is less than ideal to say the least. I only discovered the internal list of results while inspecting the implementation of some of your base assertion methods (like Assert.Pass/Fail), because I was interested in the way Assert.Multiple worked: "how did they make this work without stopping execution on the first failure if this is all in the same lambda?" "oh, I see... the assertions themselves do not throw when under a Multiple scope so they instead populate this shared list...". It's highly unlikely that a not-so-curious developer would perform this kind of inspection so it is incredibly hidden from view and unintuitive to check a shared collection for assertion failures instead of the exception itself.

The problem with using exceptions to report the result when you don't want to terminate is that a useful exception, with stack trace, cannot be be created on all platforms except by throwing and immediately catching it. I assume that's what you did in your own custom methods.

Pretty much. Our custom method used an IEnumerable<Action> and looped over it, doing a try/catch and storing any intermediate assertion exceptions. At the end of enumeration, it aggregated the results in a single exception with a detailed summary of what it found. I believe you actually discussed that possibility when designing Assert.Multiple, but favored the single lambda approach (which I personally prefer as well, so 👍 from me :P).

PROPOSAL

All the info you want is available at the time the MultipleAssertionException is thrown. It's just not there for you to look at easily. Therefore, my suggestion is that we simply add a Result field to the exception. This would be a reference to the result in the current TestExecutionContext, which is eventually used by NUnit to report the results.

That sounds reasonable and doesn't require the conversion of AssertionFailure to an exception object, so 👍 . Yet, I'd still vote for having a more verbose ToString implementation on the exception itself to go along with this change so that these results are surfaced in an easier to see manner. Putting it on a property in the exception still requires the user to perform some extra clicks to get to it, and people on a debug session would probably rather avoid that.

As I mentioned originally, maybe if we just surfaced the general details about the failures and left the full information inside the "InnerExceptions-like" property, that would be ideal. I believe IoC frameworks usually go with an approach kinda like that: summary of errors on ToString, detailed info as inner exceptions.

Ultimately, it would be possible for all NUnit exceptions that terminate test execution to work this way, but I think that's a matter for another issue.

Yep, seeing your comments on how warnings work, I totally agree.

[CharliePoole]

Sounds like we have an approach here.

Regarding the detail about a ToString() that lists all the failure messages...

The TestResult already contains that - at least eventually - because we add a Message attribute for use by legacy runners that don't know about AssertionResults. I'll have to check the code to see when we are doing it. If it's only after catching the exception, we could probably to do it first, so that it would be embedded in the exception. Then the ToString() would work automatically in the debugger by clicking on the field.

[CharliePoole]

Since this comes back to my earlier work, I'll take this one on. @nunit/framework-team Comments on the proposed design solution?

[jnm2]

You can add thumbs up to the post itself if that makes you feel better. 😄

Unless you're on mobile. 😢

The solution looks good to me.

I haven't seen enough evidence to convince me that to abort tests on failed assertions is an antipattern of any kind. However, if true, that means the idiomatic C# model would then be cooperative aborting. (Sort of like how I wish NUnit would inject a CancellationToken argument if the parameter is present and rely on cooperative cancellation rather than using the dangerous and platform-dependent Thread.Abort).

[CharliePoole]

@jnm2 We hadn't been talking about Thread.Abort but simply about throwing an exception from the code. If one of us used the word "abort" we just meant it generally, as in "immediately stop executing the test."

[jnm2]

@CharliePoole Understood. Cancellation (Thread.Abort vs cooperative) has similarities to assertion failures (throwing vs cooperative) which is why I drew the comparison.

[CharliePoole]

I've experimented with the proposed approach and would like to get a design review from at least a few of the @nunit/framework-team members as well as @julealgon .

1. I would add a TestResult property to at least AssertionException and MultipleAssertException but possibly to all of our ResultStateExceptions. The TestResult would match exactly what is to be used for creating the final test result, If there were multiple errors, failures or warnings. The Message property of both the result and the exception would be populated with the legacy result message listing all the assertion messages individually.

2. One impact of this is that it would no longer be possible to suppress a failure by catching the exception. The failure would already have been recorded. However, you could suppress it by use of Assert.Throw or Assert.Catch instead, in case you wanted to. Note that we have been telling people not to catch AssertionException for years, in anticipation that we might one day do this.

3. On the bright side, folks could still run tests programmatically by executing the test method and catching any exception. They would get all the necessary information, including multiple assert failures, warnings and errors.

If item 2 is considered a problem, we can continue to allow it by having a different path of execution for failures outside of a multiple assert block. This is basically what we do now and it's pretty ugly so I'd rather get rid of it.

I've done some experiments but I'll stop working on this till there is some review of the approach.

[CharliePoole]

Just an added comment in case it isn't obvious.... what I am doing in my code is to move code that currently runs when the exception is caught and executing it earlier, before throwing the exception. That gives us better debug info but with the drawback that it's harder to simply ignore a failure once it has happened.

[CharliePoole]

Changed my mind and did a PR. It may be easier to discuss this with the code in front of us.