@bording Thanks for reporting.

Just because you didn't get an error in 4.1, because there was no checking, doesn't mean the code behaved as expected.

Some background.
To support nested Assert.Multiple the code maintains a counter which is increased for every call to the method and decreased after the delegate call is finished.
As long as that counter is greater than zero, all assertion failures are added to a list.
Only if the counter is 0, it will throw an exception if there are any assertion failures.

In your example case, it looks like what should be 1000 independent Assert.Multiple, but they are not.
Let me make an example with a few scenarios:

Scenario 1: Reporting failures for the wrong iteration:

• i=1: Assert.Multiple Starts, increment counter to 1
• i=2: Assert.Multiple Starts, increment counter to 2
• i=2: Assume Assert failure, not reported because multiple counter > 0
• i=2: Assert.Multiple Finishes, decrement counter to 1, but because the counter > 0, it doesn't report its failure!
• i=1: Assert.Multiple Finishes, decrement counter to 0, report all pending failures despite i = 1 has no errors.

Scenario 2: Out of order.

• i=1: Assert.Multiple Starts, increment counter to 1
• i=2: Assert.Multiple Starts, increment counter to 2
• i=1: Assume Assert failure, not reported because multiple counter > 0
• i=1: Assert.Multiple Finishes, decrement counter to 1. This will trigger the release 4.2 out of order decrement as the delegate started with counter 0 and finishes with counter 1. Again the assertion failure for i = 1, is not reported.
• i=2: Assert.Multiple Finishes, decrement counter to 0 and raised failure cause by i = 1.

Your code acts more or less like:

Assert.Multiple(() =>
{
    Parallel.For(1, 1000, i => Assert.That(x, Is.EqualTo(1)));
});

Parallel execution of Assert.Multiple will not trigger errors where they occur, but could raise failures in threads that had no errors at all only because that thread finished last. As that is likely a different thread each time that would lead to very hard to track down failures.

So even though your code didn't throw out-of-order errors, they do occur in release 4.1

Because several part of NUnit use static properties, there is no real way to get independence inside a Parallel.For

So is there any way to get an Assert.Multiple style of thing working from inside of Parallel.For or should I just abandon the idea?

It seems to me that a simpler version that doesn't need the counter and doesn't supported nested Multiple calls would be useful to have as well.

I agree this might be be nice to have a thread-localized or thread-safe multiple scope context. Though I would also suggest that if something weren't thread-safe then it better to fail noisily than fail silently.

@bording
I'm also curious about the nature of your test. Typically asserts are fairly quick or focused. What is it about the nature of your test that requires running Assert.Multiple() in parallel within a single test? I'm wondering if it might be equally preferable if it were instead a single test method which itself were run multiple times in parallel. For example (untested code):

[Test]
[Parallelizable]
public void Repro([Range(1, 1000)] int x)
{
    Assert.Multiple(() => {
        Assert.That(x, Is.EqualTo(1)
    });
}

I'm also curious about the nature of your test. Typically asserts are fairly quick or focused. What is it about the nature of your test that requires running Assert.Multiple() in parallel within a single test?

We recently upgraded from NUnit 3.14.0 to NUnit 4.1.0, and as part of that upgrade process we added NUnit.Analyzers and applied all the fixes that the analyzer suggested. I assume those tests had Assert.Multiple added because of that.

In order to successfully upgrade to 4.2.2, I've gone ahead and removed Assert.Multiple from those tests, which you can see in Particular/NServiceBus#7152

Ah, I see! Thanks for showing an example. It looks like the test is itself testing that something can succeed when run in parallel.

That is a good point too about analyzers prompting to convert towards Assert.Multiple()

@nunit/framework-team I think this might be a good thing for us to fix.

I think another important point here is that, previously, the lack of thread safety in a multiple scope would only be visible in a failure case. Now, our disposal checks are making the lack of thread safety visible on the happy path too, when all assertions would otherwise pass

@stevenaw I don't see this as a bug. Assert.Multipe never supported multi-threading, it just didn't report it.

I would go so far that a lot of NUnit doesn't support multi-threading in a single test.
Code that uses TestExecutionContext.CurrentContext creates an AdHocContext when called from a non-NUnit initiated thread.
When Assert is thrown in a different thread, that is not awaited in the test itself, it is simply lost.

As far as the analyzers go suggestion Assert.Multiple the two assert shown are independent and can be grouped.
The analyzer cannot know if code is executed in multiple threads or not. The developer still needs to think before blindly doing suggested code changes.

Perhaps enhancement then?

My thinking was that most asserts are thread safe, but the original design of Assert.Multiple() wasn't. It would be nice if that inconsistency were fixed so that consumers needn't worry about the difference.