perf: self-contained TestEntry<T> with consolidated switch invokers eliminates per-test JIT

[thomhurst]

Summary

Source generation mode was slower than reflection for large test suites due to O(N) JIT compilations at startup (~16K methods for 91K tests). This PR introduces TestEntry<T> — a self-contained per-test registration object with class-level shared switch delegates — reducing generated methods from O(N) per class to O(1).

Design

Each test is a TestEntry<T> with everything pre-built by the source generator:

TestEntry<T>
├── Data (zero JIT, for filtering)
│   ├── MethodName, FullyQualifiedName, FilePath, LineNumber
│   ├── Categories[], CustomProperties[], DependsOn[]
│   ├── HasDataSource, RepeatCount
│   └── Dependencies[] (pre-built TestDependency objects)
├── Pre-built objects (shared per class, constructed in .cctor)
│   └── MethodMetadata
├── Class-level shared delegates (4 methods total per class)
│   ├── InvokeBody — switch dispatches by MethodIndex (1 per class)
│   ├── CreateAttributes — switch dispatches by AttributeGroupIndex (1 per class)
│   └── CreateInstance — shared factory (1 per class)
├── Pre-separated data sources (no runtime scanning)
│   ├── TestDataSources[] (method-level IDataSourceAttribute)
│   └── ClassDataSources[] (class-level IDataSourceAttribute)
└── Properties[] (InjectableProperty descriptors for injection)

Key: all entries in a class share the same 3 delegate references. Per-test differentiation is via MethodIndex and AttributeGroupIndex indices. Zero per-test methods generated.

What was eliminated

Component	Before	After
Methods per class	O(N) — GetTests, EnumerateDescriptors, N×InvokeTest, N×Materialize, M×CreateAttributes, CreateInstance	4 — __Invoke switch, __Attributes switch, __CreateInstance, __InitMethodMetadatas
Runtime data source scanning	is IDataSourceAttribute loop per test	0 — pre-separated by source gen
Runtime class data source reflection	typeof(T).GetCustomAttributes() per test	0 — pre-built by source gen
Runtime dependency extraction	DependsOnAttribute scanning per test	0 — pre-built TestDependency[]
TestMetadata construction	Source generator Materialize() switch	Engine copies fields from TestEntry

Results (91,452-test benchmark)

Metric	Before	After	Change
Assembly size	44.6 MB	27.1 MB	-39%
Generated methods per class	~16 (for 6 test methods)	4	-75%
Runtime reflection	per-test	0	eliminated
All tests pass	✓	✓	zero regressions

New types

• TestEntry<T> — self-contained per-test registration unit with shared delegates
• TestEntrySource<T> / ITestEntrySource — thin non-generic wrapper for engine access
• InjectableProperty — describes properties with data source attributes
• TestEntryFilterData — lightweight struct for pure-data filtering

Test plan

• BasicTests pass (3/3)
• DataDrivenTests pass (15/15)
• Full benchmark pass (91,452/91,452)
• Snapshot tests need updating (.verified.txt files)
• Full CI suite
• AOT publish verification

[claude]

Code Review: perf: self-contained TestEntry with consolidated switch invokers

This is a well-designed, ambitious performance optimization. The design document in the PR body is excellent and the benchmarks are compelling. The core idea — replacing O(N) per-test JIT methods with a 4-method-per-class design using index-based dispatch — is architecturally sound. Below are observations ranging from blocking concerns to suggestions.

---

Blocking: Snapshot tests not updated

The PR explicitly marks "Snapshot tests need updating (.verified.txt files)" as unchecked. Per the project's CLAUDE.md rules, this is a hard requirement before merging:

Changes to source generator output or public APIs require running snapshot tests. Commit .verified.txt files. NEVER commit .received.txt.

The source generator output has fundamentally changed (from GetTests()/EnumerateTestDescriptors() to Entries[]), so every snapshot will be different. These must be updated and committed before this can land.

---

Potential Bug: __Attributes referenced unconditionally, generated conditionally

In TestMetadataGenerator.cs, the __Attributes switch method is only emitted when classGroup.AttributeGroups.Length > 0:

if (classGroup.AttributeGroups.Length > 0)
{
    writer.AppendLine("private static global::System.Attribute[] __Attributes(int groupIndex)");
    ...
}

