Closed obligaron closed 1 month ago
@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.
@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:
TestThreadContext
allows a test user to reuse expansive objects between tests. This is a type of object pooling that is included in Reqnroll.TestThreadContext
is managed by Reqnroll. This is currently tied to the TestRunner
and the TestWorkerId
.My idea:
TestThreadContext
lifetime. This would also remove the problematic logic for MsTest v4 (see above).TestThreadContext
for all test frameworks. Similar to how we currently handle xUnit
.TestThreadContext
objects when creating new runners for parallel scenario execution.TestWorkerId
if it's only needed for pooling/reusing the TestThreadContext
.What do you think?
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.
@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. 🙂
This PR removes the special
DelayedFixtureTearDown
for MsTest. The workaround was used to ensure thatAfterFeature
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:
ClassInitialize
hook running on the same thread (sameManagedThreadId
). 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
Checklist: