Enhance NUnit analyzer converters with additional patterns#4268
Enhance NUnit analyzer converters with additional patterns#4268
Conversation
Add support for:
- Has.Count.EqualTo(n) -> Count().IsEqualTo(n)
- Has.Exactly(n).Items -> Count().IsEqualTo(n)
- Is.Not.Zero -> IsNotZero() (improved from IsNotEqualTo(0))
- Assert.Multiple(() => { ... }) -> using (Assert.Multiple()) { ... }
Fixes #4191
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SummaryEnhances the NUnit-to-TUnit analyzer with four new conversion patterns. Critical IssuesNone found - all changes look good. Suggestions
VerdictAPPROVE - No blocking issues. Great work on test coverage! |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the NUnit to TUnit migration analyzer with four new conversion patterns:
Has.Count.EqualTo(n)→Count().IsEqualTo(n)for collection count assertionsHas.Exactly(n).Items→Count().IsEqualTo(n)for exact item count checksIs.Not.Zero→IsNotZero()for improved zero-check assertionsAssert.Multiple(() => { ... })→using (Assert.Multiple()) { ... }for batched assertions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| TUnit.Analyzers.Tests/NUnitMigrationAnalyzerTests.cs | Adds 4 comprehensive test cases validating each new conversion pattern with expected input/output code |
| TUnit.Analyzers.CodeFixers/NUnitMigrationCodeFixProvider.cs | Implements the conversion logic for all 4 patterns including VisitExpressionStatement override for Assert.Multiple, constraint conversion methods, and CreateCountAssertion helper |
| SyntaxList<StatementSyntax> statements; | ||
| if (lambda.Body is BlockSyntax block) | ||
| { | ||
| var convertedStatements = block.Statements.Select(s => (StatementSyntax)Visit(s)!).ToArray(); |
There was a problem hiding this comment.
This Visit call uses a null-forgiving operator but could potentially return null. Consider adding null checks before casting to avoid potential NullReferenceExceptions.
| } | ||
| else if (lambda.Body is ExpressionSyntax expr) | ||
| { | ||
| var visitedExpr = (ExpressionSyntax)Visit(expr)!; |
There was a problem hiding this comment.
This Visit call uses a null-forgiving operator but could potentially return null. Consider adding null checks before casting to avoid potential NullReferenceExceptions.
| private SyntaxNode ConvertAssertMultipleSimpleLambda(ExpressionStatementSyntax originalStatement, SimpleLambdaExpressionSyntax lambda) | ||
| { | ||
| SyntaxList<StatementSyntax> statements; | ||
| if (lambda.Body is BlockSyntax block) | ||
| { | ||
| var convertedStatements = block.Statements.Select(s => (StatementSyntax)Visit(s)!).ToArray(); | ||
| statements = SyntaxFactory.List(convertedStatements); | ||
| } | ||
| else if (lambda.Body is ExpressionSyntax expr) | ||
| { | ||
| var visitedExpr = (ExpressionSyntax)Visit(expr)!; | ||
| statements = SyntaxFactory.SingletonList<StatementSyntax>( | ||
| SyntaxFactory.ExpressionStatement(visitedExpr)); | ||
| } | ||
| else | ||
| { | ||
| return originalStatement; | ||
| } | ||
|
|
||
| return CreateUsingMultipleStatement(originalStatement, statements); | ||
| } |
There was a problem hiding this comment.
The ConvertAssertMultipleSimpleLambda and ConvertAssertMultipleLambda methods contain identical implementation logic. Consider extracting the common code into a single helper method that both can call, passing the lambda body as a parameter. This would reduce code duplication and improve maintainability.
| // Handle Has.Exactly(n).Items -> Count().IsEqualTo(n) | ||
| // Pattern: constraint.Name is "Items", constraint.Expression is Has.Exactly(n) invocation | ||
| if (memberName == "Items" && | ||
| constraint.Expression is InvocationExpressionSyntax exactlyInvocation && | ||
| exactlyInvocation.Expression is MemberAccessExpressionSyntax exactlyMemberAccess && | ||
| exactlyMemberAccess.Expression is IdentifierNameSyntax { Identifier.Text: "Has" } && | ||
| exactlyMemberAccess.Name.Identifier.Text == "Exactly" && | ||
| exactlyInvocation.ArgumentList.Arguments.Count > 0) | ||
| { | ||
| // Extract the count argument from Has.Exactly(n) | ||
| var countArg = exactlyInvocation.ArgumentList.Arguments[0]; | ||
| return CreateCountAssertion(actualValue, "IsEqualTo", message, countArg); | ||
| } |
There was a problem hiding this comment.
This code block for handling Has.Exactly(n).Items is duplicated in the ConvertConstraintMemberToTUnit method (lines 729-741). Consider extracting this pattern matching logic into a shared helper method to reduce code duplication and improve maintainability.
| // Handle Has.Count.EqualTo(n) -> Count().IsEqualTo(n) | ||
| // Pattern: Has.Count is a MemberAccess, then .EqualTo(n) is invoked on it | ||
| if (memberAccess.Expression is MemberAccessExpressionSyntax hasCountAccess && | ||
| hasCountAccess.Expression is IdentifierNameSyntax { Identifier.Text: "Has" } && | ||
| hasCountAccess.Name.Identifier.Text == "Count") | ||
| { | ||
| return methodName switch | ||
| { | ||
| "EqualTo" => CreateCountAssertion(actualValue, "IsEqualTo", message, constraint.ArgumentList.Arguments.ToArray()), | ||
| "GreaterThan" => CreateCountAssertion(actualValue, "IsGreaterThan", message, constraint.ArgumentList.Arguments.ToArray()), | ||
| "LessThan" => CreateCountAssertion(actualValue, "IsLessThan", message, constraint.ArgumentList.Arguments.ToArray()), | ||
| "GreaterThanOrEqualTo" => CreateCountAssertion(actualValue, "IsGreaterThanOrEqualTo", message, constraint.ArgumentList.Arguments.ToArray()), | ||
| "LessThanOrEqualTo" => CreateCountAssertion(actualValue, "IsLessThanOrEqualTo", message, constraint.ArgumentList.Arguments.ToArray()), | ||
| _ => CreateCountAssertion(actualValue, "IsEqualTo", message, constraint.ArgumentList.Arguments.ToArray()) | ||
| }; | ||
| } |
There was a problem hiding this comment.
The Has.Count.EqualTo(n) pattern handling is implemented in ConvertConstraintToTUnitWithMessage (lines 480-495) but is missing in the ConvertConstraintToTUnit method (line 589+). While ConvertConstraintToTUnit is primarily used for handling chained constraints like .Within(), it should still handle Has.Count patterns consistently for completeness and to avoid potential bugs if the code is refactored in the future.
| var convertedStatements = block.Statements.Select(s => (StatementSyntax)Visit(s)!).ToArray(); | ||
| statements = SyntaxFactory.List(convertedStatements); | ||
| } | ||
| else if (lambda.Body is ExpressionSyntax expr) | ||
| { | ||
| // Single expression lambda - convert it | ||
| var visitedExpr = (ExpressionSyntax)Visit(expr)!; |
There was a problem hiding this comment.
The Visit method is being called with null-forgiving operators (!) on lines 266, 272, 289, and 294, but CSharpSyntaxRewriter.Visit can return null for certain node types. While in this specific case the Visit calls are likely safe (they're visiting statements/expressions that will be converted), it would be more defensive to add null checks before casting to avoid potential NullReferenceExceptions if the Visit method returns null in unexpected cases.
| var convertedStatements = block.Statements.Select(s => (StatementSyntax)Visit(s)!).ToArray(); | |
| statements = SyntaxFactory.List(convertedStatements); | |
| } | |
| else if (lambda.Body is ExpressionSyntax expr) | |
| { | |
| // Single expression lambda - convert it | |
| var visitedExpr = (ExpressionSyntax)Visit(expr)!; | |
| var convertedStatements = block.Statements | |
| .Select(s => (StatementSyntax)(Visit(s) ?? s)) | |
| .ToArray(); | |
| statements = SyntaxFactory.List(convertedStatements); | |
| } | |
| else if (lambda.Body is ExpressionSyntax expr) | |
| { | |
| // Single expression lambda - convert it | |
| var visitedExpr = (ExpressionSyntax)(Visit(expr) ?? expr); |
| else if (lambda.Body is ExpressionSyntax expr) | ||
| { | ||
| // Single expression lambda - convert it | ||
| var visitedExpr = (ExpressionSyntax)Visit(expr)!; |
There was a problem hiding this comment.
Similar to line 266, this Visit call uses a null-forgiving operator but could potentially return null. Consider adding null checks before casting.
Summary
Has.Count.EqualTo(n)→Count().IsEqualTo(n)conversionHas.Exactly(n).Items→Count().IsEqualTo(n)conversionIs.Not.Zeroto useIsNotZero()instead ofIsNotEqualTo(0)Assert.Multiple(() => { ... })→using (Assert.Multiple()) { ... }conversionFixes #4191
Test plan
NUnit_HasCount_Converted- verifies Has.Count.EqualTo patternNUnit_HasExactlyItems_Converted- verifies Has.Exactly(n).Items patternNUnit_IsNotZero_Converted- verifies Is.Not.Zero patternNUnit_AssertMultiple_Lambda_Converted- verifies Assert.Multiple lambda conversion🤖 Generated with Claude Code