But every TestEntry<T> initializer unconditionally writes:

writer.AppendLine($"CreateAttributes = __Attributes,");

If a test class somehow has zero attribute groups, this would generate code that references an undefined method — a compile error in the generated source. In practice, every test probably has at least [Test], making this always non-zero. But the guard should either be removed (generate a throwing stub unconditionally) or the entry writer should handle the null case. A defensive stub would be safer:

// Always emit, even if no groups
writer.AppendLine("private static global::System.Attribute[] __Attributes(int groupIndex)");
writer.AppendLine("    => throw new global::System.ArgumentOutOfRangeException(nameof(groupIndex));");

---

Suspicious: PropertyInjectionData.ValueFactory = static () => null

In TestEntry<T>.BuildPropertyInjections():

result[i] = new PropertyInjectionData
{
    PropertyName = prop.Name,
    PropertyType = prop.Type,
    Setter = prop.SetValue,
    ValueFactory = static () => null,  // ← always null?
};

ValueFactory always returns null — this would mean injected properties never receive a value from the factory. If ValueFactory is supposed to invoke the DataSource to produce the actual value, this is a bug that would silently produce null-injected properties. Either:

• ValueFactory is populated elsewhere by the engine (in which case, a comment explaining this is needed), or
• This should be () => prop.DataSource.GetValue(...) or similar.

The pre-existing BuildPropertyDataSources() does correctly expose DataSource, so the engine may use that separately — but the relationship between PropertyDataSources and PropertyInjections in TestMetadata<T> should be clarified.

---

Architectural Concern: Dual-path coupling via internal on TestMetadata<T>

The PR adds four internal properties to TestMetadata<T>:

internal Func<T, int, object?[], CancellationToken, ValueTask>? ClassInvoker { get; init; }
internal int InvokeMethodIndex { get; init; } = -1;
internal Func<int, Attribute[]>? ClassAttributeFactory { get; init; }
internal int AttributeGroupIndex { get; init; } = -1;

These exist solely to support the new code path, but TestMetadata<T> is a public class (used by the ITestSource path). This creates a hidden coupling where the class secretly supports two different execution models selected by null-checking at runtime. Over time, this makes TestMetadata<T> harder to reason about.

A cleaner approach would be to have TestEntrySource<T>.Materialize() produce a subclass or a dedicated type that carries this dispatch info, rather than polluting the base class. Alternatively, the ToTestMetadata() on TestEntry<T> could construct InvokeTypedTest by closing over the index — keeping the existing single-invoker contract:

InvokeTypedTest = (instance, args, ct) => InvokeBody(instance, MethodIndex, args, ct),

This would eliminate the 4 internal properties entirely while keeping the consolidated switch benefit.

---

Minor: Sequential filtering could bottleneck large suites

CollectTestsFromTestEntries Phase 1 (filter scan) is sequential:

foreach (var kvp in Sources.TestEntries)
{
    for (var i = 0; i < source.Count; i++)
    {
        var filterData = source.GetFilterData(i);
        ...
    }
}

For 91K tests, this is fine today. But the existing CollectTests path already uses Parallel.For batching (referenced in the merge commit message). Consider using Parallel.ForEach on the outer dictionary enumeration for consistency with the other path, especially since GetFilterData is a pure struct copy with no side effects.

---

Minor: GetFilterData allocates on every call during filtering

TestEntrySource<T>.GetFilterData(int index) copies 8 fields from TestEntry<T> into a new TestEntryFilterData struct per call. For the 91K-test filtering scan, this is 91K struct constructions. Since TestEntry<T> already has all these fields, consider either:

1. Making TestEntry<T> directly implement a filtering interface (IFilterable), or
2. Caching FilterData on TestEntry<T> as a lazily built field.

This is a minor point — struct copies on the stack are cheap — but given the stated goal of minimizing JIT and allocation pressure, it's worth tracking.

---

Minor: GetAllAttributes vs GetTestAttributes comment mismatch

The new PreGenerateAttributeFactoryBody (for the old ITestSource path) was given a comment saying it returns "ALL attributes including data source attributes" — but this modifies the behavior of the old method from what it was before. It's worth confirming this doesn't break the existing ITestSource-registered tests by including data sources in the attribute array that the engine then double-counts (once from the array, once from whatever data source scanning the old path still does).

