Fix InheritsTests source location to show actual test method location

[thomhurst]

Fixes #3055

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

• Read and understand the core changes in TestMetadataGenerator.cs
• Review the reproduction test case
• Analyze snapshot file changes
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance implications
• Review security considerations
• Evaluate test coverage
• Provide comprehensive feedback

---

Review Summary

This PR successfully fixes the source location issue for InheritsTests by leveraging the TestAttribute's built-in CallerFilePath and CallerLineNumber parameters. The fix is well-implemented and follows good practices.

✅ Strengths

Clean Implementation

• Leverages existing compiler-provided location information via [CallerFilePath] and [CallerLineNumber]
• Removes complex syntax tree parsing in favor of simpler attribute parameter access
• Prioritizes attribute constructor arguments over fallback location sources

Good Code Quality

• Clear variable naming (filePath, lineNumber)
• Proper null coalescing with sensible fallbacks
• Maintains backward compatibility with existing behavior

Comprehensive Testing

• Added reproduction test case in SourceLocationRepro.cs:TUnit.TestProject/Bugs/SourceLocationRepro.cs:12-20
• Updated 11 snapshot files showing corrected line numbers (e.g., line 11 instead of 23/26/29)
• Follows TUnit's snapshot testing practices

🔍 Technical Analysis

The Fix (TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs:136-141)

var filePath = testAttribute.ConstructorArguments.ElementAtOrDefault(0).Value?.ToString() ?? 
              classInfo.ClassSyntax.GetLocation().SourceTree?.FilePath ?? 
              classInfo.ClassSyntax.SyntaxTree.FilePath;

var lineNumber = (int?)testAttribute.ConstructorArguments.ElementAtOrDefault(1).Value ?? 
                classInfo.ClassSyntax.GetLocation().GetLineSpan().StartLinePosition.Line + 1;

This correctly extracts the caller information that TestAttribute automatically captures when applied to methods.

Performance: ✅ Excellent

• Removes expensive syntax tree traversal
• Uses simple array access instead of complex parsing
• More efficient than the previous implementation

Security: ✅ No concerns

• Standard attribute parameter access
• No external input handling
• Maintains existing security posture

Potential Issues: ⚠️ Minor

• ElementAtOrDefault() calls could theoretically throw if constructor arguments are malformed, but this is extremely unlikely in practice since these are compiler-generated
• No additional error handling, but the existing fallback chain should handle edge cases

🎯 Test Coverage Assessment

The snapshot changes demonstrate the fix works across various inheritance scenarios:

• Abstract base classes (AbstractTests.Concrete1.verified.txt)
• Concrete inheritance (ConcreteClassTests.Test.verified.txt)
• Cross-project inheritance (InheritedTestsFromDifferentProjectTests.Test.verified.txt)
• Multiple inheritance levels

💡 Minor Suggestions

1. Consider defensive coding: While unlikely to be needed, you could add a safety check:

   var lineNumber = testAttribute.ConstructorArguments.Length > 1 
       ? (int?)testAttribute.ConstructorArguments[1].Value 
       : null ?? classInfo.ClassSyntax.GetLocation().GetLineSpan().StartLinePosition.Line + 1;

2. Documentation: The change is self-documenting, but a brief comment explaining the priority order would be helpful for future maintainers.

✅ Overall Assessment

This is a high-quality fix that:

• Solves the reported issue effectively
• Improves performance by removing complex parsing
• Maintains backward compatibility
• Includes proper test coverage
• Follows TUnit's architectural patterns

The approach of using the TestAttribute's built-in caller information is elegant and aligns well with the framework's design philosophy.

Recommendation: Approve and merge ✅

---

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

• Read and understand the core changes in TestMetadataGenerator.cs
• Review the new reproduction test case in SourceLocationRepro.cs
• Analyze snapshot file changes to understand fix impact
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance implications
• Review security considerations
• Evaluate test coverage
• Provide comprehensive feedback

---

Review Summary

This PR successfully fixes the source location issue for InheritsTests by leveraging the TestAttribute's built-in CallerFilePath and CallerLineNumber parameters. The fix is well-implemented and follows good practices.

✅ Strengths

Clean Implementation

• Leverages existing compiler-provided location information via [CallerFilePath] and [CallerLineNumber]
• Removes complex syntax tree parsing in favor of simpler attribute parameter access
• Prioritizes attribute constructor arguments over fallback location sources

Good Code Quality

• Clear variable naming (filePath, lineNumber)
• Proper null coalescing with sensible fallbacks
• Maintains backward compatibility with existing behavior

Comprehensive Testing

• Added reproduction test case in SourceLocationRepro.cs:TUnit.TestProject/Bugs/SourceLocationRepro.cs:12-20
• Updated 11 snapshot files showing corrected line numbers (e.g., line 11 instead of 23/26/29)
• Follows TUnit's snapshot testing practices

🔍 Technical Analysis

The Fix (TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs:136-141)

var filePath = testAttribute.ConstructorArguments.ElementAtOrDefault(0).Value?.ToString() ?? 
              classInfo.ClassSyntax.GetLocation().SourceTree?.FilePath ?? 
              classInfo.ClassSyntax.SyntaxTree.FilePath;

var lineNumber = (int?)testAttribute.ConstructorArguments.ElementAtOrDefault(1).Value ?? 
                classInfo.ClassSyntax.GetLocation().GetLineSpan().StartLinePosition.Line + 1;

This correctly extracts the caller information that TestAttribute automatically captures when applied to methods.

Performance: ✅ Excellent

• Removes expensive syntax tree traversal
• Uses simple array access instead of complex parsing
• More efficient than the previous implementation

Security: ✅ No concerns

• Standard attribute parameter access
• No external input handling
• Maintains existing security posture

Potential Issues: ⚠️ Minor

• ElementAtOrDefault() calls could theoretically throw if constructor arguments are malformed, but this is extremely unlikely in practice since these are compiler-generated
• No additional error handling, but the existing fallback chain should handle edge cases

🎯 Test Coverage Assessment

The snapshot changes demonstrate the fix works across various inheritance scenarios:

• Abstract base classes (AbstractTests.Concrete1.verified.txt)
• Concrete inheritance (ConcreteClassTests.Test.verified.txt)
• Cross-project inheritance (InheritedTestsFromDifferentProjectTests.Test.verified.txt)
• Multiple inheritance levels

💡 Minor Suggestions

1. Consider defensive coding: While unlikely to be needed, you could add a safety check:

   var lineNumber = testAttribute.ConstructorArguments.Length > 1 
       ? (int?)testAttribute.ConstructorArguments[1].Value 
       : null ?? classInfo.ClassSyntax.GetLocation().GetLineSpan().StartLinePosition.Line + 1;

2. Documentation: The change is self-documenting, but a brief comment explaining the priority order would be helpful for future maintainers.

✅ Overall Assessment

This is a high-quality fix that:

• Solves the reported issue effectively
• Improves performance by removing complex parsing
• Maintains backward compatibility
• Includes proper test coverage
• Follows TUnit's architectural patterns

The approach of using the TestAttribute's built-in caller information is elegant and aligns well with the framework's design philosophy.

Recommendation: Approve and merge ✅

---