nunit / nunit

NUnit Framework
https://nunit.org/
MIT License
2.52k stars 733 forks source link

Detect if inside an Assert.Multiple #4130

Open manfred-brands opened 2 years ago

manfred-brands commented 2 years ago

We have internal Assert functions that do multiple separate NUnit Asserts internally. Main purpose is to compare instance of types that don't override/implement an Equals method.

E.g.:

Assert.That(actualType, Is.EqualTo(expectedType), "Type");

foreach (var property in expectedType.GetProperties())
{
   ///
}

It can be that this routine is called inside an Assert.Multiple, e.g. on a collection of elements. In the above, it doesn't make sense to continue with the properties if the types are different.

Therefore I would like to detect if the code is running inside an Assert.Multiple The property used in the NUnit code itself TestExecutionContext.MultipleAssertLevel is internal. I would like to add a bool IsInsideAssertMultiple => MultipleAssertLevel > 0; property.

Would that be acceptable?

manfred-brands commented 2 years ago

For now I worked around this by making an extension method on TestExcutionContext:

    public static class TestExecutionContextExtensions
    {
        private static readonly PropertyInfo? s_MultipleAssertLevelProperty
            = typeof(TestExecutionContext).GetProperty("MultipleAssertLevel", BindingFlags.Instance | BindingFlags.NonPublic);

        /// <summary>
        /// Detects if the current code runs in side an <see cref="Assert.Multiple(TestDelegate)"/> calls.
        /// </summary>
        /// <param name="context">The context for the test.</param>
        /// <returns><see langword="true"/> if inside an Assert.Multiple call, <see langword="false"/> otherwise.</returns>
        public static bool IsInsideAssertMultiple(this TestExecutionContext context)
        {
            return (int?)s_MultipleAssertLevelProperty?.GetValue(context) > 0;
        }
    }
jnm2 commented 2 years ago

Today you could write this:

 Assert.That(actualType, Is.EqualTo(expectedType), "Type");
+if (actualType != expectedType) return;

 foreach (var property in expectedType.GetProperties())
 {
    ///
 }

With this proposal, you would then have something like: [not sure what I was thinking]

 Assert.That(actualType, Is.EqualTo(expectedType), "Type");
+if (TestContext.IsInsideAssertMultiple) return;

 foreach (var property in expectedType.GetProperties())
 {
    ///
 }

The benefit being that you don't have to restate the constraint model assertion.

What if there was something better so that you don't even need the if statement? Maybe like this (names are placeholders only):

+Assert.NotMultiple(() =>
+{
     Assert.That(actualType, Is.EqualTo(expectedType), "Type");

     foreach (var property in expectedType.GetProperties())
     {
        ///
     }
+});

Or:

-Assert.That(actualType, Is.EqualTo(expectedType), "Type");
+Assert.OrThrow(actualType, Is.EqualTo(expectedType), "Type");

 foreach (var property in expectedType.GetProperties())
 {
    ///
 }
manfred-brands commented 2 years ago

@jnm2 Your second option, which I was actually using, is wrong:

Assert.That(actualType, Is.EqualTo(expectedType), "Type");
+if (TestContext.IsInsideAssertMultiple) return;

 foreach (var property in expectedType.GetProperties())
 {
    ///
 }

Because it would also return early if the types match when running under Assert.Multiple and then not testing the properties. The idea is it would only return if the Assert would fail. I therefore went for the duplicate test option for now.

Regarding alternatives.

We could change the return type of Assert.That returning success or failure. That would allow:

if (!Assert.That(actualType, Is.EqualTo(expectedType), "Type")) return;

The problem is that it would be a breaking change and possible result in lots of warnings for users that they ignore the return value of Assert.That everywhere.

I do like your Assert.NotMultiple proposal. It needs the least amount of changes in nunit and doesn't require a whole set of duplicate Assert.OrThrow for every overload.

stevenaw commented 2 years ago

I like this idea.

While slightly different from the initial use case, there's some times where I'm writing independent assertions where Assert.Multiple() is helpful, but other times I want it to fail fast if any of them fail. I don't want to blanket suppress the analyzer message at project level since there's sometimes I do prefer to use Assert.Multiple, but I also subjectively don't like a lot of #pragmas in my code either. It would be great to have some way to indicate to the analyzers and/or framework that I do want to keep the multiple assertions independent.

I don't know if I like the wording of an Assert.NotMultiple since it relies on knowing the behaviour of Assert.Multiple (which wouldn't be immediately relevant in the above scenario) but I also don't know what could better. Assert.FailFast() maybe - but that's just describing the default behaviour anyways when not in an Assert.Multiple(). Assert.OrThrow() is interesting in that it brings back an option for failed assertions to explicitly throw, an implicit behaviour in some earlier versions of the framework which some users because accustomed to.