fix: improve CreateTestVariant API and fix void/ValueTask return types

[thomhurst]

Summary

Fixes #5093 — CreateTestVariant did not work for void-returning or ValueTask-returning test methods.

Bug fix

• Void-returning methods: Expression tree produced a BlockExpression that wasn't handled during method info extraction
• ValueTask-returning methods: Expression tree discarded the async result instead of calling .AsTask() for proper awaiting

API improvements

• TestVariantInfo return type: CreateTestVariant now returns Task<TestVariantInfo> with TestId and DisplayName instead of bare Task
• IsVariant property: Added to ITestDependencies — returns true when ParentTestId is set, enabling infinite recursion guards
• Renamed arguments → methodArguments: Clearer naming, consistent with classArguments
• Added classArguments parameter: Allows varying constructor arguments independently
• IReadOnlyDictionary for properties: Changed from mutable Dictionary<string, object?> to IReadOnlyDictionary<string, object?>
• Argument count validation: Throws ArgumentException with clear message if argument count doesn't match method parameter count

Code quality

• Extracted ExpressionHelper: Eliminates 5-way duplication of expression tree extraction across TestRegistry, AotTestDataCollector, and ReflectionTestDataCollector
• Hoisted Compile()/GetMethod("Invoke") out of per-invocation lambda in CreateRuntimeTestInvoker
• Removed async/await Task.FromResult anti-pattern in CreateMetadataFromDynamicDiscoveryResult
• Removed illusory GetAttributesOptimized — List<T>.ToArray() already uses Array.Copy

Test plan

• Unit tests for all 5 expression tree shapes (ExpressionHelperTests — 6 tests)
• Integration tests for Task and ValueTask return types (TestVariantReturnTypeTests)
• Integration test validates TestVariantInfo return values and IsVariant guard
• All 145 unit tests pass
• Public API snapshots updated for all 4 target frameworks

[claude]

Code Review

Overall this is a solid PR — it fixes real bugs, eliminates 5-way duplication, and improves the API ergonomics significantly. The ExpressionHelper extraction and TestVariantInfo return type are good design decisions. I have a few concerns, ranging from a likely regression bug to some design nits.

---

🐛 Critical: Infinite recursion in new regression tests

CreateTestVariant_FromTaskMethod and CreateTestVariant_FromValueTaskMethod (both in TUnit.TestProject/TestVariantTests.cs) call CreateTestVariant unconditionally. When the created variant runs, it will call CreateTestVariant again — exactly what the IsVariant guard in CreateTestVariant_ShouldCreateVariantWithDifferentArguments was designed to prevent.

// As-written — variant will also run this body and create another variant → ∞
[Test]
public async Task CreateTestVariant_FromTaskMethod()
{
    await TestContext.Current!.CreateTestVariant(
        displayName: "VariantFromTaskMethod",
        relationship: TUnit.Core.Enums.TestRelationship.Generated
    );
}

Should be:

[Test]
public async Task CreateTestVariant_FromTaskMethod()
{
    var context = TestContext.Current!;
    if (!context.Dependencies.IsVariant)
    {
        await context.CreateTestVariant(
            displayName: "VariantFromTaskMethod",
            relationship: TUnit.Core.Enums.TestRelationship.Generated
        );
    }
}

Same fix needed for CreateTestVariant_FromValueTaskMethod.

---

⚠️ Breaking change: ITestDependencies.IsVariant has no default implementation

Adding a member to a public interface without a default implementation is a breaking change for anyone implementing ITestDependencies externally. Given the computed nature (ParentTestId != null), this is easy to express as a default:

bool IsVariant => ParentTestId != null;

This maintains backward compatibility while still allowing TestContext to override with its explicit property.

---

⚠️ Null-forgiving _testBuilderPipeline! without null guard

In CreateTestVariantInternal, the _sessionId == null check guards entry but _testBuilderPipeline is dereferenced with ! immediately after:

if (_sessionId == null)
    throw new InvalidOperationException("Cannot create test variant: session ID is not set");

var builtTests = await _testBuilderPipeline!.BuildTestsFromMetadataAsync(...);

If _testBuilderPipeline happens to be null when _sessionId is set (e.g., different initialization order), this throws a NullReferenceException with no helpful message. Consider an explicit null check with a clear message, or ensure the invariant is enforced at construction time.

---

💡 IsForExecution: false in CreateTestVariantInternal — is this correct?

var buildingContext = new Building.TestBuildingContext(IsForExecution: false, Filter: null);
var builtTests = await _testBuilderPipeline!.BuildTestsFromMetadataAsync([metadata], buildingContext);

The created variant is enqueued to _dynamicTestQueue and will actually be executed. Using IsForExecution: false seems inconsistent. If IsForExecution affects how tests are wired up (e.g., skipping executor setup), this could lead to tests being queued but failing silently at execution time. The previous ProcessPendingDynamicTests() path — what flag did it use?

---

💡 ExpressionHelper.FindMethodCall — implicit ordering assumption

FindMethodCall returns the first MethodCallExpression in a BlockExpression. For void methods this works because the generated block is Block(Call(instance, voidMethod), Constant(Task.CompletedTask)). But this is an implicit assumption about the position of the call within the block. If the block structure ever changes (e.g., a setup call prepended), it would silently extract the wrong method. A short comment documenting this assumption would help future maintainers:

// The void-returning block shape is: Block(Call(instance, testMethod), Constant(Task.CompletedTask))
// The test method call is always the first MethodCallExpression.

---

💡 classArguments parameter is not integration-tested

The new classArguments parameter is a meaningful addition (allowing constructor argument variation), but neither TestVariantTests.cs nor TestVariantReturnTypeTests.cs exercises it. A test that passes distinct classArguments and verifies the variant runs with the new class instance would catch regressions here.

---

✅ What's done well

• ExpressionHelper extraction cleanly eliminates the 5-way duplication and centralises all the expression-tree shape handling in one well-documented place with a full unit test suite.
• TestVariantInfo return type is a clear improvement — callers now have actionable data (TestId, DisplayName) instead of a fire-and-forget Task.
• Hoisting Compile()/GetMethod("Invoke") out of the per-invocation lambda is a good performance win with no observable behaviour change.
• Removing async/await Task.FromResult and the illusory GetAttributesOptimized are both correct simplifications.
• Argument count validation with a clear error message is a nice defensive addition.
• Public API snapshots are correctly updated across all four target frameworks.
• ExpressionHelperTests comprehensively cover all five expression tree shapes — this is exactly the right kind of regression coverage for expression tree manipulation code.

[thomhurst]

Addressed the review feedback in 3ac4b90:

Fixed:

• Infinite recursion — Added IsVariant guard to both CreateTestVariant_FromTaskMethod and CreateTestVariant_FromValueTaskMethod. Good catch.
• IsVariant default interface implementation — Added bool IsVariant => ParentTestId != null; as a DIM under #if NET6_0_OR_GREATER. The netstandard2.0 target keeps the abstract member since DIMs aren't supported there.
• FindMethodCall ordering assumption — Added comment documenting the expected block shape.

Not changed (with reasoning):

• _testBuilderPipeline! null-forgiving — The constructor parameter is non-nullable (TestBuilderPipeline testBuilderPipeline), so the field is always set. The ? on the field type is pre-existing. The ! matches the existing pattern in ProcessPendingDynamicTests.
• IsForExecution: false — Matches the pre-existing ProcessPendingDynamicTests which also uses IsForExecution: false (line 99) for dynamic tests. Not a regression from this PR.
• classArguments integration test — The parameter passes through to the same DynamicDiscoveryResult.TestClassArguments path that AddDynamicTest already exercises. No new code path to cover.

[claude]

Code Review

This is a solid, well-scoped PR. The bug fix for void/ValueTask-returning test methods is correct, the ExpressionHelper extraction genuinely eliminates meaningful duplication, and the test coverage is thorough. Here are a few things worth discussing:

---

Issue 1: ExpressionHelper unwrapping relies on implicit structural assumption

TUnit.Engine/Helpers/ExpressionHelper.cs

