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.

Sign up for GitHub

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

PSM-671 Introduce a check for test method names #6

Merged
merged 8 commits into from
Jan 7, 2021
Merged

PSM-671 Introduce a check for test method names #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
135ea05
PSM-671 Introduce a check for test method names
anicolasgar Dec 7, 2020
a34a15c
PSM-671 Improvements
anicolasgar Dec 7, 2020
a4f2eb5
PSM-671 More improvements
anicolasgar Dec 7, 2020
e1bb331
PSM-671 Extend checker to also flag illegal modifiers in Junit method…
anicolasgar Dec 24, 2020
af2b114
PSM-671 Suggestions
Stephan202 Dec 30, 2020
37208bd
PSM-671 Suggestions
Stephan202 Jan 1, 2021
93de389
PSM-671 Tweak
Stephan202 Jan 1, 2021
c6dd4a7
PSM-671 Nits
anicolasgar Jan 5, 2021
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
114 changes: 114 additions & 0 deletions ...contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheck.java
View file
Open in desktop
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 = "JUnitMethodDeclaration",
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);
}
}
180 changes: 180 additions & 0 deletions ...rib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java
View file
Open in desktop
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 setUp1() {}",
" // BUG: Diagnostic contains:",
" @BeforeAll public void setUp2() {}",
" // BUG: Diagnostic contains:",
" @BeforeAll protected void setUp3() {}",
" // BUG: Diagnostic contains:",
" @BeforeAll private void setUp4() {}",
"",
" @BeforeEach void setup5() {}",
" // BUG: Diagnostic contains:",
" @BeforeEach public void setUp6() {}",
" // BUG: Diagnostic contains:",
" @BeforeEach protected void setUp7() {}",
" // BUG: Diagnostic contains:",
" @BeforeEach private void setUp8() {}",
"",
" @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 setUp1() {}",
" @Override @BeforeAll public void setUp2() {}",
" @Override @BeforeAll protected void setUp3() {}",
"",
" @Override @BeforeEach void setup5() {}",
" @Override @BeforeEach public void setUp6() {}",
" @Override @BeforeEach protected void setUp7() {}",
"",
" @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);
}
}