Concurrency issues in junit-interface module

[jenshalm]

I believe some of the issues are also a concern in the original project the code was forked from,
however, I'll only describe them from the munit perspective.

TLDR - my recommendations:

1. Remove support for --no-stderr and -q (quiet) flags.
   They are very broken with parallel execution, which is the default in sbt, and cannot be made to work properly either.

2. Simplify EventDispatcher and RichLogger by removing their support for multi-suite and multi-thread usage,
   which is not needed by the munit implementation and which would have race conditions if those features were needed.

In more detail:

Regarding 1)

The --no-stderr and -q flags are currently not documented and also only supported for the JVM version of munit,
but a user might stumble across them in the code and try them out which would lead to a pretty bad surprise.

These options redirect System.out and System.err respectively, and they do this on a per-suite basis.
This leads to unexpected behaviour in the default, parallel run mode, as the suites start to step on each other's toes.
In most cases this will leave System.out still pointing to a ByteArrayOutputStream even after all tests completed, permanently silencing munit until sbt is restarted.
Even reverting the munit configuration change and using reload in the sbt console would not fix the issue.
sbt has to be restarted to get back to normal, making this a really bad trap for users.

The issue is easy to reproduce.

1. Set the -q flag with Test / testOptions += Tests.Argument(TestFrameworks.MUnit, "-q").
2. Leave parallelExecution at its default (true)
3. Run test or testOnly with at least a handful of suites.

Outcome:

• 1st run: The vast majority of munit logs are swallowed, only some make it through.
• Every subsequent run: All munit logs are swallowed. Only sbt's own logger still works (they probably keep a reference to System.out internally).

Given that those features are 1) undocumented, 2) not supported for JS or Native and 3) essentially unfixable,
I think it is best to remove them.

Regarding 2)

This is not a user issue, but more a maintainer's issue, so somewhat less critical.
I think the implementations of EventDispatcher and RichLogger are confusing as they suggest a use case
that does not exist.
In practice both are pinned to a single suite and a single thread (see JUnitTask.execute).
munit does not support nested suites or running the tests of a single suite in parallel.
The concurrent maps, keyed by suite, unnecessarily complicate the code, and give a false sense of security,
as their implementation for concurrent use is not even correct.
(The use of the Stack class in RichLogger has a race condition). There is also a (minimal) performance overhead.

A somewhat more defensive change would be to only remove the multi-suite support, but keep these classes thread-safe.
The current observation that usage is pinned to a thread might theoretically be undermined by future changes in
JUnitCore or MUnitRunner (e.g. when the latter would stop using a parasitic ExecutionContext).
And removing multi-suite support alone already achieves most of the desired simplification.

Both of these suggestions are very simple changes, and I could quickly do them in the coming days,
but I wanted to see whether there are any concerns or objections before I do a PR.

[olafurpg]

Thank you for doing this detailed investigation! Big 👍 on both recommendations.

The --no-stderr option doesn't seem to be used in any public repositories (https://sourcegraph.com/search?q=context:global+--no-stderr+file:.*%5C.sbt&patternType=literal) and the -q option only seems to be used with JUnit (https://sourcegraph.com/search?q=context:global+%22-q%22+file:.*%5C.sbt&patternType=literal )

[olafurpg]

@jenshalm Thank you so much for looking in these issues. Fixed in #401