Skip to content

Commit

Permalink
PSM-671 Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202
Stephan202 committed Jan 1, 2021
1 parent af2b114 commit 37208bd
Show file tree
Hide file tree
Showing 4 changed files with 294 additions and 302 deletions.
114 changes: 114 additions & 0 deletions ...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
import static com.google.errorprone.matchers.Matchers.annotations;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.isType;
import static java.util.function.Predicate.not;

import com.google.auto.service.AutoService;
import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.MultiMatcher;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.MethodTree;
import com.sun.tools.javac.code.Symbol;
import java.util.Optional;
import javax.lang.model.element.Modifier;

/** A {@link BugChecker} which flags non-canonical JUnit method declarations. */
// XXX: Consider introducing a class-level check which enforces that test classes:
// 1. Are named `*Test` or `Abstract*TestCase`.
// 2. If not `abstract`, don't have public methods and subclasses.
// 3. Only have private fields.
// XXX: If implemented, the current logic could flag only `private` JUnit methods.
@AutoService(BugChecker.class)
@BugPattern(
name = "JUnitMethodDeclarationCheck",
summary = "JUnit method declaration can likely be improved",
linkType = BugPattern.LinkType.NONE,
severity = BugPattern.SeverityLevel.SUGGESTION,
tags = BugPattern.StandardTags.SIMPLIFICATION)
public final class JUnitMethodDeclarationCheck extends BugChecker implements MethodTreeMatcher {
private static final long serialVersionUID = 1L;
private static final String TEST_PREFIX = "test";
private static final ImmutableSet<Modifier> ILLEGAL_MODIFIERS =
ImmutableSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC);
private static final MultiMatcher<MethodTree, AnnotationTree> IS_OVERRIDE_METHOD =
annotations(AT_LEAST_ONE, isType("java.lang.Override"));
private static final MultiMatcher<MethodTree, AnnotationTree> IS_TEST_METHOD =
annotations(
AT_LEAST_ONE,
anyOf(
isType("org.junit.jupiter.api.Test"),
hasMetaAnnotation("org.junit.jupiter.api.TestTemplate")));
private static final MultiMatcher<MethodTree, AnnotationTree> IS_SETUP_OR_TEARDOWN_METHOD =
annotations(
AT_LEAST_ONE,
anyOf(
isType("org.junit.jupiter.api.AfterAll"),
isType("org.junit.jupiter.api.AfterEach"),
isType("org.junit.jupiter.api.BeforeAll"),
isType("org.junit.jupiter.api.BeforeEach")));

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
// XXX: Perhaps we should also skip analysis of non-`private` non-`final` methods in abstract
// classes?
if (IS_OVERRIDE_METHOD.matches(tree, state)) {
return Description.NO_MATCH;
}

boolean isTestMethod = IS_TEST_METHOD.matches(tree, state);
if (!isTestMethod && !IS_SETUP_OR_TEARDOWN_METHOD.matches(tree, state)) {
return Description.NO_MATCH;
}

SuggestedFix.Builder builder = SuggestedFix.builder();
SuggestedFixes.removeModifiers(tree.getModifiers(), state, ILLEGAL_MODIFIERS)
.ifPresent(builder::merge);

if (isTestMethod) {
// XXX: In theory this rename could clash with an existing method or static import. In that
// case we should emit a warning without a suggested replacement.
tryCanonicalizeMethodName(tree, state).ifPresent(builder::merge);
}

return builder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, builder.build());
}

private static Optional<SuggestedFix> tryCanonicalizeMethodName(
MethodTree tree, VisitorState state) {
return Optional.ofNullable(ASTHelpers.getSymbol(tree))
.map(sym -> sym.getQualifiedName().toString())
.filter(name -> name.startsWith(TEST_PREFIX))
.map(name -> name.substring(TEST_PREFIX.length()))
.filter(not(String::isEmpty))
.map(name -> Character.toLowerCase(name.charAt(0)) + name.substring(1))
.filter(name -> !Character.isDigit(name.charAt(0)))
.map(name -> SuggestedFixes.renameMethod(tree, name, state));
}

