Assert.Multiple method should fail only if a contained assertion failed

[rockstome]

Here is code

namespace Tests
{
    public class NUnit
    {
        [TearDown]
        public void TearDown()
        {
            System.Console.WriteLine("Teardown 1");
            Assert.Multiple(() =>
            {
                Assert.That(true);
            });
            System.Console.WriteLine("Teardown 2");
        }

        [Test]
        public void Test()
        {
            System.Console.WriteLine("Test 1");
            Assert.That(false);
            System.Console.WriteLine("Test 2");
        }
    }
}

What is very strange for me, line where "Teardown 2" should be printed is not executed. Why is this happening? Assertion in teordown is not throwing any exception

[stevenaw]

Thanks @rockstome this one is particularly curious. It looks like Assert.Multiple is detecting if any assertion failed in the test and not just what was contained in the multiple block itself. For example, the following will output "Test 1" and "Test 2" but not "Test 3"

[Test]
public void Test()
{
  System.Console.WriteLine("Test 1");
  
  try
  {
      Assert.That(false);
  }
  catch { }
  
  System.Console.WriteLine("Test 2");
  Assert.Multiple(() => { });
  System.Console.WriteLine("Test 3");
}

That said, I'm not sure if I'd call this a bug per se or that it's working by-design. One almost has to "force" this to fail wholly within a test by suppressing a previous assertion failure from outside the multiple block like in the example. I don't know if it was ever considered how Assert.Multiple would behave with assertions in a TearDown but based on rprouse's comment here asserting from a teardown is something that's discouraged.

Your example is obviously a minimal repro which is appreciated, but to help understand how you originally hit this: is there something you're needing to assert in your TearDown that can't be handled elsewhere?

[manfred-brands]

Changing your Test to not fail, causes both Test 2 and Teardown 2 to be printed.
Making your Test fail again and replacing the Assert.Multiple with the Assert stops printing Test 2 as expected, but still prints Teardown 2.
It therefore looks as if the failure status is not reset and causes Assert.Multiple to report the same failure again.
Assert.Multiple reports if there are any failures, regardless if they existed before calling Assert.Multiple:

            if (context.MultipleAssertLevel == 0 && context.CurrentResult.PendingFailures > 0)

Other places in NUnit remember the count before running their own code and only report if the errors increased:

                    // If there are new assertion results here, they are warnings issued
                    // in teardown. Redo test completion so they are listed properly.
                    if (context.CurrentResult.AssertionResults.Count > oldCount)
                        context.CurrentResult.RecordTestCompletion();

So maybe we need to update the two implementation of Assert.Multiple to follow the pattern above.

[stevenaw]

Good catch, thanks @manfred-brands ! I agree, it's expected that Assert.Multiple() should only be concerned with the contents of its containing block.

[mikkelbu]

Please note that we have often discouraged assertions in the teardowns - see e.g. #3957 or #2938 - but that being said @manfred-brands proposal sounds reasonable.