Parallelize without modifying collections

[MatisseHack]

Closes #1 by running tests in parallel without splitting them up into separate collections. Leaving the collection structure unmodified means fixtures will be properly reused between tests for setup and teardown. Most of this code was taken from the original xUnit source with only slight modifications to make tests run in parallel.

I have not yet re-implemented support for disabling parallelization with DisableDiscoveryEnumeration and explicit Collection attributes. Using the Collection attribute to disable parallelization didn't make as much sense to me now that we aren't modifying the collection structure at all. DisableDiscoveryEnumeration also has a different purpose unrelated to parallelization.

I propose we introduce a new attribute specific to this package (e.g. DisableParallelizationAttribute) if we want to maintain this functionality. It would be a breaking change, but unfortunately the changes in this PR might already be considered breaking since they affect how tests are executed.

[meziantou]

Thanks for your contribution! I'll review it this weekend or next week if you think the PR is ready (I'm not sure based on the PR description).

Using the Collection attribute to disable parallelization didn't make as much sense to me now that we aren't modifying the collection structure at all

I know multiple projects relying on this behavior. So, I would avoid a change here.

I propose we introduce a new attribute specific to this package (e.g. DisableParallelizationAttribute)

It would make sense to add a new attribute. But it should come in addition to the explicit collection name to not break projects and give them time to migrate to the new way. You can add a new assembly attribute to disable the explicit collection behavior. So, you can already disable it, and projects that rely on the collection attribute behavior won't get any breaking change.

[tskarman]

I am not sure, that this is the best place to comment on that. But since I based by experiments on the code in this pull requests, I thought I'd start here. And since this explicity talks about the reuse of test collection invariants (e.g. fixtures), I think it fits this PR.

The problem I was facing is that any test class using the Xunit-injected ITestOutputHelper (constructor) would run into exceptions of this kind: System.InvalidOperationException : There is no currently active test..

This is because of the fact that Xunit special-cases the handling of that particular constructor argument:

• Special initialization here
• Special uninitialization here

I.e. the lifetime of the TestOutputHelper is designed to be 1 test execution. And since the parallelization turns that on its head (kind of), we arrive at the above exception.

Now to address this, one could choose to venture a bit deeper and subclass a few more Xunit classes (namely XunitTestCase, XunitTestCaseRunner and XunitTestRunner) and create a new implementation of ITestOutputHelper to override some more default behaviors.

But I'd instead suggest, modifying the implementation of ParallelTestMethodRunner some more. The idea is to take the special casing of TestOutputHelper a bit further and to replace the instances before they are used for constructing the test class. Sadly, it's not easily possible to reuse the original instance of TestOutputHelper that way (because we can't know if it's the first use of it or not). But since its not a heavy-weight type, I think that's an OK trade-off for now.

I also want to note that Xunit 3 might introduce another way of handling that. See this code that would prob. allow placing a Func<ITestOutputHelper> in the constructor arguments.

public class ParallelTestMethodRunner : XunitTestMethodRunner
{
    // capture the diagnostic message sink a field so that we can access it in our overriding implementation
    private readonly IMessageSink _diagnosticMessageSink;

    // capture the constructor arguments in a field so that we can access them in our overriding implementation
    private readonly object[] _constructorArguments;

    public ParallelTestMethodRunner(ITestMethod testMethod, IReflectionTypeInfo @class, IReflectionMethodInfo method, IEnumerable<IXunitTestCase> testCases, IMessageSink diagnosticMessageSink, IMessageBus messageBus, ExceptionAggregator aggregator, CancellationTokenSource cancellationTokenSource, object[] constructorArguments)
        : base(testMethod, @class, method, testCases, diagnosticMessageSink, messageBus, aggregator, cancellationTokenSource, constructorArguments)
    {
        _diagnosticMessageSink = diagnosticMessageSink;
        _constructorArguments = constructorArguments;
    }

    // This method has been slightly modified from the original implementation to run tests in parallel
    // https://github.com/xunit/xunit/blob/2.4.2/src/xunit.execution/Sdk/Frameworks/Runners/TestMethodRunner.cs#L130-L142
    protected override async Task<RunSummary> RunTestCasesAsync()
    {
        // Unmodified
    }

    // This method has been slightly modified from the original implementation to run tests in parallel and make the execution fit for TestOutputHelper
    // https://github.com/xunit/xunit/blob/2.4.2/src/xunit.execution/Sdk/Frameworks/Runners/XunitTestMethodRunner.cs#L44
    protected override async Task<RunSummary> RunTestCaseAsync(IXunitTestCase testCase)
    {
        var rewrittenConstructorArguments = _constructorArguments.ToArray();

        for (var ii = 0; ii < rewrittenConstructorArguments.Length; ii++)
        {
            if (rewrittenConstructorArguments[ii] is TestOutputHelper)
            {
                rewrittenConstructorArguments[ii] = new TestOutputHelper();
                break;
            }
        }

        // Mirroring XunitTestMethodRunner.RunTestCaseAsync(IXunitTestCase testCase)
        return await Task.Run(() => testCase.RunAsync(_diagnosticMessageSink, MessageBus, rewrittenConstructorArguments, new ExceptionAggregator(Aggregator), CancellationTokenSource));
    }
}

Here is a repro that should fail pretty consistently under low load:

public class TestWithLogging
{
    private readonly ITestOutputHelper _output;

    public TestWithLogging(ITestOutputHelper output)
    {
        _output = output;
    }

    [Fact]
    public async Task Run1()
    {
        _output.WriteLine("Start fact 1");

        await Task.Delay(TimeSpan.FromSeconds(1));

        _output.WriteLine("Stop fact 1");
    }

    [Fact]
    public async Task Run2()
    {
        _output.WriteLine("Start fact 2");

        await Task.Delay(TimeSpan.FromSeconds(0));

        _output.WriteLine("Stop fact 2");
    }
}

I just wanted to get this out there. Let me know if I should put this somewhere else or repackage it after this PR here has merged.

[MatisseHack]

Thanks for the quick reply @meziantou!

I know multiple projects relying on this behavior. So, I would avoid a change here.

That's what I figured, but wanted to make sure before I implemented anything. I've fixed the logic so that tests using an explicit Collection or DisableDiscoveryEnumeration will run sequentially.

I also added a new DisableParallelization attribute and updated the readme with information on how to use it. I didn't add an assembly attribute to turn off the old behavior, but I can add one if you think it's a requirement.

This PR should be ready for review now!

[MatisseHack]

@tskarman thank you as well for testing these changes and finding a bug so quickly! I agree with your assessment that the best workaround is to create a new TestOutputHelper for each test by modifying the constructor arguments. Hopefully xUnit 3 will allow us to implement a more satisfying solution.

I've gone ahead and implemented your proposed changes in this PR. Thanks for the help!

[meziantou]

Thanks for your contribution 🚀 I'll publish a new version soon

[meziantou]

Sorry for the bad news, but there is a bug in the logic. I tried the new package on an existing project, and many tests that require sequential execution failed.

You can easily reproduce the issue with the following class

[DisableParallelization]
public class SequentialTheory2Tests : IClassFixture<ConcurrencyFixture>
{
    private readonly ConcurrencyFixture fixture;

    public SequentialTheory2Tests(ConcurrencyFixture fixture)
    {
        this.fixture = fixture;
    }

    [Theory]
    [InlineData(1)]
    [InlineData(2)]
    public async Task Theory(int value)
    {
        Assert.Equal(1, await fixture.CheckConcurrencyAsync().ConfigureAwait(false));
    }
}

I think the fix is to replace

protected override Task<RunSummary> RunTestMethodAsync(ITestMethod testMethod, IReflectionMethodInfo method, IEnumerable<IXunitTestCase> testCases, object[] constructorArguments)

with

// Or give it a new name
protected new Task<RunSummary> RunTestMethodAsync(ITestMethod testMethod, IReflectionMethodInfo method, IEnumerable<IXunitTestCase> testCases, object[] constructorArguments)

Otherwise the base class still use the new parallel implementation.

[meziantou]

Could you please review https://github.com/meziantou/Meziantou.Xunit.ParallelTestFramework/pull/6?