Design Multiple Assert syntax as seen by users

[CharliePoole]

This issue is intended to define a syntax users will make use of in order to interact with the multiple assert feature. Various proposals have been scattered in other issues and I'd like to gather them here. I'm hoping that the folks who proposed each alternative will do that. :-)

[CharliePoole]

The original syntax we discussed was mbunit-like. In mbUnit, you were able to use multiple assertions in two ways:

1. Tag the method with [MultipleAsserts]:
2. Use the Assert.Multiple function and pass in a delegate:

For discussion purposes, I'm calling this the original syntax, since it's the one we started discussing.

We could implement only the second approach, using Assert.Multiple or we could implement both. The attribute-based approach is equivalent to wrapping all the test method's code in an Assert.Multiple block.

In an (actual or implied) Assert.Multiple block, any code may be included. Asserts work differently, not reporting an error until the end of the block. Multiple errors are possible and will be reported.

The block may be terminated prematurely if an unexpected exception is thrown or if the user invokes Assert.Fail, Assert.Pass, Assert.Ignore or Assert.Inconclusive.

[CharliePoole]

@jcansdale You had proposed two alternative syntaxes. @yetibrain I think you have a different syntax in mind based on your comment on #391.

Please describe what you are suggesting as a comment on this issue.

[CharliePoole]

@jcansdale You commented elsewhere that the proposed syntax would break third-party assertion frameworks. I'd appreciate it it if you could provide an example of what might be broken by this syntax, separately from any alternative you might want to propose. I understand that it's possible to break anything with a bad implementation but what I'm trying to understand is why you believe this syntax is intrinsically bad.

[jcansdale]

I don't like option (1) for the same reason I don't like [ExpectedException]. Having asserts scattered throughout a test method isn't something that I'd want to do (or encourage). Option (2) encourages asserts to be grouped together, which IMHO is a much better idea. I'll focus on option (2). 😄

There are 3 issues I see with option (2) above.

1. The stack trace will originate from the Assert.Multiple method, not the assertion that failed. When navigating back to my tests, I really want to be taken to the (maybe first) assertion that failed.
2. I can see how this might work with NUnit's native assertions, but what about with popular 3rd party asserts and mocks? I'd want to use Assert.Multiple just as much with them.
3. With coverage based test runners, it matters that an exception is thrown from a failed assert. They use this to mark the point (or points) where a test failed.

An alternative syntax would be to specify each individual assertion as a lambda:

            Assert.Multiple(
                () => Assert.That(foo.Bar, Is.EqualTo("foobar")),
                () => MyAssertLib.CheckBar(foo, "foobar"),
                () => foo.Received(1).SetBar("foobar")
            );

This has they advantage that exceptions originate from user code, 3rd party assertion and mocking exceptions can be handled and coverage based test runners can see where the test failed (even at multiple locations).

Here is an example of how the different options might look:

I think the output from the first (individual lambda based syntax) contains more useful information. You can see all the points where the test failed. Also, I suspect the implementation will be simpler (no caching of assertion objects and potential multi-treading issues).

Does any of that make sense?

[jcansdale]

@CharliePoole You beat me by 3 minutes, dammit! 😉 I've elaborated a bit more in the post above (that crossed with yours).

[CharliePoole]

@jcansdale I deleted my comment.

[CharliePoole]

@jcansdale I don't particularly like option 1 either, but it was requested somewhere and I can live with it so long as it doesn't grow more features the way ExpectedException did. I can imagine using it for a very simple test that checked a number of properties. I could live without it as well.

Your list of three things you don't like about the Assert.Multiple syntax seems to be based on the assumption that proper stack traces would not be available. I don't understand why you think the form of syntax would have anything to do with that. It is true that in your example implementations, the single-lambda approach doesn't give a proper stack trace, but that's just - dare I say - an error. :-)

It's possible to implement the single lambda approach in a way that preserves stack traces. I already did it once, when this issue got started.

Further, with the multiple lambda approach, there is no guarantee that the user won't include a bunch of other code in a particular lambda. There might even be two asserts! So any implementation has to deal with multiple assertions within the code block unless we make the definition of Assert.Multiple highly restrictive with lots of "It won't work if you..." warnings to the user.

One thing I don't understand is how ncrunch is marking the failing asserts in your second example. You throw an aggregate exception which, IIRC, we are not yet handling or reporting in any intelligent way. Maybe I've forgotten some enhancement we did.

[CharliePoole]

@jcansdale OK... I understand the last bit now. We added some support for AggregateException when we were doing async. It's not pretty, but it does pull out every inner exception. Our stack trace field actually contains all the different stack traces, in a form that ncrunch is able to parse.

I think this is one of those "accidental features" and I don't think it's really the best way to support multiple errors, but we probably have to maintain it for compatibility.

[CharliePoole]

Another thing I feel a bit queasy about with the multiple-lambda syntax is that some users will assume we are executing the lambdas in order, which is how you implemented it, while others will wonder if the order is guaranteed and even if we might be executing them in parallel. The single format is just a block of code, which doesn't seem to leave any room for doubt.

[jcansdale]

@CharliePoole,

Your list of three things you don't like about the Assert.Multiple syntax seems to be based on the assumption that proper stack traces would not be available. I don't understand why you think the form of syntax would have anything to do with that.

With the multiple lambda approach, exceptions pass through user code and are caught by the Assert.Multiple method (kind of like nested unit tests).

With the the single lambda approach, assert results are stored and a single exception is thrown from the Assert.Multiple method. Even if exceptions are thrown for each failed assert, they won't pass through user code and no user stack frames will appear in them. It might be possible to fake the stack trace, but this just adds complexity.

So any implementation has to deal with multiple assertions within the code block unless we make the definition of Assert.Multiple highly restrictive with lots of "It won't work if you..." warnings to the user.

I think if the assert takes two or more lambdas as parameters (we can require that), is should be pretty evident what's going on.

Another thing I feel a bit queasy about with the multiple-lambda syntax is that some users will assume we are executing the lambdas in order, which is how you implemented it, while others will wonder if the order is guaranteed and even if we might be executing them in parallel.

The asserts should just be doing a bunch of logic, not order dependent madness. If users are worried about the order, they're doing it wrong.

We've maybe got a bit side tracked with AggregateException. The idea is to throw a bundle of real exceptions from the Assert.Multiple method and deal with them when the usual exception handling happens. It doesn't have to be AggregateException, that just seemed like a good fit (compatible with async and maybe 3rd-party multi-asserts).

One thing I don't understand is how ncrunch is marking the failing asserts in your second example. You throw an aggregate exception which, IIRC, we are not yet handling or reporting in any intelligent way.

I'll let @remcomulder explain what's going on with NCrunch. It's not doing what you think it's going. 😉

[CharliePoole]

But the single-lambda approach (I think of it as the multple-asserts-per-lambda approach) doesn't have to be implemented as you did it. In fact, if necessary, we can let the asserts throw, but immediately catch the result and save it, just as you do in the multiple-lambda approach.

I agree we can live with the ordering thing - I merely feel queazy, but I'm not yet throwing up. :-) Still, a lambda is an arbitrary block of code and we can't readily enforce what users do inside it.

I'm a big adherent of the notion that we should first figure out what should be useful to users. Easy for third-party developers to support comes second. Ease of implementation for us comes last.

[jcansdale]

In fact, if necessary, we can let the asserts throw, but immediately catch the result and save it, just as you do in the multiple-lambda approach.

Ah, but there lies the problem. You're throwing and immediately catching inside NUnit code. The stack trace won't contain any user code. What you're suggesting is how I sketched out my multple-asserts-per-lambda implementation (from the AssertMultipleY screen grab above).

Here it is:

    public class Assert
    {
        [ThreadStatic]
        static List<Exception> exceptions;

        [ThreadStatic]
        static bool multiple;

        public static void Multiple(Action action)
        {
            try
            {
                multiple = true;
                action();
                if (exceptions != null)
                {
                    throw new AggregateException(exceptions);
                }
            }
            finally
            {
                exceptions = null;
                multiple = false;
            }
        }

        // repeat for all other assert methods
        public static void That<TValue>(TValue actual, IResolveConstraint constraint)
        {
            try
            {
                NUnit.Framework.Assert.That(actual, constraint);
            }
            catch (Exception e)
            {
                if(!multiple)
                {
                    throw;
                }

                if (exceptions == null)
                {
                    exceptions = new List<Exception>();
                }

                exceptions.Add(e);
            }
        }
    }

