Reqnroll icon indicating copy to clipboard operation
Reqnroll copied to clipboard

Support for scenario-level parallel execution

Open gasparnagy opened this issue 1 year ago • 1 comments

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

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

♻️ Anything particular you want feedback on?

📋 Checklist:

  • [x] I've changed the behaviour of the code
    • [x] I have added/updated tests to cover my changes.
  • [x] My change requires a change to the documentation.
    • [x] I have updated the documentation accordingly.
  • [x] Users should know about my change
    • [x] 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 avatar Oct 03 '24 16:10 gasparnagy

@obligaron please check the code, especially the commit https://github.com/reqnroll/Reqnroll/pull/277/commits/d56b04fbafea3b0a609f3be4382c0b1293683c5d

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

  • [x] 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)
  • [x] The concept should also be ported for NUnit
  • [x] The concept should also be ported for xUnit
  • [ ] Remove the obsolete stuff (all class initialize and class cleanup stuff)
  • [x] Fix Generator unit tests
  • [x] 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.
  • [x] Fix that all remaining after feature hooks are called at the end

gasparnagy avatar Oct 03 '24 16:10 gasparnagy

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?

gasparnagy avatar Oct 14 '24 08:10 gasparnagy

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 avatar Oct 14 '24 08:10 Code-Grump

@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 avatar Oct 14 '24 09:10 gasparnagy

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

gasparnagy avatar Oct 14 '24 12:10 gasparnagy

Hi @gasparnagy, please check this as well https://github.com/orgs/reqnroll/discussions/117#discussioncomment-10886899

sampath-ganesan avatar Oct 14 '24 13:10 sampath-ganesan

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

gasparnagy avatar Oct 14 '24 14:10 gasparnagy