fix: generic methods with [GenerateGenericTest] + [MethodDataSource] now discovered (#4440)

[thomhurst]

Summary

Fixes #4440 - Generic test methods combining [GenerateGenericTest] with [MethodDataSource] were not being discovered in reflection mode.

Root cause: IsDataCompatibleWithExpectedTypes in TestBuilder.cs incorrectly filtered out tests where method parameters (e.g., string) didn't match the generic type argument (e.g., int).

Fix: Check whether method parameters actually use the method's generic type parameters. For methods like GenericMethod<T>(string input), the string parameter doesn't depend on T, so data compatibility with T is irrelevant.

Changes

• Add ParameterUsesMethodGenericType helper to detect if parameters use method generics
• Skip data compatibility check when no parameters use method generic types
• Add ResolveMethodInstantiations to create concrete method instances from [GenerateGenericTest]
• Add comprehensive tests covering:
  • Non-generic class + generic method + data source (original bug)
  • Non-generic class + generic method (no data source)
  • Generic class + non-generic method + data source
  • Generic class + generic method + data source (cartesian product)
  • Multiple type parameters
  • Constrained generics

Test plan

• Verified 4 tests discovered for NonGenericClassWithGenericMethodAndDataSource (2 types × 2 data items)
• All scenarios pass in both source-generated and reflection modes
• Existing tests continue to pass

🤖 Generated with Claude Code

[thomhurst]

Summary

Fixes generic methods with [GenerateGenericTest] + [MethodDataSource] not being discovered in reflection mode by adding ResolveMethodInstantiations method and fixing data compatibility checks.

Critical Issues

1. .gitignore Error

The change to .gitignore appears incorrect. It changes from .worktrees/ to .worktrees/.worktrees/ which looks like a typo that will break the gitignore for worktrees. Should this be reverted to .worktrees/?

Location: .gitignore:441

Suggestions

1. Dual-Mode Verification

The changes only touch reflection mode (TUnit.Engine). Per TUnit Rule 1, I would like to confirm tests verify BOTH modes produce identical behavior. The test file TUnit.Engine.Tests/GenericMethodWithDataSourceTests.cs uses InvokableTestBase(testMode) which suggests it tests both modes. Can you confirm the tests pass in both source-gen and reflection modes?

2. Code Quality

• Proper error handling with visible failed tests
• Good AOT annotations on CreateFailedMethodGenericMetadata
• Clear comments explaining the logic
• The ParameterUsesMethodGenericType helper is elegant

Verdict

REQUEST CHANGES - The .gitignore issue must be fixed before merging.

Once the .gitignore is corrected, this is a solid fix that properly brings reflection mode into parity with source generator mode.

[thomhurst]

Summary

Fixes reflection mode to discover generic test methods combining [GenerateGenericTest] with [MethodDataSource] by adding method instantiation resolution and fixing data compatibility checks.

Critical Issues

1. .gitignore Error (Previously Reported - STILL PRESENT)

Location: .gitignore:441

The change from .worktrees/ to .worktrees/.worktrees/ appears to be an accidental typo/error. This will break the gitignore pattern for git worktrees.

Expected: .worktrees/
Current in PR: .worktrees/.worktrees/

This should be reverted to .worktrees/ (the original value). The only intentional change should be adding a trailing newline.

Suggestions

None - the core implementation is solid.

Previous Review Status

The previous review (2026-01-16 12:24:11) identified the .gitignore issue and requested changes. This issue has NOT been fixed yet. All other aspects were approved.

Code Quality Notes

✅ Dual-Mode Compliance: Changes only touch reflection mode (TUnit.Engine). Tests inherit from InvokableTestBase(testMode) which runs tests in both SourceGenerated and Reflection modes, ensuring parity per TUnit Rule 1.

✅ AOT Compatible: Proper [DynamicallyAccessedMembers] annotations on CreateFailedMethodGenericMetadata.

✅ Error Handling: Failed discoveries become visible failed tests in the explorer rather than silently disappearing.

✅ Performance: No obvious hot-path allocations. The ResolveMethodInstantiations method uses iterators efficiently.

✅ Modern C#: Good use of tuple deconstruction, collection expressions, and pattern matching.

✅ Well-documented: The ParameterUsesMethodGenericType helper is elegant and well-commented.

Verdict

⚠️ REQUEST CHANGES - The .gitignore issue must be fixed before merging.

Once the .gitignore is corrected, this is an excellent fix that properly brings reflection mode into parity with source generator mode for generic methods with data sources.

[thomhurst]

Summary

Fixes generic methods with [GenerateGenericTest] + [MethodDataSource] not being discovered in reflection mode, achieving parity with source-generated mode.

Critical Issues

1. Incorrect .gitignore change
The .gitignore change appears incorrect:

-.worktrees/
+.worktrees/.worktrees/

This changes the pattern from .worktrees/ to .worktrees/.worktrees/ which doesn't make sense. Should likely be reverted to -.worktrees/ or clarified if intentional.

Suggestions

1. Consider performance optimization in ParameterUsesMethodGenericType
In TestBuilder.cs:1287, the recursive method could benefit from memoization since it's called in a hot path (test discovery). Currently it will re-traverse the same type hierarchies multiple times for methods with multiple parameters.

// Current implementation traverses type hierarchy on every call
private static bool ParameterUsesMethodGenericType(TypeInfo? typeInfo)
{
    return typeInfo switch
    {
        GenericParameter { IsMethodParameter: true } => true,
        ConstructedGeneric cg => cg.TypeArguments.Any(ParameterUsesMethodGenericType),
        _ => false
    };
}

Consider caching results or moving the check outside the parameter loop since the answer is the same for all parameters of a given method.

2. Minor: Whitespace-only changes
Lines 88 and 99 in ReflectionTestDataCollector.cs contain whitespace-only changes (adding/removing blank lines). These are harmless but add noise to the diff.

TUnit Rules Compliance

✅ Dual-Mode (Rule 1): PASS - Changes only affect reflection mode to achieve parity with source generator. Source gen already handles this correctly (lines 3676-3751 in TestMetadataGenerator.cs).

✅ Snapshot Testing (Rule 2): N/A - No source generator output or public API changes. Only runtime reflection discovery logic.

✅ Performance (Rule 4): GOOD - Implemented as IEnumerable (synchronous) rather than allocating lists. Minor optimization opportunity noted above.

✅ AOT Compatible (Rule 5): PASS - Proper [DynamicallyAccessedMembers] annotation added on CreateFailedMethodGenericMetadata at line 1210.

Test Coverage

Excellent comprehensive test coverage:

• GenericMethodWithDataSourceTests.cs - Integration tests for all scenarios
• GenericMethodDiscoveryTests.cs - Test fixtures covering:
  • Non-generic class + generic method + data source (original bug)
  • Generic class + generic method (cartesian product)
  • Multiple type parameters
  • Constrained generics

All critical scenarios are tested in both source-gen and reflection modes.

Previous Review Status

No previous comments found.

Verdict

⚠️ REQUEST CHANGES - The .gitignore change needs to be corrected before merge. The core fix is solid and well-tested.