fix: add TestDataRow<T> unwrapping to analyzer and tests for named properties

[thomhurst]

Summary

• Fixes analyzer incorrectly flagging TestDataRow<T> return types as type mismatches
• Adds comprehensive analyzer tests for named properties (DisplayName, Skip, Categories) on [Arguments] and [MethodDataSource] attributes

Changes

Bug Fix (TUnit.Analyzers/TestDataAnalyzer.cs)

Added TestDataRow<T> unwrapping logic to the UnwrapTypes method. When a method data source returns IEnumerable<TestDataRow<T>> or IEnumerable<Func<TestDataRow<T>>>, the analyzer now correctly extracts the inner type T instead of treating TestDataRow<T> as the data type.

Tests Added

ArgumentsAnalyzerTests.cs (5 new tests):

• Arguments_With_DisplayName_Property_No_Error
• Arguments_With_Skip_Property_No_Error
• Arguments_With_Categories_Property_No_Error
• Arguments_With_Multiple_Named_Properties_No_Error
• Generic_Arguments_With_DisplayName_Property_No_Error

MethodDataSourceAnalyzerTests.cs (5 new tests):

• Method_Data_Source_With_TestDataRow_DisplayName_No_Error
• Method_Data_Source_With_TestDataRow_Skip_No_Error
• Method_Data_Source_With_TestDataRow_Categories_No_Error
• Method_Data_Source_With_TestDataRow_All_Metadata_No_Error
• Method_Data_Source_With_Func_TestDataRow_No_Error

Test plan

• All existing analyzer tests pass (434 tests)
• New tests verify [Arguments] with named properties don't trigger false positives
• New tests verify TestDataRow<T> wrapper is correctly unwrapped

Fixes #4212

🤖 Generated with Claude Code

[thomhurst]

Summary

This PR fixes an analyzer false positive where TestDataRow<T> wrapper types were not being unwrapped, causing type mismatch errors, and adds comprehensive tests for named properties (DisplayName, Skip, Categories) on test data attributes.

Critical Issues

None found ✅

Suggestions

1. Consider placement of TestDataRow unwrapping logic

The TestDataRow unwrapping is added after the Func<T> unwrapping at TUnit.Analyzers/TestDataAnalyzer.cs:797-804. However, looking at the test case Method_Data_Source_With_Func_TestDataRow_No_Error, you can have IEnumerable<Func<TestDataRow<T>>>.

The current ordering handles this correctly because:

1. IEnumerable gets unwrapped first (line 773-776)
2. Func gets unwrapped (line 790-795) → leaves TestDataRow
3. TestDataRow gets unwrapped (new code at 797-804) → leaves T

This is correct! However, it might be worth adding a code comment explaining the unwrapping order matters, or verifying that the test Method_Data_Source_With_Func_TestDataRow_No_Error actually exercises the Func<TestDataRow<>> path (not just the simpler TestDataRow<> path).

2. TestDataRow type check could be more robust

TUnit.Analyzers/TestDataAnalyzer.cs:800 checks testDataRowType.Name == "TestDataRow" which is a string comparison. Consider using a more robust check:

if (type is INamedTypeSymbol { IsGenericType: true, TypeArguments.Length: 1 } testDataRowType
    && testDataRowType.OriginalDefinition.ToDisplayString() == "TUnit.Core.TestDataRow<T>")

This ensures you're matching the exact type from TUnit.Core, not any type named "TestDataRow" from another namespace. However, the current approach is likely fine given the context (it's already inside test data analysis logic).

3. Minor: Whitespace cleanup already included

The diff shows a trailing whitespace removal on line 725 of MethodDataSourceAnalyzerTests.cs (change from to empty line). Good catch! ✅

Verdict

✅ APPROVE - No critical issues

The bug fix is sound and correctly placed in the unwrapping sequence. The comprehensive test coverage for both the TestDataRow fix and the named properties ensures this won't regress. The only suggestions are minor code quality improvements that don't block merging.

[thomhurst]

Summary

Fixes analyzer to correctly unwrap TestDataRow wrapper types and adds comprehensive tests for named properties (DisplayName, Skip, Categories) on Arguments and MethodDataSource attributes.

Critical Issues

Type Safety Concern - Minor but Worth Addressing

In TUnit.Analyzers/TestDataAnalyzer.cs:800, the TestDataRow unwrapping only checks .Name == "TestDataRow" without verifying the namespace:

if (type is INamedTypeSymbol { IsGenericType: true, TypeArguments.Length: 1 } testDataRowType
    && testDataRowType.Name == "TestDataRow")
{
    type = testDataRowType.TypeArguments[0];
}

This could theoretically match any type named TestDataRow from any namespace, not just TUnit.Core.TestDataRow.

Recommendation: Follow the pattern used elsewhere in this file for robust type checking. See the pattern used for System.Func<> at line 745-746 and Task<> at line 763-766.

Suggested fix:

var testDataRowType = context.Compilation.GetTypeByMetadataName("TUnit.Core.TestDataRow`1");
if (testDataRowType != null
    && type is INamedTypeSymbol { IsGenericType: true } namedType
    && SymbolEqualityComparer.Default.Equals(namedType.OriginalDefinition, testDataRowType))
{
    type = namedType.TypeArguments[0];
}

While unlikely to cause issues in practice, using proper type resolution ensures the analyzer won't misbehave if someone creates another TestDataRow type.

Suggestions

None beyond the type safety issue above.

Verdict

REQUEST CHANGES - Minor type safety improvement needed for consistency with existing code patterns

The fix itself is solid and the tests are comprehensive. Just needs stronger type checking to match the codebase standards.

[Copilot]

Pull request overview

This pull request fixes an analyzer bug where TestDataRow<T> wrapper types were not being properly unwrapped during type checking, causing false positive type mismatch errors. It also adds comprehensive test coverage for named properties (DisplayName, Skip, Categories) on test data attributes.

Key changes:

• Added TestDataRow<T> unwrapping logic in the analyzer's UnwrapTypes method
• Added 5 new tests for [Arguments] attribute with named properties
• Added 5 new tests for [MethodDataSource] attribute with TestDataRow<T> metadata

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
TUnit.Analyzers/TestDataAnalyzer.cs	Added TestDataRow unwrapping logic to extract the inner data type when methods return IEnumerable<TestDataRow>
TUnit.Analyzers.Tests/ArgumentsAnalyzerTests.cs	Added 5 regression tests verifying that Arguments attribute with named properties (DisplayName, Skip, Categories) don't trigger false analyzer warnings
TUnit.Analyzers.Tests/MethodDataSourceAnalyzerTests.cs	Added 5 tests verifying TestDataRow wrapper is correctly handled with metadata properties, plus minor whitespace cleanup

[Copilot]

The TestDataRow unwrapping logic only checks the type name without validating the namespace. This could incorrectly match any generic type named "TestDataRow" from any namespace, not just TUnit.Core.TestDataRow. Consider using a more robust check similar to other type checks in this codebase, such as comparing the fully qualified type name or using GetTypeByMetadataName to get the expected type and comparing with SymbolEqualityComparer.

Suggested change

	if (type is INamedTypeSymbol { IsGenericType: true, TypeArguments.Length: 1 } testDataRowType
	&& testDataRowType.Name == "TestDataRow")
	{
	type = testDataRowType.TypeArguments[0];
	if (type is INamedTypeSymbol { IsGenericType: true, TypeArguments.Length: 1 } testDataRowType)
	{
	var tunitTestDataRowSymbol = context.Compilation.GetTypeByMetadataName("TUnit.Core.TestDataRow`1");
	if (tunitTestDataRowSymbol is not null
	&& SymbolEqualityComparer.Default.Equals(testDataRowType.ConstructedFrom, tunitTestDataRowSymbol))
	{
	type = testDataRowType.TypeArguments[0];
	}

[thomhurst]

Summary

Fixes analyzer to correctly unwrap TestDataRow wrapper types and adds comprehensive test coverage for named properties.

Critical Issues

None found ✅

Previous Review Status

✅ Type safety concern addressed - Now uses GetTypeByMetadataName and SymbolEqualityComparer
✅ Unwrapping order documented with helpful comment

TUnit Rules Applied

• Rule 2 (Snapshot Testing): ✅ Not applicable - analyzer logic only, no API changes
• Rule 5 (AOT Compatible): ✅ Verified - proper type resolution used

Code Review

The implementation is solid:

1. Correct placement after Func unwrapping
2. Type-safe with SymbolEqualityComparer
3. Well-tested with 10 new test cases
4. No regressions - only adds new unwrapping step

Verdict

✅ APPROVE - All previous concerns addressed. Fix is correct, well-tested, and follows TUnit code patterns.