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

reqnroll / Reqnroll

Open-source Cucumber-style BDD test automation framework for .NET.

https://reqnroll.net

BSD 3-Clause "New" or "Revised" License

302 stars 33 forks source link

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

Closed obligaron closed 3 weeks ago

obligaron commented 1 month ago

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 (https://github.com/microsoft/testfx/issues/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 #119).

Types of changes

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

Checklist:

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

gasparnagy commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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.