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

avoid code duplication for disabling parallelization with xUnit #62132

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

adamsitnik
Copy link
Member

Few of the tests projects were defining:

[CollectionDefinition("NoParallelTests", DisableParallelization = true)]
public partial class NoParallelTests { }

I've removed the duplications, introduced a single type for it and used nameof instead of magic strings to make sure that compiler gives us an error if the type is not referenced in the project.

@ghost
Copy link

ghost commented Nov 29, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Few of the tests projects were defining:

[CollectionDefinition("NoParallelTests", DisableParallelization = true)]
public partial class NoParallelTests { }

I've removed the duplications, introduced a single type for it and used nameof instead of magic strings to make sure that compiler gives us an error if the type is not referenced in the project.

Author: adamsitnik
Assignees: -
Labels:

area-System.Net

Milestone: -

@ghost
Copy link

ghost commented Nov 29, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Few of the tests projects were defining:

[CollectionDefinition("NoParallelTests", DisableParallelization = true)]
public partial class NoParallelTests { }

I've removed the duplications, introduced a single type for it and used nameof instead of magic strings to make sure that compiler gives us an error if the type is not referenced in the project.

Author: adamsitnik
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@adamsitnik adamsitnik force-pushed the disableParallelization branch from c086539 to 0d9fa77 Compare November 29, 2021 13:00
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Seems like the new cs file needs to be referenced in additional projects?

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs(16,24): 
error CS0103: The name 'NonParallelTestCollection' does not exist in the current context

@adamsitnik
Copy link
Member Author

Seems like the new cs file needs to be referenced in additional projects?

You are right, I've fixed a build failure and tested it locally (it's not part of -subset clr+libs+libs.tests to my suprise).

@adamsitnik adamsitnik added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 29, 2021
@adamsitnik
Copy link
Member Author

The CI has failed and it's most likely caused by:

Fixtures can be shared across assemblies, but collection definitions must be in the same assembly as the test that uses them.

Source: https://xunit.net/docs/shared-context#collection-fixture

@adamsitnik adamsitnik removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 29, 2021
@adamsitnik adamsitnik force-pushed the disableParallelization branch from 5e201bb to d303408 Compare November 29, 2021 15:51
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.

@adamsitnik adamsitnik merged commit 8f87ac7 into dotnet:main Nov 30, 2021
@adamsitnik adamsitnik deleted the disableParallelization branch November 30, 2021 14:52
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants