Repository navigation
ExecutionContext is flowed between unrelated tests #3283
Description
Same as #2821, but for the ExecutionContext rather than the SynchronizationContext. (.NET tracks them individually.)
One test should not be able to affect another test simply because the test execution scheduler happens to run them on the same worker thread. This has unstable results depending on how many worker threads there are and on how NUnit decides to order them over time. But more importantly, it's an invisible factor that influences every test so that you may not be actually testing what you think you are testing.
Each test is inheriting an unrelated ExecutionContext from the last test the worker happened to pick up. This can cause some tests to fail when they should pass. Much worse, as in some cases just now for me, it can silently render some tests ineffective (passing whether or not they should fail).
Repro
Some tests designed to demonstrate the problem by failing:
public static class SystemUnderTest
{
private static readonly AsyncLocal<int> ValueFlowedWithExecutionContext = new AsyncLocal<int>();
public static int Increment() => ++ValueFlowedWithExecutionContext.Value;
}
public static class Tests
{
// Only one test succeeds because the .NET ExecutionContext is flowed between tests
// in whatever order they happen to execute.
[Test]
public static void Test1()
{
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(1));
}
[Test]
public static void Test2()
{
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(1));
}
[Test]
public static void Test3()
{
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(1));
}
}Workarounds
The easiest workaround I'm aware of is to put [RequireThread] on every test method. This comes with a performance hit. It's also easy to forget if it breaks your test by causing it to pass rather than by causing it to fail.
A more efficient workaround would be:
private static void DoInIsolatedExecutionContext(Action action)
{
using (var copy = ExecutionContext.Capture().CreateCopy())
{
ExecutionContext.Run(copy, state => ((Action)state).Invoke(), state: action);
}
}
[Test]
public static void TestN()
{
DoInIsolatedExecutionContext(() =>
{
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(1));
});
}Concerns
Would some NUnit users be making use of this behavior as a feature when all tests execute on a single thread and they are constrained to execute in order?
public static class Tests
{
[Test, Order(1)]
public static void Test1()
{
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(1));
}
[Test, Order(2)]
public static void Test2()
{
// Build on the previous test (where ‘previous’ is codified via OrderAttribute)
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(2));
}
[Test, Order(3)]
public static void Test3()
{
// Build on the previous test (where ‘previous’ is codified via OrderAttribute)
Assert.That(SystemUnderTest.Increment(), Is.EqualTo(3));
}
}The SynchronizationContext is no longer flowed between tests as of 3.11 (#2821) which is very similar though it affects different things. No one has reported noticing the difference.
Activity
[-]ExecutionContext should not be flowed between tests[/-][+]ExecutionContext is flowed between unrelated tests[/+]jnm2 commentedon Jun 15, 2019
A workaround attribute is at https://gist.github.com/jnm2/6066becf23fbf0607091fe98057966ca and can be used like this:
jnm2 commentedon Jun 15, 2019
And here's a safety feature to fail all tests in the assembly that do not have the
[PreventExecutionContextLeaks]workaround applied to the test method: https://gist.github.com/jnm2/6066becf23fbf0607091fe98057966caUsage (described further in the gist):
jnm2 commentedon Jun 19, 2019
assembly: RequireOnAllTestMethodsjust saved my day.CharliePoole commentedon Jun 19, 2019
I wonder we should just give each test a clean thread to run on and save ourselves a lot of bother. That, of course, would require a complete redesign of the ParallelWorkItemDispatcher.
oznetmaster commentedon Jun 19, 2019
You realize that is what I had originally done, years ago?
Every one seemed to dislike the thread per test approach, and hence the rather involved approach we currently have.
The issues seemed to be keeping the setup, test, and teardown together on the same thread. That was not happening in the original approach. The setup was running on a different thread than the test and teardown (which were on the same thread), if I remember correctly.
I have always thought that giving each test a clean thread was the best approach. Certainly simplifies the forced abort problem.
CharliePoole commentedon Jun 19, 2019
@oznetmaster You proposed a particular way of running each test on it's own thread, which, as you say , was turned down.
If we were to decide to run each test on a separate thread, we'd be back to the point of how to implement it. See the second sentence of my initial comment.
@jnm2 At the beginning of NUnit3, it still appeared that tests corrupting the thread context was pretty rare. Now, particularly with async tests, it feels like a bigger problem. Does it seem so to you?
jnm2 commentedon Jun 19, 2019
A thread per test case is something I'd be reluctant to do. Controlling the ExecutionContext is a much smaller change for us to implement; we just have to wrap the execution of setup to teardown with this:
On .NET Core especially, they've gotten ExecutionContext so light it hardly does anything. (Everything it flows is in terms of IAsyncLocal.) If we started up a new thread for each task, I would expect a performance hit much larger than switching to a new copy of the execution context, and that's on top of us having to rework the code.
@CharliePoole I'm not sure, but I don't think async tests would make the problem larger. After an await that completes asynchronously, the execution context is already copied and changes to it are isolated. That's exactly what we want to do, but we want to also isolate changes before the first await and in non-async tests which have no await.
CharliePoole commentedon Jun 19, 2019
I'll have to defer to you on the ExecutionContext. Remember, none of that stuff was open source when we did the work. In fact, Simone, wrote our code for async just by trying things till they worked, plus a few bits of help from MS folks. That may be why I view async as complicating the problem and we may now be past that point.
All that said, isolation by thread, by AppDomain and even by process is something I'd like to see eventually. I just don't like the model where a "worker" thread actually does no work but just waits for the real thread to finish.
I also think that the performance hit could be gotten around by use of a Thread Pool or possibly by using a Task-based design in .NET 4.0 and higher, as we do for .NET Core.
jnm2 commentedon Jun 19, 2019
A thread pool would require copying the the execution context to prevent sharing between tests for the same reasons that our current worker threads require this. Why not implement ExecutionContext isolation, which is simpler and more incremental since it would be required in any solution that doesn't create a new thread for each test case?
CharliePoole commentedon Jun 19, 2019
Good point. Do the simple thing first.
jnm2 commentedon Jun 19, 2019
I'll get started on a PR soon. I mainly wanted to confirm that we didn't think this would break anyone's existing tests or whether we wanted to perhaps preserve the current behavior (flow the same execution context) between nonparallel tests in the same fixture with
[Order]attributes. (See the example in the Concerns section at the top.)CharliePoole commentedon Jun 19, 2019
I think that would be a serious misuse of
OrderAttributesince it does not guarantee that one test will finish before the next one starts. If (and I think it's a big IF) you were to do anything special, I believe it should be forSingleThreadedAttributewhich guarantees that all the tests run on the same thread as one-time setup and teardown. Somebody could use this because they are saving info in theExecutionContext. However, I'd wait for a bug report.jnm2 commentedon Jun 19, 2019
Okay, sounds good. I appreciate the input.
MaceWindu commentedon Mar 27, 2021
@jnm2 what it
RequireOnAllTestMethodsattribute? I cannot find it even mentioned anywhere except this issue. I'm trying to apply workaround for #3802 based onIWrapSetUpTearDownbut it is ignored on fixture level and adding it to each test is not an option...jnm2 commentedon Apr 22, 2021
@MaceWindu It's defined here: https://gist.github.com/jnm2/baae5b19c2191078433e2ca57f46c760