-
Notifications
You must be signed in to change notification settings - Fork 290
Add analyzer for OSCondition (MSTEST0059) #7015
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?
Conversation
Co-authored-by: Evangelink <[email protected]>
src/Analyzers/MSTest.Analyzers/UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzer.cs
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers/UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers/UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzer.cs
Show resolved
Hide resolved
...ests/MSTest.Analyzers.UnitTests/UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzerTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Evangelink <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: Evangelink <[email protected]>
src/Analyzers/MSTest.Analyzers/UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzer.cs
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers/UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzer.cs
Show resolved
Hide resolved
Evangelink
left a comment
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.
@copilot let's add some test cases with trivias to see how the code fix reacts. For example
// Skip on windows
if (RuntimeInformation.IsOSPLatform(OSPlatform.Windows)or with trivias after the if block
Co-authored-by: Evangelink <[email protected]>
Added test cases with trivias in cbd04fa:
|
src/Analyzers/MSTest.Analyzers.CodeFixes/UseOSConditionAttributeInsteadOfRuntimeCheckFixer.cs
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers.CodeFixes/UseOSConditionAttributeInsteadOfRuntimeCheckFixer.cs
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers.CodeFixes/UseOSConditionAttributeInsteadOfRuntimeCheckFixer.cs
Show resolved
Hide resolved
| return; | ||
| } | ||
|
|
||
| IMethodSymbol? isOSPlatformMethod = runtimeInformationSymbol.GetMembers("IsOSPlatform") |
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.
Do we also want to handle OperatingSystem.Is<OS> methods?
| blockContext.RegisterOperationAction( | ||
| operationContext => AnalyzeIfStatement(operationContext, isOSPlatformMethod, assertSymbol), | ||
| OperationKind.Conditional); |
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 will analyze conditions that appear anywhere in the method body. But if I have a test method like the following:
[TestMethod]
public void MyTestMethod()
{
// arrange
// some assertions
// if windows, return
// some other assertions that are non-Windows only.
}This shouldn't get flagged.
|
|
||
| // Check if it's an invocation of RuntimeInformation.IsOSPlatform | ||
| if (actualCondition is not IInvocationOperation invocation || | ||
| !SymbolEqualityComparer.Default.Equals(invocation.TargetMethod.OriginalDefinition, isOSPlatformMethod)) |
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.
Is accessing OriginalDefinition really needed here?
| foreach (IOperation operation in blockOperation.Operations) | ||
| { | ||
| if (IsReturnOrAssertInconclusive(operation, assertSymbol)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; |
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.
Is this correct? Shouldn't we only look at the first operation?
| } | ||
|
|
||
| // Find the if statement to remove | ||
| IfStatementSyntax? ifStatement = diagnosticNode.FirstAncestorOrSelf<IfStatementSyntax>(); |
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 diagnostic node here is what? Shouldn't it be the IfStatementSyntax in question already? Why do we need FirstAncestorOrSelf?
Adds a new code analyzer
MSTEST0059that detects usage ofRuntimeInformation.IsOSPlatformwith early return orAssert.Inconclusivein test methods and suggests using the[OSCondition]attribute instead.Fixes #7011
Changes Made
Analyzer: Created
UseOSConditionAttributeInsteadOfRuntimeCheckAnalyzerthat detects:if (RuntimeInformation.IsOSPlatform(...)) { return; }patterns (suggestsConditionMode.Exclude)if (!RuntimeInformation.IsOSPlatform(...)) { return; }patterns (suggestsConditionMode.Include)Assert.Inconclusivecalls within the if blocksCode Fixer: Created
UseOSConditionAttributeInsteadOfRuntimeCheckFixerthat:[OSCondition]attribute with the appropriateConditionModeandOperatingSystemsvalueInfrastructure:
MSTEST0059in DiagnosticIds.csTests: Comprehensive unit tests covering:
Assert.InconclusivepatternsOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.