perf: make test metadata creation fully synchronous

[thomhurst]

Summary

• Replace IAsyncEnumerable<TestMetadata> with IReadOnlyList<TestMetadata> throughout the test metadata creation pipeline
• The async iterators were generating unique async state machine types per test method (6 JIT-compiled interface methods each), causing massive JIT overhead at startup
• ITestSource.GetTestsAsync() → GetTests() returning IReadOnlyList<TestMetadata>
• TestDescriptor.Materializer delegate made synchronous
• AotTestDataCollector and ReflectionTestDataCollector made synchronous
• Source generator emits List<TestMetadata> with .Add() instead of yield return
• Remove unused ToAsyncEnumerable helper from TestBuilderPipeline

Performance (10,000 source-generated tests)

Metric	Before	After	Improvement
Wall time	~14.6s	~5.5s	62% faster
Test duration	~12.5s	~4.9s	61% faster
Total JIT compilations	136,291	105,309	23% fewer
Async state machine JITs	~60,000	345	99.4% eliminated
DLL size	52MB	35MB	33% smaller

Closes #5043

Test plan

• Source generator snapshot tests pass (456/456)
• TUnit.TestProject smoke tests pass (BasicTests, DataDrivenTests, NumberArgumentTests)
• Reproduction project: 10,000 tests pass with 0 failures
• CI passes

[claude]

Code Review: perf: make test metadata creation fully synchronous

The performance improvements here are exceptional — 62% faster startup and 99.4% elimination of async state machine JIT compilations directly addresses a real scalability bottleneck. The root-cause analysis (each async iterator generating 6 unique JIT-compiled interface methods × test count = massive overhead) is well-documented and clearly proven by the benchmarks.

What's Excellent

The fundamental insight is correct: test metadata discovery is a CPU-bound, in-memory operation that was being forced through the async/await machinery for no real benefit. IReadOnlyList<TestMetadata> is the right abstraction — it's honest about the synchronous nature of the operation and allows the caller to access by index or enumerate freely.

The DLL size reduction (52MB → 35MB) is an unexpected but welcome bonus, since each async IAsyncEnumerable<T> method generates a compiler-synthesized state machine class.

Both source-gen and reflection paths are updated consistently (satisfying the dual-mode requirement from CLAUDE.md).

---

Concerns

1. Breaking Public API — Version Bump Needed

ITestSource is a public interface in TUnit.Core, and TestDescriptor.Materializer is a public required property on a public readonly struct. Both have changed signatures:

• IAsyncEnumerable<TestMetadata> GetTestsAsync(string, CancellationToken) → IReadOnlyList<TestMetadata> GetTests(string)
• Func<string, CancellationToken, IAsyncEnumerable<TestMetadata>> → Func<string, IReadOnlyList<TestMetadata>>

Any user who implemented ITestSource directly (e.g. a custom test adapter or integration tool), or captured/stored a TestDescriptor.Materializer delegate, will get compile errors. This warrants a major version bump (or at minimum a clear migration note in the changelog). The [Obsolete] bridge pattern could ease migration, though given the source-gen nature of most implementations this may not be necessary in practice.

2. Cancellation Token Removed Without Replacement

CollectTestsAsync on AotTestDataCollector still takes a CancellationToken, but that token is never passed down to GetTests(testSessionId). For suites with thousands of tests, pressing Ctrl+C during startup will now block until all metadata has been collected synchronously, rather than short-circuiting.

A lightweight fix would be to thread the CT through ITestSource.GetTests:

IReadOnlyList<TestMetadata> GetTests(string testSessionId, CancellationToken cancellationToken = default);

Generated code doesn't need to check it on every iteration, but the engine can check between descriptor.Materializer(...) calls in the inner loops. Alternatively, wrap the outer collection loop with periodic cancellationToken.ThrowIfCancellationRequested() checks.

3. Parallel Collection Path Silently Dropped

CollectTestsTraditionalAsync previously parallelised collection across large test source sets:

// Before: parallel path for Count >= MinItemsForParallel
return await testSourcesList
    .SelectManyAsync(testSource => testSource.GetTestsAsync(testSessionId))
    .ProcessInParallel();

