You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR Review: Fix nested classes conflicting with the same testid
This PR addresses a critical bug where nested classes with the same test method names would generate conflicting test IDs. I've analyzed the changes thoroughly and have both positive feedback and important concerns.
Todo List:
Read CLAUDE.md for TUnit-specific guidelines
Analyze the changes in TestIdentifierService.cs
Check for dual-mode compatibility (source-gen vs reflection)
Evaluate code quality and best practices
Identify potential bugs or issues
Assess performance implications
Review security considerations
Evaluate test coverage needs
Provide comprehensive feedback
✅ Positive Aspects
1. Performance Optimizations
The code follows TUnit's performance-first philosophy excellently:
ArrayPool usage: Uses ArrayPool<Type>.Shared to avoid heap allocations for temporary Type arrays
StringBuilder optimization: Pre-sizes StringBuilder with estimated capacity (256 bytes)
Span usage: Leverages ReadOnlySpan<Type> for parameter handling
Per TUnit Rule 1, you MUST implement the same fix in the source generator:
// In TestMethodAnalyzer.cs, replace:TestId=$"{typeSymbol.ToDisplayString()}.{methodSymbol.Name}_{{{{TestIndex}}}}"// With proper hierarchy building similar to GetTypeNameWithGenerics
dotnet test TUnit.Core.SourceGenerator.Tests
# Accept any snapshot changes
🔍 Security & Architecture
Security: No security concerns identified
AOT Compatibility: Current reflection usage appears compatible with existing AOT annotations
Breaking Changes: None - this is a bug fix maintaining backward compatibility
📊 Test Coverage Assessment
The existing test files (Bugs/3684/*) provide basic coverage for the nested class scenario, but comprehensive testing should include:
Complex nesting scenarios
Generic type combinations
Both execution modes explicitly
Performance regression tests
🎯 Recommendation
CONDITIONAL APPROVAL - This PR addresses a real bug with good performance optimizations, but has a critical flaw:
✅ Approve the reflection mode implementation - well-executed with good performance practices
❌ BLOCK until source-generated mode is implemented - violates TUnit Rule 1
⚠️Consider performance optimizations for the hot path
📝 Add comprehensive test coverage for complex scenarios
The dual-mode requirement is non-negotiable per TUnit guidelines. Without it, users will experience different behavior depending on their execution mode, which defeats TUnit's reliability guarantees.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check the following before creating a Pull Request