-
Notifications
You must be signed in to change notification settings - Fork 289
Analyzer implementation to enhance assertions #6720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Analyzer implementation to enhance assertions #6720
Conversation
…Null and Asset.IsNotNull using Single, SingleOrDefaut, WhereSingle and WhereSingleOrDefualt
|
@Youssef1313 @Evangelink Pls I need this pull request to be reviewed.. I don't really know how to request a review directly to a maintainer If they are not already part of the pull request reviewers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the MSTest analyzer to suggest cleaner assertion patterns when testing collection contents with predicates. The analyzer now detects patterns like Assert.IsTrue(collection.Any(predicate)) and suggests the more expressive Assert.Contains(predicate, collection).
Key Changes
- Added detection for LINQ predicate patterns (
Any,Count,Single,SingleOrDefault,Wherecombinations) - Introduced
Assert.ContainsandAssert.DoesNotContainsuggestions with predicate support - Introduced
Assert.ContainsSinglesuggestions for single-element checks - Added comprehensive test coverage for new patterns
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Analyzers/MSTest.Analyzers/UseProperAssertMethodsAnalyzer.cs | Added LinqPredicateCheckStatus enum, RecognizeLinqPredicateCheck method, and logic to detect and suggest predicate-based assertions for IsNull/IsNotNull, IsTrue/IsFalse, and AreEqual patterns |
| test/UnitTests/MSTest.Analyzers.UnitTests/UseProperAssertMethodsAnalyzerTests.cs | Added 14 new test methods covering various LINQ predicate patterns with Any, Where, Count, Single, and SingleOrDefault combinations |
| test/IntegrationTests/MSTest.IntegrationTests/OutputTests.cs | Updated assertion to demonstrate new Assert.DoesNotContain usage (contains a bug - see comments) |
| // Check for enumerable.Where(predicate).Any() | ||
| if (operation is IInvocationOperation whereAnyInvocation && | ||
| whereAnyInvocation.TargetMethod.Name == "Any" && | ||
| whereAnyInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereAnyInvocation.Arguments.Length == 1 && | ||
| whereAnyInvocation.Arguments[0].Value is IInvocationOperation whereInvocation && | ||
| whereInvocation.TargetMethod.Name == "Where" && | ||
| whereInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereInvocation.Arguments.Length == 2) | ||
| { | ||
| collectionExpression = whereInvocation.Arguments[0].Value.Syntax; | ||
| predicateExpression = whereInvocation.Arguments[1].Value.Syntax; | ||
| return LinqPredicateCheckStatus.WhereAny; | ||
| } | ||
|
|
||
| // Check for enumerable.Where(predicate).Count() | ||
| if (operation is IInvocationOperation whereCountInvocation && | ||
| whereCountInvocation.TargetMethod.Name == "Count" && | ||
| whereCountInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereCountInvocation.Arguments.Length == 1 && | ||
| whereCountInvocation.Arguments[0].Value is IInvocationOperation whereInvocation2 && | ||
| whereInvocation2.TargetMethod.Name == "Where" && | ||
| whereInvocation2.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereInvocation2.Arguments.Length == 2) | ||
| { | ||
| collectionExpression = whereInvocation2.Arguments[0].Value.Syntax; | ||
| predicateExpression = whereInvocation2.Arguments[1].Value.Syntax; | ||
| countOperation = operation; | ||
| return LinqPredicateCheckStatus.WhereCount; | ||
| } | ||
|
|
||
| // Check for enumerable.Where(predicate).Single() | ||
| if (operation is IInvocationOperation whereSingleInvocation && | ||
| whereSingleInvocation.TargetMethod.Name == "Single" && | ||
| whereSingleInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereSingleInvocation.Arguments.Length == 1 && | ||
| whereSingleInvocation.Arguments[0].Value is IInvocationOperation whereInvocation3 && | ||
| whereInvocation3.TargetMethod.Name == "Where" && | ||
| whereInvocation3.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereInvocation3.Arguments.Length == 2) | ||
| { | ||
| collectionExpression = whereInvocation3.Arguments[0].Value.Syntax; | ||
| predicateExpression = whereInvocation3.Arguments[1].Value.Syntax; | ||
| return LinqPredicateCheckStatus.WhereSingle; | ||
| } | ||
|
|
||
| // Check for enumerable.Where(predicate).SingleOrDefault() | ||
| if (operation is IInvocationOperation whereSingleOrDefaultInvocation && | ||
| whereSingleOrDefaultInvocation.TargetMethod.Name == "SingleOrDefault" && | ||
| whereSingleOrDefaultInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereSingleOrDefaultInvocation.Arguments.Length == 1 && | ||
| whereSingleOrDefaultInvocation.Arguments[0].Value is IInvocationOperation whereInvocation4 && | ||
| whereInvocation4.TargetMethod.Name == "Where" && | ||
| whereInvocation4.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" && | ||
| whereInvocation4.Arguments.Length == 2) | ||
| { | ||
| collectionExpression = whereInvocation4.Arguments[0].Value.Syntax; | ||
| predicateExpression = whereInvocation4.Arguments[1].Value.Syntax; | ||
| return LinqPredicateCheckStatus.WhereSingleOrDefault; | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The analyzer implementation has significant code duplication in the RecognizeLinqPredicateCheck method. The patterns for checking Where().Single(), Where().SingleOrDefault(), Where().Any(), and Where().Count() follow nearly identical logic with only method names differing. Consider extracting this repetitive logic into a helper method to improve maintainability.
Example refactoring approach:
private static bool TryMatchWherePattern(
IInvocationOperation invocation,
string methodName,
out SyntaxNode? collectionExpression,
out SyntaxNode? predicateExpression)
{
if (invocation.TargetMethod.Name == methodName &&
invocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" &&
invocation.Arguments.Length == 1 &&
invocation.Arguments[0].Value is IInvocationOperation whereInvocation &&
whereInvocation.TargetMethod.Name == "Where" &&
whereInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable" &&
whereInvocation.Arguments.Length == 2)
{
collectionExpression = whereInvocation.Arguments[0].Value.Syntax;
predicateExpression = whereInvocation.Arguments[1].Value.Syntax;
return true;
}
collectionExpression = null;
predicateExpression = null;
return false;
}| if (linqStatus is LinqPredicateCheckStatus.Single or | ||
| LinqPredicateCheckStatus.SingleOrDefault or | ||
| LinqPredicateCheckStatus.WhereSingle or | ||
| LinqPredicateCheckStatus.WhereSingleOrDefault && |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operator precedence issue: The condition on line 308 is missing parentheses around the && operation, making the intent ambiguous. The current condition evaluates as:
(linqStatus is ... or ... or ... or ...) && (linqCollectionExpr != null)
However, based on the logic, it appears the intent is:
linqStatus is (...WhereSingle or WhereSingleOrDefault) && linqCollectionExpr != null
Add parentheses to clarify the intended precedence:
if ((linqStatus is LinqPredicateCheckStatus.Single or
LinqPredicateCheckStatus.SingleOrDefault or
LinqPredicateCheckStatus.WhereSingle or
LinqPredicateCheckStatus.WhereSingleOrDefault) &&
linqCollectionExpr != null)| if (linqStatus is LinqPredicateCheckStatus.Single or | |
| LinqPredicateCheckStatus.SingleOrDefault or | |
| LinqPredicateCheckStatus.WhereSingle or | |
| LinqPredicateCheckStatus.WhereSingleOrDefault && | |
| if ((linqStatus is LinqPredicateCheckStatus.Single or | |
| LinqPredicateCheckStatus.SingleOrDefault or | |
| LinqPredicateCheckStatus.WhereSingle or | |
| LinqPredicateCheckStatus.WhereSingleOrDefault) && |
| Assert.Contains("TestCleanup", message.Text); | ||
| Assert.IsFalse(shouldNotContain.Any(message.Text.Contains)); | ||
| // Assert.IsFalse(shouldNotContain.Any(message.Text.Contains)); | ||
| Assert.DoesNotContain(message.Text.Contains, shouldNotContain); |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change appears incorrect. The original code Assert.IsFalse(shouldNotContain.Any(message.Text.Contains)) is checking that none of the strings in shouldNotContain are contained in message.Text.
The new code Assert.DoesNotContain(message.Text.Contains, shouldNotContain) reverses the logic: it's checking whether the method message.Text.Contains (as a delegate) exists in the shouldNotContain collection, which is a string array. This will always pass (or fail to compile) since it's comparing a method delegate to string values.
The correct assertion should remain as the original, or if you want to use Assert.DoesNotContain, it should be written as:
foreach (string item in shouldNotContain)
{
Assert.DoesNotContain(item, message.Text);
}| Assert.DoesNotContain(message.Text.Contains, shouldNotContain); | |
| foreach (string item in shouldNotContain) | |
| { | |
| Assert.DoesNotContain(item, message.Text); | |
| } |
| // Special-case: enumerable.Count(predicate) > 0 → Assert.Contains(predicate, enumerable) | ||
| if (conditionArgument is IBinaryOperation binaryOp && | ||
| binaryOp.OperatorKind == BinaryOperatorKind.GreaterThan) | ||
| { | ||
| if (binaryOp.LeftOperand is IInvocationOperation countInvocation && | ||
| binaryOp.RightOperand.ConstantValue.HasValue && | ||
| binaryOp.RightOperand.ConstantValue.Value is int intValue && | ||
| intValue == 0 && | ||
| countInvocation.TargetMethod.Name == "Count") | ||
| { | ||
| SyntaxNode? countCollectionExpr = null; | ||
| SyntaxNode? countPredicateExpr = null; | ||
|
|
||
| if (countInvocation.Instance != null && countInvocation.Arguments.Length == 1) | ||
| { | ||
| countCollectionExpr = countInvocation.Instance.Syntax; | ||
| countPredicateExpr = countInvocation.Arguments[0].Value.Syntax; | ||
| } | ||
| else if (countInvocation.Instance == null && countInvocation.Arguments.Length == 2) | ||
| { | ||
| countCollectionExpr = countInvocation.Arguments[0].Value.Syntax; | ||
| countPredicateExpr = countInvocation.Arguments[1].Value.Syntax; | ||
| } | ||
|
|
||
| if (countCollectionExpr != null && countPredicateExpr != null) | ||
| { | ||
| string properAssertMethod = isTrueInvocation ? "Contains" : "DoesNotContain"; | ||
|
|
||
| ImmutableDictionary<string, string?>.Builder properties = ImmutableDictionary.CreateBuilder<string, string?>(); | ||
| properties.Add(ProperAssertMethodNameKey, properAssertMethod); | ||
| properties.Add(CodeFixModeKey, CodeFixModeAddArgument); | ||
|
|
||
| context.ReportDiagnostic( | ||
| context.Operation.CreateDiagnostic( | ||
| Rule, | ||
| additionalLocations: ImmutableArray.Create( | ||
| conditionArgument.Syntax.GetLocation(), | ||
| countPredicateExpr.GetLocation(), | ||
| countCollectionExpr.GetLocation()), | ||
| properties: properties.ToImmutable(), | ||
| properAssertMethod, | ||
| isTrueInvocation ? "IsTrue" : "IsFalse")); | ||
|
|
||
| return; | ||
| } | ||
| } | ||
| } |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Count pattern checking (lines 860-906) doesn't validate that the Count method is actually from LINQ (System.Linq.Enumerable) or has a predicate. This could lead to false positives with other Count methods (e.g., string.Count from LINQ, or custom Count methods on collections).
The check should verify:
- That it's the LINQ
Countextension method with a predicate - That it's from
System.Linq.Enumerable
Example:
if (countInvocation.TargetMethod.Name == "Count" &&
countInvocation.TargetMethod.ContainingType?.ToDisplayString() == "System.Linq.Enumerable")This is especially important since RecognizeLinqPredicateCheck already properly checks for this, so this code should either reuse that method or apply similar validation.
This Pull request fixes #6360
This implementation involves changes to the Analyzer to suggest a much cleaner approach to write Assertions that involves the use of Predicate functions. for example
Analyzer will suggest (Expected Behaviour)
Assert.Contains(x => x == 1, _enumerable);Example 2:
Analyzer will suggest (Expected Behaviour)
Assert.DoesNotContain(x => x == 1, _enumerable);