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

Use tags to opt out of feature parallelization #2409

Merged
merged 13 commits into from
Jun 9, 2021

Conversation

shack05
Copy link
Contributor

@shack05 shack05 commented Apr 24, 2021

I was looking at fixing issue #2066 but realised the approach in #746 of opting in to parallelization doesn't align well with the unit test frameworks which provide opt out behaviour.

MSTest v2, NUnit, and XUnit each have a mechanism to opt out of parallelism for a test class.

This PR proposes to add a NonParallelizableDecorator class which, given a feature has a tag matching one of the values in configuration, will call IUnitTestGeneratorProvider.SetTestClassNonParallelizable(TestClassGenerationContext generationContext) to decorate a class with an attribute which indicates the class should not be run in parallel.

Provider Implementation
NUnit3 Add NonParallelizableAttribute to generated class
xUnit Add CollectionAttribute with value NonParallelizable to generated class, see Custom test collections in Running tests in parallel
MSTest v2 Add DoNotParallelizeAttribute to class, as described here
MSTest Not supported, MSTestGeneratorProvider does not have UnitTestGeneratorTraits.ParallelExecution

This PR is not complete, there are a few outstanding items:

1. XUnit CollectionDefinition

By default any tests within the same collection will not run in parallel, however tests within a collection may run in parallel with tests in any other collection. This behaviour is different than what the above implementations for MSTest v2 and NUnit3 provide where any test class which has opted out of parallelization will not run in parallel with any other class at all.

To get the same behaviour in XUnit you can define a CollectionDefinition in the test assembly:

    [CollectionDefinition("NonParallelizable", DisableParallelization = true)]
    public class NonParallelizableCollectionDefinition
    {
    }

