-
Notifications
You must be signed in to change notification settings - Fork 42
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 for scenario-level parallel execution #277
Conversation
@obligaron please check the code, especially the commit d56b04f Basically what it does:
I also had to modify the test, because the test assumed that there will be only ONE FeatureContext per feature, but this is not true. There might be multiple ones (when the tests of them run parallel), but each scenario should have a feature context that belongs to their feature. TODO (as far as I see):
|
…inux CI that they don't run in parallel)
…el-prototype * origin/main: Fix: Reqnroll generates invalid code for rule backgrounds in Visual Basic (#284)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to have a test that proves steps are running concurrently (e.g. by having them all wait on a semaphore that is reduced by each parallel step and only proceeds when all the parallel steps are blocking together)
public virtual async Task DisposeAsync() | ||
{ | ||
if (Interlocked.CompareExchange(ref _wasDisposed, 1, 0) == 0) | ||
{ | ||
await FireRemainingAfterFeatureHooks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this result in a late release of the feature context?
For example
- Feature A runs for 1 second
- Feature B runs 60 seconds
Feature A's `AfterFeature' hook would only run after 59 seconds or?
I don't think this happens often or is a really bad thing (in MsTest this was always the case before #128). Just wanted to point out the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obligaron Yes, this is correct. Such delayed release of feature context could happen. (But it always runs before the After Test Run hook even in that case.)
@Code-Grump I think we have this. Just maybe not that visible. I have ported the concept we used for the Specs tests earlier:
Do you think this is sufficient? |
It will be sufficient, provided the steps are only used within a single feature. If that step is used by another feature, it will only prove that we can run features in parallel, rather than individual scenarios. |
@Code-Grump Ah, I see. No, this does not prove the scenario-level parallelization only parallelization in general. Actually scenario-level parallelization (that requires method-level parallelization) only possible with MsTest and NUnit. I was wondering if it is our responsibility to have an ongoing check whether the test frameworks did proper method-level parallelization or it is enough to prove that our solution works with their method-level parallelization setting, this is why I did not do an extra check. But the tests were failing before the implementation was done, so they are correct. |
@Code-Grump I have finally added the extra check for the scenario-level parallel execution. |
Hi @gasparnagy, |
@sampath-ganesan thx for pointing this out, let's discuss it there. |
docs/execution/parallel-execution.md
Outdated
* You have to ensure that your code does not conflict on static state. | ||
* You must not use the static context properties of Reqnroll `ScenarioContext.Current`, `FeatureContext.Current` or `ScenarioStepContext.Current` (see further information below). | ||
* You have to configure the test runner to execute the Reqnroll features in parallel with each other (see configuration details below). | ||
|
||
### Execution Behavior | ||
|
||
* `[BeforeTestRun]` and `[AfterTestRun]` hooks (events) are executed only once on the first thread that initializes the framework. Executing tests in the other threads is blocked until the hooks have been fully executed on the first thread. | ||
* All scenarios in a feature must be executed on the **same thread**. See the configuration of the test runners below. This ensures that the `[BeforeFeature]` and `[AfterFeature]` hooks are executed only once for each feature and that the thread has a separate (and isolated) `FeatureContext`. | ||
* As a general guideline, **we do not suggest using `[BeforeFeature]` and `[AfterFeature]` hooks and the `FeatureContext` when running the tests parallel**, because in that case it is not guaranteed that these hooks will be executed only once and there will be only one instance of `FeatureContext` per feature. The lifetime of the `FeatureContext` (that starts and finishes by invoking the `[BeforeFeature]` and `[AfterFeature]` hooks) is the consecutive execution of scenarios of a feature on the same parallel execution worker thread. In case of running the scenarios parallel, the scenarios of a feature might be distributed to multiple workers and therefore might have their onw non-unique `FeatureContext`. Because of this behavior the `FeatureContext` is never shared between parallel threads so it does not have to be handled in a thread-safe way. If you wish to have a singleton `FeatureContext` and `[BeforeFeature]` and `[AfterFeature]` hook execution, scenarios in a feature must be executed on the **same thread**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... therefore might have their onw non-unique FeatureContext
...
The word own
is misspelled.
The phrase could be misinterpreted. Is this correct: If scenarios are distributed, they will have re-use the FeatureContext with other scenarios from the same Feature which happen to also be assigned that worker. This FeatureContext will be distinct from other instances of the FeatureContext for scenarios from that Feature that happen to be executed on other workers.
So, the term 'non-unique' here doesn't convey, to me anyway, what we really want to say. Would 'dedicated' be a better term? 'Re-used'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. i replaced it with the word "dedicated"
🤔 What's changed?
Implementing support for scenario-level (method-level) parallel execution for MsTest and NUnit based on the work of @obligaron in #119.
⚡️ What's your motivation?
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist:
This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.