CI timeouts and flakiness related to new Async tests.

[majocha]

Some new tests related to Async.RunSynchronouslyImmediate are flaky and/or causing the testhost to timeout:

https://dev.azure.com/dnceng-public/public/_build/results?buildId=1560835&view=ms.vss-test-web.build-test-results-tab

https://github.com/dotnet/fsharp/runs/96071354150

etc.

Historically StartImmediate was not playing nice with xUnit, because of xUnit's custom SynchronizationContext, IIRC. I'm not sure this is still the case with v3.

To mitigate we may try to take the offending tests out of parallel run with

[<Collection(nameof FSharp.Test.NotThreadSafeResourceCollection)>]

cc @bartelink

[bartelink]

Thanks for flagging. As you've probably seen I did take some tests out of that Collection as part of recent work (flagged by copilot review; I manually verified that only the two modules/types that remain tagged are in the Collection based on the global Ambient CT) so putting those back in would definitely be reasonable even without the potential xunit/SynchronizationContext issues. I won't personally be able to dig deeper in next day or two.

(I/copilot also encountered similar tests taking a long time issues based on yesterday's main in #20294, and those tests are pretty straightforward - maybe those test runs give more clues)

[bartelink]

@majocha note the 'hung' one was a 5s luxurious timeout on a wait that should be 30s for CI/heavy load concurrency, made it consistent with other tests in 8171878

[majocha]

Yeah, more irritating are the deadlocks that cause whole test run to timeout after 120m with zero indication what failed. I recall that with xUnit v2 we had a working general 5 minutes timeout that listed suspect test cases.

[bartelink]

Perhaps --long-running and others from these MTP mode dotnet test should be applied in general (independent of this triage exercise)

[bartelink]

@majocha build completed this time, but two windows builds took >1h with low memory warnings etc
One thing I realized/discovered is that any test using Async.CancelDefaultToken needs to share a collection with pretty much ALL Async.RunSynchronously (or RSI but also many more), due to that shared default token being the default if no CT is supplied). Have added a comment to that effect in 75ea396 and pushed; it might be good to get it merged so we get more data as to reduced build hangs across other builds now

[majocha]

Yes I once did an audit of all test cases cancelling default token and deparallelized them. I guess some new might have been added, with more tests brought in from the old perl runner suite and VS.

[bartelink]

Turns out I actually misunderstood - because the NotThreadSafeResourceCollection blocks all other concurrent tests with DisableParallelization = true my point is incorrect - the only ones that need this tag strictly are ones that actualy CancelDefaultToken. I'll be removing that comment. When this triage is complete, I'll also be removing the markings from the 4/5 suites that only use it as a precaution (in other words I'm once again confident that this tagging should not be the issue, but we will reinstate them tagged 20306 for the moment)

[bartelink]

@T-Gro I see a RunTestCasesInSequenceAttribute - but can't find anything in web searches for that or RunTestCasesInSequence apart from one or two things that suggest it was a short lived Skill? Should I remove it in my PR, or should we log a cleanup issue to do that at a later date? OR, should it be replaced with [<Collection(nameof NotThreadSafeResourceCollection)>] ?

[majocha]

RunTestCasesInSequenceAttribute worked in xUnit v2. The default was to run internally parallel all the theories and test modules. RunTestCasesInSequenceAttribute was to run some modules internally sequential. Some test cases were depending on a common or cooperatively modified fixture or state.
xUnit v3 does not allow that level of parallelism (sad). The attribute is there just in case the functionality comes back :)

This was speeding up things in local tests runs mostly, CI did not benefit that much.

[bartelink]

Hm, thanks for that hint. Digging deeper (because it very much clashed with everything I knew), I see these were repo-specific custom extensions to Xunit V2. I've removed the last reference to this in 6abcd1b. However this does leave unfinished business: the references to it that remain: RunTestCasesInSequenceAttribute

• are inert and confusing - there is no doc (as they no longer exist and won't)
• have a SKILL attached to them that should probably be removed?
• some tests and suites are still tagged with them and may actually a) need isolation or b) benefit from concurrency, but that has not been in play for a long time

I thus feel it's high time for this to be removed, or an issue written to cover analyzing whether any of them should actually be in included in NotThreadSafeResourceCollection ... or I can simply delete all usages of the attribute given it's inert and there's already enough things to wonder about without it being in the picture (I highly doubt xunit will sprout implicit/default concurrent execution of theory cases any time soon, though I can imagine it being useful on an opt-in basis to allow subcases to be generated and run concurrently on the fly)