-
Notifications
You must be signed in to change notification settings - Fork 36
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Introduce
MockitoMockClassReference
check (#454)
Flags Mockito mock and spy creation expressions that explicitly specify the type of mock or spy to create, while this information can also be inferred from context.
- Loading branch information
Showing
3 changed files
with
231 additions
and
5 deletions.
There are no files selected for viewing
90 changes: 90 additions & 0 deletions
90
...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
package tech.picnic.errorprone.bugpatterns; | ||
|
||
import static com.google.errorprone.BugPattern.LinkType.CUSTOM; | ||
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; | ||
import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; | ||
import static com.google.errorprone.matchers.Matchers.allOf; | ||
import static com.google.errorprone.matchers.Matchers.argument; | ||
import static com.google.errorprone.matchers.Matchers.isSameType; | ||
import static com.google.errorprone.matchers.Matchers.isVariable; | ||
import static com.google.errorprone.matchers.Matchers.not; | ||
import static com.google.errorprone.matchers.Matchers.staticMethod; | ||
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; | ||
|
||
import com.google.auto.service.AutoService; | ||
import com.google.errorprone.BugPattern; | ||
import com.google.errorprone.VisitorState; | ||
import com.google.errorprone.bugpatterns.BugChecker; | ||
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; | ||
import com.google.errorprone.fixes.SuggestedFixes; | ||
import com.google.errorprone.matchers.Description; | ||
import com.google.errorprone.matchers.Matcher; | ||
import com.google.errorprone.util.ASTHelpers; | ||
import com.sun.source.tree.ExpressionTree; | ||
import com.sun.source.tree.LambdaExpressionTree; | ||
import com.sun.source.tree.MethodInvocationTree; | ||
import com.sun.source.tree.MethodTree; | ||
import com.sun.source.tree.Tree; | ||
import com.sun.source.tree.VariableTree; | ||
import java.util.List; | ||
|
||
/** | ||
* A {@link BugChecker} that flags the use of {@link org.mockito.Mockito#mock(Class)} and {@link | ||
* org.mockito.Mockito#spy(Class)} where instead the type to be mocked or spied can be derived from | ||
* context. | ||
*/ | ||
// XXX: This check currently does not flag method invocation arguments. When adding support for | ||
// this, consider that in some cases the type to be mocked or spied must be specified explicitly so | ||
// as to disambiguate between method overloads. | ||
// XXX: This check currently does not flag (implicit or explicit) lambda return expressions. | ||
// XXX: This check currently does not drop suppressions that become obsolete after the | ||
// suggested fix is applied; consider adding support for this. | ||
@AutoService(BugChecker.class) | ||
@BugPattern( | ||
summary = "Don't unnecessarily pass a type to Mockito's `mock(Class)` and `spy(Class)` methods", | ||
link = BUG_PATTERNS_BASE_URL + "MockitoMockClassReference", | ||
linkType = CUSTOM, | ||
severity = SUGGESTION, | ||
tags = SIMPLIFICATION) | ||
public final class MockitoMockClassReference extends BugChecker | ||
implements MethodInvocationTreeMatcher { | ||
private static final long serialVersionUID = 1L; | ||
private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY = | ||
allOf( | ||
argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))), | ||
staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy")); | ||
|
||
/** Instantiates a new {@link MockitoMockClassReference} instance. */ | ||
public MockitoMockClassReference() {} | ||
|
||
@Override | ||
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
if (!MOCKITO_MOCK_OR_SPY.matches(tree, state) || !isTypeDerivableFromContext(tree, state)) { | ||
return Description.NO_MATCH; | ||
} | ||
|
||
List<? extends ExpressionTree> arguments = tree.getArguments(); | ||
return describeMatch(tree, SuggestedFixes.removeElement(arguments.get(0), arguments, state)); | ||
} | ||
|
||
private static boolean isTypeDerivableFromContext(MethodInvocationTree tree, VisitorState state) { | ||
Tree parent = state.getPath().getParentPath().getLeaf(); | ||
switch (parent.getKind()) { | ||
case VARIABLE: | ||
return !ASTHelpers.hasNoExplicitType((VariableTree) parent, state) | ||
&& areSameType(tree, parent, state); | ||
case ASSIGNMENT: | ||
return areSameType(tree, parent, state); | ||
case RETURN: | ||
Tree context = state.findEnclosing(LambdaExpressionTree.class, MethodTree.class); | ||
return context instanceof MethodTree | ||
&& areSameType(tree, ((MethodTree) context).getReturnType(), state); | ||
default: | ||
return false; | ||
} | ||
} | ||
|
||
private static boolean areSameType(Tree treeA, Tree treeB, VisitorState state) { | ||
return ASTHelpers.isSameType(ASTHelpers.getType(treeA), ASTHelpers.getType(treeB), state); | ||
} | ||
} |
136 changes: 136 additions & 0 deletions
136
...ntrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
package tech.picnic.errorprone.bugpatterns; | ||
|
||
import com.google.errorprone.BugCheckerRefactoringTestHelper; | ||
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; | ||
import com.google.errorprone.CompilationTestHelper; | ||
import org.junit.jupiter.api.Test; | ||
|
||
final class MockitoMockClassReferenceTest { | ||
@Test | ||
void identification() { | ||
CompilationTestHelper.newInstance(MockitoMockClassReference.class, getClass()) | ||
.addSourceLines( | ||
"A.java", | ||
"import static org.mockito.Mockito.mock;", | ||
"import static org.mockito.Mockito.spy;", | ||
"import static org.mockito.Mockito.withSettings;", | ||
"", | ||
"import java.util.List;", | ||
"import java.util.Objects;", | ||
"import org.mockito.invocation.InvocationOnMock;", | ||
"", | ||
"class A {", | ||
" {", | ||
" Double d = Objects.requireNonNullElseGet(null, () -> mock(Double.class));", | ||
" Double d2 =", | ||
" Objects.requireNonNullElseGet(", | ||
" null,", | ||
" () -> {", | ||
" return mock(Double.class);", | ||
" });", | ||
" }", | ||
"", | ||
" void m() {", | ||
" Number variableMock = 42;", | ||
" // BUG: Diagnostic contains:", | ||
" variableMock = mock(Number.class);", | ||
" // BUG: Diagnostic contains:", | ||
" variableMock = mock(Number.class, \"name\");", | ||
" // BUG: Diagnostic contains:", | ||
" variableMock = mock(Number.class, InvocationOnMock::callRealMethod);", | ||
" // BUG: Diagnostic contains:", | ||
" variableMock = mock(Number.class, withSettings());", | ||
" variableMock = mock(Integer.class);", | ||
" variableMock = 42;", | ||
" // BUG: Diagnostic contains:", | ||
" List rawMock = mock(List.class);", | ||
" // BUG: Diagnostic contains:", | ||
" List<String> genericMock = mock(List.class);", | ||
" var varMock = mock(Integer.class);", | ||
" Class<? extends Number> numberType = Integer.class;", | ||
" Number variableTypeMock = mock(numberType);", | ||
" Object subtypeMock = mock(Integer.class);", | ||
"", | ||
" Number variableSpy = 42;", | ||
" // BUG: Diagnostic contains:", | ||
" variableSpy = spy(Number.class);", | ||
" variableSpy = spy(Integer.class);", | ||
" variableSpy = 42;", | ||
" // BUG: Diagnostic contains:", | ||
" List rawSpy = spy(List.class);", | ||
" // BUG: Diagnostic contains:", | ||
" List<String> genericSpy = spy(List.class);", | ||
" var varSpy = spy(Integer.class);", | ||
" Number variableTypeSpy = spy(numberType);", | ||
" Object subtypeSpy = spy(Integer.class);", | ||
" Object objectSpy = spy(new Object());", | ||
"", | ||
" Objects.hash(mock(Integer.class));", | ||
" Integer i = mock(mock(Integer.class));", | ||
" String s = new String(mock(String.class));", | ||
" }", | ||
"", | ||
" Double getDoubleMock() {", | ||
" return Objects.requireNonNullElseGet(", | ||
" null,", | ||
" () -> {", | ||
" return mock(Double.class);", | ||
" });", | ||
" }", | ||
"", | ||
" Integer getIntegerMock() {", | ||
" // BUG: Diagnostic contains:", | ||
" return mock(Integer.class);", | ||
" }", | ||
"", | ||
" <T> T getGenericMock(Class<T> clazz) {", | ||
" return mock(clazz);", | ||
" }", | ||
"", | ||
" Number getSubTypeMock() {", | ||
" return mock(Integer.class);", | ||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
|
||
@Test | ||
void replacement() { | ||
BugCheckerRefactoringTestHelper.newInstance(MockitoMockClassReference.class, getClass()) | ||
.addInputLines( | ||
"A.java", | ||
"import static org.mockito.Mockito.mock;", | ||
"import static org.mockito.Mockito.spy;", | ||
"import static org.mockito.Mockito.withSettings;", | ||
"", | ||
"import org.mockito.invocation.InvocationOnMock;", | ||
"", | ||
"class A {", | ||
" void m() {", | ||
" Number simpleMock = mock(Number.class);", | ||
" Number namedMock = mock(Number.class, \"name\");", | ||
" Number customAnswerMock = mock(Number.class, InvocationOnMock::callRealMethod);", | ||
" Number customSettingsMock = mock(Number.class, withSettings());", | ||
" Number simpleSpy = spy(Number.class);", | ||
" }", | ||
"}") | ||
.addOutputLines( | ||
"A.java", | ||
"import static org.mockito.Mockito.mock;", | ||
"import static org.mockito.Mockito.spy;", | ||
"import static org.mockito.Mockito.withSettings;", | ||
"", | ||
"import org.mockito.invocation.InvocationOnMock;", | ||
"", | ||
"class A {", | ||
" void m() {", | ||
" Number simpleMock = mock();", | ||
" Number namedMock = mock(\"name\");", | ||
" Number customAnswerMock = mock(InvocationOnMock::callRealMethod);", | ||
" Number customSettingsMock = mock(withSettings());", | ||
" Number simpleSpy = spy();", | ||
" }", | ||
"}") | ||
.doTest(TestMode.TEXT_MATCH); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters