-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Scenario-Level Parallelization for MsTest #119
Conversation
@obligaron Thx. I will check the permission issue. I need a bit more time to make a proper review of this, but have three questions/notes that we can start thinking on already:
|
Take your time. We are in no hurry. 🙂
Yes the tests were failing before. When I run my newly added test with main branch. They fail with both settings (the new test verifies that the test runs parallel). Running with `ExecutionScope.MethodLevel` in main branch
Running with `ExecutionScope.ClassLevel` in main branch
Can you elaborate?
I put them on the same place where the other parallel execution tests are. 😀 Should all tests move together (in a separate PR) or should I move my test in this PR? |
Thx. Super.
Got the question, but I need the deeper look to make anything more meaningful. In general, Reqnroll should be able to manage Before/After feature without the use of the class-level support from the frameworks, but I don't remember now how. (There must be a code somewhere that "realizes" that you are on a new feature and "close" the previous one and "start" the new one.)
Yeah... we have some legacy to work on. I don't have a strong preference whether it should be a separate PR or in this one. I think I would include it to this one, because then you have more flexibility for experimenting on how convenient they are in the current form. I don't think we can do an "as-is" port of them to the new system, this is why I delayed those hoping for someone, like you to see them with a fresh mind... 😊. So feel free to move them and if you feel that they are not convenient or more complex/slow than it should be, feel free to change. |
Since I'm deep in all the test frameworks thanks to looking at code-generation, I think I should chip in here. The before-/after-feature hooks are definitely the most troublesome to get right, and in a world of parallel execution, trying to talk about them in terms of "previous" and "next" feature is going the wrong way. We have to imagine what happens if all the features were to be executed at the same time or entirely sequentially, or overlapped with each other, or interleaved. I believe we can really only make two guarantees:
That might mean all of the "befores" run immediately at the start of test execution and all the "afters" run right before the test-runner terminates. Trying to control exactly when these things happen in relation to each-other is probably not beneficial, and for those trying to get highly parallelised execution it's actively harmful. Which leads us to relying on the test-framework's hooks to signal when these events occur and dealing with the scope problems it produces, like having to resolve a separate test-runner for the feature-level scopes. |
@Code-Grump Thx for the useful insight. That is another topic that we should have a discussion about. I tell you how it is intended to work currently:
This is the current behavior. From that, it is obviously visible that before/after feature hooks are senseless in a parallel execution environment, but they are also senseless if you run your tests (even single threaded) with a runner that randomizes the execution order. I'm just highlighting this last paragraph, to indicate that we should not over-think the before/after feature hooks problem, because their usage is anyway meaningless in most of the current situations. In fact we should maybe make them obsolete in my opinion, but of course we then would need to have a story to provide backwards compatibility (e.g. make a sample with before/after scenario that simulates the before/after feature by remember the last feature). So probably NOW we should have a solution that is convenient for us (not involving the magical class-level features of the runners) and announce them as obsolete. But let's not spend time on a "better" before/after feature hooks. What do you think? |
I'll give it a try later. 🙂
I have only looked at the MsTest code (yet). But when using MsTest the before/after feature hooks are not invoked by the scenario execution. Before/after feature is triggered by the So from my understanding it should be possible to have a before/after feature implemented correctly (for MsTest at least). I tried to verify it with my added tests and it looks good. But I'm new to the Reqnroll codebase and maybe I'm missing something. So if you could give me a point where I could look at or debug to get more into the problem, I would be glad.
With this PR I have tried to implement this as you describe. There is a static |
@Code-Grump @obligaron I had time to review a bit deeper the current before/after feature implementation and you are right. Actually that code that automatically triggers before/after feature without the need for calling in Before/After feature from generated code does not exists 😀. Maybe I was only dreaming of it or it was just a never-finished PR. I did a quick try now and it seems to be possible, but it depends on another issue (#123). I will take care of that, in the meantime please keep working with the current generation model (with the note that it will be simplified anyway soon hopefully). So ignore my comment about the static test runner field for now, I still need to do the proper review of this PR. Let's continue the whole before/after feature discussion topic at https://github.com/orgs/reqnroll/discussions/124. |
Converting to draft for now, until
|
The PR #277 is a continuation/replacement of this, so I am closing this one. |
Behaviour changes
ExecutionScope.MethodLevel
)ITestRunner
,ITestExecutionEngine
andIContextManager
instance (before one instance was reused)ITestRunner
,ITestExecutionEngine
andIContextManager
have new methodsImplementation notes
BeforeFeature
/AfterFeature
is only called once. This is done with a staticITestRunner
(same behaviour as before). TheITestRunner
needs to be created, because it can be injected as an (optional) parameter into theBeforeFeature
/AfterFeature
methods.ITestRunner
/ITestExecutionEngine
/IContextManager
is created for each Scenario.IObjectContainer
at scenario-level. So hopefully they should not share state and the DI-Logic will still work for them. They override the feature-level objects.IContextManager
toIStepArgumentTypeConverter
to make it independent from DI (needed now there are multiple instances of IContextManager in DI)Open questions
new
instead of the BoDi-Logic. Can a new instance be registered in a child-IObjectContainer
when a instance already exists in the parent-IObjectContainer
?ITestRunner
be registered in theITestRunnerManager
?TestWorkerId
be handled in the per-scenarioITestRunner
? (Currently null)NUnit
support?ITestRunner
,ITestExecutionEngine
andIContextManager
per scenario) should be classified as a breaking change. They should work the same as before (they have the same information etc.) but technically they are not the same instance. So if someone puts them in a static field, they might see some differences in the behaviour.Types of changes
Checklist:
I'm glad that Reqnroll exists and that there is a future for BDD/Cucumber in the .NET ecosystem. This is my first major change for the project. I hope you like it and I'm looking forward to your feedback. 🙂