Skip to content

Conversation

@thomhurst
Copy link
Owner

No description provided.

@claude
Copy link
Contributor

claude bot commented Nov 4, 2025

Claude finished @thomhurst's task —— View job


PR Review: Fix filename generation to include all containing types

I've completed a comprehensive review of this PR. This is a well-implemented bug fix that addresses filename collision issues for nested test classes.

Summary

Problem: The filename generator only included the innermost class name, causing collisions when multiple nested classes had the same name (e.g., Class1Tests.Constructor and Class2Tests.Constructor both generated Constructor.g.cs).

Solution: Enhanced filename generation to include the full type hierarchy from outer-to-inner classes.

📋 Detailed Analysis

Code QualityExcellent

  • Clean implementation: Clear separation of concerns with well-structured logic
  • Consistent patterns: Both methods (GetDeterministicFileName and GetDeterministicFileNameForMethod) updated consistently
  • Modern C# conventions: Proper use of StringBuilder, collection expressions, and clear variable naming
  • Good documentation: Clear XML documentation explaining the filename format

Algorithm CorrectnessRobust

  • Proper hierarchy traversal: Correctly walks up the containment chain using currentType.ContainingType
  • Correct ordering: Uses reverse to ensure outer-to-inner class ordering
  • Edge case handling: Properly handles single classes (no containing types)
  • Generic type support: Maintains generic type argument handling

PerformanceEfficient

  • Minimal allocations: Single StringBuilder instance, pre-sized list
  • No LINQ overhead: Uses simple loops instead of LINQ (important for source generators)
  • Cached operations: Reuses the same pattern in both methods efficiently

Security & RobustnessSafe

  • File path safety: Comprehensive character sanitization prevents directory traversal
  • Input validation: Handles null containing types gracefully
  • Deterministic output: Ensures consistent filename generation across runs

Test CoverageAdequate

The test files perfectly demonstrate the issue:

// Before: Both would generate "Constructor.g.cs" (collision!)
Class1Tests.Constructor -> Constructor.g.cs  
Class2Tests.Constructor -> Constructor.g.cs  

// After: Unique filenames
Class1Tests.Constructor -> TUnit_TestProject_Bugs__3684_Class1Tests_Constructor.g.cs
Class2Tests.Constructor -> TUnit_TestProject_Bugs__3684_Class2Tests_Constructor.g.cs

Compliance with TUnit GuidelinesPerfect

  • Source generator focus: This change only affects the source generation path (appropriate for this bug)
  • No dual-mode concerns: Filename generation is purely a source generator concern
  • Performance optimized: Follows TUnit's performance-first approach with minimal allocations
  • No breaking changes: Maintains backward compatibility for existing single-level classes

🚀 Strengths

  1. Targeted fix: Addresses the root cause without over-engineering
  2. Backward compatible: Existing single-level classes continue working
  3. Future-proof: Handles arbitrary nesting levels
  4. Consistent naming: Maintains predictable filename patterns
  5. Clear test cases: The test files clearly demonstrate the problem and solution

🔍 Minor Observations

  • Filename length: For deeply nested hierarchies, filenames could become long, but this is acceptable given the need for uniqueness
  • Performance impact: Minimal - the traversal is O(depth) which is typically small for real-world scenarios

Recommendation

APPROVE - This is a high-quality bug fix that:

  • Solves a real filename collision problem
  • Follows TUnit's coding standards perfectly
  • Has appropriate test coverage
  • Introduces no breaking changes or performance regressions
  • Is well-documented and maintainable

The implementation is production-ready and should be merged.


@thomhurst thomhurst merged commit c437e11 into main Nov 4, 2025
12 of 13 checks passed
@thomhurst thomhurst deleted the bug/3684 branch November 4, 2025 09:30
This was referenced Nov 4, 2025
This was referenced Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build Error in Source Generator Test Project with TUnit 0.25.21+

2 participants