Skip to content

Conversation

@thomhurst
Copy link
Owner

Fixes #3984

Copy link

Copilot AI left a comment

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 improves support for F# Async<'T> return types in test methods by enabling both source-generated and reflection-based test execution modes to properly handle F# async workflows. Previously, F# async tests would not execute correctly as the framework only handled Task and ValueTask return types.

Key Changes:

  • Extended AsyncConvert.ConvertObject usage to handle F# Async and other awaitable return types in both execution modes
  • Added scoping blocks in generated switch cases to support multiple variable declarations
  • Added comprehensive F# async test coverage in TUnit.TestProject.FSharp

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs Modified test invoker generation to detect void vs non-void return types and route non-async, non-void methods through AsyncConvert.ConvertObject for F# Async support; added scoping blocks in switch cases to prevent variable name collisions
TUnit.Engine/Discovery/ReflectionTestDataCollector.cs Changed reflection path to use AsyncConvert.ConvertObject for all non-Task/non-ValueTask results instead of returning Task.CompletedTask, enabling F# Async support in reflection mode
TUnit.TestProject.FSharp/AsyncTests.fs Added 6 new tests covering F# Async execution scenarios including basic execution, return values, async operations, Task interop, and assertions
TUnit.TestProject.FSharp/TUnit.TestProject.FSharp.fsproj Registered the new AsyncTests.fs file in the project

Comment on lines +30 to +33
member _.FSharpAsync_WithReturnValue() : Async<int> = async {
do! Async.Sleep 10
return 42
}
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The test FSharpAsync_WithReturnValue() returns Async<int> but doesn't verify that the return value (42) is actually captured and processed correctly by the framework. Consider adding a follow-up test that validates the return value, similar to how VerifyFSharpAsyncExecuted() validates execution state.

Example:

static member val private ReturnedValue = 0 with get, set

[<Test>]
member _.FSharpAsync_WithReturnValue() : Async<int> = async {
    do! Async.Sleep 10
    AsyncTests.ReturnedValue <- 42
    return 42
}

[<Test>]
[<DependsOn("FSharpAsync_WithReturnValue")>]
member _.VerifyFSharpAsyncReturnValue() =
    if AsyncTests.ReturnedValue <> 42 then
        failwith "F# Async return value was not processed!"
    Task.CompletedTask

Copilot uses AI. Check for mistakes.
if (result is ValueTask valueTask)
{
return valueTask.AsTask();
}
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The unconditional use of AsyncConvert.ConvertObject(result).AsTask() introduces a performance regression for void methods. The old code returned Task.CompletedTask (a cached singleton), while the new code creates an async state machine allocation even for null results.

Consider checking the return type or result before calling AsyncConvert:

if (result is null)
{
    return Task.CompletedTask;
}
return AsyncConvert.ConvertObject(result).AsTask();

This preserves the original performance for the common case of void methods while adding F# Async support.

Suggested change
}
}
// Preserve performance for void methods (null result)
if (result is null)
{
return Task.CompletedTask;
}

Copilot uses AI. Check for mistakes.
@thomhurst thomhurst enabled auto-merge (squash) December 5, 2025 17:58
@thomhurst thomhurst disabled auto-merge December 5, 2025 19:32
@thomhurst thomhurst merged commit 557e2b3 into main Dec 5, 2025
11 of 13 checks passed
@thomhurst thomhurst deleted the bug/3984 branch December 5, 2025 19:32
@claude claude bot mentioned this pull request Dec 6, 2025
1 task
This was referenced Dec 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

F# - async tests don't actually run (false positive)

2 participants