The new CollectTestsTraditional is fully sequential. While the synchronous nature eliminates async I/O parallelism benefits, there may still be value in using Parallel.ForEach (or PLINQ) for CPU-bound metadata construction across many sources in the reflection path. The benchmarks are for the source-gen path; reflection-heavy suites may regress. Worth measuring before closing this.

4. List<TestMetadata> Allocation for Single-Test Methods (Generated Code)

In the source generator, every GetTests() method creates a List<TestMetadata>() even for the extremely common case of a non-parameterised test that yields exactly one result:

// Generated for every single [Test] method:
var __results = new global::System.Collections.Generic.List<global::TUnit.Core.TestMetadata>();
// ... build metadata ...
__results.Add(metadata);
return __results;

For 10,000 tests this is 10,000 List<T> allocations. The reflection-based MaterializeSingleTest already does this correctly:

return [metadata]; // single-element array — more efficient

The source generator could detect at compile time whether a test source can only ever produce one result (no [Arguments], no [ClassDataSource], no [Repeat], etc.) and emit return [metadata]; directly. For parameterised tests, keeping the List makes sense since the count is unknown at generation time.

5. Minor: foreach vs. index for Inconsistency in MaterializeFromDescriptors

Both AotTestDataCollector.MaterializeFromDescriptors and ReflectionTestDataCollector.MaterializeFromDescriptors use for (var i = 0; i < materialized.Count; i++) to iterate the result, then yield return materialized[i]. Since IReadOnlyList<T> supports indexing this is correct and avoids enumerator allocation, but it's inconsistent with a simple foreach (var m in materialized) yield return m; which would be clearer and equally efficient for list implementations. Minor readability nit.

---

Summary

Area	Verdict
Performance improvement	Excellent — well-proven with data
Dual-mode consistency (source-gen + reflection)	Correct
Snapshot tests	Updated correctly
Breaking API change	Needs changelog/version callout
Cancellation support	Regressed — worth addressing
Parallel collection	Implicitly dropped — measure reflection impact
Allocation efficiency (single-test)	Follow-up opportunity

The core change is correct and the performance gains are compelling. The main things to address before merge are the breaking API concern (documentation/versioning) and the cancellation token gap.

[claude]

Code Review: perf: make test metadata creation fully synchronous (Follow-up Pass)

This is a follow-up to my previous review. None of the prior comments appear to have been addressed yet, so I'm re-raising the key ones and adding new observations from reading the full diff.

---

Still-Outstanding Concerns from Previous Review

1. Breaking Public API — Changelog/Version Note Still Needed

ITestSource.GetTests and TestDescriptor.Materializer are both public API surface. Any external adapter or integration project that implemented ITestSource directly or stored a Materializer delegate will get compile errors. This is acceptable given TUnit's source-gen model, but it needs a clear callout in the release notes and ideally a major version bump (or at minimum a migration note in CHANGELOG).

2. Cancellation Token Gap — Still Dropped

CollectTestsAsync still receives a CancellationToken in its signature:

public async Task<IEnumerable<TestMetadata>> CollectTestsAsync(string testSessionId, ..., CancellationToken cancellationToken)

But that token is now never passed to CollectTestsWithTwoPhaseDiscovery or CollectTestsTraditional. The old code at least had cancellationToken.ThrowIfCancellationRequested() in MaterializeFromDescriptorsAsync. A Ctrl+C during startup on a 10,000-test suite will block until all synchronous metadata collection completes.

A minimal fix — add periodic CT checks between descriptor materializations in the outer loops:

foreach (var descriptor in descriptorsToMaterialize)
{
    cancellationToken.ThrowIfCancellationRequested(); // re-add this
    var materialized = descriptor.Materializer(testSessionId);
    ...
}

3. Parallel Collection Silently Dropped — Reflection Path May Regress

CollectTestsTraditional previously had a parallel path for large test source sets (Count >= MinItemsForParallel). The PR removes this entirely and is now fully sequential. The benchmarks shown are for the source-gen path. Reflection-heavy suites with hundreds of types may regress. This is worth measuring before closing.

---

New Observations

4. static Lambda on InstanceFactory — Excellent and Underappreciated

The change from (typeArgs, args) => to static (typeArgs, args) => on InstanceFactory in InstanceFactoryGenerator.cs is a meaningful win that the PR description doesn't highlight. Static lambdas can be cached as a single delegate instance by the JIT (no closure allocation per call-site), which matters when there's one factory per test class type across 10,000 tests. Good catch.

