NUnit2055 should not fire on ClassicAssert#955
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #953 by preventing the NUnit2055 analyzer (InstanceOf) from firing on ClassicAssert calls. The key changes refactor constraint expression handling to properly distinguish between Assert.That calls (which should be analyzed) and ClassicAssert calls (which should not).
Key Changes:
- Extracted constraint expression evaluation logic into reusable extension methods
- Modified
AssertHelper.TryGetActualAndConstraintOperationsto handle assertions with omitted constraints (likeAssert.That(condition)) - Updated analyzers to use centralized constraint evaluation instead of duplicated logic
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ConstraintExpressionPartExtensions.cs |
Adds IsTrueOrFalse() method to evaluate if a constraint part represents true/false, considering negation |
ConstraintExpressionExtensions.cs |
Adds IsTrueOrFalse() method to evaluate constraint expressions, handling empty constraint lists |
ConstraintExpression.cs |
Makes expression operation nullable and handles null cases for assertions without explicit constraints |
InstanceOfAnalyzer.cs |
Refactors to use centralized constraint helper and removes duplicate constraint evaluation logic |
AssertHelper.cs |
Updates to support assertions with omitted constraints and single-argument Assert.That calls |
BaseConditionConstraintAnalyzer.cs |
Refactors constraint evaluation to use centralized helper, properly handling both modern and classic assertions |
InstanceOfCodeFixTests.cs |
Adds test case for Is.Not.True constraint pattern |
InstanceOfAnalyzerTests.cs |
Adds test for Is.Not.False and verifies ClassicAssert is not analyzed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mikkelbu
left a comment
There was a problem hiding this comment.
Looks good. I only had one comment which is a little unrelated to this PR, so I'm also happy to merge this as it is
|
|
||
| public static bool? IsTrueOrFalse(this ConstraintExpressionPart constraintExpressionPart) | ||
| { | ||
| bool not = constraintExpressionPart.GetPrefix(NUnitFrameworkConstants.NameOfIsNot) is not null; |
There was a problem hiding this comment.
Should we rather count the number of Nots ? to handle people who have "odd" assert like
var value2 = 42;
Assert.That(value2 > 40, Is.Not.Not.True);
or is this too much work ?
There was a problem hiding this comment.
Done. I thought it was far fetched to think about Is.Not.False, but Is.Not.Not.False is simply confusing.
I wish NUnit would not allow .Not.Not and as long as it does we might want to think about a rule to says that .Not.Not is the same as nothing.
Also exclude classic assert.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
17878f4 to
03f271d
Compare
Fixes #953