// XXX: Move to a `MoreMatchers` utility class.
private static Matcher<AnnotationTree> hasMetaAnnotation(String annotationClassName) {
TypePredicate typePredicate = hasAnnotation(annotationClassName);
return (tree, state) -> {
Symbol sym = ASTHelpers.getDeclaredSymbol(tree);
return sym != null && typePredicate.apply(sym.type, state);
};
}

// XXX: Move to a `MoreTypePredicates` utility class.
private static TypePredicate hasAnnotation(String annotationClassName) {
return (type, state) -> ASTHelpers.hasAnnotation(type.tsym, annotationClassName, state);
}
}
98 changes: 0 additions & 98 deletions ...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JunitMethodDeclarationCheck.java
View file

This file was deleted.

180 changes: 180 additions & 0 deletions ...rib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
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 JUnitMethodDeclarationCheckTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(JUnitMethodDeclarationCheck.class, getClass());
private final BugCheckerRefactoringTestHelper refactoringTestHelper =
BugCheckerRefactoringTestHelper.newInstance(new JUnitMethodDeclarationCheck(), getClass());

@Test
public void testIdentification() {
compilationTestHelper
.addSourceLines(
"A.java",
"import org.junit.jupiter.api.AfterAll;",
"import org.junit.jupiter.api.AfterEach;",
"import org.junit.jupiter.api.BeforeAll;",
"import org.junit.jupiter.api.BeforeEach;",
"import org.junit.jupiter.api.Test;",
"import org.junit.jupiter.params.ParameterizedTest;",
"",
"class A {",
" @BeforeAll void setUp5() {}",
" // BUG: Diagnostic contains:",
" @BeforeAll public void setUp6() {}",
" // BUG: Diagnostic contains:",
" @BeforeAll protected void setUp7() {}",
" // BUG: Diagnostic contains:",
" @BeforeAll private void setUp8() {}",
"",
" @BeforeEach void setup1() {}",
" // BUG: Diagnostic contains:",
" @BeforeEach public void setUp2() {}",
" // BUG: Diagnostic contains:",
" @BeforeEach protected void setUp3() {}",
" // BUG: Diagnostic contains:",
" @BeforeEach private void setUp4() {}",
"",
" @AfterEach void tearDown1() {}",
" // BUG: Diagnostic contains:",
" @AfterEach public void tearDown2() {}",
" // BUG: Diagnostic contains:",
" @AfterEach protected void tearDown3() {}",
" // BUG: Diagnostic contains:",
" @AfterEach private void tearDown4() {}",
"",
" @AfterAll void tearDown5() {}",
" // BUG: Diagnostic contains:",
" @AfterAll public void tearDown6() {}",
" // BUG: Diagnostic contains:",
" @AfterAll protected void tearDown7() {}",
" // BUG: Diagnostic contains:",
" @AfterAll private void tearDown8() {}",
"",
" @Test void method1() {}",
" // BUG: Diagnostic contains:",
" @Test void testMethod2() {}",
" // BUG: Diagnostic contains:",
" @Test public void method3() {}",
" // BUG: Diagnostic contains:",
" @Test protected void method4() {}",
" // BUG: Diagnostic contains:",
" @Test private void method5() {}",
"",
" @ParameterizedTest void method6() {}",
" // BUG: Diagnostic contains:",
" @ParameterizedTest void testMethod7() {}",
" // BUG: Diagnostic contains:",
" @ParameterizedTest public void method8() {}",
" // BUG: Diagnostic contains:",
" @ParameterizedTest protected void method9() {}",
" // BUG: Diagnostic contains:",
" @ParameterizedTest private void method10() {}",
"",
" @BeforeEach @BeforeAll @AfterEach @AfterAll void testNonTestMethod1() {}",
" public void testNonTestMethod2() {}",
" protected void testNonTestMethod3() {}",
" private void testNonTestMethod4() {}",
" @Test void test5() {}",
"}")
.addSourceLines(
"B.java",
"import org.junit.jupiter.api.AfterAll;",
"import org.junit.jupiter.api.AfterEach;",
"import org.junit.jupiter.api.BeforeAll;",
"import org.junit.jupiter.api.BeforeEach;",
"import org.junit.jupiter.api.Test;",
"import org.junit.jupiter.params.ParameterizedTest;",
"",
"final class B extends A {",
" @Override @BeforeAll void setUp5() {}",
" @Override @BeforeAll public void setUp6() {}",
" @Override @BeforeAll protected void setUp7() {}",
"",
" @Override @BeforeEach void setup1() {}",
" @Override @BeforeEach public void setUp2() {}",
" @Override @BeforeEach protected void setUp3() {}",
"",
" @Override @AfterEach void tearDown1() {}",
" @Override @AfterEach public void tearDown2() {}",
" @Override @AfterEach protected void tearDown3() {}",
"",
" @Override @AfterAll void tearDown5() {}",
" @Override @AfterAll public void tearDown6() {}",
" @Override @AfterAll protected void tearDown7() {}",
"",
" @Override @Test void method1() {}",
" @Override @Test void testMethod2() {}",
" @Override @Test public void method3() {}",
" @Override @Test protected void method4() {}",
"",
" @Override @ParameterizedTest void method6() {}",
" @Override @ParameterizedTest void testMethod7() {}",
" @Override @ParameterizedTest public void method8() {}",
" @Override @ParameterizedTest protected void method9() {}",
"",
" @Override @BeforeEach @BeforeAll @AfterEach @AfterAll void testNonTestMethod1() {}",
" @Override public void testNonTestMethod2() {}",
" @Override protected void testNonTestMethod3() {}",
" @Override @Test void test5() {}",
"}")
.doTest();
}

