Optimize creation of test-thread context using test framework independent resource pooling

[obligaron]

This PR removes ITestRunner.TestWorkerId and related code.

Notes

• Based on the idea stated in another PR,
• The reuse/caching of TestThreadContext is handled by ITestRunnerManager.
• This PR makes Reqnroll less Test-Framework dependent.
  • Prepare for MsTest V4.0 (Convert MSTest to async pattern microsoft/testfx#1284).
  • Removes special pooling logic done for XUnit.
• Each time a ITestRunner is requested, a new instance is created (no caching). This should help to support scenario parallelization (for example Support Scenario-Level Parallelization for MsTest #119).

Types of changes

• Bug fix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).
• Performance improvement
• Refactoring (so no functional change)
• Other (docs, build config, etc)

Checklist:

• I've added tests for my code. (most of the time mandatory)
• I have added an entry to the changelog. (mandatory)
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[gasparnagy]

I'm not fully finished with my review, but sharing my comments so far.

The idea (of breaking the 1-1 relationship between test runner and test thread container) first sounded strange, but the more I think on it the better I like it. Basically that means that you can anytime "borrow" a test runner (that will have an own worker thread (test-thread)) and use it until needed, but then return. As the runner interface also implies (with start and finish methods), a single test runner cannot be used to run parallel tests, but you can acquire multiple test runners if you want. That sounds good.

My comments:

• We should keep backwards compatibility in regards of the ITestRunner interface, because that is commonly used by plugins and I would like to delay releasing a v3. So we should keep the existing methods and properties on that interface and remove them only with the next major release.
• Regardless of the backwards compatibility, I would keep the TestWorkerId property for diagnostic reasons and make the code in a way that assigns some human-readable ID. (I would also keep it in the trace messages.) This is really useful if you need to understand parallel issues. I have used that several times. (Whether the id is passed to the runner from the runner manager or it is stored in the container does not matter, both would be fine.)
• As far as I understood the code (correct me if I'm wrong), we only keep a registry of the available test worker (test-thread) containers, but not the ones that have been given out for execution. That means if a code borrowed a test runner but never returned (some error), than the container will never be disposed. I'm not sure if it is bad, but we should be aware of this. Maybe we should keep a registry for all test workers to know at least that some of them was not properly returned (and show it in a diag message). What do you think?

[gasparnagy]

And one more comment:

• XUnit and MsTest never had a proper representation of test worker id, so with the new concept we don't lose anything. But NUnit had an a TestWorkerId and we used that. Basically we have also lost that now. I'm not saying that this is a big problem, but good to be aware of it.

[obligaron]

As far as I understood the code (correct me if I'm wrong), we only keep a registry of the available test worker (test-thread) containers, but not the ones that have been given out for execution. That means if a code borrowed a test runner but never returned (some error), than the container will never be disposed. I'm not sure if it is bad, but we should be aware of this. Maybe we should keep a registry for all test workers to know at least that some of them was not properly returned (and show it in a diag message). What do you think?

I think it's a good idea. I was also thinking about asserting/writing a warning in TestRunnerManager.DisposeAsync when we have missing / active TestThreadContainers.
This should be easy with the current code, because the number of active/used containers is _usedTestThreadCount. But perhaps it would be better to store them also in a ConcurrentBag so we can log there Ids/Guids (see question above regarding TestWorkerId).

[obligaron]

Regarding the logging of active TestThreadContainers.
I have implemented a collection of currently active/used containers that we can change in the TestRunnerManager disposed method.
How should the logging be done? Which method would be appropriate (can be seen in all test frameworks)? 🤔

[gasparnagy]

Made a few smaller comments and raised the question of "when to release the test thread context". See below.

[gasparnagy]

@obligaron Side note: For me it is better if you do not apply rebase or force push after an initial review has been done, because this way I have to make a full review again and I cannot just see the "diff". Merging the main to the PR branch is fine - we will squash it anyway.

[gasparnagy]

I reviewed the PR in detail once again and I think this is good to go. I requested a small change in the change log.
I would wait for the v2.0.3 to be released (hopefully today), before merging this, because I think this is more a new feature/improvement, so fits to v2.1.

[gasparnagy]

thx!