But the single-lambda approach (I think of it as the multple-asserts-per-lambda approach) doesn't have to be implemented as you did it.

It's quite possible I've missed an alternative approach. How would you go about resolving the missing stack frames issue?

[CharliePoole]

I'll give it a shot when I get some other stuff done that I'm working on. I did it about a year ago as a spike, which I didn't keep, but it should be re-creatable.

[remcomulder]

I think there's a little bit of confusion about how NCrunch is handling these two options. I'm personally fairly indifferent to the syntax either way, but perhaps I can shed some light on NCrunch would handle each scenario.

In the current version of NCrunch, @jcansdale 's example of creating an assertion pattern that uses multiple lambdas and AggregateException will cause NCrunch to parse the .ToString() output of AggregateException. Because AggregateException.ToString() reports all stack lines across all exceptions, this will cause NCrunch to think of it all as a single exception with ALL stack lines from all nested exceptions. Technically, this is incorrect as all the navigation for multiple exceptions gets rolled together. A side effect of this is that NCrunch does show Xs next to each of the assertion failures, which may be an advantage.

Ideally, we want a fully separate exception reported for each individual assertion failure. This would be structurally more correct, but it cannot be handled without being able to represent a test with multiple exception results in the runner. NCrunch does have structural handling for this but it isn't well tested, as so far there hasn't really been any way of using it outside of exceptions thrown on background threads.

One way to handle this would be for NCrunch to implement special parsing for AggregateException's string output, splitting it up into its original components. This is a bit messy but would give good support for other frameworks or toolsets that want to report multiple exceptions for a single test.

From my understanding the main two options for multiple assertion syntax can each be implemented using AggregateException. Having a single lambda for all the multiple assertions looks more difficult to implement from NUnit's side, but it certainly looks possible to me. I presume that each individual assertion method would need to be rewritten to store its exception into some kind of state that is rolled up at the end of the multiple block into AggregateException.

I still think that using AggregateException is the best way to report the multiple assertion failures. I understand that @charliepoole wants to support this anyway, but if its used as the sole mechanism, I will need to do less work as I don't need to implement another way to capture this data :)

Using AggregateException also gives a least some backwards compatibility with prior versions of NCrunch by showing the Xs in the gutter next to each assertion failure, even if the exceptions aren't represented correctly in the runner's internal model.

[CharliePoole]

@remcomulder More than a little! :laughing:

My main confusion is that I don't understand how you could possibly be handling exceptions at all, since they never leave the framework. Aren't you limited to parsing what's in the XML output? Or are you doing something tricky we don't know about. :wink:

[remcomulder]

@CharliePoole All NCrunch is doing is parsing each line of the exception's string output. NCrunch actually doesn't know its an AggregateException, but because AggregateException.ToString() calls .ToString() on all its nested exceptions, the exception stack lines all end up in the same dump. NCrunch thinks its dealing with one big exception, and each failure is just one stack frame in that exception.

This is entirely accidental. I never saw it coming :)

[CharliePoole]

I'll add that unless you are doing something tricky, what you get in the output is completely removed from our use or non-use of an AggregateException or some other internal implementation, and even farther removed from the syntax.

I think of it like this...

      User Syntax =/=> Internal Implementation =/=> Output XML

where the broken arrow stands for some opaque transformation we are performing. This is already the case in the current release. The internals will change again in implementing this feature, but my feeling has been that we don't have to worry so much about that if we can get the external part right.

But if something I'm thinking of as internal is actually used externally, that throws it all into a cocked hat and I'd like to know about it ahead of time.

BTW, we can't actually use an AggregateException. Because not all our builds support it, we would have to use a work-alike exception of our own if we decided we need something like that. But that's still an internal implementation detail. :smile:

[CharliePoole]

@remcomulder But how are you getting any exception out of the framework? Any escaping exception is a bug as far as we are concerned.

[remcomulder]

@CharliePoole I get the exception text from the NUnit XML, not the exception itself :)

But this exception text seems to include all the stack trace lines from all exceptions nested inside AggregateException. I assumed that NUnit was calling .ToString(), but perhaps you are doing something more involved?

I understand the build limitations with AggregateException only existing in .NET 4.0 and above. An equivalent type declared in NUnit itself would work just as well :)

[CharliePoole]

