TUnit.AspNetCore Analyzers #4138
Conversation
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
Pull Request Review: TUnit.AspNetCore AnalyzersThank you for this contribution! This PR adds important compile-time safety checks for the TUnit.AspNetCore testing framework. Here's my comprehensive review: ✅ Strengths1. Excellent Code Quality
2. Strong Test Coverage
3. Clear Documentation
4. Good Architecture
🔍 Issues & RecommendationsCRITICAL: Missing Dual-Mode VerificationPer CLAUDE.md Rule 1, all features must work in both source-generated AND reflection modes. This PR adds analyzers but doesn't verify behavior in both execution modes. Required Action: [Test]
[Arguments(ExecutionMode.SourceGenerated)]
[Arguments(ExecutionMode.Reflection)]
public async Task Factory_Access_In_Test_Works_In_Both_Modes(ExecutionMode mode)
{
// Verify no false positives that would break actual test execution
}CRITICAL: Missing Public API Snapshot TestsPer CLAUDE.md Rule 2, changes to public APIs require snapshot testing. Required Action: cd TUnit.PublicAPI
dotnet test
# Review .received.txt files
# If changes are intentional:
for f in *.received.txt; do mv "$f" "${f%.received.txt}.verified.txt"; done
git add *.verified.txt📝 Code-Specific FeedbackTUnit.AspNetCore.Analyzers/WebApplicationFactoryAccessAnalyzer.csLine 145-169: Type checking logic
// Suggestion: Add caching
private static readonly ConcurrentDictionary<INamedTypeSymbol, bool> TypeCache = new();
private static bool IsWebApplicationTestType(INamedTypeSymbol? type)
{
if (type == null) return false;
return TypeCache.GetOrAdd(type, CheckTypeHierarchy);
}
private static bool CheckTypeHierarchy(INamedTypeSymbol type)
{
// Current logic here
}Line 171-201: GetContainingMethod
private static IMethodSymbol? GetContainingMethod(IOperation operation)
{
var syntax = operation.Syntax;
var methodNode = syntax.AncestorsAndSelf()
.OfType<MethodDeclarationSyntax>()
.FirstOrDefault()
?? syntax.AncestorsAndSelf().OfType<ConstructorDeclarationSyntax>().FirstOrDefault();
if (methodNode == null) return null;
return operation.SemanticModel?.GetDeclaredSymbol(methodNode) as IMethodSymbol;
}TUnit.AspNetCore/TestWebApplicationFactory.csLines 44-45: Exception throwing
TUnit.AspNetCore.Analyzers.Tests/Verifiers/LineEndingNormalizingVerifier.csLines 1-101: Cross-platform testing
Bug Fix: public IVerifier PushContext(string context)
{
return new LineEndingNormalizingVerifierWithContext(_defaultVerifier.PushContext(context));
}
private class LineEndingNormalizingVerifierWithContext : LineEndingNormalizingVerifier
{
public LineEndingNormalizingVerifierWithContext(IVerifier wrappedVerifier)
{
_defaultVerifier = (DefaultVerifier)wrappedVerifier; // This cast may fail!
}
}Issue: The wrapped verifier might not be a Recommended Fix: private readonly IVerifier _innerVerifier;
public LineEndingNormalizingVerifier(IVerifier? innerVerifier = null)
{
_innerVerifier = innerVerifier ?? new DefaultVerifier();
}
public IVerifier PushContext(string context)
{
return new LineEndingNormalizingVerifier(_innerVerifier.PushContext(context));
}
// Update all methods to use _innerVerifier instead of _defaultVerifierTUnit.AspNetCore.Analyzers.Tests/WebApplicationFactoryAccessAnalyzerTests.csLines 427-440: Test organization
🔒 Security & PerformanceSecurity
Performance
Impact: Low priority - analyzers run at compile-time, not runtime 🧪 Missing Test Coverage
📦 Build & IntegrationConcerns:
Missing:
✅ Pre-Commit Checklist (from CLAUDE.md)Based on CLAUDE.md requirements:
📋 Action ItemsBefore Merge:
Future Enhancements:
🎯 Overall AssessmentRating: ⭐⭐⭐⭐ (4/5) - Excellent work with minor required fixes This is a high-quality contribution that adds important safety guardrails to TUnit.AspNetCore. The analyzer design is sound, test coverage is comprehensive, and the code follows modern C# best practices. Blocking Issues: Snapshot testing and dual-mode verification per TUnit development guidelines. Once addressed, this will be ready to merge! Great work on improving developer experience with compile-time safety. Review conducted following TUnit CLAUDE.md development guidelines |
There was a problem hiding this comment.
Pull request overview
This PR adds Roslyn analyzers for TUnit.AspNetCore to catch common misuse patterns at compile-time. The analyzers enforce proper usage of the WebApplicationTest pattern, preventing developers from accessing properties before they're initialized and ensuring test isolation by preventing direct access to GlobalFactory's shared members.
Key changes:
- Introduces two new analyzer rules (TUnit0062 and TUnit0063) to detect premature property access and test isolation violations
- Implements multi-version Roslyn support (4.4, 4.7, 4.14) following TUnit's existing analyzer patterns
- Adds comprehensive analyzer tests with cross-platform line ending normalization
- Updates TUnit.AspNetCore package to include analyzers in NuGet distribution
- Adds NuGet integration tests to validate the analyzers work when consumed as a package
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TUnit.AspNetCore.Analyzers/WebApplicationFactoryAccessAnalyzer.cs | Core analyzer detecting premature access to Factory/Services/HttpCapture and misuse of GlobalFactory |
| TUnit.AspNetCore.Analyzers/Rules.cs | Diagnostic rule definitions for TUnit0062 and TUnit0063 |
| TUnit.AspNetCore.Analyzers/ConcurrentDiagnosticAnalyzer.cs | Base class for thread-safe analyzers, consistent with existing TUnit patterns |
| TUnit.AspNetCore.Analyzers/Resources.resx | Localized error messages and descriptions for analyzer diagnostics |
| TUnit.AspNetCore.Analyzers/Resources.Designer.cs | Auto-generated resource accessor class |
| TUnit.AspNetCore.Analyzers/TUnit.AspNetCore.Analyzers.csproj | Main analyzer project configuration |
| TUnit.AspNetCore.Analyzers/AnalyzerReleases.*.md | Analyzer release tracking files |
| TUnit.AspNetCore.Analyzers.Roslyn44/TUnit.AspNetCore.Analyzers.Roslyn44.csproj | Roslyn 4.4 compatibility project |
| TUnit.AspNetCore.Analyzers.Roslyn47/TUnit.AspNetCore.Analyzers.Roslyn47.csproj | Roslyn 4.7 compatibility project |
| TUnit.AspNetCore.Analyzers.Roslyn414/TUnit.AspNetCore.Analyzers.Roslyn414.csproj | Roslyn 4.14 compatibility project |
| TUnit.AspNetCore.Analyzers.Tests/WebApplicationFactoryAccessAnalyzerTests.cs | Comprehensive analyzer tests covering all scenarios |
| TUnit.AspNetCore.Analyzers.Tests/Verifiers/*.cs | Test infrastructure with line-ending normalization for cross-platform compatibility |
| TUnit.AspNetCore.Analyzers.Tests/TUnit.AspNetCore.Analyzers.Tests.csproj | Test project configuration |
| TUnit.AspNetCore/TestWebApplicationFactory.cs | Adds defensive properties throwing NotSupportedException to enforce analyzer patterns at runtime |
| TUnit.AspNetCore/TUnit.AspNetCore.csproj | Configures analyzer packaging for all Roslyn versions |
| tools/tunit-nuget-tester/TUnit.AspNetCore.NugetTester/* | Integration tests validating analyzer behavior when consumed as NuGet package |
| TUnit.Pipeline/Modules/TestAspNetCoreNugetPackageModule.cs | Pipeline module for testing NuGet package |
| TUnit.Pipeline/Modules/RunAspNetCoreAnalyzersTestsModule.cs | Pipeline module for running analyzer tests |
| TUnit.sln | Adds new analyzer and test projects to solution |
| Directory.Build.props | Configures common properties for AspNetCore.Analyzers.Roslyn* projects |
| Directory.Packages.props | Adds TUnit.AspNetCore package version |
| AspNetCore.Analyzer.props | Shared props file for AspNetCore analyzer projects |
Files not reviewed (1)
- TUnit.AspNetCore.Analyzers/Resources.Designer.cs: Language not supported
| public Task InitializeAsync() | ||
| { | ||
| // Eagerly start the server to catch configuration errors early | ||
| _ = Server; |
There was a problem hiding this comment.
The InitializeAsync method attempts to access the Server property, but the base class TestWebApplicationFactory explicitly throws NotSupportedException when Server is accessed (see TestWebApplicationFactory.cs line 45). This will cause a runtime exception when the factory is initialized.
The GlobalFactory (which is a TestWebApplicationFactory) should not have its Server or Services properties accessed directly. Instead, the pattern is to call GetIsolatedFactory() which returns a regular WebApplicationFactory where Server is accessible. If you need to eagerly start the server in InitializeAsync, you should either remove this code or rethink the initialization pattern.
| { | ||
| // Verifies that POST request body is captured | ||
| var client = Factory.CreateClient(); | ||
| var content = new StringContent("test payload", Encoding.UTF8, "text/plain"); |
There was a problem hiding this comment.
Disposable 'StringContent' is created but not disposed.
| // Verifies that filtering by HTTP method works | ||
| var client = Factory.CreateClient(); | ||
| await client.GetAsync("/ping"); | ||
| await client.PostAsync("/echo", new StringContent("test")); |
There was a problem hiding this comment.
Disposable 'StringContent' is created but not disposed.
| else if (containingMethod.Name == "SetupAsync" && containingMethod.IsOverride) | ||
| { | ||
| // Only Factory/Services/HttpCapture are invalid in SetupAsync | ||
| // GlobalFactory IS available in SetupAsync | ||
| if (isRestrictedInBoth) | ||
| { | ||
| contextName = "SetupAsync"; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| else if (containingMethod.Name == "SetupAsync" && containingMethod.IsOverride) | |
| { | |
| // Only Factory/Services/HttpCapture are invalid in SetupAsync | |
| // GlobalFactory IS available in SetupAsync | |
| if (isRestrictedInBoth) | |
| { | |
| contextName = "SetupAsync"; | |
| } | |
| else if (containingMethod.Name == "SetupAsync" && containingMethod.IsOverride && isRestrictedInBoth) | |
| { | |
| // Only Factory/Services/HttpCapture are invalid in SetupAsync | |
| // GlobalFactory IS available in SetupAsync | |
| contextName = "SetupAsync"; |
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
Pull Request Review: TUnit.AspNetCore AnalyzersThank you for adding analyzers to TUnit.AspNetCore! This is a valuable addition that will help developers catch common mistakes at compile-time. Here's my comprehensive review: ✅ Strengths1. Excellent Test Coverage
2. Well-Designed Analyzer Logic
3. Good Error Messages
4. Proper Integration
🔍 Issues & RecommendationsCRITICAL: Potential Performance IssueLocation: private static IMethodSymbol? GetContainingMethod(IOperation operation)
{
var current = operation;
while (current != null)
{
if (current is IMethodBodyOperation or IBlockOperation)
{
var syntax = current.Syntax;
while (syntax != null)
{
if (syntax is MethodDeclarationSyntax or ConstructorDeclarationSyntax)
{
var semanticModel = operation.SemanticModel; // ⚠️ Issue here
if (semanticModel != null)
{
var symbol = semanticModel.GetDeclaredSymbol(syntax);
// ...Problem: This method has two nested loops and repeatedly accesses Recommendation: private static IMethodSymbol? GetContainingMethod(IOperation operation)
{
var semanticModel = operation.SemanticModel; // Cache outside loops
if (semanticModel == null)
return null;
var syntax = operation.Syntax;
while (syntax != null)
{
if (syntax is MethodDeclarationSyntax or ConstructorDeclarationSyntax)
{
if (semanticModel.GetDeclaredSymbol(syntax) is IMethodSymbol methodSymbol)
{
return methodSymbol;
}
}
syntax = syntax.Parent;
}
return null;
}Why:
MODERATE: Missing AOT/Trimming AnnotationsLocation: Per CLAUDE.md Rule 5, reflection usage should be annotated. The Recommendation: While analyzers typically run at build-time (not in trimmed apps), consider adding a comment explaining why trimming annotations aren't needed, or add them for consistency: // Note: Analyzers run at compile-time only, so trimming is not a concern
private static bool IsWebApplicationTestType(INamedTypeSymbol? type)MINOR: Code Style ConsistencyLocation: Issue: Excellent implementation, but could use XML documentation for the class-level purpose. Suggestion: /// <summary>
/// A custom verifier that normalizes line endings to LF before comparison to support cross-platform testing.
/// This prevents tests from failing due to differences between Windows (CRLF) and Unix (LF) line endings.
/// By normalizing to LF (the universal standard), tests pass consistently on all platforms.
/// </summary>✅ This is already present! No action needed. MINOR: Resource File FormattingLocation: The description message is quite long (could be >200 chars). This is fine, but consider if users will see this in tooltips - some IDEs truncate long descriptions. Current:
This is actually good - descriptive is better than terse. ✅ No change needed. 📦 Package & DistributionQuestion: Packaging StrategyI notice Verify: Will the analyzers be included in the Expected: The analyzers should ship with 🧪 TestingExcellent Coverage:
Missing Test (Low Priority):Consider adding a test for local variable assignments in constructors: public MyTests()
{
var factory = this.Factory; // Should still error
var f = Factory; // Already tested
}The current implementation should handle this correctly via 🎯 CLAUDE.md Compliance Check
📋 Pre-Merge ChecklistBefore merging, verify:
🎉 Overall AssessmentRating: 9/10 - Excellent work! This is a well-designed analyzer with comprehensive tests. Primary Action Item: Optimize Secondary: Verify NuGet packaging includes all analyzer DLLs. Everything else looks great - the analyzer logic is sound, error messages are helpful, and test coverage is excellent. Once the performance optimization is applied, this is ready to merge! Great contribution to TUnit! 🚀 |
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
GitHub Actions
Inactive
Pull Request Review: TUnit.AspNetCore AnalyzersThis PR adds Roslyn analyzers to prevent common mistakes when using the TUnit.AspNetCore testing framework. The implementation is well-structured and comprehensive. ✅ Strengths1. Excellent Code Quality
2. Comprehensive Test Coverage
3. Strong Documentation
4. Proper Integration
🔍 Issues and RecommendationsHIGH: Code Duplication in GetContainingMethodThe GetContainingMethod implementation (lines 172-202) walks the syntax tree manually. Consider simplifying with Roslyn built-in APIs. Current implementation has nested loops that could be flattened. MEDIUM: Performance ConsiderationsPer CLAUDE.md Rule 4 (Performance First), the analyzer runs on every compilation:
MEDIUM: Potential False Negative - Indirect AccessThe analyzer detects direct property access but may miss indirect patterns (helper methods called from constructors). Add test case or document limitation. LOW: Test Code DuplicationWebApplicationTestStub and WebApplicationFactoryStub are repeated throughout tests. Extract to helper methods. LOW: XML Documentation MissingAnalyzer classes lack XML documentation comments explaining purpose and diagnostics. 🎯 Questions
🚀 Additional Suggestions
📊 Overall Assessment
Recommendation: Approve with minor suggestions ✅ The core implementation is solid and ready to merge. Suggestions above are optimizations and nice-to-haves for follow-up PRs. 🎉 Nice Touches
Great work on this addition to TUnit! 🚀 |
No description provided.