-
Notifications
You must be signed in to change notification settings - Fork 5
Add rule to explicitly pick MockBehavior #226
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
Changes from all commits
82ac176
e4262de
12d7d8b
b9a4556
4992005
1c0ade3
4f6a565
4f8b41f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| # Moq1400: Explicitly choose a mocking behavior instead of relying on the default (Loose) behavior | ||
|
|
||
| | Item | Value | | ||
| | --- | --- | | ||
| | Enabled | True | | ||
| | Severity | Warning | | ||
| | CodeFix | False | | ||
| --- | ||
|
|
||
| Mocks use the `MockBehavior.Loose` by default. Some people find this default behavior undesirable, as it can lead to | ||
|
rjmurillo marked this conversation as resolved.
|
||
| unexpected behavior if the mock is improperly set up. To fix, specify either `MockBehavior.Loose` or | ||
| `MockBehavior.Strict` to signify acknowledgement of the mock's behavior. | ||
|
rjmurillo marked this conversation as resolved.
|
||
|
|
||
| ## Examples of patterns that are flagged by this analyzer | ||
|
|
||
| ```csharp | ||
| interface ISample | ||
| { | ||
| int Calculate() => 0; | ||
| } | ||
|
|
||
| var mock = new Mock<ISample>(); // Moq1400: Moq: Explicitly choose a mock behavior | ||
| var mock2 = Mock.Of<ISample>(); // Moq1400: Moq: Explicitly choose a mock behavior | ||
| ``` | ||
|
|
||
| ```csharp | ||
| interface ISample | ||
| { | ||
| int Calculate() => 0; | ||
| } | ||
|
|
||
| var mock = new Mock<ISample>(MockBehavior.Default); // Moq1400: Moq: Explicitly choose a mock behavior | ||
| var mock2 = Mock.Of<ISample>(MockBehavior.Default); // Moq1400: Moq: Explicitly choose a mock behavior | ||
| var repo = new MockRepository(MockBehavior.Default); // Moq1400: Moq: Explicitly choose a mock behavior | ||
| ``` | ||
|
rjmurillo marked this conversation as resolved.
|
||
|
|
||
| ## Solution | ||
|
|
||
| ```csharp | ||
| interface ISample | ||
| { | ||
| int Calculate() => 0; | ||
| } | ||
|
|
||
| var mock = new Mock<ISample>(MockBehavior.Strict); // Or `MockBehavior.Loose` | ||
| var mock2 = new Mock.Of<ISample>(MockBehavior.Strict); // Or `MockBehavior.Loose` | ||
| var repo = new MockRepository(MockBehavior.Strict); // Or `MockBehavior.Loose` | ||
| ``` | ||
|
rjmurillo marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,8 @@ | ||
| ; Unshipped analyzer release | ||
| ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md | ||
|
|
||
| ### New Rules | ||
|
rjmurillo marked this conversation as resolved.
|
||
|
|
||
| Rule ID | Category | Severity | Notes | ||
| --------|----------|----------|------- | ||
| Moq1400 | Moq | Warning | SetExplicitMockBehaviorAnalyzer, [Documentation](https://github.com/rjmurillo/moq.analyzers/blob/main/docs/rules/Moq1400.md) | ||
|
MattKotsenas marked this conversation as resolved.
MattKotsenas marked this conversation as resolved.
MattKotsenas marked this conversation as resolved.
rjmurillo marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,136 @@ | ||||||||||||||||||||||||||||||||
| using Microsoft.CodeAnalysis.Operations; | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
rjmurillo marked this conversation as resolved.
rjmurillo marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| namespace Moq.Analyzers; | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||
| /// Mock should explicitly specify a behavior and not rely on the default. | ||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||||||||||||||||||||||||||||||||
| public class SetExplicitMockBehaviorAnalyzer : DiagnosticAnalyzer | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| private static readonly LocalizableString Title = "Moq: Explicitly choose a mock behavior"; | ||||||||||||||||||||||||||||||||
| private static readonly LocalizableString Message = "Explicitly choose a mocking behavior instead of relying on the default (Loose) behavior"; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private static readonly DiagnosticDescriptor Rule = new( | ||||||||||||||||||||||||||||||||
| DiagnosticIds.SetExplicitMockBehavior, | ||||||||||||||||||||||||||||||||
| Title, | ||||||||||||||||||||||||||||||||
| Message, | ||||||||||||||||||||||||||||||||
| DiagnosticCategory.Moq, | ||||||||||||||||||||||||||||||||
| DiagnosticSeverity.Warning, | ||||||||||||||||||||||||||||||||
| isEnabledByDefault: true, | ||||||||||||||||||||||||||||||||
| helpLinkUri: $"https://github.com/rjmurillo/moq.analyzers/blob/{ThisAssembly.GitCommitId}/docs/rules/{DiagnosticIds.SetExplicitMockBehavior}.md"); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /// <inheritdoc /> | ||||||||||||||||||||||||||||||||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /// <inheritdoc /> | ||||||||||||||||||||||||||||||||
| public override void Initialize(AnalysisContext context) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||||||||||||||||||||||||||||||||
| context.EnableConcurrentExecution(); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| context.RegisterCompilationStartAction(RegisterCompilationStartAction); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private static void RegisterCompilationStartAction(CompilationStartAnalysisContext context) | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| // Ensure Moq is referenced in the compilation | ||||||||||||||||||||||||||||||||
| ImmutableArray<INamedTypeSymbol> mockTypes = context.Compilation.GetMoqMock(); | ||||||||||||||||||||||||||||||||
| if (mockTypes.IsEmpty) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Look for the MockBehavior type and provide it to Analyze to avoid looking it up multiple times. | ||||||||||||||||||||||||||||||||
| INamedTypeSymbol? mockBehaviorSymbol = context.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MoqBehavior); | ||||||||||||||||||||||||||||||||
| if (mockBehaviorSymbol is null) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Look for the Mock.Of() method and provide it to Analyze to avoid looking it up multiple times. | ||||||||||||||||||||||||||||||||
| #pragma warning disable ECS0900 // Minimize boxing and unboxing | ||||||||||||||||||||||||||||||||
| ImmutableArray<IMethodSymbol> ofMethods = mockTypes | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| .SelectMany(mockType => mockType.GetMembers(WellKnownTypeNames.Of)) | ||||||||||||||||||||||||||||||||
| .OfType<IMethodSymbol>() | ||||||||||||||||||||||||||||||||
| .Where(method => method.IsGenericMethod) | ||||||||||||||||||||||||||||||||
| .ToImmutableArray(); | ||||||||||||||||||||||||||||||||
| #pragma warning restore ECS0900 // Minimize boxing and unboxing | ||||||||||||||||||||||||||||||||
|
Comment on lines
+52
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Clarify Suppression of 'ECS0900' Warning The suppression of the Apply this diff to add an explanatory comment: #pragma warning disable ECS0900 // Minimize boxing and unboxing
+// Suppressing this warning because the LINQ query does not significantly impact performance in this context.
ImmutableArray<IMethodSymbol> ofMethods = mockTypes📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| context.RegisterOperationAction( | ||||||||||||||||||||||||||||||||
| context => AnalyzeNewObject(context, mockTypes, mockBehaviorSymbol), | ||||||||||||||||||||||||||||||||
| OperationKind.ObjectCreation); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (!ofMethods.IsEmpty) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| context.RegisterOperationAction( | ||||||||||||||||||||||||||||||||
| context => AnalyzeInvocation(context, ofMethods, mockBehaviorSymbol), | ||||||||||||||||||||||||||||||||
| OperationKind.Invocation); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private static void AnalyzeNewObject(OperationAnalysisContext context, ImmutableArray<INamedTypeSymbol> mockTypes, INamedTypeSymbol mockBehaviorSymbol) | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (context.Operation is not IObjectCreationOperation creationOperation) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (creationOperation.Type is not INamedTypeSymbol namedType) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (!namedType.IsInstanceOf(mockTypes)) | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| foreach (IArgumentOperation argument in creationOperation.Arguments) | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (argument.Value is IFieldReferenceOperation fieldReferenceOperation) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| ISymbol field = fieldReferenceOperation.Member; | ||||||||||||||||||||||||||||||||
| if (field.ContainingType.IsInstanceOf(mockBehaviorSymbol) && IsExplicitBehavior(field.Name)) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+89
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle Explicit Behaviors Specified via Variables or Expressions The current implementation checks for mock behaviors specified directly as field references. Consider extending the analysis to handle scenarios where the mock behavior is provided via variables, method calls, or expressions to avoid false positives. For example, if a mock behavior is assigned to a variable and then passed as an argument, the analyzer may incorrectly report a diagnostic. Enhancing the logic to perform a more comprehensive analysis of the argument values can improve accuracy. |
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| context.ReportDiagnostic(creationOperation.Syntax.GetLocation().CreateDiagnostic(Rule)); | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Provide More Specific Diagnostic Location When reporting the diagnostic, consider pinpointing the location of the default mock behavior argument rather than the entire object creation syntax. This will help developers identify the exact issue more efficiently. Apply this diff to report the diagnostic at the specific argument location: -context.ReportDiagnostic(creationOperation.Syntax.GetLocation().CreateDiagnostic(Rule));
+if (creationOperation.Arguments.Length > 0)
+{
+ context.ReportDiagnostic(creationOperation.Arguments[0].Syntax.GetLocation().CreateDiagnostic(Rule));
+}
+else
+{
+ context.ReportDiagnostic(creationOperation.Syntax.GetLocation().CreateDiagnostic(Rule));
+}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private static void AnalyzeInvocation(OperationAnalysisContext context, ImmutableArray<IMethodSymbol> wellKnownOfMethods, INamedTypeSymbol mockBehaviorSymbol) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (context.Operation is not IInvocationOperation invocationOperation) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| IMethodSymbol targetMethod = invocationOperation.TargetMethod; | ||||||||||||||||||||||||||||||||
| if (!targetMethod.IsInstanceOf(wellKnownOfMethods)) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| foreach (IArgumentOperation argument in invocationOperation.Arguments) | ||||||||||||||||||||||||||||||||
|
MattKotsenas marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (argument.Value is IFieldReferenceOperation fieldReferenceOperation) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| ISymbol field = fieldReferenceOperation.Member; | ||||||||||||||||||||||||||||||||
| if (field.ContainingType.IsInstanceOf(mockBehaviorSymbol) && IsExplicitBehavior(field.Name)) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+117
to
+126
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Unified Argument Analysis across Methods The argument analysis logic in Extract the argument analysis into a helper method: private static bool HasExplicitBehavior(ImmutableArray<IArgumentOperation> arguments, INamedTypeSymbol mockBehaviorSymbol)
{
foreach (IArgumentOperation argument in arguments)
{
if (argument.Value is IFieldReferenceOperation fieldReferenceOperation)
{
ISymbol field = fieldReferenceOperation.Member;
if (field.ContainingType.IsInstanceOf(mockBehaviorSymbol) && IsExplicitBehavior(field.Name))
{
return true;
}
}
}
return false;
}Then update the methods: // In AnalyzeNewObject
-if (/* argument analysis logic */)
+if (HasExplicitBehavior(creationOperation.Arguments, mockBehaviorSymbol))
// In AnalyzeInvocation
-if (/* argument analysis logic */)
+if (HasExplicitBehavior(invocationOperation.Arguments, mockBehaviorSymbol)) |
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| context.ReportDiagnostic(invocationOperation.Syntax.GetLocation().CreateDiagnostic(Rule)); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private static bool IsExplicitBehavior(string symbolName) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| return string.Equals(symbolName, "Loose", StringComparison.Ordinal) || string.Equals(symbolName, "Strict", StringComparison.Ordinal); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| using Verifier = Moq.Analyzers.Test.Helpers.AnalyzerVerifier<Moq.Analyzers.SetExplicitMockBehaviorAnalyzer>; | ||
|
|
||
| namespace Moq.Analyzers.Test; | ||
|
|
||
| public class SetExplicitMockBehaviorAnalyzerTests | ||
| { | ||
| public static IEnumerable<object[]> TestData() | ||
| { | ||
| IEnumerable<object[]> mockConstructors = new object[][] | ||
| { | ||
| ["""{|Moq1400:new Mock<ISample>()|};"""], | ||
| ["""{|Moq1400:new Mock<ISample>(MockBehavior.Default)|};"""], | ||
| ["""new Mock<ISample>(MockBehavior.Loose);"""], | ||
| ["""new Mock<ISample>(MockBehavior.Strict);"""], | ||
| }.WithNamespaces().WithMoqReferenceAssemblyGroups(); | ||
|
|
||
| IEnumerable<object[]> fluentBuilders = new object[][] | ||
| { | ||
| ["""{|Moq1400:Mock.Of<ISample>()|};"""], | ||
| ["""{|Moq1400:Mock.Of<ISample>(MockBehavior.Default)|};"""], | ||
| ["""Mock.Of<ISample>(MockBehavior.Loose);"""], | ||
| ["""Mock.Of<ISample>(MockBehavior.Strict);"""], | ||
| }.WithNamespaces().WithNewMoqReferenceAssemblyGroups(); | ||
|
|
||
| IEnumerable<object[]> mockRepositories = new object[][] | ||
| { | ||
| ["""{|Moq1400:new MockRepository(MockBehavior.Default)|};"""], | ||
| ["""new MockRepository(MockBehavior.Loose);"""], | ||
| ["""new MockRepository(MockBehavior.Strict);"""], | ||
| }.WithNamespaces().WithNewMoqReferenceAssemblyGroups(); | ||
|
|
||
| return mockConstructors.Union(fluentBuilders).Union(mockRepositories); | ||
| } | ||
|
rjmurillo marked this conversation as resolved.
|
||
|
|
||
| [Theory] | ||
| [MemberData(nameof(TestData))] | ||
| public async Task ShouldAnalyzeMocksWithoutExplictMockBehavior(string referenceAssemblyGroup, string @namespace, string mock) | ||
| { | ||
| await Verifier.VerifyAnalyzerAsync( | ||
| $$""" | ||
| {{@namespace}} | ||
|
|
||
| public interface ISample | ||
| { | ||
| int Calculate(int a, int b); | ||
| } | ||
|
|
||
| internal class UnitTest | ||
| { | ||
| private void Test() | ||
| { | ||
| {{mock}} | ||
| } | ||
| } | ||
| """, | ||
| referenceAssemblyGroup); | ||
| } | ||
|
rjmurillo marked this conversation as resolved.
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.