---

Overall Assessment

The design is excellent and the performance numbers justify the complexity. The main blockers are:

1. Snapshot tests must be updated
2. The __Attributes conditional generation vs unconditional reference should be reconciled
3. The ValueFactory = static () => null in property injection needs clarification or fixing

The internal dual-path coupling is a design debt item worth addressing before this becomes established — it'll be much easier to clean up now than after other code starts depending on the shape of TestMetadata<T>.

[claude]

Follow-up Review: perf: self-contained TestEntry with consolidated switch invokers

The last commit (1009d2b2 — "perf: convert generic tests from ITestSource to TestEntry") extends the TestEntry<T> pattern to cover generic and inherited tests, completing the optimization story. This is a substantial addition that merits dedicated review.

---

Status of Previous Review Issues

Still Blocking:

1. Snapshot tests still unchecked — The PR checklist still shows [ ] Snapshot tests need updating. The source generator output has changed fundamentally again (generic tests now emit TestEntry<T>[] instead of ITestSource), so all related snapshots must be updated. Per CLAUDE.md this is a hard requirement.

2. ValueFactory = static () => null still unresolved — TestEntry<T>.BuildPropertyInjections() still sets ValueFactory = static () => null. The previous review flagged this as a potential silent null-injection bug. No change or clarifying comment was added. If the engine uses PropertyDataSources exclusively for injection and PropertyInjections.ValueFactory is truly unused/overwritten, a comment is needed. If not, this is a bug.

3. internal dual-path coupling still present — TestMetadata<T> still exposes ClassInvoker, InvokeMethodIndex, ClassAttributeFactory, and AttributeGroupIndex as internal properties solely to support the new path. The suggestion from the prior review to close over the index in ToTestMetadata() instead would eliminate all four:

// In TestEntry<T>.ToTestMetadata():
InvokeTypedTest = (instance, args, ct) => InvokeBody(instance, MethodIndex, args, ct),
AttributeFactory = () => CreateAttributes(AttributeGroupIndex),
// Remove ClassInvoker, InvokeMethodIndex, ClassAttributeFactory, AttributeGroupIndex from TestMetadata<T>

This keeps TestMetadata<T>'s contract clean without any architectural change.

4. __Attributes conditionally generated, unconditionally referenced (per-class path) — Still present in the existing per-class code path:

// Emitting the entry:
writer.AppendLine($"CreateAttributes = __Attributes,");  // always emitted

// But the method is only generated when:
if (classGroup.AttributeGroups.Length > 0)
{
    writer.AppendLine("private static global::System.Attribute[] __Attributes(int groupIndex)");
    ...
}

The generic path (new code) is fine — it uses __Attributes_{groupIndex} which is always emitted per group. But the original per-class path still has this mismatch.

---

New Issues in the Generic Test Conversion

5. Duplicate processing in CollectConcreteInstantiations — The method processes class-level ArgumentsAttribute twice:

• First in the early if (testMethod is { IsGenericType: true, IsGenericMethod: true }) / else block, which calls TryAddInstantiation for classArgumentsAttributes
• Then again later in the explicit "Process class-level Arguments attributes for generic classes" section at the bottom

The processedTypeCombinations HashSet prevents actual duplicate test entries, but the double-iteration adds cognitive overhead and signals that the method's control flow may be unintentionally overlapping. This method is already ~150 lines of branching logic — if the HashSet guard is the only protection against duplicates, any future change that misses one of the branches silently produces wrong results rather than a compile error.

A cleaner approach would be to define non-overlapping, mutually exclusive collection phases and document exactly what each handles.

6. EmitConcreteInstanceCreation throws for ClassConstructorAttribute — When the test class has [ClassConstructor], the generated code unconditionally throws:

if (InstanceFactoryGenerator.HasClassConstructorAttribute(testMethod.TypeSymbol))
{
    writer.AppendLine("throw new global::System.NotSupportedException(\"Instance creation for classes with ClassConstructor attribute is handled at runtime\");");
}

This means any generic test class decorated with [ClassConstructor] will silently compile but throw at runtime when instantiation is attempted. This is a regression from the legacy path which handled ClassConstructor correctly. Either:

• Skip these tests in CollectConcreteInstantiations and fall back to the legacy ITestSource path (preferable — keeps behavior correct), or
• Emit a source generator error (context.ReportDiagnostic(...)) so the failure is surfaced at build time rather than at test runtime.

7. Missing helper method visibility in diff — Several methods called in CollectConcreteInstantiations are not visible in this diff:

• InferClassTypesFromMethodDataSource
• InferTypesFromTypedDataSourceForClass
• InferClassTypesFromMethodArguments
• ValidateClassTypeConstraints
• BuildTypeKey

These presumably exist in the existing TestMetadataGenerator.cs, but since the file is 800+ lines of additions, it's worth verifying these methods exist and are not dead stubs that always return null (which would silently skip test registrations for generic classes with these attribute combinations).

8. Registration index mismatch in generic path — In GenerateTestMetadata, the outer class is closed before the registration calls:

writer.Unindent();
writer.AppendLine("}");  // closes the class

// Registration calls
registrationIndex = 0;
foreach (var group in groupedByConcreteClass)
{
    var concreteClassName = group.Key;
    EmitRegistrationField(writer, $"{uniqueClassName}_{registrationIndex}",
        $"global::TUnit.Core.SourceRegistrar.RegisterEntries({uniqueClassName}.Entries_{registrationIndex})");
    registrationIndex++;
}

The registration fields appear to be emitted outside the class body, which would not compile unless EmitRegistrationField emits a top-level field or there is a wrapping class higher up the call chain. This is worth verifying — if EmitRegistrationField emits a module-level or outer-class field then this is fine, but if it expects to be inside the class body, there's a structural issue.

---

What's Good in the New Commit

• Extending TestEntry<T> to generic tests completes the optimization end-to-end rather than leaving a two-tier system indefinitely
• The cartesian product approach for class × method type argument combinations is correct and clean
• processedTypeCombinations HashSet correctly prevents duplicate registrations even when multiple attribute processing paths could produce the same type combination
• The GenerateTestMetadataLegacyFallback escape hatch when no concrete types can be resolved is a good defensive measure

---

Priority

Before merging:

1. Update snapshot tests (hard requirement)
2. Clarify or fix ValueFactory = static () => null
3. Fix [ClassConstructor] + generic test combination (silent runtime throw)
4. Verify registration fields are emitted inside a valid scope

Design debt (can follow up):
5. Eliminate the 4 internal properties from TestMetadata<T> via delegate closure in ToTestMetadata()
6. Refactor CollectConcreteInstantiations to eliminate overlapping processing paths

[claude]

Follow-up Review: Final Commit — "refactor: remove ITestSource collection path from engine"

This review covers commit 2ee3b19 which removes the legacy ITestSource collection path from the engine. The refactor is clean and the engine simplification is real (-452 lines across 4 files). However, it introduces a new blocking issue and leaves all four previously flagged items unresolved.

---

New Blocking Issue: GenerateTestMetadataLegacyFallback now generates unreachable tests

The source generator still emits ITestSource classes for the fallback path (when no concrete type instantiations can be resolved for a generic test):

// TestMetadataGenerator.cs — still present in this PR
private static void GenerateTestMetadataLegacyFallback(...)
{
    writer.AppendLine($"internal sealed class {uniqueClassName} : global::TUnit.Core.Interfaces.SourceGenerator.ITestSource, ...");
    ...
}

These classes register into Sources.TestSources. But AotTestDataCollector.CollectTestsAsync now only processes Sources.TestEntries:

if (!Sources.TestEntries.IsEmpty)
{
    allResults.AddRange(CollectTestsFromTestEntries(testSessionId, filterHints));
}
// Sources.TestSources is never checked — legacy fallback tests are silently dropped

Any generic test class where CollectConcreteInstantiations returns empty will hit the fallback, compile successfully, register into TestSources, and then silently not run. The benchmark suite passes because none of its generic tests hit the fallback — but user tests with unusual generic patterns could silently disappear.

The fix is either:

1. Keep minimal Sources.TestSources processing for the fallback case (a few lines), or
2. Remove GenerateTestMetadataLegacyFallback entirely and emit a source generator error diagnostic instead, which is the safer approach:

context.ReportDiagnostic(Diagnostic.Create(
    new DiagnosticDescriptor("TUNIT001", "Cannot resolve generic test", 
        "Could not resolve concrete type arguments for {0} — test will not run",
        "TUnit", DiagnosticSeverity.Error, true),
    testMethod.MethodSymbol.Locations.First(),
    testMethod.MethodSymbol.Name));
return; // emit nothing, no silent fallback

This surfaces the problem at build time rather than silently dropping tests at runtime.

---

Unresolved from Previous Reviews

1. Snapshot tests still unchecked — PR checklist still shows [ ] Snapshot tests need updating. Per CLAUDE.md this is a hard requirement before merging. The source generator output has changed fundamentally across multiple commits — all related snapshots must be regenerated and committed.

2. ValueFactory = static () => null still unresolved — TestEntry<T>.BuildPropertyInjections() still sets ValueFactory = static () => null. Three reviews have flagged this. It either needs a clarifying comment explaining why the engine overwrites this value, or it needs to be corrected to invoke the data source.

3. internal dual-path coupling still present — TestMetadata<T> still carries four internal properties (ClassInvoker, InvokeMethodIndex, ClassAttributeFactory, AttributeGroupIndex) used exclusively by the new path. The suggested fix from the previous two reviews is still the cleanest resolution — close over the index in ToTestMetadata():

InvokeTypedTest = (instance, args, ct) => InvokeBody(instance, MethodIndex, args, ct),
AttributeFactory = () => CreateAttributes(AttributeGroupIndex),

This eliminates all four internal properties with no behavioral change.

4. __Attributes conditionally generated but unconditionally referenced (per-class path) — The per-class code path still generates CreateAttributes = __Attributes, unconditionally but only emits the __Attributes method when classGroup.AttributeGroups.Length > 0. In practice every test has at least [Test], so this may never fire — but it is an invariant violation that should be guarded defensively.

---

Minor Concern in New Code: Dependency BFS re-scans all entries

CollectTestsFromTestEntries Phase 2 (dependency expansion) builds the byClassAndMethod lookup by re-scanning all entries across Sources.TestEntries:

foreach (var kvp in Sources.TestEntries)
{
    var source = kvp.Value;
    for (var i = 0; i < source.Count; i++)
    {
        var fd = source.GetFilterData(i);
        byClassAndMethod[(fd.ClassName, fd.MethodName)] = (source, i);
        // ...
    }
}

The old two-phase code built this index during Phase 1 (single pass). The new code does Phase 1 (filter), then if hasDependencies, does a full second scan to build the lookup before the BFS. For the 91K benchmark this is 91K extra GetFilterData calls in the dependency case. Consider building the byClassAndMethod index lazily during Phase 1 (unconditionally) so Phase 2 can use it without re-scanning, matching the previous approach's efficiency.

---

What's Good in This Commit

• Removing CollectTestsWithTwoPhaseDiscovery, CollectTests, EnumerateDescriptors, and MaterializeFromDescriptors from both AotTestDataCollector and ReflectionTestDataCollector is the right call — these were added as part of this PR's own earlier iterations and are cleanly superseded.
• The new CollectTestsFromTestEntries is significantly more readable than the two-phase method it replaces.
• The ITestDataCollector interface simplification (down to just CollectTestsAsync) is correct.

---

Priority Summary

Issue	Severity
Legacy fallback tests silently dropped	Blocking
Snapshot tests not updated	Blocking
ValueFactory = null	Blocking (or needs comment)
[ClassConstructor] on generic classes throws at runtime	Blocking
internal dual-path coupling	Design debt
__Attributes guard missing	Low risk (defensive fix)
Dependency BFS double-scan	Performance concern

[claude]

Code Review: perf: self-contained TestEntry with consolidated switch invokers (follow-up)

This is a follow-up to the previous review. Checking which prior concerns have been addressed and adding new observations from a closer read of the diff.

---

Prior Issues — Status

1. Snapshot tests not updated — still unresolved (explicitly unchecked in PR description). Per project rules, .verified.txt files must be committed before merge.

2. __Attributes referenced unconditionally, generated conditionally — still present. The switch method is only emitted when classGroup.AttributeGroups.Length > 0, but every TestEntry<T> initializer unconditionally emits CreateAttributes = __Attributes,. A class with zero attribute groups would produce a compile error in generated code. A defensive always-emitted stub (even just => throw new ArgumentOutOfRangeException(...)) would close this gap.