@Test
public void testReplacement() {
refactoringTestHelper
.addInputLines(
"in/A.java",
"import org.junit.jupiter.api.AfterAll;",
"import org.junit.jupiter.api.AfterEach;",
"import org.junit.jupiter.api.BeforeAll;",
"import org.junit.jupiter.api.BeforeEach;",
"import org.junit.jupiter.api.RepeatedTest;",
"import org.junit.jupiter.api.Test;",
"import org.junit.jupiter.params.ParameterizedTest;",
"",
"class A {",
" @BeforeAll public void setUp1() {}",
" @BeforeEach protected void setUp2() {}",
" @AfterEach private void setUp3() {}",
" @AfterAll private void setUp4() {}",
"",
" @Test void testFoo() {}",
" @ParameterizedTest void testBar() {}",
"",
" @Test public void baz() {}",
" @RepeatedTest(2) private void qux() {}",
" @ParameterizedTest protected void quux() {}",
"}")
.addOutputLines(
"out/A.java",
"import org.junit.jupiter.api.AfterAll;",
"import org.junit.jupiter.api.AfterEach;",
"import org.junit.jupiter.api.BeforeAll;",
"import org.junit.jupiter.api.BeforeEach;",
"import org.junit.jupiter.api.RepeatedTest;",
"import org.junit.jupiter.api.Test;",
"import org.junit.jupiter.params.ParameterizedTest;",
"",
"class A {",
" @BeforeAll void setUp1() {}",
" @BeforeEach void setUp2() {}",
" @AfterEach void setUp3() {}",
" @AfterAll void setUp4() {}",
"",
" @Test void foo() {}",
" @ParameterizedTest void bar() {}",
"",
" @Test void baz() {}",
" @RepeatedTest(2) void qux() {}",
" @ParameterizedTest void quux() {}",
"}")
.doTest(TestMode.TEXT_MATCH);
}
}
Loading

0 comments on commit 37208bd

Please sign in to comment.