Conversation
…on a class with tuples (#5118) Two bugs caused class-level MethodDataSource<T> with tuple data to fail: 1. Source generator didn't extract T from generic attribute type arguments, so it couldn't find the data source method and skipped Factory generation. 2. Runtime reflection path used test method parameters instead of constructor parameters for tuple unwrapping, causing truncation of tuple elements.
There was a problem hiding this comment.
Code Review
Overall this is a solid, well-targeted bug fix with clear documentation of the root cause. The dual-mode fix (source generator + runtime reflection) is exactly right. A few points worth addressing:
Issue 1: Dead code — GetParameterTypes is now unreachable
After the change in MethodDataSourceAttribute.cs, GetParameterTypes(ParameterMetadata[]?) is never called. It should be removed to avoid confusion about which helper is canonical.
- private static Type[]? GetParameterTypes(ParameterMetadata[]? parameters)
- {
- if (parameters == null || parameters.Length == 0) return null;
- var types = new Type[parameters.Length];
- for (var i = 0; i < parameters.Length; i++)
- types[i] = parameters[i].Type;
- return types;
- }Leaving it around invites future contributors to accidentally reach for the wrong helper again — which is essentially what caused the original bug.
Issue 2: Missing snapshot for the new test class
The diff includes TUnit.TestProject/Bugs/5118/AsyncClassMethodDataSourceTests.cs, which uses [MethodDataSource<AsyncTestDataSources>(...)] at the class level — the exact pattern the source generator fix targets. However, no new or updated .verified.txt snapshot file appears in the diff.
Given the CLAUDE.md rule:
Changes to source generator output or public APIs require running snapshot tests. Commit
.verified.txtfiles.
If the source generator now emits different code for MethodDataSource<T>(string) on a class-level attribute (which is the whole point of the fix), there should be a corresponding snapshot. Was the new test class intentionally excluded from snapshot coverage? If so, it's worth a comment explaining why.
Minor: Redundant condition in source generator fix
if (attr.AttributeClass is { IsGenericType: true, TypeArguments.Length: > 0 })TypeArguments.Length > 0 is always true when IsGenericType is true for a bound generic type, so the second check is redundant. It's harmless, but could be simplified to:
if (attr.AttributeClass is { IsGenericType: true })What's done well
- The root cause analysis in the PR description is clear and correct: the source generator was looking for the method on the test class instead of
T, causing a reflection fallback; and the reflection path was comparing against the wrong member list (method params instead of constructor params for class-level data sources). GetMemberTypescorrectly handles bothParameterMetadataandPropertyMetadatawith a safetypeof(object)fallback.- The test in
AsyncClassMethodDataSourceTestsdirectly reproduces the failing scenario from #5118. - The fix is appropriately minimal — only the two impacted paths are changed.
The dead code cleanup and snapshot verification are the main things I'd want resolved before merging.
- Remove dead GetParameterTypes method (replaced by GetMemberTypes) - Add snapshot test for AsyncClassMethodDataSourceTests - Simplify redundant IsGenericType check in source generator
There was a problem hiding this comment.
Code Review
This is a well-targeted, two-pronged bug fix that addresses the root cause cleanly. The PR description accurately diagrams both failure modes. Here is my analysis:
What is fixed and why it works
Source generator path (TestMetadataGenerator.cs): When MethodDataSource<T>(string) is used at the class level, the attribute's AttributeClass is the closed generic MethodDataSourceAttribute<AsyncTestDataSources>. Previously, targetType was unconditionally set to typeSymbol (the test class), so the generator looked for GetAsyncTestData on AsyncClassMethodDataSourceTests instead of AsyncTestDataSources, found nothing, and fell back to runtime reflection — which then failed with an IndexOutOfRangeException because the reflection path had the second bug.
The fix is correct: when IsGenericType is true, extract TypeArguments[0] as the target type. The generated snapshot confirms the Factory lambda now calls AsyncTestDataSources.GetAsyncTestData() directly.
Reflection path (MethodDataSourceAttribute.cs): UnwrapTupleWithTypes uses expectedTypes.Length to decide how many tuple elements to emit. Before the fix, paramTypes was derived from TestInformation.Parameters (always the test method parameters — [CancellationToken] in this case, length 1 after the CT is stripped, which is actually 0 since it was not yet stripped at that point), causing the tuple (int, string, DateTime) to be truncated or produce an IndexOutOfRangeException. Switching to MembersToGenerate gives the constructor parameters [int, string, DateTime] for a class-level data source, which is the correct length for tuple unwrapping.
Issues
1. Empty assertion body in the snapshot test
In TUnit.Core.SourceGenerator.Tests/Bugs/5118/Tests5118.cs, the async generatedFiles => lambda has no assertions:
async generatedFiles =>
{
});The snapshot verification is done by Verify (the .verified.txt file), so the snapshot test does validate the generated output. However, the empty lambda body plus the dangling }) formatting looks like a copy-paste artefact. Other similar snapshot tests (e.g., ClassTupleDataSourceDrivenTests) add explicit assertions about generated file count or specific patterns. Adding even a simple await Assert.That(generatedFiles).HasCount().EqualTo(1) makes the test self-documenting and guards against the generator silently producing zero files.
2. GetMemberTypes fallback is silently lossy
_ => typeof(object)If MembersToGenerate contains an IMemberMetadata that is neither ParameterMetadata nor PropertyMetadata (e.g., MethodMetadata or ClassMetadata, both of which implement IMemberMetadata), the fallback emits typeof(object). This will cause UnwrapTupleWithTypes to pass the wrong expected type and potentially misroute tuple elements at runtime with no diagnostic. A defensive throw or at minimum a Debug.Fail would catch any future callers passing unexpected metadata kinds:
_ => throw new InvalidOperationException(
$"Unexpected member metadata type {members[i].GetType().Name} in GetMemberTypes")Since MethodMetadata and ClassMetadata should never appear in MembersToGenerate in practice, an exception would surface bugs early rather than silently producing wrong test data.
3. ConvertToAsyncEnumerable disposes too late and can leak on exception
while (true)
{
var moveNextTask = (ValueTask<bool>)moveNextMethod!.Invoke(enumerator, null)!;
if (!await moveNextTask.ConfigureAwait(false)) break;
yield return currentProperty!.GetValue(enumerator);
}
// Dispose the enumerator
var disposeMethod = enumerator.GetType().GetMethod("DisposeAsync");
if (disposeMethod != null)
{
var disposeTask = (ValueTask)disposeMethod.Invoke(enumerator, null)!;
await disposeTask.ConfigureAwait(false);
}If an exception is thrown inside the loop (e.g., the caller throws while iterating), DisposeAsync is never called and the enumerator leaks. This is pre-existing code, not introduced in this PR, but the new async test path exercises it more. A try/finally block around the iteration would fix the leak. This is a minor point for this PR but worth noting.
4. Source generator does not handle the MethodDataSource<T>(string) case in the attrTypeName emission
When the generator emits the runtime attribute object for the Factory (lines ~905–907), it always writes:
writer.AppendLine($"new {attrTypeName}(\"{methodName}\")");This emits new global::TUnit.Core.MethodDataSourceAttribute("GetAsyncTestData") — without the <AsyncTestDataSources> type argument. That is intentional because ClassProvidingDataSource is set via the (Type, string) constructor overload in the non-generic attribute, but the snapshot shows the generic attribute is also emitted in __CreateAttributes_0:
new global::TUnit.Core.MethodDataSourceAttribute<global::TUnit.TestProject.Bugs._5118.AsyncTestDataSources>("GetAsyncTestData")The Factory-carrying attribute is the (string) overload, which at runtime calls ClassProvidingDataSource = null and falls back to reflection. Because Factory is set, reflection is never reached, so this is safe. But if someone removes the Factory property (e.g., in a future refactor), the runtime attribute would look for GetAsyncTestData on the test class rather than on AsyncTestDataSources, reintroducing the original bug silently. A comment explaining why ClassProvidingDataSource is intentionally omitted from the Factory-carrying attribute would guard against this regression.
What is done well
- The PR description is accurate and concise — it maps each symptom to its exact root cause.
GetMemberTypescorrectly coversPropertyMetadatain addition toParameterMetadata, making it more general than the helper it replaces.GetParameterTypeshas been fully removed (confirmed in the current codebase) — no dead code left behind.- The snapshot file
Tests5118.Test.verified.txtis committed, satisfying the project's snapshot-testing contract. - The test in
AsyncClassMethodDataSourceTestsdirectly reproduces the issue scenario from #5118 with multiple data rows, covering the constructor injection of(int, string, DateTime)that previously caused theIndexOutOfRangeException. - Both the source-gen and reflection modes are fixed in the same PR, keeping them in sync as required by the project's dual-mode rule.
- The fix is appropriately minimal — only the two impacted code paths are changed.
Overall this is a solid fix. The empty assertion body and the silent fallback in GetMemberTypes are the items most worth addressing before merging.
There was a problem hiding this comment.
Code Review
This is a well-targeted, two-pronged bug fix that addresses the root cause cleanly. The PR description accurately diagrams both failure modes. Here is my analysis:
What is fixed and why it works
Source generator path (TestMetadataGenerator.cs): When MethodDataSource<T>(string) is used at the class level, the attribute's AttributeClass is the closed generic MethodDataSourceAttribute<AsyncTestDataSources>. Previously targetType was unconditionally set to typeSymbol (the test class), so the generator looked for GetAsyncTestData on AsyncClassMethodDataSourceTests instead of AsyncTestDataSources, found nothing, and fell back to runtime reflection — which then failed with an IndexOutOfRangeException because the reflection path had the second bug. The fix is correct: when IsGenericType is true, extract TypeArguments[0] as the target type.
Reflection path (MethodDataSourceAttribute.cs): UnwrapTupleWithTypes uses expectedTypes.Length to decide how many tuple elements to emit. Before the fix, paramTypes was derived from TestInformation.Parameters (always the test method parameters), so for a class-level data source it saw the method's parameter list rather than the constructor's, causing the 3-element tuple to be truncated or produce an IndexOutOfRangeException. Switching to MembersToGenerate gives the constructor parameters [int, string, DateTime] for a class-level data source — the correct length for tuple unwrapping.
Issues
1. Empty assertion body in the snapshot test
In TUnit.Core.SourceGenerator.Tests/Bugs/5118/Tests5118.cs, the async generatedFiles => lambda has no assertions:
async generatedFiles =>
{
});The snapshot file Tests5118.Test.verified.txt covers the generated output, but the empty lambda also means no assertion guards against the generator producing zero files silently. Other snapshot tests add at least a file-count assertion. Something like:
async generatedFiles =>
{
await Assert.That(generatedFiles).HasCount().EqualTo(1);
}would make the test self-documenting and catch a silent generator failure.
2. Silent fallback in GetMemberTypes could mask future bugs
_ => typeof(object)If MembersToGenerate ever contains an IMemberMetadata that is neither ParameterMetadata nor PropertyMetadata (e.g. MethodMetadata or ClassMetadata, both of which implement IMemberMetadata), the wrong type silently flows into UnwrapTupleWithTypes and produces incorrect test data with no diagnostic. Since those metadata types should never appear in MembersToGenerate, a defensive throw would surface any future misuse early:
_ => throw new InvalidOperationException(
$"Unexpected member metadata type {members[i].GetType().Name} in GetMemberTypes")3. Missing explanatory comment on why ClassProvidingDataSource is absent from the Factory-carrying attribute
The generator emits the runtime attribute via the (string) overload, which leaves ClassProvidingDataSource = null. If the Factory property were ever removed in a future refactor, the attribute would fall back to looking for the data-source method on the test class rather than on T, silently reintroducing the original bug. A brief comment explaining that the Factory delegate makes explicit type routing unnecessary would protect against that regression.
What is done well
- Root cause analysis in the PR description is accurate and concise.
GetParameterTypeshas been fully removed from the file (confirmed) — no dead code left behind.GetMemberTypescorrectly addsPropertyMetadatasupport, making it more general than the helper it replaces.- The snapshot file
Tests5118.Test.verified.txtis committed, satisfying the project's snapshot-testing contract, and the generated code correctly showsAsyncTestDataSources.GetAsyncTestData()in the Factory lambda. - The test in
AsyncClassMethodDataSourceTestsdirectly reproduces the #5118 scenario with multiple data rows and constructor injection of(int, string, DateTime). - Both source-gen and reflection modes are fixed in the same PR, keeping them in sync as required by the dual-mode rule.
- The fix is appropriately minimal — only the two impacted code paths are changed, with no unrelated refactoring.
Overall a solid fix. The empty assertion body (issue 1) and the silent typeof(object) fallback (issue 2) are the items most worth addressing before merging.
- Add file-count assertion to snapshot test for 5118 - Replace silent typeof(object) fallback with defensive throw - Add comment explaining why Factory makes ClassProvidingDataSource unnecessary
There was a problem hiding this comment.
Code Review — Follow-up
All three issues raised in the previous review have been addressed:
Issue 1 — Dead code (): ✅ Resolved. The old helper has been removed and replaced entirely by .
Issue 2 — Empty assertion body in snapshot test: ✅ Resolved. Tests5118.cs now includes await Assert.That(generatedFiles).HasCount().EqualTo(1), making the test self-documenting and guarding against a silent empty-file scenario.
Issue 3 — Silent typeof(object) fallback in GetMemberTypes: ✅ Resolved. The wildcard arm now throws InvalidOperationException with the actual type name, which will surface any future misuse early rather than silently producing wrong test data.
The previously-mentioned comment about why ClassProvidingDataSource is intentionally omitted from the Factory-carrying attribute is also present in the diff.
Current state
The fix is clean and complete:
- Source generator path: correctly extracts
TypeArguments[0]fromMethodDataSource<T>so the Factory lambda callsAsyncTestDataSources.GetAsyncTestData()directly instead of falling back to reflection on the test class. - Reflection path:
MembersToGeneratecorrectly supplies constructor parameters for class-level data sources, preventing the tuple-unwrapping truncation that caused theIndexOutOfRangeException. - Snapshot:
Tests5118.Test.verified.txtis committed and shows the correct generated code. - Test:
AsyncClassMethodDataSourceTestsdirectly reproduces the #5118 scenario with a 3-element(int, string, DateTime)tuple and constructor injection.
Both the source-gen and reflection modes are fixed in the same PR, satisfying the dual-mode requirement from CLAUDE.md. The fix is minimal and focused — no unrelated changes.
This looks good to merge.
There was a problem hiding this comment.
Code Review — Follow-up
All issues raised in previous reviews have been addressed:
Issue 1 — Dead code (GetParameterTypes): Resolved. The old helper has been removed and replaced entirely by GetMemberTypes.
Issue 2 — Empty assertion body in snapshot test: Resolved. Tests5118.cs now includes await Assert.That(generatedFiles).HasCount().EqualTo(1), making the test self-documenting and guarding against a silent empty-file scenario.
Issue 3 — Silent typeof(object) fallback in GetMemberTypes: Resolved. The wildcard arm now throws InvalidOperationException with the actual type name, surfacing future misuse early rather than silently producing wrong test data.
The comment explaining why ClassProvidingDataSource is intentionally omitted from the Factory-carrying attribute is also present.
Current state
The fix is clean and complete:
- Source generator path: correctly extracts TypeArguments[0] from MethodDataSource so the Factory lambda calls the data source method on the correct type instead of falling back to reflection on the test class.
- Reflection path: MembersToGenerate correctly supplies constructor parameters for class-level data sources, preventing the tuple-unwrapping truncation that caused the IndexOutOfRangeException.
- Snapshot: Tests5118.Test.verified.txt is committed and shows the correct generated code.
- Test: AsyncClassMethodDataSourceTests directly reproduces the #5118 scenario with a 3-element (int, string, DateTime) tuple and constructor injection.
Both source-gen and reflection modes are fixed in the same PR, satisfying the dual-mode requirement. The fix is minimal and focused with no unrelated changes.
This looks good to merge.
The source generator fix for MethodDataSource<T> now correctly resolves the target type and generates Factory-based code, changing the output for this existing test.
There was a problem hiding this comment.
Code Review: fix/5118-methoddatasource-generic-class-level
Overall this is a clean, well-targeted fix that correctly diagnoses and addresses two distinct root causes. The commit history shows good iteration based on prior review feedback. My observations below.
Source Generator Fix — TestMetadataGenerator.cs
The change is correct and idiomatic:
if (attr.AttributeClass is { IsGenericType: true })
{
targetType = attr.AttributeClass.TypeArguments[0];
}
else
{
targetType = typeSymbol;
}Extracting T from the attribute's own type arguments is the right approach here. Previously the generator always fell back to the test class type symbol when the MethodDataSource(string) constructor overload was used, which meant MethodDataSource<T> at class level looked for the data method on the test class rather than on T.
One minor observation: the comment added to the MethodDataSource(string) branch is helpful, but it could also note that the ClassProvidingDataSource being null is intentional on the reflection path specifically because the source-gen path short-circuits via Factory before ClassProvidingDataSource is even consulted. That makes the intent even clearer for future readers who encounter the reflection fallback in isolation.
Runtime Reflection Fix — MethodDataSourceAttribute.cs
Replacing GetParameterTypes(dataGeneratorMetadata.TestInformation?.Parameters) with GetMemberTypes(dataGeneratorMetadata.MembersToGenerate) is the correct fix.
The old code always used the test method parameters for tuple-unwrapping type hints, even when the data source was class-level. For a class with a 3-element tuple constructor and a test method taking only CancellationToken, paramTypes would have been a 1-element array — causing UnwrapTupleWithTypes to truncate the tuple to one element and then throw IndexOutOfRangeException on the missing constructor args.
The defensive throw new InvalidOperationException replacing the old silent typeof(object) fallback is a good improvement — fail fast with a clear message beats silently producing wrong behaviour.
The switch expression cleanly covers both ParameterMetadata and PropertyMetadata, which are the only two concrete IMemberMetadata implementations that should ever appear here. The exhaustive pattern is the right choice.
Snapshot Changes
InheritedTestsFromDifferentProjectTests: The snapshot now shows MethodDataSource<TestData>("Foo") generating a Factory delegate (previously it emitted the typed attribute verbatim). This is a correct consequence of the generator fix — the source gen can now find TestData.Foo and emit the static Factory path. The snapshot change is expected and verified.
Tests5118.Test.verified.txt: The generated output correctly shows:
classDataSourceswithFactorycallingAsyncTestDataSources.GetAsyncTestData()using theawait foreachasync enumerable path.__CreateAttributes_0still emitting the original typed attributeMethodDataSourceAttribute<AsyncTestDataSources>— this is correct because attribute inspection (e.g. test display names) should reflect the actual declared attribute, not the generated wrapper.
Test Coverage
The new test class AsyncClassMethodDataSourceTests directly reproduces the reported scenario: class-level MethodDataSource<T> with an async enumerable returning named tuples. The three assertions cover all three constructor parameters, ensuring the tuple elements are correctly unpacked.
The snapshot test Tests5118.cs validating HasCount().EqualTo(1) on generated files confirms the source generator produces output (previously it would have silently skipped Factory generation).
Minor Observations
-
AsyncTestDataSourcesplacement: Declaring the helper class in the same file as the test class is pragmatic for a bug repro, consistent with the existing pattern inTUnit.TestProject/Bugs. -
__classMetadataduplication in snapshot: BothGetTestsand__Materialize_*re-initialize__classMetadata. This pre-exists the PR and is a characteristic of the generator output, not a concern introduced here.
Summary
Both the source generator and runtime reflection fixes are well-reasoned and surgically scoped. The dead code removal (GetParameterTypes), defensive error handling, and snapshot coverage all show the PR has been properly hardened through review iterations. No blocking issues.
There was a problem hiding this comment.
Code Review: fix/5118-methoddatasource-generic-class-level
Overall this is a clean, well-targeted fix that correctly diagnoses and addresses two distinct root causes. The commit history shows good iteration based on prior review feedback. My observations below.
Source Generator Fix — TestMetadataGenerator.cs
The change is correct and idiomatic. Extracting T from attr.AttributeClass.TypeArguments[0] is the right approach — previously the generator always fell back to the test class type symbol for the MethodDataSource(string) constructor overload, so MethodDataSource<T> at class level looked for the data method on the test class rather than on T.
The comment added to the MethodDataSource(string) branch is helpful. It could also note that ClassProvidingDataSource being null is intentional on the runtime attribute specifically because the source-gen path short-circuits via Factory before ClassProvidingDataSource is consulted — making the intent even clearer for readers who encounter the reflection fallback in isolation.
Runtime Reflection Fix — MethodDataSourceAttribute.cs
Replacing GetParameterTypes(dataGeneratorMetadata.TestInformation?.Parameters) with GetMemberTypes(dataGeneratorMetadata.MembersToGenerate) is the correct fix. The old code always used the test method parameters for tuple-unwrapping type hints, even when the data source was class-level. For a class with a 3-element tuple constructor and a test method taking only CancellationToken, paramTypes was a 1-element array — causing UnwrapTupleWithTypes to truncate the tuple and then throw IndexOutOfRangeException on the missing constructor args.
The defensive throw new InvalidOperationException replacing the silent typeof(object) fallback is a good improvement — fail fast with a clear message beats silently producing wrong behaviour. The switch expression cleanly covers both ParameterMetadata and PropertyMetadata, which are the only concrete IMemberMetadata implementations expected here.
Snapshot Changes
InheritedTestsFromDifferentProjectTests: The snapshot now shows MethodDataSource<TestData>("Foo") generating a Factory delegate rather than emitting the typed attribute verbatim. This is a correct consequence of the generator fix and the snapshot update is expected.
Tests5118.Test.verified.txt: The generated output correctly shows classDataSources with a Factory calling AsyncTestDataSources.GetAsyncTestData() via the await foreach async enumerable path. Notably, __CreateAttributes_0 still emits the original typed attribute MethodDataSourceAttribute<AsyncTestDataSources> — correct, since attribute inspection should reflect the declared attribute rather than the generated wrapper.
Test Coverage
The new AsyncClassMethodDataSourceTests directly reproduces the reported scenario: class-level MethodDataSource<T> with an async enumerable returning named tuples. Three assertions cover all three constructor parameters, confirming tuple elements are correctly unpacked. The snapshot test's HasCount().EqualTo(1) confirms the source generator now produces output where previously it silently skipped Factory generation.
Summary
Both fixes are well-reasoned and surgically scoped. Dead code removal (GetParameterTypes), defensive error handling, and snapshot coverage show the PR has been properly hardened through review iterations. No blocking issues — looks good to merge.
Code Review: fix/5118-methoddatasource-generic-class-levelOverall this is a clean, well-targeted fix that correctly diagnoses and addresses two distinct root causes. The commit history shows good iteration based on prior review feedback. Source Generator Fix —
|
Summary
Fixes #5118 —
IndexOutOfRangeExceptionwhen usingMethodDataSource<T>with tuple data at the class level.TfromMethodDataSource<T>attribute so the Factory is properly generated (previously fell back to runtime reflection because it looked for the method on the test class instead ofT)MembersToGenerate(constructor params for class-level, method params for method-level) instead ofTestInformation.Parameters(always test method params) when computing expected types for tuple unwrapping — the mismatch causedUnwrapTupleWithTypesto truncate the tupleTest plan
TUnit.TestProject/Bugs/5118/AsyncClassMethodDataSourceTests.csreproduces the exact scenario from the issue (3/3 pass)ClassTupleDataSourceDrivenTestsstill passes (4/4)NestedTupleDataSourceTestsstill passes (17/17)ClassTupleDataSourceDrivenTestspasses