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

WIP Mockito strict stubs enforcement #5

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions error-prone-contrib/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,11 @@
<artifactId>mockito-core</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.reactivestreams</groupId>
<artifactId>reactive-streams</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
import static com.google.errorprone.matchers.Description.NO_MATCH;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.annotations;
import static com.google.errorprone.matchers.Matchers.hasArgumentWithValue;
import static com.google.errorprone.matchers.Matchers.isSameType;
import static com.google.errorprone.matchers.Matchers.isType;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.LinkType;
import com.google.errorprone.BugPattern.ProvidesFix;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.BugPattern.StandardTags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.MultiMatcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ImportTree;
import com.sun.source.tree.Tree;

/** A {@link BugChecker} which flags classes importing Mockito, but not enforcing strict mocks. */
@AutoService(BugChecker.class)
@BugPattern(
name = "MockitoAnnotation",
summary = "Prefer using strict stubs with Mockito",
linkType = LinkType.NONE,
severity = SeverityLevel.SUGGESTION,
tags = StandardTags.STYLE,
providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)
public final class MockitoAnnotationCheck extends BugChecker implements ClassTreeMatcher {
private static final long serialVersionUID = 1L;
private static final String MOCKITO_SETTINGS = "org.mockito.junit.jupiter.MockitoSettings";
private static final String STRICT_STUBS = "org.mockito.quality.Strictness.STRICT_STUBS";
private static final String MOCKITO_ANNOTATION = "@MockitoSettings(strictness = STRICT_STUBS)";
Copy link
Member

Choose a reason for hiding this comment

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

Since STRICT_STUBS is the default, I think @MockitoSettings would do.

private static final MultiMatcher<Tree, AnnotationTree> HAS_STRICT_STUBS_ANNOTATION =
annotations(
AT_LEAST_ONE,
allOf(
isType(MOCKITO_SETTINGS),
hasArgumentWithValue("strictness", isSameType(STRICT_STUBS))));

@Override
public Description matchClass(ClassTree clazz, VisitorState state) {
if (ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class) != null
|| HAS_STRICT_STUBS_ANNOTATION.matches(clazz, state)
|| !importsMockito(state)) {
Copy link
Member

Choose a reason for hiding this comment

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

We should exclude non-test code, like the Mockito references under picnic-java-support-modules/test-support/src/main. For this looking at a file's path is a bad practice; instead we could look at the class name (fast, but "meh") or check for the presence of a JUnit test method, like in #6. Currently slightly tending to the latter.

return NO_MATCH;
}

return describeMatch(clazz, buildFix(clazz));
}

private static boolean importsMockito(VisitorState state) {
return state.getPath().getCompilationUnit().getImports().stream()
.map(ImportTree::getQualifiedIdentifier)
.map(Object::toString)
.anyMatch(importLine -> importLine.startsWith("org.mockito"));
}

private static SuggestedFix buildFix(ClassTree clazz) {
return SuggestedFix.builder()
.addImport(MOCKITO_SETTINGS)
.addStaticImport(STRICT_STUBS)
.prefixWith(clazz, MOCKITO_ANNOTATION)
.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
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;

public final class MockitoAnnotationCheckTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(MockitoAnnotationCheck.class, getClass());
private final BugCheckerRefactoringTestHelper refactoringTestHelper =
BugCheckerRefactoringTestHelper.newInstance(new MockitoAnnotationCheck(), getClass());

@Test
public void testIdentification() {
Copy link
Member

Choose a reason for hiding this comment

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

We should also add negative test cases: already-annotated classes, inner classes.

compilationTestHelper
.addSourceLines(
"A.java",
"import static org.mockito.Mockito.mock;",
"",
"import org.junit.jupiter.api.Tag;",
"import org.junit.jupiter.api.Test;",
"",
"@Tag(\"unit\")",
"// BUG: Diagnostic contains:",
"class MockitoTest {",
" @Test",
" void mockitoTest() {",
" mock(String.class);",
" }",
"}")
.doTest();
}

@Test
public void testReplacement() {
refactoringTestHelper
.addInputLines(
"in/A.java",
"import static org.mockito.Mockito.mock;",
"",
"import org.junit.jupiter.api.Tag;",
"import org.junit.jupiter.api.Test;",
"",
"@Tag(\"unit\")",
"class MockitoTest {",
" @Test",
" void mockitoTest() {",
" mock(String.class);",
" }",
"}")
.addOutputLines(
"out/A.java",
"import static org.mockito.Mockito.mock;",
"import static org.mockito.quality.Strictness.STRICT_STUBS;",
"",
"import org.junit.jupiter.api.Tag;",
"import org.junit.jupiter.api.Test;",
"import org.mockito.junit.jupiter.MockitoSettings;",
"",
"@MockitoSettings(strictness = STRICT_STUBS)",
"@Tag(\"unit\")",
"class MockitoTest {",
" @Test",
" void mockitoTest() {",
" mock(String.class);",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
}
}
5 changes: 5 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,11 @@
<artifactId>mockito-core</artifactId>
<version>${version.mockito}</version>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<version>${version.mockito}</version>
</dependency>
<dependency>
<groupId>org.reactivestreams</groupId>
<artifactId>reactive-streams</artifactId>
Expand Down