-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Working on Issue #3156, Adding MockitoMockedStatic and MockitoMockedS… #3161
base: main
Are you sure you want to change the base?
Changes from all commits
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,35 @@ | ||
/* | ||
* Copyright (c) 2017 Mockito contributors | ||
* This program is made available under the terms of the MIT License. | ||
*/ | ||
package org.mockito.errorprone.bugpatterns; | ||
|
||
import com.google.auto.service.AutoService; | ||
import com.google.errorprone.BugPattern; | ||
import com.google.errorprone.BugPattern.SeverityLevel; | ||
import com.google.errorprone.VisitorState; | ||
import com.google.errorprone.bugpatterns.BugChecker; | ||
import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; | ||
import com.google.errorprone.matchers.Description; | ||
import com.sun.source.tree.VariableTree; | ||
|
||
@AutoService(BugChecker.class) | ||
@BugPattern( | ||
name = "MockitoMockedStatic", | ||
summary = "Fields or parameters annotated with @Mock in MockedStatic can lead to compilation issues.", | ||
explanation = "Fields or parameters annotated with @Mock in MockedStatic can lead to compilation issues.", | ||
severity = SeverityLevel.ERROR) | ||
public class MockitoMockedStatic extends BugChecker implements VariableTreeMatcher { | ||
|
||
@Override | ||
public Description matchVariable(VariableTree tree, VisitorState state) { | ||
if (tree.getModifiers().getAnnotations().stream() | ||
.anyMatch(annotation -> | ||
state.getSourceForNode(annotation).equals("@org.mockito.Mock") | ||
&& state.getSourceForNode(tree.getType()).equals("org.mockito.MockedStatic"))) { | ||
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. Should this check for MockedConstruction too? Both have the same issue I think in that mocking them results in a lifecycle-bound mock that has to be closed to prevent interfering with external components 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. Would you reckon that should be a separate file or in the same one? 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. Not entirely sure. Might be best to wait for confirmation from one of the maintainers! I'd assume it is almost identical logic minus the naming though so whether it is something that is reusable by just passing the class name and error message or not..? They might not even want to cover the construction case though, so I'd definitely wait for confirmation :-) 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. Let's check for both in this same check. In that case, let's name the checker |
||
return describeMatch(tree); | ||
} | ||
|
||
return Description.NO_MATCH; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
/* | ||
* Copyright (c) 2017 Mockito contributors | ||
* This program is made available under the terms of the MIT License. | ||
*/ | ||
package org.mockito.errorprone.bugpatterns; | ||
|
||
import com.google.errorprone.CompilationTestHelper; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
import org.junit.runner.RunWith; | ||
import org.junit.runners.JUnit4; | ||
|
||
@RunWith(JUnit4.class) | ||
public class MockitoMockedStaticTest { | ||
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. I think you missed the case where you have a |
||
|
||
private CompilationTestHelper compilationTestHelper; | ||
|
||
@Before | ||
public void setup() { | ||
compilationTestHelper = | ||
CompilationTestHelper.newInstance(MockitoMockedStatic.class, getClass()); | ||
} | ||
|
||
@Test | ||
public void mockedStaticWithMockField_shouldError() { | ||
compilationTestHelper | ||
.addSourceLines( | ||
"input.java", | ||
"import org.mockito.Mock;", | ||
"import org.mockito.MockedStatic;", | ||
TimvdLippe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"import org.mockito.Mockito;", | ||
"class Test {", | ||
" @Mock", | ||
" private Object mock;", | ||
" public void test() {", | ||
" try (MockedStatic<Object> mockedStatic = Mockito.mockStatic(Object.class)) {", | ||
" // Do something with the mock", | ||
" }", | ||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
|
||
@Test | ||
public void mockedStaticWithMockParameter_shouldError() { | ||
compilationTestHelper | ||
.addSourceLines( | ||
"input.java", | ||
"import org.mockito.Mock;", | ||
"import org.mockito.MockedStatic;", | ||
"import org.mockito.Mockito;", | ||
"class Test {", | ||
TimvdLippe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
" public void test(@Mock Object mock) {", | ||
" try (MockedStatic<Object> mockedStatic = Mockito.mockStatic(Object.class)) {", | ||
" // Do something with the mock", | ||
" }", | ||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
|
||
@Test | ||
public void mockedStaticWithoutMock_shouldNotError() { | ||
compilationTestHelper | ||
.addSourceLines( | ||
"input.java", | ||
"import org.mockito.MockedStatic;", | ||
"import org.mockito.Mockito;", | ||
"class Test {", | ||
" public void test() {", | ||
" try (MockedStatic<Object> mockedStatic = Mockito.mockStatic(Object.class)) {", | ||
" // Do something without a mock", | ||
" }", | ||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
} |
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 not use
getSourceForNode
to compare annotations, as that is equivalent to comparingtoString()
. Instead, use https://errorprone.info/api/latest/com/google/errorprone/util/ASTHelpers.html#isSameType(com.sun.tools.javac.code.Type,com.sun.tools.javac.code.Type,com.google.errorprone.VisitorState)