5. Inconsistent Return Type Between Source-Gen and Reflection Materializers

The reflection path (MaterializeSingleTest) returns TestMetadata[] (array):

private static TestMetadata[] MaterializeSingleTest(...) => return [metadata];

The source-gen path returns List<global::TUnit.Core.TestMetadata>. Both satisfy IReadOnlyList<TestMetadata>, but it's an inconsistency: arrays are slightly cheaper for single-element cases (no internal buffer), and aligning the source-gen path to also emit return [metadata]; for non-parameterised tests would be cleaner. Not a correctness issue, but the inconsistency could confuse future maintainers.

6. List Pre-allocation Still Missing for Known-Count Sources

For parameterised tests where the count is statically determinable at code-gen time (e.g. [Arguments(1, 2, 3)] — 3 known rows), the generator could emit new List<...>(3) with an initial capacity, avoiding internal resizes. For dynamic sources this isn't possible, but for static [Arguments] attributes it is. This is a follow-up opportunity, not a blocker.

7. DiscoverTestsInAssemblyStreaming Still Async — Consistent?

DiscoverTestsInAssemblyStreaming in ReflectionTestDataCollector is still an async IAsyncEnumerable. The calls inside it to BuildTestMetadata are now synchronous, but the method's async iterator machinery still generates a state machine. If the goal is to eliminate state machine overhead for discovery, this method is an incomplete conversion. However, since it's in the streaming (non-AOT) path, it may be less critical.

---

Summary

Area	Status
Core performance improvement	Excellent — proven by data
static lambda on InstanceFactory	Great bonus fix
Breaking API callout	Needs changelog/version note
Cancellation regression	Still unaddressed
Parallel collection dropped	Still unaddressed — measure reflection impact
Source-gen vs reflection return type inconsistency	Minor, follow-up
Single-test List vs array allocation	Minor, follow-up

The change is fundamentally correct and the performance wins are compelling. The two actionable items before merge are: (1) re-add cancellation checks in the materialization loops, and (2) explicitly document the breaking API change.

[claude]

Code Review: perf: make test metadata creation fully synchronous

This is a well-motivated, high-impact performance optimization. The benchmark numbers are compelling — 62% faster startup, 99.4% fewer async state machine JITs, and 33% smaller DLL size for large source-generated test suites. The approach correctly identifies that async IAsyncEnumerable was the wrong abstraction for inherently synchronous, compile-time-known metadata.

---

Positives

• Root cause diagnosis is correct: Each async iterator method generates a unique state machine type with 6 JIT-compiled interface methods. Eliminating ~60,000 async state machine JITs is a genuine win, not just a micro-optimization.
• static lambdas on InstanceFactory: A nice secondary optimization — avoids hidden closure allocations on every test source class.
• Consistent transformation: The change is thorough across both the source-gen and reflection code paths, which is required by the dual-mode rule.
• Snapshot tests updated: All 456 verified snapshots are committed correctly.

---

Issues and Suggestions

1. Breaking API change deserves explicit callout

ITestSource.GetTestsAsync() and TestDescriptor.Materializer's delegate signature are both public API. Any consumer implementing ITestSource (e.g., third-party test adapters, custom test sources) will get a compile error. This should be highlighted in the changelog/release notes as a breaking change, even if it's intentional and the right trade-off.

2. Cancellation support removed without a replacement path

Both GetTests() and Materializer no longer accept a CancellationToken. For reflection-based discovery (which scans assemblies, invokes data source methods, etc.), this means there is no way to abort long-running metadata collection during a cancellation request (e.g., user presses Ctrl+C during test discovery).

Why it matters: In the synchronous-only source-gen path this is fine since collection is instantaneous. But the reflection path (ReflectionTestDataCollector) can involve I/O, expensive expression compilation, and dynamic invocation. The removed cancellation checks in MaterializeSingleTest and MaterializeFromDescriptors are a real regression for those scenarios.

Suggested approach: Consider threading CancellationToken through the higher-level CollectTestsAsync boundary only, and checking it between top-level source items rather than inside the materializer delegate. That keeps the hot-path interface clean while preserving cancellation at the orchestration level.

3. Parallel collection removed without replacement

CollectTestsTraditional previously had a ProcessInParallel() path for large test source sets:

// Before (removed):
if (testSourcesList.Count < ParallelThresholds.MinItemsForParallel)
    // sequential
else
    return await testSourcesList
        .SelectManyAsync(testSource => testSource.GetTestsAsync(testSessionId))
        .ProcessInParallel();

This is now unconditionally sequential. For the source-gen path this is fine since each GetTests() is O(1). But for reflection-based discovery, where each source item can involve expensive work, removing parallelism could be a net regression in wall time.

Suggested approach: Consider using Parallel.ForEach with a ConcurrentBag<TestMetadata> (or lock-protected List) for the reflection path when there are many sources, controlled by the existing ParallelThresholds constant.

4. Code smell: addToResultsList boolean parameter in GenerateTestMetadataInstance

private static void GenerateTestMetadataInstance(CodeWriter writer, TestMethodMetadata testMethod, string className, bool addToResultsList = true)

A boolean parameter that controls whether the method emits __results.Add(metadata) is a classic code smell — it means the method does two slightly different things depending on a flag. The non-list path (addToResultsList: false) also relies on the caller manually writing the return statement, which creates an implicit coupling that's easy to break.

Better approach: Split into two methods (GenerateTestMetadataInstanceAddToList / GenerateTestMetadataInstanceReturn) or have GenerateTestMetadataInstance always emit __results.Add(metadata) and handle the single-item path differently at the call site. Explicit is better than implicit here, especially in code generators where bugs are multiplied across every generated file.

5. Minor: needsList condition has redundant clause

var needsList = testMethod.IsGenericType || testMethod is { IsGenericMethod: true, MethodSymbol.TypeParameters.Length: > 0 };

If IsGenericMethod is true, TypeParameters.Length > 0 is always true by definition. The second clause simplifies to just testMethod.IsGenericMethod. This is a minor readability nit.

6. Minor: Index-based loop vs foreach in MaterializeFromDescriptors

for (var i = 0; i < materialized.Count; i++)
{
    yield return materialized[i];
}

Since materialized is IReadOnlyList<T>, this avoids enumerator allocation — a micro-optimization that is valid in a hot path. However, MaterializeFromDescriptors itself already allocates a state machine via yield return, which negates most of the benefit. A plain foreach would be more idiomatic. This is not a correctness issue.

---

Summary

Area	Assessment
Core optimization	✅ Excellent — eliminates the right bottleneck
Dual-mode correctness	✅ Both paths updated
Breaking change documentation	⚠️ Needs explicit callout
Cancellation support	⚠️ Regression in reflection path
Parallel collection	⚠️ Removed without equivalent for reflection path
Code generator structure	⚠️ addToResultsList bool flag is fragile

The optimization itself is sound and the impact is real. The main concerns are around the regression in the reflection path (cancellation + parallelism) and the missing documentation of the breaking API change. Worth addressing before merge, particularly the cancellation regression which may not be obvious in synthetic benchmarks but matters for real-world usage under cancellation pressure.

[claude]

Code Review: perf: make test metadata creation fully synchronous (Pass 4)

Follow-up to my previous reviews. Two items from the earlier reviews have been addressed by subsequent commits — acknowledging those before revisiting what remains.

---

Newly Addressed ✅

Static InstanceFactory lambdas (commit 08ba6e9b91a3): Good. No closure allocation per test source class.

Single-element array for non-generic test sources (commits b3f419de7582 + b76b623b276e): The generated code now correctly emits:

return new global::TUnit.Core.TestMetadata[] { metadata };

…for the common non-generic case, avoiding 10,000 List<T> allocations. Both source-gen path and snapshot tests updated consistently. Well done.

---

Still Outstanding from Previous Reviews

1. Breaking API — No Changelog/Version Callout (3rd mention)

ITestSource.GetTests() and TestDescriptor.Materializer's delegate type are both public API surface. These have been changed without any accompanying entry in CHANGELOG or release notes. This is still not addressed after three review cycles. Any external adapter implementing ITestSource will get a compile error at upgrade time. At minimum, add a breaking change entry to the changelog before merging.

2. Cancellation Token Gap — Still Unaddressed (3rd mention)

CollectTestsAsync still accepts a CancellationToken in its signature, but that token is now passed to nothing:

