Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& state.getSourceForNode(tree.getType()).equals("org.mockito.MockedStatic"))) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 :-)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 MockitoIncompatibleMockTypes

return describeMatch(tree);
}

return Description.NO_MATCH;
}
}
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed the case where you have a @Mock private MockedStatic<Object> in the original issue description. You can use // BUG: diagnostic contains: to verify that the compilation would fail. Example: https://github.com/google/error-prone/blob/18d5cdf10ec1cc798a588d5c48a9e02a8888c6a1/core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java#L60


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();
}
}