Skip to content

Make the ComponentTests checker tests asynchronous - #20461

Open
xperiandri wants to merge 2 commits into
dotnet:mainfrom
xperiandri:tests/componenttests-async
Open

Make the ComponentTests checker tests asynchronous#20461
xperiandri wants to merge 2 commits into
dotnet:mainfrom
xperiandri:tests/componenttests-async

Conversation

@xperiandri

Copy link
Copy Markdown
Contributor

Description

Test-only change. The checker tests in FSharp.Compiler.ComponentTests that blocked on Async.RunSynchronously now return Task and bind the FCS Async results inside task { }, so they no longer park an xunit worker thread while the checker runs, and they read like the Task-based tests already in the project.

  • FSharpChecker/SemanticClassificationRegressions.fsgetClassifications, classificationsForIdent and expectUnionCaseClassifications are asynchronous; every test binds them with let!. Source snippets keep their exact text and columns, only the code around them moved.
  • FSharpChecker/FindReferences.fsfindRefsInSource, testFindRefsInSource and singleFileCheckerWithName are asynchronous; the tests that call them return the resulting Task.
  • FSharpChecker/CommonWorkflows.fs, FSharpChecker/TransparentCompiler.fs (Fuzzing, getParseResult), CompilerDirectives/Line.fs, Miscellaneous/TestUtilities.fs, TypeChecks/Graph/CompilationFromCmdlineArgsTests.fs — the same, one site each.
  • CompilerService/AsyncMemoize.fswaitUntil, expect and assertTaskCanceled return Async/Task and the tests await them; .Result/.Wait() on the tasks those tests create became let!/do! too. Stress test is untouched: its Task.Wait with a timeout is the point of the test.
  • FSharp.Test.Utilities/ProjectGeneration.fssingleFileChecker returns Async; its only callers are the files above.

Left as is: Async.RunSynchronously inside the F# source strings that the tests compile or run (the behaviour under test), TypeChecks/Graph/Utils.fs's parseSourceCode (called from record literals in 30 places across the trie/graph tests; a separate change), and FSharp.Compiler.Service.Tests, which has a few hundred more sites.

Checklist

  • Test cases added — no new tests; the touched modules were run locally after the change.
  • Performance benchmarks added in case of performance changes — n/a.
  • Release notes entry updated — tests only, no release note.

Tests that blocked on Async.RunSynchronously now return Task and bind the FCS Async results inside task { }: SemanticClassificationRegressions, FindReferences, CommonWorkflows, TransparentCompiler (Fuzzing, getParseResult), CompilerDirectives/Line, Miscellaneous/TestUtilities, CompilationFromCmdlineArgsTests and AsyncMemoize, plus the singleFileChecker helper in FSharp.Test.Utilities. Async.RunSynchronously inside the F# sources under test, the graph tests' parseSourceCode and AsyncMemoize's Stress test are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

✅ No release notes required

@xperiandri xperiandri changed the title Make the ComponentTests checker tests asynchronous Make the ComponentTests checker tests asynchronous Sep 6, 2026
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Sep 6, 2026

@xperiandri xperiandri left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reviewed cd3849f as a full diff plus the untouched callers of every helper whose shape changed.

Verdict: the conversion is sound; nothing blocking. Two inline remarks — one fold-the-duplicate (non-blocking, but this PR is the natural place for it), one nit.

What was checked:

  • Every caller of a helper that changed shape (singleFileChecker, parse, getParseResult, findRefsInSource, getClassifications, classificationsForIdent, waitUntil/expect/assertTaskCanceled) is converted, and none exists outside FSharp.Compiler.ComponentTests. Stress test and the two pre-existing task tests in AsyncMemoize.fs use none of them, so leaving them alone is right.
  • let!/do! on an Async inside task { } goes through Async.StartImmediateAsTask with the default cancellation token — the same token and exception propagation Async.RunSynchronously had — so there is no semantic drift, including the try … finally around Environment.CurrentDirectory in CompilationFromCmdlineArgsTests (both theories are skipped anyway).
  • Source snippets in triple-quoted strings keep their columns; the UTF-8 BOM on the five files that had one is preserved; git diff --check is clean.
  • Ran the touched modules locally against a Debug/net11.0 build of this commit: AsyncMemoize, SemanticClassificationRegressions, FindReferences, CommonWorkflows, CompilerDirectives.Line, Miscellaneous.TestUtilities — 128 passed, 2 skipped, 0 failed. Fuzzing is Skipped on main, so its conversion is compile-verified only.

Non-blocking observation: the explicit : Task return annotation is new to this project — the existing task { } tests, including two in AsyncMemoize.fs and those in TransparentCompiler.fs, rely on the inferred Task<unit>. It is a good guard (an accidentally returned Async<unit> becomes a compile error instead of a value xunit has no way to await), so I would keep it; the two neighbours in AsyncMemoize.fs could get it too for uniformity, or not — either is fine.

Comment thread tests/FSharp.Compiler.ComponentTests/FSharpChecker/FindReferences.fs Outdated
Comment thread tests/FSharp.Compiler.ComponentTests/CompilerService/AsyncMemoize.fs Outdated
Review follow-up: Helpers.singleFileCheckerWithName takes the file name and singleFileChecker is its test.fs case, so the module-local copy in FindReferences.fs goes away; findRefsInFile/testFindRefsInFile carry the name and the #line test becomes a one-liner. assertTaskCanceled's parameter is job, not task, and the two remaining task tests in AsyncMemoize.fs get the same : Task annotation as the rest.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

1 participant