As far as I can tell XUnit does not provide a way to define a collection definition in an external assembly (see xunit/xunit#1541). Is there a way for specflow to automatically add that class definition to users test projects or would documentation be required to inform users that they must add it themselves?

Also, I'm not particularly familiar with xunit so if you are aware of an easier alternative please let me know :)

2. ParallelizeDecorator

This PR provides an alternative to ParallelizeDecorator and IUnitTestGeneratorProvider.SetTestClassParallelizable(TestClassGenerationContext generationContext).
Should I remove them? Would that be considered a breaking change (I can't find any documentation for them, and the current implementation doesn't work)?

3. Existing support for MSTest v2 DoNotParallelize

I've noticed that MSTest v2 already has support for DoNotParallelize by using tag @MsTest:donotparallelize, see #996.
I can't find any documentation or existing tests for that. I don't think this PR conflicts with this existing behaviour but just mentioning it here in case.

4. Changelog

Is this change considered breaking? Also, is it a bugfix or a new feature?

5. Documentation

I can update Configuration and Parallel Execution.

edit: added later

6. Configuration

This PR is reusing the generator.SkipParallelizableMarkerForTags configuration property from the ParallelizeDecorator feature t to allow the user to specify which tags should be used to mark a feature as non-parallelizable. We should either introduce a new configuration property or rename the existing, based on the outcome of item 2.

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Performance improvement
  • Refactoring (so no functional change)
  • Other (docs, build config, etc)

Checklist:

  • I've added tests for my code. (most of the time mandatory)
  • I have added an entry to the changelog. (mandatory)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@epresi
Copy link
Contributor

epresi commented May 10, 2021

Hey @shack05,

Thanks for the PR.
Because of this change we have to think about parallelization in SpecFlow (how to opt-in, opt-out, configs, etc) a bit more. For that we would need more time.
I noticed that the linked issue is not fixed, so it is still generating bad codebehind files if you set markFeaturesParallelizable to true. Is it intended? Are you plan to fix that "path", or you just opened the PR with this new feature request?

@shack05
Copy link
Contributor Author

shack05 commented May 10, 2021

Hi @epresi,

I do not intend to fix the linked issue. My proposal is that this PR will make the ParallelizeDecorator redundant because I think this change (NonParallelizableDecorator) solves the same problem (allowing some, but not all, features to run in parallel) in a way that applies to MSTest, NUnit, and xUnit whereas the original change (ParallelizableDecorator) could only apply to NUnit and xUnit.
AFAIK there is no way to opt-in to parallelisation at a class level using MSTest - you must enable parallelisation at an assembly level and then mark any test classes/methods which you do not want to run in parallel.

Also, this change is more closely aligned to existing SpecFlow documentation around parallelisation.

I linked the issue for visibility in case anybody else was interested in fixing the issue.

Also, looking at the original PR which introduced the ParallelizeDecorator, it looks like the issues outlined in the linked issue have been there from the start (for example look at line 44 of NUnit3TestGeneratorProvider, the value of generationContext.TestClass.Name should not be passed as an argument to the ParallelizeAttribute).

Of course I am open to any suggestions and feedback, I'm just trying to make my reasoning clear.

Thanks

@epresi
Copy link
Contributor

epresi commented Jun 2, 2021

Thanks for your reply, and sorry that mine is so late.

We discussed this PR with the Team and we would merge it.
One thing left in this PR, the documentation. Because the original feature (#746) was not documented at all (doc request still open #1497 ), can you please create a page near/under Parallel Execution?

@@ -94,7 +97,7 @@ protected bool Equals(SpecFlowConfiguration other)
AllowRowTests == other.AllowRowTests && ObsoleteBehavior == other.ObsoleteBehavior && TraceSuccessfulSteps == other.TraceSuccessfulSteps && TraceTimings == other.TraceTimings &&
MinTracedDuration.Equals(other.MinTracedDuration) && StepDefinitionSkeletonStyle == other.StepDefinitionSkeletonStyle &&
Equals(AdditionalStepAssemblies, other.AdditionalStepAssemblies) && MarkFeaturesParallelizable == other.MarkFeaturesParallelizable &&
Equals(SkipParallelizableMarkerForTags, other.SkipParallelizableMarkerForTags);
Equals(SkipParallelizableMarkerForTags, other.SkipParallelizableMarkerForTags) && Equals(AddNonParallelizableMarkerForTags, other.AddNonParallelizableMarkerForTags);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that an Equals override is being defined so that instances of SpecFlowConfiguration can be compared by value?
If so, Equals(SkipParallelizableMarkerForTags, other.SkipParallelizableMarkerForTags) and Equals(AddNonParallelizableMarkerForTags, other.AddNonParallelizableMarkerForTags) should be changed because those two checks are currently comparing by reference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, can you please change it to SequenceEqual ?

Note, in addition to the above configuration, xUnit requires additional configuration to ensure that non parallelizable features do not run in parallel with any other feature. This class must be defined within the test assembly:

``` C#
[CollectionDefinition("SpecFlowNonParallelizableFeatures", DisableParallelization = true)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this collection definition was included in the xUnit.AssemblyHooks.template.cs so users don't need to add this themselves?

An equivalent definition could be added to the vb template.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful idea! Can you please add it to the two templates?
Also, I would mention that SpecFlowNonParallelizableFeatures is shipped with SpecFlow, but if the user would like to add his/her own, an example will be there.

@shack05
Copy link
Contributor Author

shack05 commented Jun 5, 2021

I've added some more changes and left some review comments.
I haven't touched the original ParallelizeDecorator class or related code, so that feature is still not working.

I haven't updated the changelog - I'm not sure how because my fork is now a few weeks behind master.

Copy link
Contributor

@epresi epresi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the updates and tests! I have a few suggestions.

Also, I would like to reply to a few things mentioned in the PR description:

  • You should leave ParallelizeDecorator as it is in this PR
    • After we merge it, we would be happy if you provide a new PR where you remove it!
  • I think we should not touch the "enhancements" of MsTestV2GeneratorProvider
    • yet another list of undocumented "features", but they will not conflict with this feature
  • Changelog
    • This will be a breaking change and a new feature
    • You can also tick the "My change requires a change to the documentation." and "I have updated the documentation accordingly." in the checklist
    • You should also add an entry to the changelog after you updated your fork (see below)

To update your fork: https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@@ -94,7 +97,7 @@ protected bool Equals(SpecFlowConfiguration other)
AllowRowTests == other.AllowRowTests && ObsoleteBehavior == other.ObsoleteBehavior && TraceSuccessfulSteps == other.TraceSuccessfulSteps && TraceTimings == other.TraceTimings &&
MinTracedDuration.Equals(other.MinTracedDuration) && StepDefinitionSkeletonStyle == other.StepDefinitionSkeletonStyle &&
Equals(AdditionalStepAssemblies, other.AdditionalStepAssemblies) && MarkFeaturesParallelizable == other.MarkFeaturesParallelizable &&
Equals(SkipParallelizableMarkerForTags, other.SkipParallelizableMarkerForTags);
Equals(SkipParallelizableMarkerForTags, other.SkipParallelizableMarkerForTags) && Equals(AddNonParallelizableMarkerForTags, other.AddNonParallelizableMarkerForTags);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, can you please change it to SequenceEqual ?

Given there is something");

var provider = new MsTestGeneratorProvider(new CodeDomHelper(CodeDomProviderLanguage.CSharp));
var converter = provider.CreateFeatureGenerator(new string[] { "nonparallelizable" });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename the variable to something, which represents that it is a FeatureGenerator?
This occurs in a few places in the ProviderTests (MsTest, NUnit, xUnit).

Given there is something");

var provider = new MsTestGeneratorProvider(new CodeDomHelper(CodeDomProviderLanguage.CSharp));
var converter = provider.CreateFeatureGenerator(new string[] { "nonparallelizable" });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first parameter of CreateFeatureGenerator is for skipParallelizableMarkerForTags, but these tests are for addNonParallelizableMarkerForTags - please adjust it in the below test as well.

Note, in addition to the above configuration, xUnit requires additional configuration to ensure that non parallelizable features do not run in parallel with any other feature. This class must be defined within the test assembly:

``` C#
[CollectionDefinition("SpecFlowNonParallelizableFeatures", DisableParallelization = true)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful idea! Can you please add it to the two templates?
Also, I would mention that SpecFlowNonParallelizableFeatures is shipped with SpecFlow, but if the user would like to add his/her own, an example will be there.

Copy link
Contributor

@epresi epresi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@epresi epresi merged commit c13d0c9 into SpecFlowOSS:master Jun 9, 2021
@epresi
Copy link
Contributor

epresi commented Jun 9, 2021

I created a new issue to remove the ParallelizeDecorator: #2442
If you are willing to do it, that would make us really happy!

@shack05 shack05 deleted the feature/optoutparallelization branch June 9, 2021 21:53
@SabotageAndi
Copy link
Contributor

Thanks for your contribution to SpecFlow.
Please submit your contributions to our SpecFlow Community Heroes program at https://specflow.org/community/submit-a-contribution/

Code-Grump pushed a commit to Code-Grump/SpecFlow that referenced this pull request Mar 29, 2023
* Added NonParallelizableDecorator which allows opting out of feature level parallelization with user configured tags.

* Fixing typos, removing whitespace.

* Handle case where configured tags is null.

* Fix incorrect test names.

* Introduce 'AddNonParallelizableMarkerForTags' configuration option.

* Added docs for excluding features from parallel execution.

* Make xUnit collection name more specific.

* Compare collections by sequence in SpecFlowConfiguration.

* Fix variable name for featuregenerators in unit tests.

* Fix set up in unit tests.

* Include required xunit collection definition via xunit assembly hooks file.

* Add NonParallelizableDecorator feature to changelog.
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.

None yet

3 participants