3. ValueFactory = static () => null — still present in TestEntry<T>.BuildPropertyInjections(). If ValueFactory is supposed to produce injection values, this will silently inject null into all properties. Needs either a comment explaining the engine populates it separately, or the correct factory.

4. internal properties on TestMetadata<T> — ClassInvoker, InvokeMethodIndex, ClassAttributeFactory, AttributeGroupIndex are still added as internal nullable sentinel-checked fields. The simpler fix (closing over the index in ToTestMetadata to produce a standard InvokeTypedTest) would avoid this coupling entirely:

InvokeTypedTest = (instance, args, ct) => InvokeBody(instance, MethodIndex, args, ct),

---

New Finding: Silent data loss — legacy fallback tests dropped

AotTestDataCollector.CollectTestsAsync now only reads from Sources.TestEntries:

if (!Sources.TestEntries.IsEmpty)
{
    allResults.AddRange(CollectTestsFromTestEntries(testSessionId, filterHints));
}
// Dynamic tests only after this — Sources.TestSources is never read

The old CollectTests/CollectTestsWithTwoPhaseDiscovery paths (which read Sources.TestSources) are completely removed. But GenerateTestMetadataLegacyFallback still exists and still emits a GenerateTestRegistrationField call that invokes SourceRegistrar.RegisterReturn — which writes to Sources.TestSources, not Sources.TestEntries.

Result: any generic test that falls into the legacy fallback path (no concrete types resolvable at source-gen time) registers to Sources.TestSources but is never read, so it silently vanishes from the test run. Users would see those tests disappear with no error.

Fix options:

• Add a Sources.TestSources drain back into allResults after the TestEntries block, or
• Make GenerateTestMetadataLegacyFallback emit RegisterEntries instead of RegisterReturn (requires a non-generic ITestEntrySource wrapper).

---

New Finding: FilePath double-escaping bug

In both EmitTestEntryFields and PreGenerateTestEntryDataFields:

writer.AppendLine($"FilePath = @\"{(testMethod.FilePath ?? "").Replace("\\", "\\\\")}\",");

This emits a verbatim string literal (@"...") in the generated code, but also escapes backslashes as if for a regular string. In a C# verbatim string, \ is already literal — no escaping needed. The Replace("\\", "\\\\") turns every \ into \\, so on Windows a path like C:\tests\Foo.cs would be emitted as:

FilePath = @"C:\\tests\\Foo.cs",  // wrong: C:\\tests\\Foo.cs

Either remove the .Replace(...) (verbatim string handles it), or switch to a regular string with proper escaping. This is a Windows-only correctness regression.

---

New Finding: Unbounded spin loop in RegisterEntries

while (true)
{
    if (Sources.TestEntries.TryGetValue(key, out var existing))
    {
        if (existing is TestEntrySource<T> existingSource)
        {
            existingSource.AddEntries(entries);
            return 0;
        }
    }

    if (Sources.TestEntries.TryAdd(key, new TestEntrySource<T>(entries)))
        return 0;
    // retry
}

If existing is not a TestEntrySource<T> (type mismatch), the loop never exits — it will spin forever. The comment says "Another thread added it concurrently — retry to merge", but a type mismatch is not a concurrency issue, it's a programmer error. The is check failure should throw rather than loop. Also, since this runs from .cctor-triggered static field initializers (which the CLR serializes per-type), true concurrency between the check and TryAdd for the same T is unlikely — the while(true) adds complexity for a case that cannot occur in practice for identical T.

---

Minor: Materialize-always vs. lazy under filter

CollectTestsFromTestEntries builds the full matching list (with capacity = total entry count) before any materialization. For the common case of filtered runs, this could over-allocate. Using new List<>(capacity: 64) or a more conservative starting size would reduce memory pressure for large suites with narrow filters. Low priority.

---

Summary

Issue	Severity	Status
Snapshot tests	Blocking	Not resolved
__Attributes compile error	High	Not resolved
Sources.TestSources data loss	High	New
FilePath double-escaping	Medium	New
ValueFactory = null	Medium	Not resolved
Unbounded spin loop	Low	New
internal properties on TestMetadata<T>	Low	Not resolved

The performance design is excellent, but the data-loss regression and compile-error risk should be addressed before landing.