public async Task<IEnumerable<TestMetadata>> CollectTestsAsync(string testSessionId, ..., CancellationToken cancellationToken)
{
    standardTestMetadatas = CollectTestsWithTwoPhaseDiscovery(Sources.TestSources, testSessionId, filterHints); // CT dropped
    // or
    standardTestMetadatas = CollectTestsTraditional(testSourcesList, testSessionId); // CT dropped
}

The old MaterializeFromDescriptors had cancellationToken.ThrowIfCancellationRequested() in the loop — that check is now gone too. A Ctrl+C during startup of a 10,000-test suite will block until all synchronous metadata collection completes.

Minimal fix: Add periodic CT checks at the outer loop boundary in CollectTestsWithTwoPhaseDiscovery and CollectTestsTraditional:

foreach (var descriptor in descriptorsToMaterialize)
{
    cancellationToken.ThrowIfCancellationRequested(); // re-add
    var materialized = descriptor.Materializer(testSessionId);
    ...
}

The synchronous path doesn't need CT all the way down — just checking between top-level items is sufficient and keeps the ITestSource interface clean.

3. Parallel Collection Dropped — Reflection Path (3rd mention)

CollectTestsTraditionalAsync previously had a ProcessInParallel() path for large source sets (Count >= MinItemsForParallel). This is now unconditionally sequential. The performance benchmarks shown are for the source-gen path only. The reflection path can involve assembly scanning, expression compilation, and dynamic invocation. It's possible there's a net regression in wall time for reflection-heavy suites. This should be measured before closing.

---

New Observations from Commits 3 & 4

4. addToResultsList Boolean Parameter is a Code Smell (Introduced in Commit 3)

The optimization in commit b3f419de7582 introduced this signature:

private static void GenerateTestMetadataInstance(CodeWriter writer, TestMethodMetadata testMethod, string className, bool addToResultsList = true)

And the call site:

GenerateTestMetadataInstance(writer, testMethod, className, addToResultsList: false);
writer.AppendLine("return new global::TUnit.Core.TestMetadata[] { metadata };");

A boolean flag that controls whether the method emits __results.Add(metadata) is a fragile design — it means there's an implicit contract between the caller and the method: when addToResultsList: false, the caller must manually write the return statement, or the generated code will be incomplete. This is the kind of coupling that causes subtle bugs when the method is called from a new path in the future.

Better approach: Always emit __results.Add(metadata) inside GenerateTestMetadataInstance, and handle the single-item path by initializing __results at the call site and returning it. Or simply have two clearly named private methods. The goal is to keep the contract explicit and self-contained.

5. needsList Redundant Clause (Minor)

var needsList = testMethod.IsGenericType || testMethod is { IsGenericMethod: true, MethodSymbol.TypeParameters.Length: > 0 };

The pattern { IsGenericMethod: true, MethodSymbol.TypeParameters.Length: > 0 } is redundant — a method for which IsGenericMethod is true by definition has at least one type parameter. Simplify to:

var needsList = testMethod.IsGenericType || testMethod.IsGenericMethod;

---

Summary

Area	Status
Core optimization	✅ Proven, correct
Static lambdas	✅ Newly addressed
Single-element array for non-generic tests	✅ Newly addressed
Breaking API changelog	❌ Still not documented (3rd cycle)
Cancellation token regression	❌ Still unaddressed (3rd cycle)
Parallel collection dropped	❌ Still unaddressed (3rd cycle)
addToResultsList boolean flag	⚠️ New issue introduced in commit 3
needsList redundant clause	⚠️ Minor — easy one-line fix

The core change is correct and the incremental commits are moving in the right direction. The two items that need resolution before merge are: (1) re-adding cancellation checks in the materialization loops (a 3-line fix), and (2) documenting the breaking API change in the changelog. The addToResultsList flag should also be refactored — it introduces fragile implicit coupling into the code generator.

[claude]

Code Review: perf: make test metadata creation fully synchronous (Pass 5)

Follow-up to my previous review. Checking what the latest commit (a7c0083976 — public API snapshots) addresses, and revisiting what remains.

---

Newly Addressed ✅

Public API snapshots updated (commit a7c0083976): The TUnit.PublicAPI verified files now reflect:

• ITestSource.GetTests(string testSessionId) (was GetTestsAsync)
• TestDescriptor.Materializer as Func<string, IReadOnlyList<TestMetadata>> (was Func<string, CancellationToken, IAsyncEnumerable<TestMetadata>>)

This is TUnit's first-party mechanism for tracking public API surface changes, so the breaking change is now formally captured in the snapshot test suite. This partially satisfies my earlier concern about documentation — the project's own API diffing tests will flag this to any contributor who encounters it. No CHANGELOG file exists in the repo, so this snapshot approach appears to be the project convention. Accepting this as addressed.

---

Still Outstanding (Pass 5 — 4th cycle for some)

1. Cancellation Token Gap — 4th Mention ❌

CollectTestsAsync still accepts a CancellationToken and never acts on it:

// AotTestDataCollector.cs
public async Task<IEnumerable<TestMetadata>> CollectTestsAsync(string testSessionId, ..., CancellationToken cancellationToken)
{
    standardTestMetadatas = CollectTestsWithTwoPhaseDiscovery(Sources.TestSources, testSessionId, filterHints); // CT unused
    // or
    standardTestMetadatas = CollectTestsTraditional(testSourcesList, testSessionId); // CT unused
}

The old MaterializeFromDescriptors explicitly called cancellationToken.ThrowIfCancellationRequested() in the inner loop — that check is now gone. For a 10,000-test suite, Ctrl+C during startup will now block until all synchronous metadata collection finishes.

Minimum fix (3 lines): Pass cancellationToken to the inner loops in both CollectTestsWithTwoPhaseDiscovery and CollectTestsTraditional and add cancellationToken.ThrowIfCancellationRequested() at the top of each iteration. There is no need to thread it all the way into ITestSource.GetTests().

2. Parallel Collection Dropped — 4th Mention ❌

CollectTestsTraditional is now unconditionally sequential. The removed parallel path (ProcessInParallel) previously activated above ParallelThresholds.MinItemsForParallel. For the source-gen path this is fine (each GetTests() is O(1)). The reflection path can involve assembly scanning and expression compilation. A regression for reflection-heavy suites is plausible and has not been measured.

Suggested approach: Use Parallel.ForEach with a lock-protected list or ConcurrentBag for the reflection path when testSourcesList.Count >= ParallelThresholds.MinItemsForParallel. This avoids reintroducing Task-based overhead while preserving the parallelism benefit.

3. addToResultsList Boolean Parameter — 2nd Mention ⚠️

// TestMetadataGenerator.cs
private static void GenerateTestMetadataInstance(
    CodeWriter writer, TestMethodMetadata testMethod, string className,
    bool addToResultsList = true)  // ← fragile control-flow flag

When called with addToResultsList: false, the method doesn't emit __results.Add(metadata), but the caller must then manually write the return statement — an implicit contract that's easy to violate when adding new call sites. The boolean flag is a code smell specifically because it inverts a generator method's side effect based on a hidden assumption about the call context.

Better: Two clearly named private methods, or always emit __results.Add(metadata) and let the single-item path share the same list (the allocation cost is identical to today's single-element array).

4. needsList Redundant Clause — 2nd Mention ⚠️

// Before (current):
var needsList = testMethod.IsGenericType || testMethod is { IsGenericMethod: true, MethodSymbol.TypeParameters.Length: > 0 };
// After (simplified):
var needsList = testMethod.IsGenericType || testMethod.IsGenericMethod;

A method with IsGenericMethod == true by definition has TypeParameters.Length > 0. The additional check is dead code. One-line fix.

---

Summary

Area	Status
Core sync optimization	✅ Proven, correct
Static InstanceFactory lambdas	✅ Addressed
Single-element array for non-generic tests	✅ Addressed
Public API snapshots updated	✅ Newly addressed (this pass)
Cancellation token regression	❌ Still outstanding (4th cycle)
Parallel collection dropped	❌ Still outstanding (4th cycle)
addToResultsList boolean flag	⚠️ Still outstanding (2nd cycle)
needsList redundant clause	⚠️ Still outstanding (2nd cycle)

The most important remaining item before merge is the cancellation token gap — it's a 3-line fix and directly restores a safety net that was present before this PR. The addToResultsList boolean refactor is a code quality issue in the generator that will become fragile as the generator grows.