Make System.Diagnostics.Tracing.Tests run sequentially

[davmason]

Fixes #91769
Fixes #91304
Fixes #88027

Apparently the build.cmd -test method of running tests needs a different way of disabling parallelization

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #91769
Fixes #91304
Fixes #88027

Apparently the build.cmd -test method of running tests needs a different way of disabling parallelization

Author:	davmason
Assignees:	davmason
Labels:	area-System.Diagnostics.Tracing
Milestone:	9.0.0

[danmoseley]

@davmason what does this property do - pass some flag to xunit that tells it to ignore the attribute? What is that?

This is not the only test library that has those no parallelism annotations.

[davmason]

@danmoseley I have no idea why the attributes are ignored, but through reverse engineering I found that this property sets maxthreads to 1 here

runtime/eng/testing/xunit/xunit.console.targets

Line 18 in d170785

<RunScriptCommand Condition="'$(TestDisableParallelization)' == 'true'">$(RunScriptCommand) -maxthreads 1</RunScriptCommand>

From my testing it seemed like if you ran dotnet test in the folder it would respect the no parallelization attributes, but if you ran with build.cmd/sh -test like the CI legs do you have to set that property to disable parallelization

[danmoseley]

@bradwilson any idea why the attribute is being ignored? Context: #91769 (comment)

[bradwilson]

First: Is the console runner display broken?

The display from the console runner is based on an "outside" view of what's going on; that is, it only displays what the runner has requested the test framework do (based on defaults + configuration files + command line switches). The test framework does not report back out what it's actually doing. We provided no mechanism to do that in v2: we only support outside-in communication, not inside-out communication, regarding parallelism. So the assembly-level attribute does not get reflected into the display on the console runner.

That means this:

Starting:    System.Diagnostics.Tracing.Tests (parallel test collections = on, max threads = 4) <<<<<<<<<<<<<

is expected behavior.

The best way to get what you want is to remove the assembly-level attribute and add/update xunit.runner.json with parallelizeTestCollections set to false. (You can also pass -parallel assemblies or -parallel none to the console runner via command line switch, but this is not recommended in your situation, since you want to permanently disable parallelization in the test assembly and the VSTest adapter used from within Test Explorer does not provide a way to specify command line switches like this.)

Second: Is the attribute broken?

Through experimentation, I have found that the CollectionBehavior.CollectionPerClass and CollectionBehavior.CollectionPerAssembly is performing as I expected. 3 test classes, 1 test each with Task.Delay(3000) and I will see 3 seconds vs. 9 seconds of execution due to the differing default collection behavior. Of course, this only changes the default collection assignment behavior. If you have any custom test collections defined, then any tests in those collections will continue to run in parallel against the default assembly-wide collection.

Unexpectedly, using DisableTestParallelization = true did not actually appear to disable test parallelization (everything took 3 seconds instead of 9), which means that this particular flag was broken at some point. The code break appears to have happened on the runner side sometime between 2.0.0 and 2.1.0, which I can verify by running the code with the older runners:

Image deleted because it's no longer relevant

So, congratulations! You've uncovered a bug that's more than 8 years old. 😄 The fact that this has been broken for that long tells me that people are overwhelmingly using the configuration file approach and not the attribute approach to disable test parallelization.

#### Recommendation

As such, I strongly recommend you use xunit.runner.json to specify parallelism being disabled, at least until I can fix the issue with [CollectionBehavior(DisableTestParallelization = true)]. Using the configuration file is the only 100% replacement for the attribute, and I can verify that the configuration file approach does work (and has the added benefit of showing up correctly in the console runner display).

[bradwilson]

Actually, ignore that. I tripped myself by having a configuration file that had parallelTestCollections set to true. sigh I suppose I should be in bed at 1am and not trying to repro bugs. 😂

I now believe you can ignore the second half of what I just wrote. [CollectionBehavior(DisableTestParallelization = true)] works as expected, and is not bugged. I am going to strikeout all the incorrect text above, and whatever the issue is that you're experiencing, if it's because of parallelism being enabled when you don't expect it, perhaps that's because you've enabled it in your xunit.runner.json file?

[bradwilson]

[danmoseley]

thanks @bradwilson . I realize that for some reason I had assumed that the attribute would win over any xunit.runner.json or command line flag, but apparently it's the reverse
https://github.com/xunit/xunit/blob/d300c8850978195b6e78d3cba17b78bb8a4e68f4/src/xunit.execution/Sdk/Frameworks/Runners/XunitTestAssemblyRunner.cs#L93-L101

So the command line or xunit.runner.json must be overriding it. (Curiously, I'm not sure how our xunit.runner.json would have parallelizeTestCollections: true. The various layers around xunit in this repo and all the various ways it can be invoked, including a custom dummy xunit with fewer dependencies, make it hard to see, and I won't look further (eg actually run build.sh -test..))

One question though. Is there any functional difference between disabling parallelism and running with a limit of 1 thread? I guess this PR is doing the latter.
https://github.com/xunit/xunit/blob/d300c8850978195b6e78d3cba17b78bb8a4e68f4/src/xunit.execution/Sdk/Frameworks/Runners/XunitTestAssemblyRunner.cs#L191-L194

[danmoseley]