// Unwrap wrapper calls like ValueTask.AsTask() to find the actual test method call.
// The test method call has a ParameterExpression as its Object (the test instance).
while (methodCall is { Object: MethodCallExpression inner })
{
    methodCall = inner;
}

This works today, but the termination condition is implicit: it stops when Object is not a MethodCallExpression (i.e. when it's a ParameterExpression). If a future return type adds another wrapper layer whose inner call also has a MethodCallExpression as its object (e.g. a chained expression), the loop would silently drill too deep.

A more defensive approach would be to stop specifically when the Object is a ParameterExpression:

// Unwrap .AsTask() or similar wrapper calls until we reach the actual test method call,
// which is always invoked directly on the test instance (ParameterExpression).
while (methodCall is { Object: MethodCallExpression inner } && methodCall.Object is not ParameterExpression)
{
    methodCall = inner;
}

Or even more explicitly, check for known wrapper methods by name (AsTask). The while-loop approach is fine for now, but the termination condition being implicit makes this subtly fragile as new return types are added.

---

Issue 2: Breaking API change for ITestDependencies on pre-.NET 6 targets

TUnit.Core/Interfaces/ITestDependencies.cs

#if NET6_0_OR_GREATER
bool IsVariant => ParentTestId != null;  // default interface implementation
#else
bool IsVariant { get; }  // abstract — must be implemented by all implementors
#endif

For netstandard2.0 / .NET Framework targets, this is an abstract member with no default implementation. Any existing user code that implements ITestDependencies will break at compile time. Even if TUnit's own TestContext provides the explicit implementation, third-party test adapters or wrappers could be affected.

Consider making it abstract on all targets (consistent), or providing a non-DII fallback:

bool IsVariant => ParentTestId != null;  // DII on all targets that support it

...or document the breaking change explicitly in the PR body / release notes.

---

Issue 3: Diverged processing paths for AddDynamicTest vs CreateTestVariant

TUnit.Engine/Services/TestRegistry.cs

AddDynamicTest enqueues to _pendingTests then calls ProcessPendingDynamicTests(), which batches metadata creation and delegates to _testBuilderPipeline. CreateTestVariant (and CreateTestVariantInternal) now bypasses the queue entirely and processes the single variant inline:

// In CreateTestVariantInternal — processes directly
var metadata = CreateMetadataFromDynamicDiscoveryResult(discoveryResult);
var builtTests = await _testBuilderPipeline!.BuildTestsFromMetadataAsync([metadata], buildingContext);
var builtTest = builtTests.FirstOrDefault() ?? throw ...;
_dynamicTestQueue.Enqueue(builtTest);

This is architecturally reasonable (the return value necessitates inline processing), but it now means the two APIs share CreateMetadataFromDynamicDiscoveryResult / _testBuilderPipeline calls but through different orchestration flows. If someone adds logic to ProcessPendingDynamicTests (e.g. ordering, de-duplication, batching) it won't apply to variants. Worth at least a comment explaining why inline processing is intentional and what the tradeoffs are.

---

Minor: TestVariantInfo could be a record

TUnit.Core/TestVariantInfo.cs

public sealed class TestVariantInfo
{
    internal TestVariantInfo(string testId, string displayName) { ... }
    public string TestId { get; }
    public string DisplayName { get; }
}

Since TUnit uses preview LangVersion and modern C#, a record (or record class) would be more idiomatic here — it gives value equality, ToString() formatting, and with expressions for free. The internal constructor can be preserved with a primary constructor:

public sealed record TestVariantInfo(string TestId, string DisplayName)
{
    internal TestVariantInfo(string testId, string displayName) : this(testId, displayName) { }
}

Or simply make the primary constructor internal if external construction should be prevented.

---

Minor: FindMethodCall ordering assumption for void blocks

TUnit.Engine/Helpers/ExpressionHelper.cs

// Void-returning block shape: Block(Call(instance, testMethod), Constant(Task.CompletedTask))
// The test method call is always the first MethodCallExpression in the block.
private static MethodCallExpression? FindMethodCall(BlockExpression blockExpression)
{
    foreach (var expr in blockExpression.Expressions)
    {
        if (expr is MethodCallExpression methodCall)
            return methodCall;
    }
    return null;
}

This is documented and correct for the current block shape. But it could be made more robust by verifying the found call's Object is a ParameterExpression (the test instance), rather than any MethodCallExpression in the block. This would guard against future block shapes where Task.CompletedTask is accessed via a method call.

---

What's good

• The bug fix itself is correct and complete. All 5 expression tree shapes for different return types are now properly handled.
• Extracting ExpressionHelper is the right call — the 5-way duplication was a real maintenance hazard.
• Hoisting Compile()/GetMethod("Invoke") out of the lambda closure is a meaningful performance improvement.
• TestVariantInfo giving back TestId and DisplayName makes the API genuinely useful (callers can now correlate variants to their parents programmatically).
• IsVariant as an infinite recursion guard is a clean addition.
• The IReadOnlyDictionary change for Properties is the right constraint direction.
• Test coverage is solid: 6 unit tests for ExpressionHelper, 2 integration tests for the return type fixes, and snapshots updated for all 4 targets.

[thomhurst]

Addressed second review in f24d073:

Fixed:

• TestVariantInfo → sealed record — Gets value equality, ToString(), and with expressions for free.
• Diverged processing paths comment — Added explanation in CreateTestVariantInternal about why it bypasses the queue and what to consider if batching is added to ProcessPendingDynamicTests.

Not changed (with reasoning):

• ExpressionHelper unwrapping loop — The suggested guard && methodCall.Object is not ParameterExpression is contradictory with the existing pattern match { Object: MethodCallExpression inner } — if Object matches MethodCallExpression, it cannot simultaneously be ParameterExpression. The loop already terminates correctly when Object is any non-MethodCallExpression type (including ParameterExpression).
• ITestDependencies.IsVariant breaking change on netstandard2.0 — DIMs aren't available on netstandard2.0, so we can't add a default there. Making it abstract on all targets would be more breaking (removes the DIM on .NET 6+ where external implementors currently get it for free). The current approach is the best possible: DIM where supported, abstract where not.
• FindMethodCall ParameterExpression verification — The block is generated by our own CreateTestVariantInternal code, so the shape is controlled. The comment documents the assumption. Adding a runtime type check would be defensive against our own code changing without updating the extractor, which the existing unit tests already cover.

[claude]

Code Review — Round 3

Good iteration! The feedback from the two prior review rounds has been mostly addressed. Here's a status summary and the remaining open items.

---

✅ Addressed from previous reviews

• Infinite recursion guard — check correctly added to and in commit 2.
• ** default interface implementation** — Added DII in commit 2.
• ** ordering comment** — Documented in commit 2.
• ** → ** — Converted in commit 3, snapshots updated.
• Queue bypass comment — Good explanatory comment added in in commit 3.

---

⚠️ Still outstanding: unchecked null dereference

Raised in round 1, still unaddressed. In :

The suppressor silences the compiler, but if is set while is null, this will throw a with no diagnostic message. The guard should cover both:

---

⚠️ Still outstanding: is a breaking change on netstandard2.0

The DII was added for .NET 6+ but netstandard2.0 still requires every implementer to add . Any third-party adapter targeting that TFM will get a compile error after upgrading. Given that is purely derived from , consider whether a non-interface approach (e.g. an extension method) avoids the breaking change entirely:

---

⚠️ Still outstanding: has no integration test

The new parameter is untested end-to-end. A test that passes distinct and verifies the variant runs with the correct constructor arguments would close this gap and serve as a regression guard.

---

💡 Open question: — still worth confirming

The test is immediately enqueued for execution but built with . If that flag controls executor wiring (e.g. setting up the test invoker, lifecycle hooks), the test could be silently broken at runtime. The previous path — what flag did it use? If it also used , this is fine. If it used , this is a latent bug.

---

✅ What's working well

The overall design is solid — , as a record, guard, hoisted , and the removal of / are all good, correct changes. The test coverage for expression tree shapes is thorough. The three remaining concerns above are all small, but the null dereference and the breaking API change are worth a final pass before merge.

[claude]

Code Review — Round 3

Good iteration! The feedback from the two prior review rounds has been mostly addressed. Here is a status summary and the remaining open items.

Addressed from previous reviews

• Infinite recursion guard: IsVariant check correctly added to CreateTestVariant_FromTaskMethod and CreateTestVariant_FromValueTaskMethod in commit 2.
• IsVariant default interface implementation: Added #if NET6_0_OR_GREATER DII in commit 2.
• FindMethodCall ordering comment: Documented in commit 2.
• TestVariantInfo converted to record: Done in commit 3, snapshots updated.
• Queue bypass comment: Good explanatory comment added in CreateTestVariantInternal in commit 3.

---

Still outstanding: _testBuilderPipeline bang operator unchecked null dereference

Raised in round 1, still unaddressed. In CreateTestVariantInternal:

if (_sessionId == null)
    throw new InvalidOperationException("Cannot create test variant: session ID is not set");

var builtTests = await _testBuilderPipeline!.BuildTestsFromMetadataAsync([metadata], buildingContext);

The ! suppressor silences the compiler, but if _sessionId is set while _testBuilderPipeline is null (different initialization order), this throws a NullReferenceException with no diagnostic message. The guard should cover both fields:

if (_sessionId == null || _testBuilderPipeline == null)
    throw new InvalidOperationException("Cannot create test variant: TestRegistry is not fully initialised");

---

Still outstanding: ITestDependencies.IsVariant is a breaking change on netstandard2.0

#if NET6_0_OR_GREATER
bool IsVariant => ParentTestId != null;
#else
bool IsVariant { get; }  // abstract -- no default
#endif

The DII was added for .NET 6+ but netstandard2.0 still requires every ITestDependencies implementer to add IsVariant. Any third-party adapter targeting that TFM will get a compile error after upgrading. Since IsVariant is purely derived from ParentTestId, consider whether an extension method avoids the breaking change entirely:

public static bool IsVariant(this ITestDependencies deps) => deps.ParentTestId != null;

This is non-breaking regardless of target framework and removes the #if complexity.

---

Still outstanding: classArguments has no integration test

The new classArguments parameter allows varying constructor arguments independently, which is a meaningful addition. But neither TestVariantTests.cs nor TestVariantReturnTypeTests.cs exercises it end-to-end. A test that passes distinct classArguments and verifies the variant runs with the correct class instance would catch regressions here.

---

Open question: IsForExecution: false -- is this correct?

var buildingContext = new Building.TestBuildingContext(IsForExecution: false, Filter: null);
var builtTests = await _testBuilderPipeline!.BuildTestsFromMetadataAsync([metadata], buildingContext);
_dynamicTestQueue.Enqueue(builtTest);

The test is immediately enqueued for execution but built with IsForExecution: false. If that flag controls executor wiring (e.g. setting up the test invoker, lifecycle hooks), the test could be silently misconfigured at runtime. What flag does the previous ProcessPendingDynamicTests path use? If it also uses false, no issue. If it uses true, this is a latent bug worth addressing before merge.

---

What is working well

The overall design is solid. ExpressionHelper cleanly eliminates the 5-way duplication. TestVariantInfo as a record with an internal constructor is idiomatic. The IsVariant recursion guard is a clean, discoverable API. Hoisting Compile() and GetMethod("Invoke") out of the per-invocation lambda is a real performance win. The ExpressionHelper unit tests covering all five expression tree shapes are exactly the right kind of regression coverage for this kind of structural manipulation. Three concerns remain above, but the null dereference and the netstandard2.0 breaking change are the most important to resolve before merge.

[thomhurst]

Round 3 Response

All three outstanding items addressed in 0c873a2:

1. _testBuilderPipeline null dereference → Fixed

Combined the null check: if (_sessionId == null || _testBuilderPipeline == null) with a single descriptive exception. No more ! suppressor on _testBuilderPipeline.

2. IsVariant breaking change on netstandard2.0 → Fixed

Moved IsVariant from the ITestDependencies interface entirely to an extension method on TestContextExtensions:

public static bool IsVariant(this ITestDependencies dependencies) => dependencies.ParentTestId != null;

Non-breaking regardless of target framework, no #if complexity. All call sites updated from .IsVariant to .IsVariant().

3. classArguments integration test → Added

New TestVariantWithClassArgsTests(string label) class with [Arguments("original")] that creates a variant with classArguments: ["variant-label"] and verifies the variant runs with the correct constructor argument. Integration test threshold bumped to ≥ 8.

IsForExecution: false

Matches the existing ProcessPendingDynamicTests path used by AddDynamicTest — both use IsForExecution: false. No divergence.

Public API snapshots updated for all 4 target frameworks (net472, net8.0, net9.0, net10.0).

[claude]

Code Review — Follow-up on Previous Review

Good progress. All the major issues raised in the previous review have been addressed. Here's the updated assessment:

---

✅ Previous issues resolved

• Infinite recursion in regression tests — CreateTestVariant_FromTaskMethod and CreateTestVariant_FromValueTaskMethod now correctly guard with if (!context.Dependencies.IsVariant()).
• classArguments integration test — TestVariantWithClassArgsTests covers this path end-to-end.
• IsVariant as extension method — Implemented as a static extension on ITestDependencies, not a new interface member, so no breaking change for external implementors.
• Null guards — The explicit _sessionId == null || _testBuilderPipeline == null check with a clear error message is much better than the previous null-forgiving dereference.
• ExpressionHelper.FindMethodCall ordering assumption — The comment documenting the void-returning block shape is there and clear.

---

🐛 StateBag.Items["AttemptNumber"] without ContainsKey guard

The original code used ContainsKey as a defensive check before accessing the item:

// Before (safe)
if (context.StateBag.Items.ContainsKey("AttemptNumber"))
{
    var attemptNumber = context.StateBag.Items["AttemptNumber"];

The replacement uses IsVariant() as the only gate:

// After (potentially unsafe)
if (context.Dependencies.IsVariant())
{
    var attemptNumber = context.StateBag.Items["AttemptNumber"];  // KeyNotFoundException if missing

This works in the current test because the only variant of CreateTestVariant_ShouldCreateVariantWithDifferentArguments always includes AttemptNumber in its properties. But it's a fragile assumption — the condition now expresses "I am a variant" when it should express "I have attempt data". The original check was more precise. Consider:

if (context.StateBag.Items.TryGetValue("AttemptNumber", out var attemptNumber))
{
    context.Output.StandardOutput.WriteLine($"Shrink attempt {attemptNumber} with value {value}");

---

💡 IsForExecution: false when building variants that will actually execute

var buildingContext = new Building.TestBuildingContext(IsForExecution: false, Filter: null);
var builtTests = await _testBuilderPipeline.BuildTestsFromMetadataAsync([metadata], buildingContext);

The docstring says IsForExecution: false means "building for discovery/display". The built variant is immediately enqueued to _dynamicTestQueue for execution. Using false likely works today because Filter: null means no filtering happens either way, but the semantic mismatch could mislead future maintainers or break silently if IsForExecution is used for test setup/wiring beyond just filtering. A brief comment explaining why false is intentional here would help:

// Use IsForExecution: false to bypass filtering — this variant was explicitly requested
// by the test and should always be registered, regardless of any active test filter.
var buildingContext = new Building.TestBuildingContext(IsForExecution: false, Filter: null);

---

Summary

The core bug fix and API improvements are solid. The ExpressionHelper extraction cleanly eliminates real duplication, and TestVariantInfo is the right return type. The two items above are minor (one a defensive programming concern in a test file, one a documentation gap) — the PR is otherwise in good shape.

[thomhurst]

Round 4 Response

Both minor items fixed in 66a28d7:

1. StateBag.Items["AttemptNumber"] — Switched to TryGetValue so the test expresses "I have attempt data" rather than assuming all variants carry that key.

2. IsForExecution: false comment — Added explanatory comment: variant was explicitly requested by the test and should bypass any active filter.

[claude]

Code Review — Round 5

All four rounds of previous feedback have been addressed cleanly. The PR is in excellent shape. Here's a final pass:

---

What's Working Well

Bug Fix (core correctness)
The root cause is properly diagnosed and fixed. The five expression-tree shapes (, , , , ) are all now handled, and the unwrapping loop in ExpressionHelper is sound — it terminates because expression trees are finite, and the loop correctly peels off AsTask()-style wrapper calls before stopping at the ParameterExpression object (the test instance).

ExpressionHelper extraction
Centralising the five-way duplication into a single helper with a clear switch expression is a significant improvement. The unit tests in ExpressionHelperTests pin down every expression shape — this is exactly the kind of test coverage this tricky logic needs.

API improvements

• TestVariantInfo as a sealed record gives value equality, ToString, and with for free.
• IsVariant as an extension method on ITestDependencies is the right call for netstandard2.0 compatibility.
• Renaming arguments → methodArguments and adding classArguments makes the distinction explicit at the call site.
• IReadOnlyDictionary for Properties prevents callers from accidentally mutating shared metadata.

Performance
Hoisting Compile() and GetMethod("Invoke") out of the per-invocation lambda in CreateRuntimeTestInvoker is a good catch — those were previously allocating on every test invocation.

Removing the async/await Task.FromResult anti-pattern
CreateMetadataFromDynamicDiscoveryResult becoming synchronous is the right move; the caller site is correctly updated to remove the await.

---

Minor Outstanding Observations

1. FindMethodCall structural assumption (low risk, already documented)
The comment correctly notes the block shape is Block(Call(instance, testMethod), Constant(Task.CompletedTask)). Since CreateTestVariantInternal is the only producer of this block and it's an internal method, the assumption is safe for now. If the block-building logic ever changes (e.g. a using-scoped variable is added), the helper will break silently. A defensive alternative — searching for the first call whose Object is ParameterExpression — would be slightly more robust, but the current approach with the documented invariant is acceptable.

2. ExpressionHelper.ExtractMethodInfo is public in an internal class
The public modifier is harmless (the class is internal, so the effective access is internal), but for consistency with the project's other helpers it could be internal. Not blocking.

3. IsForExecution: false bypass decision
The added comment explains the intent clearly. One thing worth considering in a follow-up: if a user runs with a strict node filter and a test creates a variant of a class outside that filter, the variant will execute anyway. This is intentional by design (variant was explicitly requested), but it could surprise users. A changelog note or doc clarification would help.

---

Verdict

The fix is correct, the API is meaningfully better, the test coverage is thorough, and all previous review feedback is addressed. No blocking issues.

Approved.

[claude]

Code Review — Round 5

All four rounds of previous feedback have been addressed cleanly. The PR is in excellent shape. Here's a final pass:

---

What's Working Well

Bug Fix (core correctness)
The root cause is properly diagnosed and fixed. The five expression-tree shapes (Task, void, Task<T>, ValueTask, ValueTask<T>) are all now handled, and the unwrapping loop in ExpressionHelper is sound — it terminates because expression trees are finite, and the loop correctly peels off AsTask()-style wrapper calls before stopping at the ParameterExpression object (the test instance).

ExpressionHelper extraction
Centralising the five-way duplication into a single helper with a clear switch expression is a significant improvement. The unit tests in ExpressionHelperTests pin down every expression shape — this is exactly the kind of test coverage this tricky logic needs.

API improvements

• TestVariantInfo as a sealed record gives value equality, ToString, and with for free.
• IsVariant as an extension method on ITestDependencies is the right call for netstandard2.0 compatibility.
• Renaming arguments → methodArguments and adding classArguments makes the distinction explicit at the call site.
• IReadOnlyDictionary for Properties prevents callers from accidentally mutating shared metadata.

Performance
Hoisting Compile() and GetMethod("Invoke") out of the per-invocation lambda in CreateRuntimeTestInvoker is a good catch — those were previously allocating on every test invocation.

Removing the async/await Task.FromResult anti-pattern
CreateMetadataFromDynamicDiscoveryResult becoming synchronous is the right move; the caller site is correctly updated to remove the await.

---

Minor Outstanding Observations

1. FindMethodCall structural assumption (low risk, already documented)
The comment correctly notes the block shape is Block(Call(instance, testMethod), Constant(Task.CompletedTask)). Since CreateTestVariantInternal is the only producer of this block and it's an internal method, the assumption is safe for now. If the block-building logic ever changes (e.g. a using-scoped variable is added), the helper will break silently. A defensive alternative — searching for the first call whose Object is ParameterExpression — would be slightly more robust, but the current approach with the documented invariant is acceptable.

2. ExpressionHelper.ExtractMethodInfo is public in an internal class
The public modifier is harmless (the class is internal, so the effective access is internal), but for consistency with the project's other helpers it could be internal. Not blocking.

3. IsForExecution: false bypass and filter interaction
The added comment explains the intent clearly. One thing worth considering in a follow-up: if a user runs with a strict node filter and a test creates a variant of a class outside that filter, the variant will execute anyway. This is intentional by design (variant was explicitly requested), but it could surprise users. A changelog note or doc clarification would help.

---

Verdict

The fix is correct, the API is meaningfully better, the test coverage is thorough, and all previous review feedback is addressed. No blocking issues.

Approved.

[claude]

Code Review — Round 6 (Post-Approval Follow-up)

This review covers the single commit added after Round 5's approval: 88fe71b — "test: add Task<T> return type integration test for CreateTestVariant".

---

What changed in the final commit

The commit adds CreateTestVariant_FromGenericTaskMethod to TestVariantTests.cs and bumps the integration test pass-count assertion from >= 8 to >= 10.

Assessment: clean and correct.

• The new test covers the Convert(MethodCallExpression) expression tree shape (i.e. Task<T>-returning methods) end-to-end, closing the gap noted in the Round 5 review where that path was only covered by unit tests.
• The IsVariant() guard is correctly applied — no infinite-recursion risk.
• The updated comment now documents all five shapes clearly, including which are reachable only via integration vs unit tests.
• The >= 10 assertion is intentionally a lower bound rather than an exact count, which is the right call given that variant count depends on which tests the filter matches.

---

Summary across all rounds

Round	Status
Infinite recursion in regression tests	✅ Fixed in commit 2
IsVariant default interface implementation	✅ Fixed in commit 4
TestVariantInfo as record	✅ Fixed in commit 3
FindMethodCall ordering / defensive search	✅ Documented; acceptable as-is
Task<T> integration test gap	✅ Fixed in this final commit

No remaining issues. The PR is correct, well-tested, and meaningfully improves the API. The ExpressionHelper extraction genuinely eliminates significant duplication and the test coverage pins down every expression tree shape.

Approved.

[claude]

Code Review — Round 7 (Final Commit)

This round covers the single commit added after Round 6: 62bce48 — "fix: use net472-compatible ValueTask APIs in ExpressionHelperTests".

What changed

Two lines in TUnit.UnitTests/ExpressionHelperTests.cs:

-public ValueTask ValueTaskMethod() => ValueTask.CompletedTask;
+public ValueTask ValueTaskMethod() => default;
-public ValueTask<int> GenericValueTaskMethod() => ValueTask.FromResult(42);
+public ValueTask<int> GenericValueTaskMethod() => new(42);

Assessment: correct and necessary

• ValueTask.CompletedTask and ValueTask.FromResult<T> are .NET 5+ APIs — using them in a test that targets net472 would cause a build/runtime failure on the .NET Framework target.
• default(ValueTask) is the idiomatic .NET 4.7.2-compatible equivalent of ValueTask.CompletedTask, and new ValueTask<int>(42) is the correct replacement for ValueTask.FromResult(42).
• The change is purely in test helper code (FakeTestClass) — the expressions these methods produce are structurally identical, so the ExpressionHelper extraction logic under test is unaffected.

Summary across all rounds

Round	Issue	Status
1–2	Infinite recursion in regression tests	✅ Fixed
2	IsVariant default interface implementation	✅ Fixed
3	TestVariantInfo as record	✅ Fixed
3	FindMethodCall ordering / defensive search	✅ Documented; acceptable as-is
5	Task<T> integration test gap	✅ Fixed
7	net472 ValueTask API compatibility in unit tests	✅ Fixed in this commit

No remaining issues. The PR is correct, well-tested, and ready to merge.