@remcomulder You guessed right that it was your reference to ToString() that was confusing me. The stack trace you see is actually constructed by NUnit here: https://github.com/nunit/nunit/blob/master/src/NUnitFramework/framework/Internal/ExceptionHelper.cs#L100

We display all inner exception stack traces, including those from AggregateException, preceding each new trace by a line like "--SomeExceptionType". We do something similar for messages.

So if what you are saying is "Don't change that format" then we're cool. 😄 The failure element will continue to work as it now does. On the other issue, I'll be putting out something so we can discuss whether we only preserve backward compatibility or actually enhance the failure element to allow for multiple assertions.

[remcomulder]

@CharliePoole Ahh. In that case, the compatibility advantages of reporting using this exception are probably a bit more limited. Other frameworks would need to implement the same functionality as you have here for NCrunch to be able to dig deeper into the exception, unless they provided access to the exception object itself.

Maintaining the existing string output format for this exception will definitely be useful :)

[jcansdale]

Sorry about the delays responding. I have a 2yr old who's idea of programming is, "I want to press a button and make it green", which isn't always conducive to getting stuff done. 😉

It seems NCrunch isn't doing what I thought it was doing. I thought its coverage instrumentation could detect the exceptions falling through and mark them accordingly. I'd glad I pulled in @remcomulder for clarification. My concerns about failed assert exceptions not passing through user code were unfounded (at least for this scenario).

I'll give it a shot when I get some other stuff done that I'm working on. I did it about a year ago as a spike, which I didn't keep, but it should be re-creatable.

I'm prototyped some code that rather than getting the stack trace from from exception object, instead recreates what it would have looked like by retrieving the calling frame using new StackFrame(1, true). Is this what you did in your spike?

Here is what a single-lambda implementation looks like (that uses new StackFrame(1, true)):

    using System;
    using System.Diagnostics;
    using System.Collections.Generic;
    using NUnit.Framework;
    using NUnit.Framework.Constraints;
    using NUnit.Framework.Internal;

    public class Assert
    {
        // Should store in text context.
        static List<Exception> multipleExceptions;

        public static void Multiple(Action action)
        {
            try
            {
                multipleExceptions = new List<Exception>();

                action();
                if (multipleExceptions.Count > 0)
                {
                    throw new MultipleAssertionException(multipleExceptions);
                }
            }
            finally
            {
                multipleExceptions = null;
            }
        }

        // Repeat for other assert methods.
        public static void That<TValue>(TValue actual, IResolveConstraint constraint)
        {
            if(multipleExceptions == null)
            {
                NUnit.Framework.Assert.That(actual, constraint);
                return;
            }

            var result = constraint.Resolve().ApplyTo(actual);
            if (!result.IsSuccess)
            {
                var writer = new TextMessageWriter();
                result.WriteMessageTo(writer);
                var message = writer.ToString();

                var stackFrame = new StackFrame(1, true); // Use the calling stack frame.
                var exception = new InnerAssertionException(message, stackFrame);

                if (multipleExceptions == null)
                {
                    multipleExceptions = new List<Exception>();
                }

                multipleExceptions.Add(exception);
            }
        }
    }

Here is what a multiple-lambda implementation looks like (using the exception object).

    using System;
    using NUnit.Framework;
    using System.Diagnostics;
    using System.Collections.Generic;

    public class Assert
    {
        public static void Multiple(params Action[] asserts)
        {
            var multipleExceptions = new List<Exception>();
            foreach (var assert in asserts)
            {
                try
                {
                    assert();
                }
                catch (Exception e)
                {
                    var frames = new StackTrace(e, true).GetFrames();
                    var frame = frames[frames.Length - 2];
                    var exception = new InnerAssertionException(e.Message, frame);
                    multipleExceptions.Add(exception);
                }
            }

            if (multipleExceptions.Count > 0)
            {
                throw new MultipleAssertionException(multipleExceptions);
            }
        }
    }

Here are the two assertion exceptions I used in both:

using NUnit.Framework;
using System;
using System.Diagnostics;
using System.Collections.Generic;

public class InnerAssertionException : AssertionException
{
    StackFrame stackFrame;

    public InnerAssertionException(string message, StackFrame stackFrame) : base(message)
    {
        this.stackFrame = stackFrame;
    }

    public override string StackTrace => new StackTrace(stackFrame).ToString();
}

public class MultipleAssertionException : AssertionException
{
    IEnumerable<Exception> innerExceptions;

    public MultipleAssertionException(IEnumerable<Exception> innerExceptions) : base("One or more assertions failed.")
    {
        this.innerExceptions = innerExceptions;
    }

    public override string StackTrace
    {
        get
        {
            var stackTrace = "";
            foreach (var exception in innerExceptions)
            {
                stackTrace += "----> ";
                stackTrace += exception.Message + Environment.NewLine;
                stackTrace += exception.StackTrace + Environment.NewLine;
            }

            return stackTrace;
        }
    }
}

After playing with both implementations, I've realized they each have advantages and disadvantages. Ideally I'd want to best of both worlds. 😉

The advantage of the multiple-lambda implementation is that it works really nicely with 3rd-party mocking frameworks. Not only does it allow multiple verifications to be made, but because we're declaring the lambda to be an assertion call, we can clean up the train-crash of a stack trace that often comes with failed verifications. I'd certainly use this feature in some of my tests.

The single-lambda implementation has the advantage of being the de facto standard. It also allows for complex control flow within within the assertion block (say I wanted to loop and assert a bunch of stuff or call a method that does multiple asserts).

I believe it is possible to have the best of both worlds, if we had a clean syntax for converting 3rd-parts asserts/verifications into NUnit asserts.

For example, say we could wrap an NSubstitute verification like this. Assert.That(() => sp.Received(1).GetService(typeof(string)));

We could then mix NUnit and 3rd-party assertions using the single-lambda approach.

In terms of clarity and functionality, I think this would be my preference.

I've attached the code and tests for the above in a single .cs file here: SingleLambda.zip

What do you think?

[oznetmaster]

You should be aware that StackFrame is not available in the CF build :(

[jcansdale]

Hmm. Does the CF have a working version of Environment.StackTrace?

[rprouse]

I know you wouldn't be happy with the decision @oznetmaster, but I would be okay if some features don't make it into all versions of the framework. We should try wherever we can, but I don't think we should compromise the design to support older frameworks. That is part of the reason we want to spin CF and Silverlight out to their own repos, so they can go in the directions that make the most sense for each platform.

[oznetmaster]

No

[CharliePoole]

Whoever actually implements this feature will decide what to do about Environment.StackTrace, including whether to support CF in some way. Then we'll all get an opportunity to comment on whatever that implementation choice is in the course of code review.

In order to avoid contributing to this mess of premature implementation detail, I'm not going to go back and recreate my earlier implementation of the "single lambda" option. We have too much code already. I'm just going to summarize how I see the options for users and pick one. Let's try to keep comments to the user interface, including third party framework users, and stay away from how each of us would code the choice.

So...

We have two main options plus a variation:

1. The original syntax pulled from mbUnit, which we have started calling the "single lambda option".

   Assert.Multiple(() => {
     // Arbitrary code, which may contain multiple assertions
   });

2. The syntax proposed by @jcansdale , which we have called the "multiple lambda option".

   Assert.Multiple(
     () => Assert...;
     () => Assert...;
     ...
   );

   On the surface, this might look like option 1 with extra lambdas. However, from our earlier discussion, it seems to limit each lambda to containing only a single Assert.

3. A variant, which pre-supposes #1, allowing an entire test to be a mutliple assert block

   [Test, MultipleAsserts]
   public void MyTest
   {
     // An implicit single lambda code block
   }

At this point, I propose we go with option 1. If it becomes useful at some future time, we could add multiple-lambda overloads, but without the implied implementation limit to one assert per entry. However, I can't think of why we would want to do that since a test can readily contain multiple Assert.Multiple blocks.

There are arguments pro and con the option 3 variant. It can be added later if it's really needed.

I'd like to make this selection and close the issue so we can get on with other things. Comments?

[rprouse]

I agree, I prefer (1) the single lambda approach 👍

[jcansdale]

Assuming we have a way to accommodate 3rd-party mocking/assert libraries, I also prefer the single lambda approach. It's both more flexible and clearer what's going on. 😄

[remcomulder]

Another vote here for single lambda.

[CharliePoole]

I updated the existing spec for this feature to match our renewed conclusions: https://github.com/nunit/docs/wiki/Multiple-Asserts-Spec