@davmason there are 18 test libraries in the tree that have CollectionBehavior(DisableTestParallelization = true but only the tracing tests have <TestDisableParallelization>true</TestDisableParallelization>. I wonder whether they're running in parallel.

[bradwilson]

I realize that for some reason I had assumed that the attribute would win over any xunit.runner.json or command line flag, but apparently it's the reverse

Yes, it's definitely the reverse, and purposefully. We presume that things you put in source define the default behavior, and you can use interactive mechanisms like command line switches to alter that default behavior. The reverse wouldn't make much sense, as you wouldn't be able to override anything in source w/o changing the source itself.

Is there any functional difference between disabling parallelism and running with a limit of 1 thread?

Yes. (Everything I'm about to describe is a summary of XunitTestAssemblyRunner.RunTestsCollectionsAsync and the base class method that it overrides from TestAssemblyRunner.)

If parallelism is globally disabled, we do not create the MaxConcurrencySyncContext and thus the number of maximum parallel threads is irrelevant (and we do not create our own custom thread pool as it's the sync context that owns the thread pool). We run the test collections sequentially, and inside the test collection we always run tests sequentially as test collection is our parallelization boundary.

If parallelism is globally enabled, we do create the MaxConcurrencySyncContext with the requested number of threads in the pool. We then determine which test collections are "parallelizable" vs. "non-parallelizable". Test collections are parallelizable by default, but can be overridden with [CollectionDefinition(DisableParallelization = true)].

For every parallelizable test collection, start the first test in the collection. These are all started in parallel. As each test in the collection finishes, we start the next test for that collection, until all the tests in the collection have run. Once all the tests from all the parallelizable test collections have run, then we sequentially run all the non-parallelizable test collections in a manner identical to the "parallelism globally disabled" behavior.

We will effectively start running n tests, where n is the number of parallelizable test collections. If your maximum thread count is 1, then at any given time, only 1 of them will actually be running, but the rest will be ready to run when the thread becomes available. So if a test in this scenario hits an await against a Task that is not already complete, that test will wait for it's Task to complete and the one thread will become available for work, so a second test will now begin to run. When that test completes (or itself awaits an incomplete Task), then another test from the list of available tests runs, as per whatever the Task scheduler algorithm dictates.

(There is an important aside here: we rely on you not escaping the sync context to ensure the limits on parallelism. That means any call to .ConfigureAwait(false) inside the test method is an error condition, which is why we recently created an analyzer which highlights this. Similarly, if you use any other parallelism mechanism other than awaiting Tasks, such as spawning your own threads, then those mechanisms are things we don't know about and do not control, and as such any code running there is outside of our max thread count system.)

Generally speaking, people more often have a subset of tests that they want to prevent running in parallel with one another, typically because of a shared resource like a database. They use test collections and collection fixtures to coordinate that. Disabling parallelism globally is a mechanism that typically only makes sense if you have a globally shared resource and every test in your test assembly needs access to that globally shared resource. This is a pretty uncommon scenario in my experience, but not unheard of.

[danmoseley]

Super helpful, thanks.

There's some begged questions about our own infra here, but this issue will be helpful if those are actually encountered.

I'm guessing that these tracing tests don't run problematic code in awaits but I can't look at the moment.

[bradwilson]

FWIW, the .ConfigureAwait(false) only applies to code in the test method itself. Any other code is irrelevant since that's the only code that needs to return to our sync context to ensure maximum # of running tests. However, any code under test that uses .ConfigureAwait(false) will continue to run on a normal thread pool thread, which means it can run in parallel with the test(s). This may or may not be an issue, though in practice it typically isn't.

Globally disabling parallelism doesn't have any effect here; it's just that the one test which may be awaiting an incomplete task will be sitting and waiting for the result, which may end up being calculated on a different thread. Even in that scenario, though, any non-parallelizable test collection (including globally disabling parallelism) means the test will just sit there and wait, while no other tests are running, rather than giving up a worker thread in the pool for another test.

[bradwilson]

I'm also pushing up an update into the next release of the v2 core framework so that max parallel thread counts are only shown when test collection parallelization is enabled.

So previously:

Starting: assembly1 (parallel test collections = on, max threads = 24)
Starting: assembly2 (parallel test collections = off, max threads = 24)

Upcoming:

Starting: assembly1 (parallel test collections = on [24 threads])
Starting: assembly2 (parallel test collections = off)

This should add clarify about thread counts being tied into parallelism as well as a bit of brevity.

https://github.com/xunit/xunit/blob/6d1bc075fcaedaa5233ad077a437898830c8b137/test/test.xunit.runner.utility/Extensibility/DefaultRunnerReporterWithTypesMessageHandlerTests.cs#L150-L156

[danmoseley]

that's helpful. what about this case [InlineData(false, null, null, null, "[Imp] => Starting: testAssembly")] .. I guess it means "defaults" -- would it make sense to say what those defaults are here? whoever reads the log probably cares more about whether the tests potentially ran in parallel than how that was achieved.

[bradwilson]

That test is specifically about what people see without diagnostic messages enabled (that's the first false). You only see the extra information when you enable diagnostic messages.

The last test item in the list (true with all nulls) shows all the defaults, except for thread count (which needs to be fixed, since the default value for thread count floats based on the number of CPU cores you have).