-
Notifications
You must be signed in to change notification settings - Fork 6
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
Is the conservative parallelization algorithm newly introduced in xUnit 2.8.0 not respected? #15
Comments
I won't implement it myself as I don't have time to spend on this project. However, I would be happy to review a PR if someone wants to implement it.
Even if all tests are run in parallel, you can limit the concurrency. This parameter is respected by this project. The implementation rely on a |
I have been looking into this also and my case heavily relies on not launching all tests at once as each test makes an infrastructural dependency which takes host resources and I wish to limit this to eg. 4 or max CPUs at least So I have been trying to set the xunit.runner.json config but it has no effect, eg. {
"maxParallelThreads": 4
} (I have verified it is copied in the bin output dir.) The tests are still executed all at once. I tried creating my own sync context with a SemaphoreSlim and hardcoding it to accept only eg. 4 entries but yet again to no effect. The SyncContext was set before all tests in a fixture CTOR. Is this really configurable or I am misunderstanding your comment @meziantou:
I guess by "this parameter" you mean the one I tried setting in the config above? I see this (ref below) in the code ie. check for a sync context but I am not sure if it really works or I do not understand it? My custom sync context should be respected? Any ideas please on how to limit concurrency here? e: Even adding P.S. |
I don't think this works. The expected sync context should be created by xunit when you set the I don't know if setting the max concurrency using the attribute is supported. |
Hi again, I managed to get this working and respecting the xunit.runner.json config, the key change was in the ParallelTestClassRunner, hope this helps others: public class ParallelTestClassRunner : XunitTestClassRunner
{
private static SemaphoreSlim _semaphore = null!;
public ParallelTestClassRunner(ITestClass testClass, IReflectionTypeInfo @class,
IEnumerable<IXunitTestCase> testCases, IMessageSink diagnosticMessageSink, IMessageBus messageBus,
ITestCaseOrderer testCaseOrderer, ExceptionAggregator aggregator,
CancellationTokenSource cancellationTokenSource, IDictionary<Type, object> collectionFixtureMappings,
ITestFrameworkExecutionOptions executionOptions)
: base(testClass, @class, testCases, diagnosticMessageSink, messageBus, testCaseOrderer, aggregator,
cancellationTokenSource, collectionFixtureMappings)
{
// make sure you pass executionOptions additionally from xunit assembly class
_semaphore = new(executionOptions.MaxParallelThreadsOrDefault());
}
// 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/TestClassRunner.cs#L194-L219
protected override async Task<RunSummary> RunTestMethodsAsync()
{
var disableParallelizationAttribute =
TestClass.Class.GetCustomAttributes(typeof(DisableParallelizationAttribute)).Any();
var disableParallelizationOnCustomCollection =
TestClass.Class.GetCustomAttributes(typeof(CollectionAttribute)).Any()
&& !TestClass.Class.GetCustomAttributes(typeof(EnableParallelizationAttribute)).Any();
var disableParallelization = disableParallelizationAttribute || disableParallelizationOnCustomCollection;
if (disableParallelization)
return await base.RunTestMethodsAsync().ConfigureAwait(false);
var summary = new RunSummary();
IEnumerable<IXunitTestCase> orderedTestCases;
try
{
orderedTestCases = TestCaseOrderer.OrderTestCases(TestCases);
}
catch (Exception ex)
{
var innerEx = Unwrap(ex);
DiagnosticMessageSink.OnMessage(new DiagnosticMessage(
$"Test case orderer '{TestCaseOrderer.GetType().FullName}' threw '{innerEx.GetType().FullName}' during ordering: {innerEx.Message}{Environment.NewLine}{innerEx.StackTrace}"));
orderedTestCases = TestCases.ToList();
}
var constructorArguments = CreateTestClassConstructorArguments();
var methodGroups = orderedTestCases.GroupBy(tc => tc.TestMethod, TestMethodComparer.Instance);
var methodTasks = methodGroups.Select(m =>
RunTestMethodAsync(m.Key, (IReflectionMethodInfo)m.Key.Method, m, constructorArguments));
var methodSummaries = await Task.WhenAll(methodTasks).ConfigureAwait(false);
foreach (var methodSummary in methodSummaries)
{
summary.Aggregate(methodSummary);
}
return summary;
}
protected override async Task<RunSummary> RunTestMethodAsync(ITestMethod testMethod, IReflectionMethodInfo method,
IEnumerable<IXunitTestCase> testCases, object[] constructorArguments)
{
await _semaphore.WaitAsync(CancellationTokenSource.Token).ConfigureAwait(false);
try
{
var summary = await new ParallelTestMethodRunner(
testMethod,
Class,
method,
testCases,
DiagnosticMessageSink,
MessageBus,
new ExceptionAggregator(Aggregator),
CancellationTokenSource,
constructorArguments).RunAsync();
return summary;
}
finally
{
_semaphore.Release();
}
}
private static Exception Unwrap(Exception ex)
{
while (true)
{
if (ex is not TargetInvocationException tiex || tiex.InnerException == null)
return ex;
ex = tiex.InnerException;
}
}
} |
xUnit 2.8 introduced an additional algorithm to handle the parallelization. That new conservative algorithm can limit the amount of tests (or test collections to be more precise I guess) that are run in parallel. In our use case that is very desirable, since otherwise it used to start all async tests immediately. That change now let's us limit that.
However, to still achieve parallelization within a test collection, we would need this framework here. However, it seems that is not respecting that new conservative algorithm, since it still spawns all tests.
I am not deep into the internals that are adjusted here, so I can not really judge what the issue is exactly.
The text was updated successfully, but these errors were encountered: