fix: file path and line number fallback methods

[thomhurst]

Fixes #3196

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: File Path and Line Number Fallback Methods

Reviewing the changes for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage.

Todo List

• Analyze main implementation changes in TestMetadataGenerator.cs
• Review snapshot test changes to understand impact
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage

Analysis Summary

Changes Overview:

• Added two new methods GetTestMethodSourceLocation for improved file path and line number fallback logic
• Refactored how file paths and line numbers are determined when source location information is unavailable
• Updated multiple snapshot tests reflecting the improved fallback behavior
• Added null safety handling for file paths

Key Changes in TestMetadataGenerator.cs:

1. New fallback methods (GetTestMethodSourceLocation overloads) at lines 2198-2251
2. Improved null safety - using ?? "" for file path handling
3. Better location resolution - tries method location first, then attribute values, then syntax tree fallback
4. Code cleanup - removed unnecessary comments and simplified conditional logic

Code Quality & Best Practices ✅

Positives:

• Good separation of concerns: Extracted file path/line number logic into dedicated methods
• Proper null safety: Added ?? "" fallbacks to prevent null reference exceptions
• Consistent naming: Method names clearly indicate their purpose
• Tuple returns: Clean API using value tuples for multiple return values
• Follows C# conventions: PascalCase naming, proper method signatures

Areas for improvement:

• Parameter validation: Consider adding null checks for critical parameters like testAttribute
• XML documentation: New public/private methods could benefit from documentation comments

Potential Bugs & Issues ⚠️

Minor concerns:

1. Line 2218: methodSyntax.SyntaxTree.FilePath ?? "" - This fallback could still return null in edge cases. Consider using string.Empty explicitly
2. Line 2248: Triple null coalescing ?? chain could be simplified for readability
3. Missing validation: No null checks on testAttribute parameter in either overload

Suggestions:

// Consider adding parameter validation
private static (string filePath, int lineNumber) GetTestMethodSourceLocation(
    MethodDeclarationSyntax methodSyntax,
    AttributeData testAttribute)
{
    ArgumentNullException.ThrowIfNull(methodSyntax);
    ArgumentNullException.ThrowIfNull(testAttribute);
    // ... rest of method
}

Performance Considerations ✅

Good performance characteristics:

• No unnecessary allocations: Methods use value tuples and simple operations
• Efficient fallback chain: Short-circuits on first successful resolution
• Minimal string operations: Only creates strings when needed

No performance regressions identified.

Security Concerns ✅

No security issues found:

• File paths are handled safely without directory traversal risks
• No user input is directly used in file operations
• Source generator context is properly sandboxed

Test Coverage ✅

Well covered by snapshot tests:

• 11 snapshot test files updated showing comprehensive coverage
• Tests cover both direct test methods and inherited test scenarios
• Changes in generated code are properly verified through snapshot diffs

The snapshot updates show:

• Empty FilePath = @"" values in generated code (expected behavior for the fallback)
• Consistent line number handling
• No breaking changes to the generated test metadata structure

Overall Assessment

This is a solid improvement that addresses file path and line number resolution issues. The changes:

✅ Improve robustness through better fallback handling
✅ Maintain backward compatibility with existing test scenarios
✅ Follow TUnit coding standards from CLAUDE.md
✅ Include comprehensive test coverage via snapshots
✅ Don't introduce performance regressions

Recommendation: APPROVED with minor suggestions for parameter validation.

Minor Suggestions

1. Consider adding parameter null checks for robustness
2. Add XML documentation for the new methods
3. Consider using string.Empty instead of "" for consistency

The changes successfully fix issue #3196 and improve the reliability of file path and line number determination in the source generator.

---

[claude]

Claude finished @thomhurst's task —— View job

---

PR Review: File Path and Line Number Fallback Methods

Reviewing the changes for code quality, best practices, potential bugs, performance considerations, security concerns, and test coverage.

Todo List

• Analyze main implementation changes in TestMetadataGenerator.cs
• Review snapshot test changes to understand impact
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Evaluate test coverage

Analysis Summary

Changes Overview:

• Added two new methods GetTestMethodSourceLocation for improved file path and line number fallback logic
• Refactored how file paths and line numbers are determined when source location information is unavailable
• Updated multiple snapshot tests reflecting the improved fallback behavior
• Added null safety handling for file paths

Key Changes in TestMetadataGenerator.cs:

1. New fallback methods (GetTestMethodSourceLocation overloads) at lines 2198-2251
2. Improved null safety - using ?? "" for file path handling
3. Better location resolution - tries method location first, then attribute values, then syntax tree fallback
4. Code cleanup - removed unnecessary comments and simplified conditional logic

Code Quality & Best Practices ✅

Positives:

• Good separation of concerns: Extracted file path/line number logic into dedicated methods
• Proper null safety: Added ?? "" fallbacks to prevent null reference exceptions
• Consistent naming: Method names clearly indicate their purpose
• Tuple returns: Clean API using value tuples for multiple return values
• Follows C# conventions: PascalCase naming, proper method signatures

Areas for improvement:

• Parameter validation: Consider adding null checks for critical parameters like testAttribute
• XML documentation: New public/private methods could benefit from documentation comments

Potential Bugs & Issues ⚠️

Minor concerns:

1. Line 2218: methodSyntax.SyntaxTree.FilePath ?? "" - This fallback could still return null in edge cases. Consider using string.Empty explicitly
2. Line 2248: Triple null coalescing ?? chain could be simplified for readability
3. Missing validation: No null checks on testAttribute parameter in either overload

Suggestions:

// Consider adding parameter validation
private static (string filePath, int lineNumber) GetTestMethodSourceLocation(
    MethodDeclarationSyntax methodSyntax,
    AttributeData testAttribute)
{
    ArgumentNullException.ThrowIfNull(methodSyntax);
    ArgumentNullException.ThrowIfNull(testAttribute);
    // ... rest of method
}

Performance Considerations ✅

Good performance characteristics:

• No unnecessary allocations: Methods use value tuples and simple operations
• Efficient fallback chain: Short-circuits on first successful resolution
• Minimal string operations: Only creates strings when needed

No performance regressions identified.

Security Concerns ✅

No security issues found:

• File paths are handled safely without directory traversal risks
• No user input is directly used in file operations
• Source generator context is properly sandboxed

Test Coverage ✅

Well covered by snapshot tests:

• 11 snapshot test files updated showing comprehensive coverage
• Tests cover both direct test methods and inherited test scenarios
• Changes in generated code are properly verified through snapshot diffs

The snapshot updates show:

• Empty FilePath = @"" values in generated code (expected behavior for the fallback)
• Consistent line number handling
• No breaking changes to the generated test metadata structure

Overall Assessment

This is a solid improvement that addresses file path and line number resolution issues. The changes:

✅ Improve robustness through better fallback handling
✅ Maintain backward compatibility with existing test scenarios
✅ Follow TUnit coding standards from CLAUDE.md
✅ Include comprehensive test coverage via snapshots
✅ Don't introduce performance regressions

Recommendation: APPROVED with minor suggestions for parameter validation.

Minor Suggestions

1. Consider adding parameter null checks for robustness
2. Add XML documentation for the new methods
3. Consider using string.Empty instead of "" for consistency

The changes successfully fix issue #3196 and improve the reliability of file path and line number determination in the source generator.

---