Skip to content

chore(Test): Inhibit RunSynchronouslyImmediate; reduce suite concurrency re #20306 - #20308

Merged
T-Gro merged 12 commits into
dotnet:mainfrom
bartelink:async-tests-collection
Aug 24, 2026
Merged

T-Gro merged 12 commits into
dotnet:mainfrom
bartelink:async-tests-collection

Conversation

@bartelink

@bartelink bartelink commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Inhibits additional concurrency, and temporarily redirects all usages of the (new) Async.RunSynchronouslyImmediate to the RunSynchronously, see #20306

Triage and analysis can/will be done by reinstating the calls to RunSynchronouslyImmediate in the shims and/or releasing the suites that have added to the Xunit.Collection on a as-yet-unsubstantiated-basis to run concurrently (all tagged #20306 in comments)

@majocha @T-Gro

@bartelink
bartelink requested a review from a team as a code owner August 21, 2026 10:27
Copilot AI lite review requested due to automatic review settings August 21, 2026 10:27
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Aug 21, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR mitigates CI timeouts/flakiness around new async tests by (a) temporarily routing most Async.RunSynchronouslyImmediate usage back to Async.RunSynchronously, and (b) reducing test-suite concurrency by placing additional async test modules into FSharp.Test.NotThreadSafeResourceCollection (tracked to #20306).

Changes:

  • Introduces local helper shims (asyncWait*) and rewrites many test call sites to avoid Async.RunSynchronouslyImmediate.
  • Adds/extends xUnit collection annotations to inhibit parallel execution for additional async test modules.
  • Cleans up the AsyncModuleFunctions test module declaration while applying the same concurrency mitigation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs Adds helper shims and switches many tests off RunSynchronouslyImmediate; expands NotThreadSafe collection usage for related async test modules.
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncModuleFunctions.fs Applies NotThreadSafe collection to the module and temporarily switches helper execution to Async.RunSynchronously.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs Outdated
Comment thread tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs Outdated
Comment thread tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs Outdated
Comment thread tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/AsyncType.fs Outdated
@bartelink
bartelink force-pushed the async-tests-collection branch 2 times, most recently from 708b911 to 8171878 Compare August 21, 2026 11:51
@bartelink

bartelink commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

First run had two CI jobs still taking > 1h

@T-Gro @majocha (edit: also disabled now, see #20308 (comment))
Note there are still some live usages of RunSynchronouslyImmediate - if they could potentially trigger compiler hangs in a worst case, I can disable them but assuming they are not suspects unless I hear otherwise
image

Happy for this to be merged to park having to consider RunSynchronously and/or the increased test collection concurrency as suspects re build stability, and then circle back to reinstating them later

Note also, as it stands the best guess in the OP (xunit synccontext stuff vs RSI synccontext stuff) remains unproven; the key diff in here is the extension of the 5s failsafe timeout to 30s #20306 (comment)

@majocha

majocha commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

If deadlocks in CI persist, I'd change all the calls inside the compiler to RunSynchronously just in case.

@bartelink

bartelink commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

@majocha you know the general run time better than me - as it stands this run is 1h; I guess that qualifies?
Adding a commit to disable them now, just in case...
=> 5ca5f7a

NOTE there are some stragglers in /vsintegration and some test suites, but none in any src, and the FSharp.Core suite definitely has none

@bartelink
bartelink force-pushed the async-tests-collection branch from 8171878 to 5ca5f7a Compare August 21, 2026 12:52
@bartelink
bartelink force-pushed the async-tests-collection branch from 5ca5f7a to 55e0fb4 Compare August 21, 2026 14:23
@github-actions github-actions Bot added the ⚠️ Affects-Test-Tooling Tooling check: PR touches test framework infrastructure label Aug 21, 2026
@github-actions

This comment has been minimized.

@bartelink
bartelink force-pushed the async-tests-collection branch 2 times, most recently from 75c3289 to 6c74c80 Compare August 21, 2026 16:47
@bartelink
bartelink force-pushed the async-tests-collection branch from 6c74c80 to 9fc39e7 Compare August 21, 2026 17:15
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Test-Tooling
Affects-Test-Tooling: modifies XunitSetup.fs and test concurrency infrastructure

Generated by PR Tooling Safety Check · opus46 3.1M ·

@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Aug 21, 2026
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Aug 21, 2026
Comment thread tests/FSharp.Test.Utilities/XunitSetup.fs
Comment thread src/Compiler/Driver/fsc.fs Outdated
@bartelink

bartelink commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

I'm thinking the next step might be to flip the RunSynchronouslyImmediate back on and see whether that changes the build outcome?

Done:

  • It seems relatively stable and the cleanups (esp the 5s -> 30s falisafe timeouts) are worth the merge.
  • The other key diff is that 4 Async/Task test modules remain excluded from test parallelization (though this slows the test run time, and the fact xunit is running conservative mode should rule out synccontext interactions)

bartelink added a commit to bartelink/fsharp that referenced this pull request Aug 23, 2026
@T-Gro

T-Gro commented Aug 24, 2026

Copy link
Copy Markdown
Member

Ok lets give it a try and observe what CI does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ Affects-Test-Tooling Tooling check: PR touches test framework infrastructure AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants