Skip to content
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

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

gasparnagy
Copy link
Contributor

@gasparnagy gasparnagy commented Oct 3, 2024

🤔 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?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

This text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.

@gasparnagy
Copy link
Contributor Author

gasparnagy commented Oct 3, 2024

@obligaron please check the code, especially the commit d56b04f

Basically what it does:

  1. Makes the "testRunner" field instance (non static), so that it does not conflict with the different threads.
  2. Removes the class initialize method (as we cannot use static methods now)
  3. Removes the class cleanup method (same reason)
  4. Obtains the testRunner in the test initialize method
  5. Handles feature changes and initialization in the test initialize method
  6. Releases the test runner in the test cleanup method

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):

  • The test should be moved to the system tests (probably to the "Generation" part, so that we can test it with all frameworks, but it can be even a new test class) and it should ensure
    • tests are run in parallel (the simple check I inserted is enough)
    • each scenario execution has a feature context and scenario context that is matching to the feature/scenario
    • for each scenario the before feature has been executed for that feature context (in a before scenario, set some value to the feature context and in the step definition check if it is already there)
  • The concept should also be ported for NUnit
  • The concept should also be ported for xUnit
  • Remove the obsolete stuff (all class initialize and class cleanup stuff)
  • Fix Generator unit tests
  • The initialization of featureInfo could be moved to a static readonly field so the comparison in the test initialize could compare the current feature info with the one in the static readonly field with ref equals instead of the feature name comparison we have now.
  • Fix that all remaining after feature hooks are called at the end

@gasparnagy gasparnagy changed the title Scenario level parallel prototype Support for scenario-level parallel execution Oct 7, 2024
@gasparnagy gasparnagy marked this pull request as ready for review October 7, 2024 14:30
Copy link
Contributor

@Code-Grump Code-Grump left a 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();
Copy link
Contributor

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.

Copy link
Contributor Author

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

@gasparnagy
Copy link
Contributor Author

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)

@Code-Grump I think we have this. Just maybe not that visible. I have ported the concept we used for the Specs tests earlier:

  1. In the beginning of the when step definition we receive a current unique execution index: var currentStartIndex = System.Threading.Interlocked.Increment(ref startIndex);
  2. At the end of the when step definition we compare the "actual" step execution index with the one we remembered if these are different (ie another step have been started in the meanwhile), we log out a parallel: true statement.
  3. In the test we assert that at least once the statement has been logged (ie there were at least one step definition that was running parallel with another one).

Do you think this is sufficient?

@Code-Grump
Copy link
Contributor

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.

@gasparnagy
Copy link
Contributor Author

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

@gasparnagy
Copy link
Contributor Author

@Code-Grump I have finally added the extra check for the scenario-level parallel execution.

@sampath-ganesan
Copy link

@gasparnagy
Copy link
Contributor Author

@sampath-ganesan thx for pointing this out, let's discuss it there.

* 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**.
Copy link
Contributor

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'?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants