MsTest: Replace DelayedFixtureTearDown special case with ClassCleanupBehavior.EndOfClass

[obligaron]

This PR removes the special DelayedFixtureTearDown for MsTest. The workaround was used to ensure that AfterFeature was called after all scenarios of a feature had been called.

This is now replaced by ClassCleanupBehavior.EndOfClass. This required a MsTest version of at least 2.2.8 (released end of 2021).

Rationale:

• This will allow us to move forward with MsTest Parallelization, as the current workaround doesn't properly support multiple running scenarios.
• The current workaround uses internal information from MsTest that is likely to change. It relies on the fact that all tests and the ClassInitialize hook running on the same thread (same ManagedThreadId). But this is not guaranteed by the framework and is likely to change with MsTest V4.0 (switch to async pattern internally).

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]

@obligaron Could you please split this PR into two (based on the two commits)? The DelayedFixtureTearDown removal is clear and we could just merge as-is, but the other part, I'm not sure yet. I would only apply that if we really see that coming with MsTest v4.

[obligaron]

@gasparnagy I removed the second commit and adjusted the first commit to have the same ThreadWorkerId as the current main branch. This should ensure that TestThreadContext works the same as before.

I would like to continue working on how the TestThreadContext is handled. So I would be appreciate some feedback.

My understanding:

• The TestThreadContext allows a test user to reuse expansive objects between tests. This is a type of object pooling that is included in Reqnroll.
• The lifetime of the TestThreadContext is managed by Reqnroll. This is currently tied to the TestRunner and the TestWorkerId.

My idea:

• We could remove the TestFramework dependency for handling the TestThreadContext lifetime. This would also remove the problematic logic for MsTest v4 (see above).
• We could handle the TestThreadContext for all test frameworks. Similar to how we currently handle xUnit.
• We could also use the object pooling of TestThreadContext objects when creating new runners for parallel scenario execution.
• We could remove the TestWorkerId if it's only needed for pooling/reusing the TestThreadContext.

What do you think?

[gasparnagy]

I would like to continue working on how the TestThreadContext is handled. So I would be appreciate some feedback.

@obligaron Your proposed solution is more or less what I was also thinking about (that is the option 1) in https://github.com/orgs/reqnroll/discussions/124#discussioncomment-9420376. But I don't think we came to a consensus there whether that should be the way to go, so I would wait with that a bit.

But if you have a bit of time to help, you can take the #123, which is also needed for the full solution anyway. That problem starts at https://github.com/reqnroll/Reqnroll/blob/main/Reqnroll/TestRunnerManager.cs#L103, where we pick one of the TestRunners in order to run the AfterTestRun hooks but does not dispose the others. Instead of that, we should dispose all the existing runners (i.e. dispose their container) and create a new one to run the AfterTestRun hook.

[gasparnagy]

This is good. One additional thing we would need is to change the MsTest dependency version in https://github.com/reqnroll/Reqnroll/blob/main/Plugins/Reqnroll.MSTest.Generator.ReqnrollPlugin/Reqnroll.MSTest.nuspec as well.

[obligaron]

@obligaron Your proposed solution is more or less what I was also thinking about (that is the option 1) in https://github.com/orgs/reqnroll/discussions/124#discussioncomment-9420376. But I don't think we came to a consensus there whether that should be the way to go, so I would wait with that a bit.

At least for me, I thought we were mixing up two related but independent topics in the discussion. Who starts BeforeFeature/AfterFeature and the TestThreadContext object pooling logic.
For me, BeforeFeature/AfterFeature should be started/handled by the test framework, because only the test framework knows when all scenarios of a feature are finished.
On the other hand, the TestThreadContext is a Reqnroll feature that we can handle 🙂

But if you have a bit of time to help, you can take the #123, which is also needed for the full solution anyway. That problem starts at https://github.com/reqnroll/Reqnroll/blob/main/Reqnroll/TestRunnerManager.cs#L103, where we pick one of the TestRunners in order to run the AfterTestRun hooks but does not dispose the others. Instead of that, we should dispose all the existing runners (i.e. dispose their container) and create a new one to run the AfterTestRun hook.

I can have a look at this later. 🙂

[gasparnagy]

thx