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

Feature: Add CollectionDefinition.DisableParallelization #1411

Merged
merged 2 commits into from
Aug 17, 2017

Conversation

NickCraver
Copy link
Contributor

This spawned from a Twitter discussion here.

In every one of our libraries, especially with settings, event handlers, etc. some tests just aren't friendly with parallelization. We want parallel testing in the vast majority of cases (that can in itself reveal issues), but specific tests (or collections) we want to run non-parallel separately from everything else.

This change allows a CollectionDefinition to specify DisableParallelization = true to support this. The behavior is: all parallel collections (any not specifying this new constructor argument specifically) run first and in parallel (no behavior change from today). Then any collections specifying DisableParallelization = true are run, each is awaited in order. This means that one 1 DisableParallelization collection is running at a time.

Note the code is a bit more complicated to reduce allocations, but if we want to allocate and simplify I can take that route.

The alternatives to this sort of change is: override [Fact] everywhere and handle semaphores externally. Something that's pretty pervasive for a base level change. Or, split all tests into another assembly with 1 thread (disabling parallelization). This (from my experience) would result in a lot of code duplication and complication.

With this change, a user can specify:

[CollectionDefinition("Event Handler Tests", DisableParallelization = true)]
public class EventHandlerTestDefinition { }

[Collection("Event Handler Tests")]
public class EventHandlerTests
{
    [Fact]
    public void EventHandlerAddRemove() { }
}

...and those tests which have global impact (causing many other tests or themselves to fail) can run at the end, safely testing everything and avoiding many of the race conditions and inconsistencies otherwise present when they interact with all tests.

Questions: Is there a better property name for this behavior?

Side notes: it'd be handle to switch to nameof() in other places attribute constructors are fetched for seeing where they're used in VS 2017. If that's wanted, I'll open another PR specifically for that.

cc @bradwilson @onovotny

This doesn't work, committing for a sanity check. In this code, collection.Item1.CollectionDefinition appears to be null in our test case...but I'm not sure why.
Also fixes unit tests and put a bit more of a complex scenario to make sure everything is supported.
@dnfclas
Copy link

dnfclas commented Aug 15, 2017

@NickCraver,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@bradwilson
Copy link
Member

Thanks!

NickCraver added a commit to DapperLib/Dapper that referenced this pull request Aug 20, 2017
This uses the new non-parallel-at-the-end I added to xUnit in xunit/xunit#1411 This way we can run any global test that are other-test-unfriendly much more easily.
thomasray711 pushed a commit to thomasray711/DapperLib-Dapper that referenced this pull request Apr 22, 2022
This uses the new non-parallel-at-the-end I added to xUnit in xunit/xunit#1411 This way we can run any global test that are other-test-unfriendly much more easily.
phoenixdev9 pushed a commit to phoenixdev9/Dapper that referenced this pull request Aug 18, 2024
This uses the new non-parallel-at-the-end I added to xUnit in xunit/xunit#1411 This way we can run any global test that are other-test-unfriendly much more easily.
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.

3 participants