diff --git a/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java b/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java index 3e90f541935..e40aa2da413 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java @@ -47,11 +47,16 @@ import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.ExpressionStatementTree; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.StatementTree; +import com.sun.source.tree.TryTree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Type.ClassType; +import java.util.Optional; import javax.lang.model.element.Modifier; /** @@ -61,18 +66,25 @@ * @author eaftan@google.com (Eddie Aftandillian) */ public final class JUnitMatchers { + public static final String JUNIT3_TEST_CASE_CLASS = "junit.framework.TestCase"; public static final String JUNIT4_TEST_ANNOTATION = "org.junit.Test"; + public static final String JUNIT5_TEST_ANNOTATION = "org.junit.jupiter.api.Test"; public static final String JUNIT4_THEORY_ANNOTATION = "org.junit.experimental.theories.Theory"; public static final String JUNIT_BEFORE_ANNOTATION = "org.junit.Before"; public static final String JUNIT_AFTER_ANNOTATION = "org.junit.After"; public static final String JUNIT_BEFORE_CLASS_ANNOTATION = "org.junit.BeforeClass"; public static final String JUNIT_AFTER_CLASS_ANNOTATION = "org.junit.AfterClass"; - public static final String JUNIT4_RUN_WITH_ANNOTATION = "org.junit.runner.RunWith"; - public static final String JUNIT4_ASSERT_CLASS = "org.junit.Assert"; - public static final String JUNIT3_TEST_CASE_CLASS = "junit.framework.TestCase"; + public static final String JUNIT5_BEFORE_EACH_ANNOTATION = "org.junit.jupiter.api.BeforeEach"; + public static final String JUNIT5_AFTER_EACH_ANNOTATION = "org.junit.jupiter.api.AfterEach"; + public static final String JUNIT5_BEFORE_ALL_ANNOTATION = "org.junit.jupiter.api.BeforeAll"; + public static final String JUNIT5_AFTER_ALL_ANNOTATION = "org.junit.jupiter.api.AfterAll"; public static final String JUNIT4_IGNORE_ANNOTATION = "org.junit.Ignore"; - public static final String JUNIT4_RUNNER_CLASS = "org.junit.runners.JUnit4"; + public static final String JUNIT5_DISABLED_ANNOTATION = "org.junit.jupiter.api.Disabled"; public static final String JUNIT3_ASSERT_CLASS = "junit.framework.Assert"; + public static final String JUNIT4_ASSERT_CLASS = "org.junit.Assert"; + public static final String JUNIT5_ASSERT_CLASS = "org.junit.jupiter.api.Assertions"; + public static final String JUNIT4_RUN_WITH_ANNOTATION = "org.junit.runner.RunWith"; + public static final String JUNIT4_RUNNER_CLASS = "org.junit.runners.JUnit4"; /** * Checks if a method, or any overridden method, is annotated with any annotation from the @@ -126,6 +138,47 @@ private static boolean hasJUnitAttr(MethodSymbol methodSym) { public static final Matcher hasJUnit4TestCases = hasMethod(hasAnnotationOnAnyOverriddenMethod(JUNIT4_TEST_ANNOTATION)); + /** Match a class which has one or more methods with a JUnit 5 @Test annotation. */ + public static final Matcher hasJUnit5TestCases = + hasMethod(hasAnnotation(JUNIT5_TEST_ANNOTATION)); + + /** Match a method annotated with JUnit 5 @BeforeEach. */ + public static final Matcher hasJUnit5BeforeEach = + hasAnnotation(JUNIT5_BEFORE_EACH_ANNOTATION); + + /** Match a method annotated with JUnit 5 @AfterEach. */ + public static final Matcher hasJUnit5AfterEach = + hasAnnotation(JUNIT5_AFTER_EACH_ANNOTATION); + + /** Match a method annotated with JUnit 5 @BeforeAll. */ + public static final Matcher hasJUnit5BeforeAll = + hasAnnotation(JUNIT5_BEFORE_ALL_ANNOTATION); + + /** Match a method annotated with JUnit 5 @AfterAll. */ + public static final Matcher hasJUnit5AfterAll = + hasAnnotation(JUNIT5_AFTER_ALL_ANNOTATION); + + /** Match a method annotated with any JUnit 5 before annotation (@BeforeEach or @BeforeAll). */ + public static final Matcher hasJUnit5BeforeAnnotations = + anyOf(hasJUnit5BeforeEach, hasJUnit5BeforeAll); + + /** Match a method annotated with any JUnit 5 after annotation (@AfterEach or @AfterAll). */ + public static final Matcher hasJUnit5AfterAnnotations = + anyOf(hasJUnit5AfterEach, hasJUnit5AfterAll); + + /** + * Returns {@code true} if the enclosing class of the given state is a JUnit 5 test class. + */ + public static boolean isJUnit5TestClass(VisitorState state) { + for (com.sun.source.tree.Tree ancestor : state.getPath()) { + if (ancestor instanceof ClassTree classTree + && hasJUnit5TestCases.matches(classTree, state)) { + return true; + } + } + return false; + } + /** * Match a class which appears to be a JUnit 3 test class. * @@ -238,12 +291,13 @@ private static boolean hasJUnitAttr(MethodSymbol methodSym) { hasAnnotationOnAnyOverriddenMethod(JUNIT4_TEST_ANNOTATION), not(hasAnnotationOnAnyOverriddenMethod(JUNIT4_IGNORE_ANNOTATION))); - /** Matches a JUnit 3 or 4 test case. */ + /** Matches a JUnit 3, 4, or 5 test case. */ public static final Matcher TEST_CASE = anyOf( isJunit3TestCase, hasAnnotation(JUNIT4_TEST_ANNOTATION), - hasAnnotation(JUNIT4_THEORY_ANNOTATION)); + hasAnnotation(JUNIT4_THEORY_ANNOTATION), + hasAnnotation(JUNIT5_TEST_ANNOTATION)); /** * A list of test runners that this matcher should look for in the @RunWith annotation. Subclasses @@ -328,5 +382,56 @@ public static Matcher isJUnit4TestRunnerOfType(Iterable public static final Matcher isAmbiguousJUnitVersion = allOf(isTestCaseDescendant, anyOf(hasJUnit4TestRunner, hasJUnit4TestCases)); + /** + * Returns {@code true} if the given method invocation is a call to a JUnit 5 assertion method, + * determined by the symbol owner being {@code org.junit.jupiter.api.Assertions}. + * + *

This is more robust than import scanning: it works with fully-qualified calls, imported + * calls, and star imports, and answers per-call rather than per-file. + */ + public static boolean isJUnit5AssertionCall(ExpressionTree tree) { + Symbol sym = getSymbol(tree); + return sym != null + && sym.owner.getQualifiedName().toString().equals(JUNIT5_ASSERT_CLASS); + } + + /** + * Returns the assertion class name appropriate for the enclosing test class: {@link + * #JUNIT5_ASSERT_CLASS} for JUnit 5 tests, {@link #JUNIT4_ASSERT_CLASS} for JUnit 4 and + * earlier. + */ + public static String getAssertionClassName(VisitorState state) { + return isJUnit5TestClass(state) ? JUNIT5_ASSERT_CLASS : JUNIT4_ASSERT_CLASS; + } + + /** + * Returns the assertion class name appropriate for the given assertion call: {@link + * #JUNIT5_ASSERT_CLASS} if the call is to a JUnit 5 assertion, {@link #JUNIT4_ASSERT_CLASS} + * otherwise. + */ + public static String getAssertionClassName(ExpressionTree tree) { + return isJUnit5AssertionCall(tree) ? JUNIT5_ASSERT_CLASS : JUNIT4_ASSERT_CLASS; + } + + /** + * Scans the try block of a {@link TryTree} for a {@code fail()} call statement. + * + * @return the {@code fail()} invocation, or empty if not found + */ + public static Optional findFailCallInTry(TryTree tryTree) { + for (StatementTree statement : tryTree.getBlock().getStatements()) { + if (statement instanceof ExpressionStatementTree est + && est.getExpression() instanceof MethodInvocationTree mit) { + Symbol sym = getSymbol(mit); + if (sym != null + && sym.getSimpleName().contentEquals("fail") + && sym.isStatic()) { + return Optional.of(mit); + } + } + } + return Optional.empty(); + } + private JUnitMatchers() {} } diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java index b8b338e33df..39d36a9c6a5 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java @@ -1509,7 +1509,11 @@ public static Matcher instanceHashCodeInvocation() { private static final Matcher ASSERT_EQUALS = staticMethod() - .onClassAny("org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .onClassAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .named("assertEquals"); /** @@ -1522,7 +1526,11 @@ public static Matcher assertEqualsInvocation() { private static final Matcher ASSERT_NOT_EQUALS = staticMethod() - .onClassAny("org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .onClassAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .named("assertNotEquals"); /** diff --git a/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java b/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java index 80f15b40ba4..8a276142098 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/UnusedReturnValueMatcher.java @@ -187,6 +187,7 @@ private static boolean exceptionTesting(ExpressionTree tree, VisitorState state) instanceMethod() .onDescendantOf("com.google.common.truth.StandardSubjectBuilder") .named("fail"), + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("fail"), staticMethod().onClass("org.junit.Assert").named("fail"), staticMethod().onClass("junit.framework.Assert").named("fail"), staticMethod().onClass("junit.framework.TestCase").named("fail")); diff --git a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java index 905bdc4d86a..6ab28e29ca6 100644 --- a/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java +++ b/check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java @@ -1482,7 +1482,7 @@ public static Type getUpperBound(Type type, Types types) { /** * Returns true if the leaf node in the {@link TreePath} from {@code state} sits somewhere - * underneath a class or method that is marked as JUnit 3 or 4 test code. + * underneath a class or method that is marked as JUnit test code. */ public static boolean isJUnitTestCode(VisitorState state) { for (Tree ancestor : state.getPath()) { @@ -1492,7 +1492,8 @@ public static boolean isJUnitTestCode(VisitorState state) { } if (ancestor instanceof ClassTree classTree && (JUnitMatchers.isTestCaseDescendant.matches(classTree, state) - || hasAnnotation(getSymbol(ancestor), JUNIT4_RUN_WITH_ANNOTATION, state))) { + || hasAnnotation(getSymbol(ancestor), JUNIT4_RUN_WITH_ANNOTATION, state) + || JUnitMatchers.hasJUnit5TestCases.matches(classTree, state))) { return true; } } diff --git a/core/pom.xml b/core/pom.xml index a27b3a6fd2c..7bb7b96040e 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -127,6 +127,13 @@ ${junit.version} test + + + org.junit.jupiter + junit-jupiter-api + ${junit5.version} + test + com.google.testparameterinjector diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java index 80606f02e3d..7169e137f7d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractJUnit4InitMethodNotRun.java @@ -16,8 +16,10 @@ package com.google.errorprone.bugpatterns; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5TestCases; import static com.google.errorprone.matchers.JUnitMatchers.isJUnit4TestClass; import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.enclosingClass; import static com.google.errorprone.matchers.Matchers.hasAnnotation; import static com.google.errorprone.matchers.Matchers.hasAnnotationOnAnyOverriddenMethod; @@ -27,6 +29,7 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.JUnitMatchers; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; import com.google.errorprone.util.ASTHelpers; @@ -49,6 +52,7 @@ abstract class AbstractJUnit4InitMethodNotRun extends BugChecker implements MethodTreeMatcher { private static final String JUNIT_TEST = "org.junit.Test"; + private static final String JUNIT5_TEST = JUnitMatchers.JUNIT5_TEST_ANNOTATION; /** * Returns a matcher that selects which methods this matcher applies to (e.g. public void setUp() @@ -63,17 +67,17 @@ abstract class AbstractJUnit4InitMethodNotRun extends BugChecker implements Meth *

If another annotation is on the method that has the same name, the import will be replaced * with the appropriate one (e.g.: com.example.Before becomes org.junit.Before) */ - protected abstract String correctAnnotation(); + protected abstract String correctAnnotation(VisitorState state); /** * Returns a collection of 'before-and-after' pairs of annotations that should be replaced on * these methods. * *

If this method matcher finds a method annotated with {@link - * AnnotationReplacements#badAnnotation}, instead of applying {@link #correctAnnotation()}, + * AnnotationReplacements#badAnnotation}, instead of applying {@link #correctAnnotation}, * instead replace it with {@link AnnotationReplacements#goodAnnotation} */ - protected abstract List annotationReplacements(); + protected abstract List annotationReplacements(VisitorState state); /** * Matches if all of the following conditions are true: 1) The method matches {@link @@ -88,7 +92,8 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) { allOf( methodMatcher(), not(hasAnnotationOnAnyOverriddenMethod(JUNIT_TEST)), - enclosingClass(isJUnit4TestClass)) + not(hasAnnotationOnAnyOverriddenMethod(JUNIT5_TEST)), + enclosingClass(anyOf(isJUnit4TestClass, hasJUnit5TestCases))) .matches(methodTree, state); if (!matches) { return Description.NO_MATCH; @@ -97,7 +102,7 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) { // For each annotationReplacement, replace the first annotation that matches. If any of them // matches, don't try and do the rest of the work. Description description; - for (AnnotationReplacements replacement : annotationReplacements()) { + for (AnnotationReplacements replacement : annotationReplacements(state)) { description = tryToReplaceAnnotation( methodTree, state, replacement.badAnnotation, replacement.goodAnnotation); @@ -108,7 +113,7 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) { // Search for another @Before annotation on the method and replace the import // if we find one - String correctAnnotation = correctAnnotation(); + String correctAnnotation = correctAnnotation(state); String unqualifiedClassName = getUnqualifiedClassName(correctAnnotation); for (AnnotationTree annotationNode : methodTree.getModifiers().getAnnotations()) { Symbol annoSymbol = ASTHelpers.getSymbol(annotationNode); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpression.java b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpression.java index 1b042ca99c5..bb6af7144a5 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpression.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpression.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import static com.google.errorprone.util.ASTHelpers.getStartPosition; import static java.util.stream.Collectors.joining; @@ -50,7 +51,9 @@ public class AssertThrowsBlockToExpression extends BugChecker implements MethodInvocationTreeMatcher { private static final Matcher MATCHER = - staticMethod().onClass("org.junit.Assert").named("assertThrows"); + anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertThrows"), + staticMethod().onClass("org.junit.Assert").named("assertThrows")); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizer.java b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizer.java index 223f29bb9af..5413cae9d1a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizer.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizer.java @@ -78,7 +78,9 @@ public class AssertThrowsMinimizer extends BugChecker implements MethodTreeMatcher { private static final Matcher MATCHER = - anyOf(staticMethod().onClass("org.junit.Assert").named("assertThrows")); + anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertThrows"), + staticMethod().onClass("org.junit.Assert").named("assertThrows")); private final ConstantExpressions constantExpressions; private final boolean useVarType; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatements.java b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatements.java index bd714c72d8e..91d00de4851 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatements.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatements.java @@ -18,6 +18,7 @@ import static com.google.common.collect.Iterables.getLast; import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import static com.google.errorprone.util.ASTHelpers.getStartPosition; @@ -55,7 +56,9 @@ public class AssertThrowsMultipleStatements extends BugChecker } private static final Matcher MATCHER = - staticMethod().onClass("org.junit.Assert").named("assertThrows"); + anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertThrows"), + staticMethod().onClass("org.junit.Assert").named("assertThrows")); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsUtils.java b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsUtils.java index fc36125b79e..fadb0bd207e 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsUtils.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AssertThrowsUtils.java @@ -29,6 +29,7 @@ import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes.VariableNamer; +import com.google.errorprone.matchers.JUnitMatchers; import com.google.errorprone.util.ErrorProneComment; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.CatchTree; @@ -109,7 +110,11 @@ public static Optional tryFailToAssertThrows( return Optional.empty(); } List catchStatements = catchTree.getBlock().getStatements(); - fix.addStaticImport("org.junit.Assert.assertThrows"); + String assertThrowsClass = + JUnitMatchers.findFailCallInTry(tryTree) + .map(JUnitMatchers::getAssertionClassName) + .orElse(JUnitMatchers.JUNIT4_ASSERT_CLASS); + fix.addStaticImport(assertThrowsClass + ".assertThrows"); List resources = tryTree.getResources(); if (!resources.isEmpty()) { fixPrefix.append( diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AssertionFailureIgnored.java b/core/src/main/java/com/google/errorprone/bugpatterns/AssertionFailureIgnored.java index de2de42c052..123add86d87 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AssertionFailureIgnored.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AssertionFailureIgnored.java @@ -34,6 +34,7 @@ import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.JUnitMatchers; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.predicates.TypePredicates; @@ -74,7 +75,11 @@ public class AssertionFailureIgnored extends BugChecker implements MethodInvocat private static final Matcher ASSERTION = staticMethod() - .onClassAny("org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .onClassAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .withNameMatching(Pattern.compile("fail|assert.*")); private static final Matcher NEW_THROWABLE = @@ -168,7 +173,7 @@ private static Optional buildFix( endPosition = getStartPosition(getLast(tryStatement.getBlock().getStatements())); } if (catchTree.getBlock().getStatements().isEmpty()) { - fix.addStaticImport("org.junit.Assert.assertThrows"); + fix.addStaticImport(JUnitMatchers.getAssertionClassName(tree) + ".assertThrows"); fix.replace( getStartPosition(tryStatement), startPosition, @@ -177,7 +182,7 @@ private static Optional buildFix( state.getSourceForNode(catchTree.getParameter().getType()))) .replace(endPosition, state.getEndPosition(catchTree), (expression ? "" : "}") + ");\n"); } else { - fix.addStaticImport("org.junit.Assert.assertThrows") + fix.addStaticImport(JUnitMatchers.getAssertionClassName(tree) + ".assertThrows") .prefixWith(tryStatement, state.getSourceForNode(catchTree.getParameter())) .replace( getStartPosition(tryStatement), diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CatchFail.java b/core/src/main/java/com/google/errorprone/bugpatterns/CatchFail.java index f4aa7366e78..c88e467c4a0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/CatchFail.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CatchFail.java @@ -72,6 +72,7 @@ public class CatchFail extends BugChecker implements TryTreeMatcher { private static final Matcher FAIL_METHOD = expressionStatement( anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("fail"), staticMethod().onClass("org.junit.Assert").named("fail"), staticMethod().onClass("junit.framework.Assert").named("fail"), staticMethod().onClass("junit.framework.TestCase").named("fail"))); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DeadException.java b/core/src/main/java/com/google/errorprone/bugpatterns/DeadException.java index 29f9ef0f4d2..4c56f9cccbd 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DeadException.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DeadException.java @@ -63,7 +63,8 @@ public class DeadException extends BugChecker implements NewClassTreeMatcher { anyOf( enclosingClass(JUnitMatchers.isJUnit3TestClass), enclosingClass(JUnitMatchers.isAmbiguousJUnitVersion), - enclosingClass(JUnitMatchers.isJUnit4TestClass)))); + enclosingClass(JUnitMatchers.isJUnit4TestClass), + enclosingClass(JUnitMatchers.hasJUnit5TestCases)))); @Override public Description matchNewClass(NewClassTree newClassTree, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java index d2fba4acbe9..644ef20b245 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/DoNotCallChecker.java @@ -90,6 +90,13 @@ public class DoNotCallChecker extends BugChecker "A Builder can never compare equal to a MessageLite instance. Use `build()`, or" + " `buildPartial()` on the argument to get a `MessageLite` for comparison" + " instead. Or, if you are passing `null`, use `isNull()`.") + .put( + staticMethod() + .onClass("org.junit.jupiter.api.Assertions") + .named("assertEquals") + .withParameters("double", "double"), + "This method always throws java.lang.AssertionError. Use assertEquals(" + + "expected, actual, delta) to compare floating-point numbers") .put( staticMethod() .onClass("org.junit.Assert") @@ -97,6 +104,13 @@ public class DoNotCallChecker extends BugChecker .withParameters("double", "double"), "This method always throws java.lang.AssertionError. Use assertEquals(" + "expected, actual, delta) to compare floating-point numbers") + .put( + staticMethod() + .onClass("org.junit.jupiter.api.Assertions") + .named("assertEquals") + .withParameters("double", "double", "java.lang.String"), + "This method always throws java.lang.AssertionError. Use assertEquals(" + + "expected, actual, delta, String) to compare floating-point numbers") .put( staticMethod() .onClass("org.junit.Assert") diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsNull.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsNull.java index 089ff310f7b..7131bd96bd4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsNull.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsNull.java @@ -56,7 +56,11 @@ public final class EqualsNull extends BugChecker implements MethodInvocationTree allOf(instanceEqualsInvocation(), argument(0, kindIs(Kind.NULL_LITERAL))); private static final Matcher INSIDE_ASSERT_CLASS = - enclosingClass(anyOf(isSubtypeOf("org.junit.Assert"), isSubtypeOf("junit.framework.Assert"))); + enclosingClass( + anyOf( + isSubtypeOf("org.junit.jupiter.api.Assertions"), + isSubtypeOf("org.junit.Assert"), + isSubtypeOf("junit.framework.Assert"))); private static final Matcher ENCLOSED_BY_ASSERT = enclosingNode( @@ -66,6 +70,7 @@ public final class EqualsNull extends BugChecker implements MethodInvocationTree .onClassAny( "com.google.common.truth.Truth", "com.google.common.truth.Truth8", + "org.junit.jupiter.api.Assertions", "junit.framework.Assert", "org.junit.Assert"))); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java index fb8c403e916..8efbfac3f31 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ExpectedExceptionChecker.java @@ -46,6 +46,7 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.JUnitMatchers; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; @@ -264,7 +265,7 @@ private static SuggestedFix finishFix( return baseFix; } SuggestedFix.Builder fix = baseFix.toBuilder(); - fix.addStaticImport("org.junit.Assert.assertThrows"); + fix.addStaticImport(JUnitMatchers.getAssertionClassName(state) + ".assertThrows"); StringBuilder fixPrefix = new StringBuilder(); String exceptionTypeName = SuggestedFixes.qualifyType(state, fix, exceptionType); if (!newAsserts.isEmpty()) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilon.java b/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilon.java index 8af1ed6022a..8d49a85d9c9 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilon.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilon.java @@ -20,6 +20,7 @@ import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; import static java.util.Locale.ROOT; @@ -36,6 +37,7 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.TypeTag; import java.util.Optional; @@ -144,15 +146,25 @@ Optional suffixLiteralIfPossible(LiteralTree literal, VisitorState state .namedAnyOf("isWithin", "isNotWithin") .withParameters(typeName))); junitWithoutMessage = - staticMethod() - .onClass("org.junit.Assert") - .named("assertEquals") - .withParameters(typeName, typeName, typeName); + anyOf( + staticMethod() + .onClass("org.junit.jupiter.api.Assertions") + .named("assertEquals") + .withParameters(typeName, typeName, typeName), + staticMethod() + .onClass("org.junit.Assert") + .named("assertEquals") + .withParameters(typeName, typeName, typeName)); junitWithMessage = - staticMethod() - .onClass("org.junit.Assert") - .named("assertEquals") - .withParameters("java.lang.String", typeName, typeName, typeName); + anyOf( + staticMethod() + .onClass("org.junit.jupiter.api.Assertions") + .named("assertEquals") + .withParameters(typeName, typeName, typeName, "java.lang.String"), + staticMethod() + .onClass("org.junit.Assert") + .named("assertEquals") + .withParameters("java.lang.String", typeName, typeName, typeName)); } abstract Number nextNumber(Number actual); @@ -167,13 +179,25 @@ private Optional match( return check(tree.getArguments().get(2), tree.getArguments().get(0)) .map( tolerance -> - suggestJunitFix(bugChecker, tree).setMessage(description(tolerance)).build()); + suggestJunitFix(bugChecker, tree, 2) + .setMessage(description(tolerance)) + .build()); } if (junitWithMessage.matches(tree, state)) { - return check(tree.getArguments().get(3), tree.getArguments().get(1)) + // JUnit 4: assertEquals(message, expected, actual, delta) - delta at index 3 + // JUnit 5: assertEquals(expected, actual, delta, message) - delta at index 2 + Symbol sym = ASTHelpers.getSymbol(tree); + boolean isJUnit5 = + sym != null + && sym.owner.getQualifiedName().toString().equals("org.junit.jupiter.api.Assertions"); + int deltaIndex = isJUnit5 ? 2 : 3; + int expectedIndex = isJUnit5 ? 0 : 1; + return check(tree.getArguments().get(deltaIndex), tree.getArguments().get(expectedIndex)) .map( tolerance -> - suggestJunitFix(bugChecker, tree).setMessage(description(tolerance)).build()); + suggestJunitFix(bugChecker, tree, deltaIndex) + .setMessage(description(tolerance)) + .build()); } if (truthOfCall.matches(tree, state)) { return check(getReceiverArgument(tree), getOnlyElement(tree.getArguments())) @@ -219,8 +243,8 @@ private static ExpressionTree getReceiverArgument(MethodInvocationTree tree) { /** Suggest replacing the tolerance with {@code 0} for JUnit assertions. */ private static Description.Builder suggestJunitFix( - BugChecker bugChecker, MethodInvocationTree tree) { - SuggestedFix fix = SuggestedFix.replace(getLast(tree.getArguments()), "0"); + BugChecker bugChecker, MethodInvocationTree tree, int deltaIndex) { + SuggestedFix fix = SuggestedFix.replace(tree.getArguments().get(deltaIndex), "0"); return bugChecker.buildDescription(tree).addFix(fix); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java b/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java index e9195f102bd..c10a83dbc0f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ImpossibleNullComparison.java @@ -96,8 +96,9 @@ public final class ImpossibleNullComparison extends BugChecker private static final Matcher ASSERT_NOT_NULL = anyOf( - staticMethod().onClass("junit.framework.Assert").named("assertNotNull"), - staticMethod().onClass("org.junit.Assert").named("assertNotNull")); + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertNotNull"), + staticMethod().onClass("org.junit.Assert").named("assertNotNull"), + staticMethod().onClass("junit.framework.Assert").named("assertNotNull")); private static final Matcher TRUTH_NOT_NULL = allOf( diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java b/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java index 3b1ded47f9e..f305e79b031 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/InputStreamSlowMultibyteRead.java @@ -117,7 +117,8 @@ private Description maybeMatchReadByte(MethodTree readByteMethod, VisitorState s while (enclosingPath != null) { ClassTree klazz = (ClassTree) enclosingPath.getLeaf(); if (JUnitMatchers.isTestCaseDescendant.matches(klazz, state) - || hasAnnotation(JUnitMatchers.JUNIT4_RUN_WITH_ANNOTATION).matches(klazz, state)) { + || hasAnnotation(JUnitMatchers.JUNIT4_RUN_WITH_ANNOTATION).matches(klazz, state) + || JUnitMatchers.hasJUnit5TestCases.matches(klazz, state)) { return Description.NO_MATCH; } enclosingPath = diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheck.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheck.java index a9f63609151..61603663186 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheck.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheck.java @@ -43,6 +43,8 @@ public class JUnitAssertSameCheck extends BugChecker implements MethodInvocation * Cases: * *

    + *
  1. org.junit.jupiter.api.Assertions.assertSame(a, a); + *
  2. org.junit.jupiter.api.Assertions.assertSame(a, a, "message"); *
  3. org.junit.Assert.assertSame(a, a); *
  4. org.junit.Assert.assertSame("message", a, a); *
  5. junit.framework.Assert.assertSame(a, a); @@ -50,7 +52,10 @@ public class JUnitAssertSameCheck extends BugChecker implements MethodInvocation *
*/ private static final Matcher ASSERT_SAME_MATCHER = - staticMethod().onClassAny("org.junit.Assert", "junit.framework.Assert").named("assertSame"); + staticMethod() + .onClassAny( + "org.junit.jupiter.api.Assertions", "org.junit.Assert", "junit.framework.Assert") + .named("assertSame"); @Override public Description matchMethodInvocation( @@ -65,9 +70,15 @@ public Description matchMethodInvocation( return describeMatch(methodInvocationTree); } - // cases: assertSame("message", a, a); - if (args.size() == 3 && ASTHelpers.sameVariable(args.get(1), args.get(2))) { - return describeMatch(methodInvocationTree); + if (args.size() == 3) { + // JUnit 4: assertSame("message", a, a) - message first + if (ASTHelpers.sameVariable(args.get(1), args.get(2))) { + return describeMatch(methodInvocationTree); + } + // JUnit 5: assertSame(a, a, "message") - message last + if (ASTHelpers.sameVariable(args.get(0), args.get(1))) { + return describeMatch(methodInvocationTree); + } } return Description.NO_MATCH; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4ClassAnnotationNonStatic.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitClassAnnotationNonStatic.java similarity index 79% rename from core/src/main/java/com/google/errorprone/bugpatterns/JUnit4ClassAnnotationNonStatic.java rename to core/src/main/java/com/google/errorprone/bugpatterns/JUnitClassAnnotationNonStatic.java index 566f56a0ae1..6282e622eac 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4ClassAnnotationNonStatic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitClassAnnotationNonStatic.java @@ -20,6 +20,8 @@ import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_AFTER_CLASS_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_BEFORE_CLASS_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_AFTER_ALL_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_BEFORE_ALL_ANNOTATION; import static com.google.errorprone.matchers.Matchers.annotations; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.isStatic; @@ -41,14 +43,24 @@ import java.util.stream.Collectors; import javax.lang.model.element.Modifier; -/** {@code @BeforeClass} or {@code @AfterClass} should be applied to static methods. */ -@BugPattern(summary = "This method should be static", severity = ERROR) -public class JUnit4ClassAnnotationNonStatic extends BugChecker implements MethodTreeMatcher { +/** + * {@code @BeforeClass}, {@code @AfterClass}, {@code @BeforeAll}, or {@code @AfterAll} should be + * applied to static methods. + */ +@BugPattern( + summary = "This method should be static", + severity = ERROR, + altNames = {"JUnit4ClassAnnotationNonStatic"}) +public class JUnitClassAnnotationNonStatic extends BugChecker implements MethodTreeMatcher { private static final MultiMatcher CLASS_INIT_ANNOTATION = annotations( AT_LEAST_ONE, - anyOf(isType(JUNIT_AFTER_CLASS_ANNOTATION), isType(JUNIT_BEFORE_CLASS_ANNOTATION))); + anyOf( + isType(JUNIT_AFTER_CLASS_ANNOTATION), + isType(JUNIT_BEFORE_CLASS_ANNOTATION), + isType(JUNIT5_BEFORE_ALL_ANNOTATION), + isType(JUNIT5_AFTER_ALL_ANNOTATION))); @Override public Description matchMethod(MethodTree tree, VisitorState state) { @@ -66,8 +78,6 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .build(); } - // Might be a bit overkill just in case people add @BeforeClass and @AfterClass to the same - // method. private static String messageForAnnos(List annotationTrees) { String annoNames = annotationTrees.stream() diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4EmptyMethods.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitEmptyLifecycleMethods.java similarity index 77% rename from core/src/main/java/com/google/errorprone/bugpatterns/JUnit4EmptyMethods.java rename to core/src/main/java/com/google/errorprone/bugpatterns/JUnitEmptyLifecycleMethods.java index 429984c66fb..0af542c8958 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4EmptyMethods.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitEmptyLifecycleMethods.java @@ -34,17 +34,18 @@ import java.util.List; /** - * Deletes empty JUnit4 {@code @Before}, {@code @After}, {@code @BeforeClass}, and - * {@code @AfterClass} methods. + * Deletes empty JUnit lifecycle methods ({@code @Before}, {@code @After}, {@code @BeforeClass}, + * {@code @AfterClass}, {@code @BeforeEach}, {@code @AfterEach}, {@code @BeforeAll}, {@code + * @AfterAll}). * * @author kak@google.com (Kurt Alfred Kluever) */ @BugPattern( summary = - "Empty JUnit4 @Before, @After, @BeforeClass, and @AfterClass methods are unnecessary and" - + " should be deleted.", - severity = WARNING) -public final class JUnit4EmptyMethods extends BugChecker implements MethodTreeMatcher { + "Empty JUnit lifecycle methods are unnecessary and should be deleted.", + severity = WARNING, + altNames = {"JUnit4EmptyMethods"}) +public final class JUnitEmptyLifecycleMethods extends BugChecker implements MethodTreeMatcher { private static final Matcher JUNIT_METHODS = anyOf( @@ -53,7 +54,11 @@ public final class JUnit4EmptyMethods extends BugChecker implements MethodTreeMa hasAnnotation(JUnitMatchers.JUNIT_BEFORE_CLASS_ANNOTATION), hasAnnotation(JUnitMatchers.JUNIT_AFTER_CLASS_ANNOTATION), hasAnnotation(JUnitMatchers.JUNIT_BEFORE_ANNOTATION), - hasAnnotation(JUnitMatchers.JUNIT_AFTER_ANNOTATION)); + hasAnnotation(JUnitMatchers.JUNIT_AFTER_ANNOTATION), + hasAnnotation(JUnitMatchers.JUNIT5_BEFORE_EACH_ANNOTATION), + hasAnnotation(JUnitMatchers.JUNIT5_AFTER_EACH_ANNOTATION), + hasAnnotation(JUnitMatchers.JUNIT5_BEFORE_ALL_ANNOTATION), + hasAnnotation(JUnitMatchers.JUNIT5_AFTER_ALL_ANNOTATION)); @Override public Description matchMethod(MethodTree method, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4SetUpNotRun.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitSetUpNotRun.java similarity index 55% rename from core/src/main/java/com/google/errorprone/bugpatterns/JUnit4SetUpNotRun.java rename to core/src/main/java/com/google/errorprone/bugpatterns/JUnitSetUpNotRun.java index a6d38d6aeec..124fff649d2 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4SetUpNotRun.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitSetUpNotRun.java @@ -21,7 +21,15 @@ import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_AFTER_CLASS_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_BEFORE_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_BEFORE_CLASS_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_AFTER_EACH_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_AFTER_ALL_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_BEFORE_EACH_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_BEFORE_ALL_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit4BeforeAnnotations; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5BeforeAll; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5BeforeEach; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5TestCases; +import static com.google.errorprone.matchers.JUnitMatchers.isJUnit5TestClass; import static com.google.errorprone.matchers.JUnitMatchers.looksLikeJUnit3SetUp; import static com.google.errorprone.matchers.JUnitMatchers.looksLikeJUnit4Before; import static com.google.errorprone.matchers.Matchers.allOf; @@ -29,34 +37,45 @@ import static com.google.errorprone.matchers.Matchers.not; import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.MethodTree; import java.util.Arrays; import java.util.List; /** - * Checks for the existence of a JUnit3 style setUp() method in a JUnit4 test class or methods - * annotated with a non-JUnit4 @Before annotation. + * Checks for the existence of a setUp() method in a JUnit test class that will not be run; suggests + * adding the appropriate lifecycle annotation. * * @author glorioso@google.com (Nick Glorioso) */ @BugPattern( - summary = "setUp() method will not be run; please add JUnit's @Before annotation", - severity = ERROR) -public class JUnit4SetUpNotRun extends AbstractJUnit4InitMethodNotRun { + summary = + "setUp() method will not be run; please add JUnit's @Before or @BeforeEach annotation", + severity = ERROR, + altNames = {"JUnit4SetUpNotRun"}) +public class JUnitSetUpNotRun extends AbstractJUnit4InitMethodNotRun { @Override protected Matcher methodMatcher() { return allOf( - anyOf(looksLikeJUnit3SetUp, looksLikeJUnit4Before), not(hasJUnit4BeforeAnnotations)); + anyOf(looksLikeJUnit3SetUp, looksLikeJUnit4Before), + not(hasJUnit4BeforeAnnotations), + not(hasJUnit5BeforeEach), + not(hasJUnit5BeforeAll)); } @Override - protected String correctAnnotation() { - return JUNIT_BEFORE_ANNOTATION; + protected String correctAnnotation(VisitorState state) { + return isJUnit5TestClass(state) ? JUNIT5_BEFORE_EACH_ANNOTATION : JUNIT_BEFORE_ANNOTATION; } @Override - protected List annotationReplacements() { + protected List annotationReplacements(VisitorState state) { + if (isJUnit5TestClass(state)) { + return Arrays.asList( + new AnnotationReplacements(JUNIT5_AFTER_EACH_ANNOTATION, JUNIT5_BEFORE_EACH_ANNOTATION), + new AnnotationReplacements(JUNIT5_AFTER_ALL_ANNOTATION, JUNIT5_BEFORE_ALL_ANNOTATION)); + } return Arrays.asList( new AnnotationReplacements(JUNIT_AFTER_ANNOTATION, JUNIT_BEFORE_ANNOTATION), new AnnotationReplacements(JUNIT_AFTER_CLASS_ANNOTATION, JUNIT_BEFORE_CLASS_ANNOTATION)); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TearDownNotRun.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitTearDownNotRun.java similarity index 56% rename from core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TearDownNotRun.java rename to core/src/main/java/com/google/errorprone/bugpatterns/JUnitTearDownNotRun.java index 0dadd9d3279..96ba571f398 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TearDownNotRun.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitTearDownNotRun.java @@ -21,7 +21,14 @@ import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_AFTER_CLASS_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_BEFORE_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_BEFORE_CLASS_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_AFTER_EACH_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_AFTER_ALL_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_BEFORE_EACH_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_BEFORE_ALL_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit4AfterAnnotations; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5AfterAll; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5AfterEach; +import static com.google.errorprone.matchers.JUnitMatchers.isJUnit5TestClass; import static com.google.errorprone.matchers.JUnitMatchers.looksLikeJUnit3TearDown; import static com.google.errorprone.matchers.JUnitMatchers.looksLikeJUnit4After; import static com.google.errorprone.matchers.Matchers.allOf; @@ -29,34 +36,45 @@ import static com.google.errorprone.matchers.Matchers.not; import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.MethodTree; import java.util.Arrays; import java.util.List; /** - * Checks for the existence of a JUnit3 style tearDown() method in a JUnit4 test class or methods - * annotated with a non-JUnit4 @After annotation. + * Checks for the existence of a tearDown() method in a JUnit test class that will not be run; + * suggests adding the appropriate lifecycle annotation. * * @author glorioso@google.com (Nick Glorioso) */ @BugPattern( - summary = "tearDown() method will not be run; please add JUnit's @After annotation", - severity = ERROR) -public class JUnit4TearDownNotRun extends AbstractJUnit4InitMethodNotRun { + summary = + "tearDown() method will not be run; please add JUnit's @After or @AfterEach annotation", + severity = ERROR, + altNames = {"JUnit4TearDownNotRun"}) +public class JUnitTearDownNotRun extends AbstractJUnit4InitMethodNotRun { @Override protected Matcher methodMatcher() { return allOf( - anyOf(looksLikeJUnit3TearDown, looksLikeJUnit4After), not(hasJUnit4AfterAnnotations)); + anyOf(looksLikeJUnit3TearDown, looksLikeJUnit4After), + not(hasJUnit4AfterAnnotations), + not(hasJUnit5AfterEach), + not(hasJUnit5AfterAll)); } @Override - protected String correctAnnotation() { - return JUNIT_AFTER_ANNOTATION; + protected String correctAnnotation(VisitorState state) { + return isJUnit5TestClass(state) ? JUNIT5_AFTER_EACH_ANNOTATION : JUNIT_AFTER_ANNOTATION; } @Override - protected List annotationReplacements() { + protected List annotationReplacements(VisitorState state) { + if (isJUnit5TestClass(state)) { + return Arrays.asList( + new AnnotationReplacements(JUNIT5_BEFORE_EACH_ANNOTATION, JUNIT5_AFTER_EACH_ANNOTATION), + new AnnotationReplacements(JUNIT5_BEFORE_ALL_ANNOTATION, JUNIT5_AFTER_ALL_ANNOTATION)); + } return Arrays.asList( new AnnotationReplacements(JUNIT_BEFORE_ANNOTATION, JUNIT_AFTER_ANNOTATION), new AnnotationReplacements(JUNIT_BEFORE_CLASS_ANNOTATION, JUNIT_AFTER_CLASS_ANNOTATION)); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TestNotRun.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitTestNotRun.java similarity index 90% rename from core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TestNotRun.java rename to core/src/main/java/com/google/errorprone/bugpatterns/JUnitTestNotRun.java index 859b474c425..026dbc2f8f0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TestNotRun.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JUnitTestNotRun.java @@ -20,6 +20,7 @@ import static com.google.errorprone.fixes.SuggestedFix.emptyFix; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.JUnitMatchers.isJUnit4TestClass; +import static com.google.errorprone.matchers.JUnitMatchers.isJUnit5TestClass; import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.hasModifier; @@ -66,10 +67,11 @@ */ @BugPattern( summary = - "This looks like a test method but is not run; please add @Test and @Ignore, or, if this" - + " is a helper method, reduce its visibility.", - severity = ERROR) -public class JUnit4TestNotRun extends BugChecker implements ClassTreeMatcher { + "This looks like a test method but is not run; please add @Test and @Ignore or @Disabled," + + " or, if this is a helper method, reduce its visibility.", + severity = ERROR, + altNames = {"JUnit4TestNotRun"}) +public class JUnitTestNotRun extends BugChecker implements ClassTreeMatcher { private static final Matcher POSSIBLE_TEST_METHOD = allOf( hasModifier(PUBLIC), @@ -117,11 +119,11 @@ private static boolean isParameterAnnotation(AnnotationTree annotation, VisitorS private static final Matcher NOT_STATIC = not(hasModifier(STATIC)); @Inject - JUnit4TestNotRun() {} + JUnitTestNotRun() {} @Override public Description matchClass(ClassTree tree, VisitorState state) { - if (!isJUnit4TestClass.matches(tree, state)) { + if (!anyOf(isJUnit4TestClass, JUnitMatchers.hasJUnit5TestCases).matches(tree, state)) { return NO_MATCH; } Map suspiciousMethods = new HashMap<>(); @@ -218,27 +220,33 @@ private Optional handleMethod(MethodTree methodTree, VisitorState s * *
    *
  1. Add @Test, remove static modifier if present. - *
  2. Add @Test and @Ignore, remove static modifier if present. + *
  3. Add @Test and @Ignore/@Disabled, remove static modifier if present. *
  4. Change visibility to private (for local helper methods). *
*/ private Description describeFixes(MethodTree methodTree, VisitorState state) { + boolean isJUnit5 = isJUnit5TestClass(state); Optional removeStatic = SuggestedFixes.removeModifiers(methodTree, state, Modifier.STATIC); + + String testAnnotation = isJUnit5 ? "org.junit.jupiter.api.Test" : "org.junit.Test"; + String disableAnnotation = + isJUnit5 ? "org.junit.jupiter.api.Disabled" : "org.junit.Ignore"; + SuggestedFix testFix = removeStatic.orElse(emptyFix()).toBuilder() - .addImport("org.junit.Test") + .addImport(testAnnotation) .prefixWith(methodTree, "@Test ") .build(); SuggestedFix ignoreFix = testFix.toBuilder() - .addImport("org.junit.Ignore") - .prefixWith(methodTree, "@Ignore ") + .addImport(disableAnnotation) + .prefixWith(methodTree, "@" + disableAnnotation.substring(disableAnnotation.lastIndexOf('.') + 1) + " ") .build(); SuggestedFix visibilityFix = SuggestedFixes.Visibility.PRIVATE.refactor(methodTree, state); - // Suggest @Ignore first if test method is named like a purposely disabled test. + // Suggest @Ignore/@Disabled first if test method is named like a purposely disabled test. String methodName = methodTree.getName().toString(); if (methodName.startsWith("disabl") || methodName.startsWith("ignor")) { return buildDescription(methodTree) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MissingFail.java b/core/src/main/java/com/google/errorprone/bugpatterns/MissingFail.java index fac29f88a11..51968afab08 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/MissingFail.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MissingFail.java @@ -22,6 +22,7 @@ import static com.google.errorprone.matchers.JUnitMatchers.JUNIT_BEFORE_ANNOTATION; import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit4TestCases; import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit4TestRunner; +import static com.google.errorprone.matchers.JUnitMatchers.hasJUnit5TestCases; import static com.google.errorprone.matchers.JUnitMatchers.isTestCaseDescendant; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.assertStatement; @@ -116,11 +117,13 @@ public class MissingFail extends BugChecker implements MethodTreeMatcher { private static final Matcher ASSERT_TRUE = Matchers.anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertTrue"), staticMethod().onClass("org.junit.Assert").named("assertTrue"), staticMethod().onClass("junit.framework.Assert").named("assertTrue"), staticMethod().onClass("junit.framework.TestCase").named("assertTrue")); private static final Matcher ASSERT_FALSE = Matchers.anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertFalse"), staticMethod().onClass("org.junit.Assert").named("assertFalse"), staticMethod().onClass("junit.framework.Assert").named("assertFalse"), staticMethod().onClass("junit.framework.TestCase").named("assertFalse")); @@ -224,7 +227,7 @@ public class MissingFail extends BugChecker implements MethodTreeMatcher { // Subtly different from JUnitMatchers: We want to match test base classes too. private static final Matcher TEST_CLASS = - Matchers.anyOf(isTestCaseDescendant, hasJUnit4TestRunner, hasJUnit4TestCases); + Matchers.anyOf(isTestCaseDescendant, hasJUnit4TestRunner, hasJUnit4TestCases, hasJUnit5TestCases); @Override public Description matchMethod(MethodTree tree, VisitorState state) { @@ -267,7 +270,14 @@ public static Fix addFailCall(TryTree tree, StatementTree lastTryStatement, Visi // Make sure that when the fail import is added it doesn't conflict with existing ones. fixBuilder.removeStaticImport("junit.framework.Assert.fail"); fixBuilder.removeStaticImport("junit.framework.TestCase.fail"); - fixBuilder.addStaticImport("org.junit.Assert.fail"); + String failClass = JUnitMatchers.getAssertionClassName(state); + // Remove the opposite assertion class's fail import to prevent conflicts + String oppositeClass = + failClass.equals(JUnitMatchers.JUNIT5_ASSERT_CLASS) + ? "org.junit.Assert" + : JUnitMatchers.JUNIT5_ASSERT_CLASS; + fixBuilder.removeStaticImport(oppositeClass + ".fail"); + fixBuilder.addStaticImport(failClass + ".fail"); return fixBuilder.build(); } @@ -473,7 +483,9 @@ public boolean matches(TryTree tryTree, VisitorState state) { // TODO(schmitt): Move to JUnitMatchers? || name.contentEquals("suite") || Matchers.hasAnnotation(JUNIT_BEFORE_ANNOTATION).matches(enclosingMethodTree, state) - || Matchers.hasAnnotation(JUNIT_AFTER_ANNOTATION).matches(enclosingMethodTree, state); + || Matchers.hasAnnotation(JUNIT_AFTER_ANNOTATION).matches(enclosingMethodTree, state) + || JUnitMatchers.hasJUnit5BeforeAnnotations.matches(enclosingMethodTree, state) + || JUnitMatchers.hasJUnit5AfterAnnotations.matches(enclosingMethodTree, state); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssertion.java b/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssertion.java index bb01f21d2b5..9df7dafc909 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssertion.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/SelfAssertion.java @@ -62,7 +62,10 @@ public final class SelfAssertion extends BugChecker implements MethodInvocationT allOf( staticMethod() .onClassAny( - "org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .namedAnyOf("assertEquals", "assertArrayEquals"), this::junitSameArguments)); @@ -74,7 +77,10 @@ public final class SelfAssertion extends BugChecker implements MethodInvocationT allOf( staticMethod() .onClassAny( - "org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .namedAnyOf("assertNotEquals"), this::junitSameArguments)); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java index 648c41c0db8..97b2ee34925 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TestExceptionChecker.java @@ -94,7 +94,7 @@ private static SuggestedFix buildFix( if (statements.isEmpty()) { return fix.build(); } - fix.addStaticImport("org.junit.Assert.assertThrows"); + fix.addStaticImport(JUnitMatchers.getAssertionClassName(state) + ".assertThrows"); StringBuilder prefix = new StringBuilder(); prefix.append( String.format("assertThrows(%s, () -> ", state.getSourceForNode(expectedException))); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java b/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java index 5f73787b458..2d4ce294707 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TooManyParameters.java @@ -58,6 +58,7 @@ public class TooManyParameters extends BugChecker implements MethodTreeMatcher { // parameters, unless it's a parameterized test --- which still are not directly // invoked!); see b/303486200 "org.junit.Test", + "org.junit.jupiter.api.Test", // dagger provider / producers "dagger.Provides", "dagger.producers.Produces", diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TryFailThrowable.java b/core/src/main/java/com/google/errorprone/bugpatterns/TryFailThrowable.java index ef20b4cf629..81f17d6fe6f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/TryFailThrowable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/TryFailThrowable.java @@ -39,6 +39,7 @@ import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.JUnitMatchers; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; import com.sun.source.tree.BlockTree; @@ -117,7 +118,8 @@ public class TryFailThrowable extends BugChecker implements TryTreeMatcher { String className = sym.owner.getQualifiedName().toString(); // TODO(cpovirk): Look for literal "throw new AssertionError()," etc. return (methodName.startsWith("assert") || methodName.startsWith("fail")) - && (className.equals("org.junit.Assert") + && (className.equals("org.junit.jupiter.api.Assertions") + || className.equals("org.junit.Assert") || className.equals("junit.framework.Assert") || className.equals("junit.framework.TestCase") || className.endsWith("MoreAsserts")); @@ -169,9 +171,9 @@ private static Fix fixWithReturn( SuggestedFix.Builder builder = SuggestedFix.builder(); builder.delete(failStatement); builder.replace(getOnlyCatch(tryTree).getBlock(), "{ return; }"); - // TODO(cpovirk): Use the file's preferred assertion API. String messageSnippet = getMessageSnippet(failStatement, state, HasOtherParameters.FALSE); builder.postfixWith(tryTree, format("fail(%s);", messageSnippet)); + builder.addStaticImport(getAssertionClass(failStatement, state) + ".fail"); return builder.build(); } @@ -181,9 +183,9 @@ private static Fix fixWithBoolean( builder.delete(failStatement); builder.prefixWith(tryTree, "boolean threw = false;"); builder.replace(getOnlyCatch(tryTree).getBlock(), "{ threw = true; }"); - // TODO(cpovirk): Use the file's preferred assertion API. String messageSnippet = getMessageSnippet(failStatement, state, HasOtherParameters.TRUE); builder.postfixWith(tryTree, format("assertTrue(%sthrew);", messageSnippet)); + builder.addStaticImport(getAssertionClass(failStatement, state) + ".assertTrue"); return builder.build(); } @@ -199,6 +201,16 @@ private static String getMessageSnippet( : ""; } + private static String getAssertionClass(StatementTree failStatement, VisitorState state) { + ExpressionTree expression = ((ExpressionStatementTree) failStatement).getExpression(); + Symbol sym = getSymbol(expression); + if (sym != null + && sym.owner.getQualifiedName().toString().equals(JUnitMatchers.JUNIT5_ASSERT_CLASS)) { + return JUnitMatchers.JUNIT5_ASSERT_CLASS; + } + return JUnitMatchers.JUNIT4_ASSERT_CLASS; + } + /** * Whether the assertion method we're inserting a call to has extra parameters besides its message * (like {@code assertTrue}) or not (like {@code fail}). diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryTestMethodPrefix.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryTestMethodPrefix.java index dbf4e3acd6c..b04f2eb3c6f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryTestMethodPrefix.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryTestMethodPrefix.java @@ -20,6 +20,8 @@ import static com.google.errorprone.fixes.SuggestedFixes.renameMethod; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.JUnitMatchers.JUNIT4_TEST_ANNOTATION; +import static com.google.errorprone.matchers.JUnitMatchers.JUNIT5_TEST_ANNOTATION; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.hasAnnotation; import com.google.errorprone.BugPattern; @@ -72,5 +74,6 @@ public Void visitMethod(MethodTree tree, Void unused) { return NO_MATCH; } - private static final Matcher TEST = hasAnnotation(JUNIT4_TEST_ANNOTATION); + private static final Matcher TEST = + anyOf(hasAnnotation(JUNIT4_TEST_ANNOTATION), hasAnnotation(JUNIT5_TEST_ANNOTATION)); } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java b/core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java index ff518362af8..120a8b5c2fa 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/Varifier.java @@ -70,7 +70,9 @@ public final class Varifier extends BugChecker implements VariableTreeMatcher { }); private static final Matcher ASSERT_THROWS = - staticMethod().onClass("org.junit.Assert").named("assertThrows"); + anyOf( + staticMethod().onClass("org.junit.jupiter.api.Assertions").named("assertThrows"), + staticMethod().onClass("org.junit.Assert").named("assertThrows")); @Override public Description matchVariable(VariableTree tree, VisitorState state) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java index 26aa86b7e92..97805b32299 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java @@ -108,9 +108,10 @@ private static boolean isThreeParameterAssert(MethodInvocationTree tree, Visitor allOf( staticMethod() .onClassAny( + "org.junit.jupiter.api.Assertions", "org.junit.Assert", - "junit.framework.TestCase", "junit.framework.Assert", + "junit.framework.TestCase", /* this final case is to allow testing without using the junit classes. we need to do this because the junit dependency might not have been compiled with parameters information which would cause the tests to fail.*/ diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AssertSameIncompatible.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AssertSameIncompatible.java index 6ca8d4232d1..8007f32bd78 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AssertSameIncompatible.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/AssertSameIncompatible.java @@ -45,7 +45,11 @@ public final class AssertSameIncompatible extends BugChecker implements MethodInvocationTreeMatcher { private static final Matcher JUNIT_MATCHER = staticMethod() - .onClassAny("org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .onClassAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .namedAnyOf("assertSame", "assertNotSame"); private static final Matcher TRUTH_MATCHER = diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java index d79469c7e5e..b755485f211 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleType.java @@ -47,7 +47,11 @@ public final class JUnitIncompatibleType extends BugChecker implements MethodInv private static final Matcher ASSERT_EQUALS = allOf( staticMethod() - .onClassAny("org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .onClassAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .namedAnyOf("assertEquals", "assertNotEquals"), anyOf( staticMethod() @@ -57,11 +61,19 @@ public final class JUnitIncompatibleType extends BugChecker implements MethodInv staticMethod() .anyClass() .withAnyName() - .withParameters("java.lang.String", "java.lang.Object", "java.lang.Object"))); + .withParameters("java.lang.String", "java.lang.Object", "java.lang.Object"), + staticMethod() + .anyClass() + .withAnyName() + .withParameters("java.lang.Object", "java.lang.Object", "java.lang.String"))); private static final Matcher ASSERT_ARRAY_EQUALS = staticMethod() - .onClassAny("org.junit.Assert", "junit.framework.Assert", "junit.framework.TestCase") + .onClassAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert", + "junit.framework.TestCase") .namedAnyOf("assertArrayEquals"); private final TypeCompatibility typeCompatibility; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java index 50775e21b58..ae054edd5dc 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/ReturnMissingNullable.java @@ -107,7 +107,10 @@ public class ReturnMissingNullable extends BugChecker implements CompilationUnit * and b/130658266 (which makes MethodMatchers look at the Tree again to extract * the receiver type, which sidesteps the getSymbol fix). */ - .onDescendantOfAny("org.junit.Assert", "junit.framework.Assert") + .onDescendantOfAny( + "org.junit.jupiter.api.Assertions", + "org.junit.Assert", + "junit.framework.Assert") .named("fail"), instanceMethod().onDescendantOf("java.lang.Runtime").namedAnyOf("exit", "halt"), staticMethod().onClass("java.lang.System").named("exit"))); diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 2086967accb..2f7652222aa 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -215,17 +215,17 @@ import com.google.errorprone.bugpatterns.IterablePathParameter; import com.google.errorprone.bugpatterns.JUnit3FloatingPointComparisonWithoutDelta; import com.google.errorprone.bugpatterns.JUnit3TestNotRun; -import com.google.errorprone.bugpatterns.JUnit4ClassAnnotationNonStatic; import com.google.errorprone.bugpatterns.JUnit4ClassUsedInJUnit3; -import com.google.errorprone.bugpatterns.JUnit4EmptyMethods; -import com.google.errorprone.bugpatterns.JUnit4SetUpNotRun; -import com.google.errorprone.bugpatterns.JUnit4TearDownNotRun; -import com.google.errorprone.bugpatterns.JUnit4TestNotRun; import com.google.errorprone.bugpatterns.JUnit4TestsNotRunWithinEnclosed; import com.google.errorprone.bugpatterns.JUnitAmbiguousTestClass; import com.google.errorprone.bugpatterns.JUnitAssertSameCheck; +import com.google.errorprone.bugpatterns.JUnitClassAnnotationNonStatic; +import com.google.errorprone.bugpatterns.JUnitEmptyLifecycleMethods; import com.google.errorprone.bugpatterns.JUnitMethodInvoked; import com.google.errorprone.bugpatterns.JUnitParameterMethodNotFound; +import com.google.errorprone.bugpatterns.JUnitSetUpNotRun; +import com.google.errorprone.bugpatterns.JUnitTearDownNotRun; +import com.google.errorprone.bugpatterns.JUnitTestNotRun; import com.google.errorprone.bugpatterns.JavaUtilDateChecker; import com.google.errorprone.bugpatterns.JdkObsolete; import com.google.errorprone.bugpatterns.LabelledBreakTarget; @@ -806,13 +806,13 @@ public static ScannerSupplier warningChecks() { IsInstanceOfClass.class, IsLoggableTagLength.class, JUnit3TestNotRun.class, - JUnit4ClassAnnotationNonStatic.class, - JUnit4SetUpNotRun.class, - JUnit4TearDownNotRun.class, - JUnit4TestNotRun.class, JUnit4TestsNotRunWithinEnclosed.class, JUnitAssertSameCheck.class, + JUnitClassAnnotationNonStatic.class, JUnitParameterMethodNotFound.class, + JUnitSetUpNotRun.class, + JUnitTearDownNotRun.class, + JUnitTestNotRun.class, JavaxInjectOnAbstractMethod.class, JodaToSelf.class, LabelledBreakTarget.class, @@ -1048,8 +1048,8 @@ public static ScannerSupplier warningChecks() { IterableAndIterator.class, JUnit3FloatingPointComparisonWithoutDelta.class, JUnit4ClassUsedInJUnit3.class, - JUnit4EmptyMethods.class, JUnitAmbiguousTestClass.class, + JUnitEmptyLifecycleMethods.class, JUnitIncompatibleType.class, JUnitMethodInvoked.class, JavaDurationGetSecondsGetNano.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpressionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpressionTest.java index 93f3f6606f1..2dffb45d1e7 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpressionTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsBlockToExpressionTest.java @@ -165,4 +165,39 @@ void f() { """) .doTest(); } + + @Test + public void refactoringJUnit5() { + compilationHelper + .addInputLines( + "Test.java", + """ + import static org.junit.jupiter.api.Assertions.assertThrows; + + class Test { + void f() { + assertThrows( + IllegalStateException.class, + () -> { + System.err.println(); + }); + assertThrows(IllegalStateException.class, () -> System.err.println()); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static org.junit.jupiter.api.Assertions.assertThrows; + + class Test { + void f() { + assertThrows(IllegalStateException.class, () -> System.err.println()); + assertThrows(IllegalStateException.class, () -> System.err.println()); + } + } + """) + .allowFormattingErrors() + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizerTest.java index 2e8a29c718e..0049e60316d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMinimizerTest.java @@ -1193,4 +1193,56 @@ void f() { """) .doTest(); } + + @Test + public void refactorJUnit5() { + compilationHelper + .addInputLines( + "Test.java", + """ + class Test { + void f() { + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + Foo.builder().setBar(new Bar()); + }); + } + } + """) + .addOutputLines( + "Test.java", + """ + class Test { + void f() { + Foo.Builder builder = Foo.builder(); + Bar bar = new Bar(); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, () -> builder.setBar(bar)); + } + } + """) + .doTest(); + } + + @Test + public void negativeJUnit5() { + compilationHelper + .addInputLines( + "Test.java", + """ + import com.google.common.collect.ImmutableList; + + class Test { + void f() { + ImmutableList.Builder builder = + ImmutableList.builder().add(1).add(Integer.valueOf(2)); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, () -> builder.add(2)); + } + } + """) + .expectUnchanged() + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatementsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatementsTest.java index e602e9e40f6..5576ae10e06 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatementsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AssertThrowsMultipleStatementsTest.java @@ -147,4 +147,61 @@ void f() { """) .doTest(); } + + @Test + public void complexSingleStatementLambdasJUnit5() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Test { + void f() { + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, () -> {}); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + System.err.println(); + }); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + int x = 1; + }); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + // BUG: Diagnostic contains: + if (true) { + System.err.println(); + } + }); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + // BUG: Diagnostic contains: + try { + System.err.println(); + } catch (Exception e) { + } + }); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + // BUG: Diagnostic contains: + { + System.err.println(); + } + }); + org.junit.jupiter.api.Assertions.assertThrows( + IllegalStateException.class, + () -> { + // BUG: Diagnostic contains: + return; + }); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AssertionFailureIgnoredTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AssertionFailureIgnoredTest.java index 92a7e8d7645..2acac0a78ff 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/AssertionFailureIgnoredTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AssertionFailureIgnoredTest.java @@ -285,4 +285,70 @@ void f() { """) .doTest(); } + + @Test + public void refactoringJUnit5() { + BugCheckerRefactoringTestHelper.newInstance(AssertionFailureIgnored.class, getClass()) + .addInputLines( + "in/Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import static org.junit.jupiter.api.Assertions.assertThrows; + + import java.io.IOException; + import org.junit.jupiter.api.Assertions; + + class Test { + void f() { + AssertionError t = assertThrows(AssertionError.class, () -> System.err.println()); + assertThat(t).isInstanceOf(AssertionError.class); + assertThrows(AssertionError.class, () -> System.err.println()); + + try { + if (true) throw new IOException(); + Assertions.fail(); + } catch (AssertionError e) { + } catch (Exception e) { + } + try { + if (true) throw new NoSuchFieldException(); + if (true) throw new NoSuchMethodException(); + Assertions.fail(); + } catch (AssertionError | NoSuchFieldException | NoSuchMethodException e) { + } + } + } + """) + .addOutputLines( + "out/Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import static org.junit.jupiter.api.Assertions.assertThrows; + + import java.io.IOException; + import org.junit.jupiter.api.Assertions; + + class Test { + void f() { + AssertionError t = assertThrows(AssertionError.class, () -> System.err.println()); + assertThat(t).isInstanceOf(AssertionError.class); + assertThrows(AssertionError.class, () -> System.err.println()); + + try { + if (true) throw new IOException(); + Assertions.fail(); + } catch (AssertionError e) { + } catch (Exception e) { + } + try { + if (true) throw new NoSuchFieldException(); + if (true) throw new NoSuchMethodException(); + Assertions.fail(); + } catch (AssertionError | NoSuchFieldException | NoSuchMethodException e) { + } + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/CatchFailTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/CatchFailTest.java index 9f27dcd8815..4c0ca9f3c83 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/CatchFailTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/CatchFailTest.java @@ -338,4 +338,84 @@ public void f() { """) .doTest(); } + + @Test + public void positiveJUnit5() { + testHelper + .addInputLines( + "in/Foo.java", + """ + import org.junit.jupiter.api.Test; + + class Foo { + @Test + public void f() { + try { + System.err.println(); + } catch (Exception expected) { + org.junit.jupiter.api.Assertions.fail(); + } + } + } + """) + .addOutputLines( + "out/Foo.java", + """ + import org.junit.jupiter.api.Test; + + class Foo { + @Test + public void f() throws Exception { + System.err.println(); + } + } + """) + .doTest(); + } + + @Test + public void negativeJUnit5() { + testHelper + .addInputLines( + "in/Foo.java", + """ + import org.junit.jupiter.api.Test; + + class Foo { + public void f() { + try { + System.err.println(); + } catch (Exception expected) { + // BUG: Diagnostic contains: + org.junit.jupiter.api.Assertions.fail(); + } + } + } + """) + .expectUnchanged() + .doTest(); + } + + @Test + public void useExceptionJUnit5() { + testHelper + .addInputLines( + "in/Foo.java", + """ + import org.junit.jupiter.api.Test; + + class Foo { + @Test + public void f() { + try { + System.err.println(); + } catch (Exception expected) { + org.junit.jupiter.api.Assertions.fail("oh no " + expected); + } + } + } + """) + .expectUnchanged() + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/DoNotCallCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/DoNotCallCheckerTest.java index 7c47484279f..da85be9513a 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/DoNotCallCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/DoNotCallCheckerTest.java @@ -358,6 +358,24 @@ public void thirdParty() { .doTest(); } + @Test + public void assertEqualJUnit5() { + testHelper + .addSourceLines( + "Test.java", + """ + class Test { + public void foo() { + // BUG: Diagnostic contains: DoNotCall + org.junit.jupiter.api.Assertions.assertEquals(2.0, 2.0); + // These are OK since they pass a tolerance + org.junit.jupiter.api.Assertions.assertEquals(2.0, 2.0, 0.01); + } + } + """) + .doTest(); + } + @Test public void javaSqlDate_toInstant() { assertThrows(UnsupportedOperationException.class, () -> new Date(1234567890L).toInstant()); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsNullTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EqualsNullTest.java index 26cdb4ed71a..e9952878054 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsNullTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/EqualsNullTest.java @@ -257,4 +257,58 @@ boolean m(Object x) { """) .doTest(); } + + @Test + public void negativeJUnit5TestClass() { + compilationTestHelper + .addSourceLines( + "Test.java", + """ + import org.junit.jupiter.api.Test; + + class TestJ5 { + @Test + void test() {} + boolean m(Object x) { + return x.equals(null); + } + } + """) + .doTest(); + } + + @Test + public void negativeEnclosedByJUnit5Assert() { + compilationTestHelper + .addSourceLines( + "TestHelper.java", + """ + import static org.junit.jupiter.api.Assertions.assertFalse; + + class TestHelper { + public static void myAssert(Object x) { + assertFalse(x.equals(null)); + } + } + """) + .doTest(); + } + + @Test + public void positiveJUnit5ProductionCode() { + compilationTestHelper + .addSourceLines( + "Test.java", + """ + import org.junit.jupiter.api.Assertions; + + class TestJ5 { + boolean m(Object x) { + // BUG: Diagnostic contains: x.equals(null) should return false + return x.equals(null); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ExpectedExceptionCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ExpectedExceptionCheckerTest.java index 0a6e84ad29e..6bc00810a14 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ExpectedExceptionCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ExpectedExceptionCheckerTest.java @@ -553,4 +553,60 @@ public void testThrow(Class clazz) throws Exception { """) .doTest(); } + + @Test + public void expectRefactoring() { + BugCheckerRefactoringTestHelper.newInstance(ExpectedExceptionChecker.class, getClass()) + .addInputLines( + "in/ExceptionTest.java", + """ + import java.io.IOException; + import java.nio.file.*; + import org.junit.Rule; + import org.junit.Test; + import org.junit.rules.ExpectedException; + + class ExceptionTest { + @Rule public ExpectedException thrown = ExpectedException.none(); + + @Test + public void test() throws Exception { + thrown.expect(IOException.class); + thrown.expectMessage("NOSUCH"); + Path p = Paths.get("NOSUCH"); + Files.readAllBytes(p); + } + } + """) + .addOutputLines( + "out/ExceptionTest.java", + """ + import static com.google.common.truth.Truth.assertThat; + import static org.junit.Assert.assertThrows; + + import java.io.IOException; + import java.nio.file.*; + import org.junit.Rule; + import org.junit.Test; + import org.junit.rules.ExpectedException; + + class ExceptionTest { + @Rule public ExpectedException thrown = ExpectedException.none(); + + @Test + public void test() throws Exception { + + IOException thrown = + assertThrows( + IOException.class, + () -> { + Path p = Paths.get("NOSUCH"); + Files.readAllBytes(p); + }); + assertThat(thrown).hasMessageThat().contains("NOSUCH"); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilonTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilonTest.java index 550fa104a97..fd3322016f1 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilonTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/FloatingPointAssertionWithinEpsilonTest.java @@ -234,4 +234,111 @@ public void testDouble() { """) .doTest(); } + + @Test + public void positiveCaseJUnit5() { + compilationHelper + .addSourceLines( + "FloatingPointAssertionWithinEpsilonPositiveCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + final class FloatingPointAssertionWithinEpsilonPositiveCasesJUnit5 { + + private static final float TOLERANCE = 1e-10f; + private static final double TOLERANCE2 = 1e-20f; + + public void testFloat() { + // BUG: Diagnostic contains: 6.0e-08 + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, TOLERANCE); + // BUG: Diagnostic contains: 6.0e-08 + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, TOLERANCE, "equal!"); + } + + public void testDouble() { + // BUG: Diagnostic contains: 1.1e-16 + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, TOLERANCE2); + // BUG: Diagnostic contains: 1.1e-16 + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, TOLERANCE2, "equal!"); + } + } + """) + .doTest(); + } + + @Test + public void fixesJUnit5() { + BugCheckerRefactoringTestHelper.newInstance( + FloatingPointAssertionWithinEpsilon.class, getClass()) + .addInputLines( + "FloatingPointAssertionWithinEpsilonPositiveCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + final class FloatingPointAssertionWithinEpsilonPositiveCasesJUnit5 { + + private static final float TOLERANCE = 1e-10f; + private static final double TOLERANCE2 = 1e-20f; + + public void testFloat() { + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, TOLERANCE); + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, TOLERANCE, "equal!"); + } + + public void testDouble() { + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, TOLERANCE2); + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, TOLERANCE2, "equal!"); + } + } + """) + .addOutputLines( + "FloatingPointAssertionWithinEpsilonPositiveCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + final class FloatingPointAssertionWithinEpsilonPositiveCasesJUnit5 { + + private static final float TOLERANCE = 1e-10f; + private static final double TOLERANCE2 = 1e-20f; + + public void testFloat() { + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, 0); + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, 0, "equal!"); + } + + public void testDouble() { + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, 0); + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, 0, "equal!"); + } + } + """) + .doTest(); + } + + @Test + public void negativeCaseJUnit5() { + compilationHelper + .addSourceLines( + "FloatingPointAssertionWithinEpsilonNegativeCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + final class FloatingPointAssertionWithinEpsilonNegativeCasesJUnit5 { + + private static final float TOLERANCE = 1e-5f; + private static final double TOLERANCE2 = 1e-10f; + + public void testFloat() { + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, TOLERANCE); + org.junit.jupiter.api.Assertions.assertEquals(1f, 1f, TOLERANCE, "equal!"); + } + + public void testDouble() { + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, TOLERANCE2); + org.junit.jupiter.api.Assertions.assertEquals(1.0, 1.0, TOLERANCE2, "equal!"); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheckTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheckTest.java index 44f4f28be07..271f4c3cf66 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheckTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitAssertSameCheckTest.java @@ -89,4 +89,45 @@ public void test(Object obj1, Object obj2) { """) .doTest(); } + + @Test + public void positiveCaseJUnit5() { + compilationHelper + .addSourceLines( + "JUnitAssertSameCheckPositiveCaseJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + public class JUnitAssertSameCheckPositiveCaseJUnit5 { + + public void test(Object obj) { + // BUG: Diagnostic contains: An object is tested for reference equality to itself using JUnit + org.junit.jupiter.api.Assertions.assertSame(obj, obj); + + // BUG: Diagnostic contains: An object is tested for reference equality to itself using JUnit + org.junit.jupiter.api.Assertions.assertSame(obj, obj, "message"); + } + } + """) + .doTest(); + } + + @Test + public void negativeCaseJUnit5() { + compilationHelper + .addSourceLines( + "JUnitAssertSameCheckNegativeCaseJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + public class JUnitAssertSameCheckNegativeCaseJUnit5 { + + public void test(Object obj1, Object obj2) { + org.junit.jupiter.api.Assertions.assertSame(obj1, obj2); + org.junit.jupiter.api.Assertions.assertSame(obj1, obj2, "message"); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4ClassAnnotationNonStaticTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitClassAnnotationNonStaticTest.java similarity index 61% rename from core/src/test/java/com/google/errorprone/bugpatterns/JUnit4ClassAnnotationNonStaticTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/JUnitClassAnnotationNonStaticTest.java index 988eb422c1c..3e6ab916cc5 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4ClassAnnotationNonStaticTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitClassAnnotationNonStaticTest.java @@ -21,12 +21,12 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Unit test of {@link JUnit4ClassAnnotationNonStatic} */ +/** Unit test of {@link JUnitClassAnnotationNonStatic} */ @RunWith(JUnit4.class) -public class JUnit4ClassAnnotationNonStaticTest { +public class JUnitClassAnnotationNonStaticTest { private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(JUnit4ClassAnnotationNonStatic.class, getClass()); + CompilationTestHelper.newInstance(JUnitClassAnnotationNonStatic.class, getClass()); @Test public void positive() { @@ -80,4 +80,54 @@ public static void shouldDoSomethingElse() {} """) .doTest(); } + + @Test + public void positiveJUnit5() { + compilationHelper + .addSourceLines( + "TestJ5.java", + """ + import org.junit.jupiter.api.AfterAll; + import org.junit.jupiter.api.BeforeAll; + import org.junit.jupiter.api.Test; + + class TestJ5 { + @BeforeAll + // BUG: Diagnostic contains: BeforeAll can only be applied to static methods. + public void shouldDoSomething() {} + + @AfterAll + // BUG: Diagnostic contains: AfterAll can only be applied to static methods. + public void shouldDoSomethingElse() {} + + @Test + public void test() {} + } + """) + .doTest(); + } + + @Test + public void negativeJUnit5() { + compilationHelper + .addSourceLines( + "TestJ5.java", + """ + import org.junit.jupiter.api.AfterAll; + import org.junit.jupiter.api.BeforeAll; + import org.junit.jupiter.api.Test; + + class TestJ5 { + @BeforeAll + public static void shouldDoSomething() {} + + @AfterAll + public static void shouldDoSomethingElse() {} + + @Test + public void test() {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4EmptyMethodsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitEmptyLifecycleMethodsTest.java similarity index 78% rename from core/src/test/java/com/google/errorprone/bugpatterns/JUnit4EmptyMethodsTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/JUnitEmptyLifecycleMethodsTest.java index 6caea1cff56..ccf66255011 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4EmptyMethodsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitEmptyLifecycleMethodsTest.java @@ -21,12 +21,12 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Tests for {@link JUnit4EmptyMethods}. */ +/** Tests for {@link JUnitEmptyLifecycleMethods}. */ @RunWith(JUnit4.class) -public class JUnit4EmptyMethodsTest { +public class JUnitEmptyLifecycleMethodsTest { private final BugCheckerRefactoringTestHelper refactoringHelper = - BugCheckerRefactoringTestHelper.newInstance(JUnit4EmptyMethods.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(JUnitEmptyLifecycleMethods.class, getClass()); @Test public void emptyMethods() { @@ -224,4 +224,55 @@ public void setUp() { .expectUnchanged() .doTest(); } + + @Test + public void emptyMethodsJUnit5() { + refactoringHelper + .addInputLines( + "FooTest.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; + + class FooTest { + @BeforeEach + void setUp() {} + + @BeforeAll + static void setUpClass() {} + + @AfterEach + void after() {} + + @AfterAll + static void afterClass() {} + + @Test + public void nonEmptyTest() { + System.out.println("test"); + } + } + """) + .addOutputLines( + "FooTest.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; + + class FooTest { + + @Test + public void nonEmptyTest() { + System.out.println("test"); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4SetUpNotRunTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitSetUpNotRunTest.java similarity index 85% rename from core/src/test/java/com/google/errorprone/bugpatterns/JUnit4SetUpNotRunTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/JUnitSetUpNotRunTest.java index 1876e3324ed..dea2d2815df 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4SetUpNotRunTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitSetUpNotRunTest.java @@ -29,12 +29,12 @@ * @author glorioso@google.com (Nick Glorioso) */ @RunWith(JUnit4.class) -public class JUnit4SetUpNotRunTest { +public class JUnitSetUpNotRunTest { private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(JUnit4SetUpNotRun.class, getClass()); + CompilationTestHelper.newInstance(JUnitSetUpNotRun.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance(JUnit4SetUpNotRun.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(JUnitSetUpNotRun.class, getClass()); @Test public void positiveCases() { @@ -320,4 +320,52 @@ public void noBeforeOnClasspath() { SuperTest.class.getEnclosingClass()) .doTest(); } + + @Test + public void positiveCasesJUnit5() { + compilationHelper + .addSourceLines( + "JUnit5SetUpNotRunPositiveCases.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.Test; + + /** Basic class with an untagged setUp method in JUnit 5 test */ + class JUnit5SetUpNotRunPositiveCases { + @Test + public void test() {} + + // BUG: Diagnostic contains: @BeforeEach + public void setUp() {} + } + """) + .doTest(); + } + + @Test + public void negativeCasesJUnit5() { + compilationHelper + .addSourceLines( + "JUnit5SetUpNotRunNegativeCases.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.BeforeEach; + import org.junit.jupiter.api.BeforeAll; + import org.junit.jupiter.api.Test; + + class JUnit5SetUpNotRunNegativeCases { + @BeforeEach + public void setUp() {} + + @BeforeAll + public static void setUpClass() {} + + @Test + public void test() {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4TearDownNotRunTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitTearDownNotRunTest.java similarity index 83% rename from core/src/test/java/com/google/errorprone/bugpatterns/JUnit4TearDownNotRunTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/JUnitTearDownNotRunTest.java index d2e292fe8cf..332034bbadb 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4TearDownNotRunTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitTearDownNotRunTest.java @@ -25,10 +25,10 @@ * @author glorioso@google.com (Nick Glorioso) */ @RunWith(JUnit4.class) -public class JUnit4TearDownNotRunTest { +public class JUnitTearDownNotRunTest { private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(JUnit4TearDownNotRun.class, getClass()); + CompilationTestHelper.newInstance(JUnitTearDownNotRun.class, getClass()); @Test public void positiveCases() { @@ -233,4 +233,52 @@ public void tearDown() {} """) .doTest(); } + + @Test + public void positiveCasesJUnit5() { + compilationHelper + .addSourceLines( + "JUnit5TearDownNotRunPositiveCases.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.Test; + + /** Basic class with an untagged tearDown method in JUnit 5 test */ + class JUnit5TearDownNotRunPositiveCases { + @Test + public void test() {} + + // BUG: Diagnostic contains: @AfterEach + public void tearDown() {} + } + """) + .doTest(); + } + + @Test + public void negativeCasesJUnit5() { + compilationHelper + .addSourceLines( + "JUnit5TearDownNotRunNegativeCases.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.AfterEach; + import org.junit.jupiter.api.AfterAll; + import org.junit.jupiter.api.Test; + + class JUnit5TearDownNotRunNegativeCases { + @AfterEach + public void tearDown() {} + + @AfterAll + public static void tearDownClass() {} + + @Test + public void test() {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4TestNotRunTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitTestNotRunTest.java similarity index 92% rename from core/src/test/java/com/google/errorprone/bugpatterns/JUnit4TestNotRunTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/JUnitTestNotRunTest.java index db440d87dce..26a0c6472b6 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit4TestNotRunTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JUnitTestNotRunTest.java @@ -27,13 +27,13 @@ * @author eaftan@google.com (Eddie Aftandilian) */ @RunWith(JUnit4.class) -public class JUnit4TestNotRunTest { +public class JUnitTestNotRunTest { private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(JUnit4TestNotRun.class, getClass()); + CompilationTestHelper.newInstance(JUnitTestNotRun.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringHelper = - BugCheckerRefactoringTestHelper.newInstance(JUnit4TestNotRun.class, getClass()); + BugCheckerRefactoringTestHelper.newInstance(JUnitTestNotRun.class, getClass()); @Test public void positiveCase1() { @@ -1002,4 +1002,81 @@ private void verify() {} """) .doTest(); } + + @Test + public void positiveCaseJUnit5() { + compilationHelper + .addSourceLines( + "JUnit5TestNotRunPositiveCase.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.Test; + + class JUnit5TestNotRunPositiveCase { + @Test + public void alreadyTest() {} + + // BUG: Diagnostic contains: @Test + public void testThisIsATest() {} + + // BUG: Diagnostic contains: @Test + public static void testThisIsAStaticTest() {} + } + """) + .doTest(); + } + + @Test + public void negativeCaseJUnit5() { + compilationHelper + .addSourceLines( + "JUnit5TestNotRunNegativeCase.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.Test; + + class JUnit5TestNotRunNegativeCase { + @Test + public void testThisIsATest() {} + } + """) + .doTest(); + } + + @Test + public void refactoringJUnit5() { + refactoringHelper + .addInputLines( + "in/Foo.java", + """ + package pkg; + + import org.junit.jupiter.api.Test; + + class Foo { + @Test + public void alreadyTest() {} + + public void testThisIsATest() {} + } + """) + .addOutputLines( + "out/Foo.java", + """ + package pkg; + + import org.junit.jupiter.api.Test; + + class Foo { + @Test + public void alreadyTest() {} + + @Test + public void testThisIsATest() {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MissingFailTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MissingFailTest.java index 9f7d5d75c5f..05b1eeebbd0 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/MissingFailTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MissingFailTest.java @@ -1219,4 +1219,32 @@ public void test() throws Exception { """) .doTest(); } + + @Test + public void positiveCasesJUnit5() { + compilationHelper + .addSourceLines( + "MissingFailPositiveCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import org.junit.jupiter.api.Assertions; + import org.junit.jupiter.api.Test; + + public class MissingFailPositiveCasesJUnit5 { + + @Test + public void expectedException_emptyCatch() { + try { + // BUG: Diagnostic contains: fail() + dummyMethod(); + } catch (Exception expected) { + } + } + + private void dummyMethod() {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/SelfAssertionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/SelfAssertionTest.java index 1b82e9bf4e2..6dd966f033e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/SelfAssertionTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/SelfAssertionTest.java @@ -256,4 +256,58 @@ void test(int x) { """) .doTest(); } + + @Test + public void junit5PositiveAssertion() { + compilationHelper + .addSourceLines( + "Test.java", + """ + abstract class Test { + void test(int x) { + // BUG: Diagnostic contains: pass + org.junit.jupiter.api.Assertions.assertEquals(x, x); + // BUG: Diagnostic contains: pass + org.junit.jupiter.api.Assertions.assertEquals(x, x, "foo"); + } + } + """) + .doTest(); + } + + @Test + public void junit5NegativeAssertion() { + compilationHelper + .addSourceLines( + "Test.java", + """ + abstract class Test { + void test(int x) { + // BUG: Diagnostic contains: fail + org.junit.jupiter.api.Assertions.assertNotEquals(x, x); + // BUG: Diagnostic contains: fail + org.junit.jupiter.api.Assertions.assertNotEquals(x, x, "foo"); + } + } + """) + .doTest(); + } + + @Test + public void junit5NegativeAssertionDifferentValues() { + compilationHelper + .addSourceLines( + "Test.java", + """ + abstract class Test { + void test(int x, int y) { + org.junit.jupiter.api.Assertions.assertEquals(x, y); + org.junit.jupiter.api.Assertions.assertEquals(x, y, "foo"); + org.junit.jupiter.api.Assertions.assertNotEquals(x, y); + org.junit.jupiter.api.Assertions.assertNotEquals(x, y, "foo"); + } + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/TooManyParametersTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/TooManyParametersTest.java index 04072b81ed9..aa8488c3938 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/TooManyParametersTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/TooManyParametersTest.java @@ -253,4 +253,52 @@ public ImmutableList provideValues(Context """) .doTest(); } + + @Test + public void testJUnit5TestMethod() { + compilationHelper + .addSourceLines( + "ExampleWithJUnit5Test.java", + """ + import org.junit.jupiter.api.Test; + + public class ExampleWithJUnit5Test { + @Test + public void myTest( + String a, + String b, + String c, + String d, + String e, + String f, + String g, + String h, + String i, + String j, + String k, + String l) + throws Exception {} + } + """) + .doTest(); + } + + @Test + public void testJUnit5TestMethodNegative() { + compilationHelper + .addSourceLines( + "ExampleWithJUnit5TestNegative.java", + """ + import org.junit.jupiter.api.Test; + + public class ExampleWithJUnit5TestNegative { + @Test + public void myTest() throws Exception {} + + @Test + public void myTestWithFewParams(String a, String b) throws Exception {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/TryFailThrowableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/TryFailThrowableTest.java index ceec771b8a7..b68c9ebb151 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/TryFailThrowableTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/TryFailThrowableTest.java @@ -400,4 +400,103 @@ private static void dummyMethod() {} """) .doTest(); } + + @Test + public void positiveCasesJUnit5() { + compilationHelper + .addSourceLines( + "TryFailThrowablePositiveCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import static org.junit.jupiter.api.Assertions.assertTrue; + + import org.junit.jupiter.api.Assertions; + + public class TryFailThrowablePositiveCasesJUnit5 { + + public static void emptyCatch_failNoMessage() { + try { + dummyMethod(); + Assertions.fail(); + // BUG: Diagnostic contains: catch (Exception t) + } catch (Throwable t) { + } + } + + public static void catchesError_lastStatement() { + try { + dummyMethod(); + Assertions.fail(); + // BUG: Diagnostic contains: remove this line + } catch (Error e) { + } + } + + public static void catchesError_notLastStatement() { + try { + dummyMethod(); + Assertions.fail(); + // BUG: Diagnostic contains: boolean threw = false; + } catch (Error e) { + } + + assertTrue(true); + } + + private static void dummyMethod() {} + } + """) + .doTest(); + } + + @Test + public void negativeJUnit5() { + compilationHelper + .addSourceLines( + "TryFailThrowableNegativeCasesJUnit5.java", + """ + package com.google.errorprone.bugpatterns.testdata; + + import static org.junit.jupiter.api.Assertions.assertTrue; + + import org.junit.jupiter.api.Assertions; + + public class TryFailThrowableNegativeCasesJUnit5 { + + public static void catchHasCode() { + try { + dummyMethod(); + Assertions.fail(); + } catch (Throwable t) { + dummyRecover(); + } + } + + public static void catchException() { + try { + dummyMethod(); + Assertions.fail(); + } catch (Exception t) { + dummyRecover(); + } + } + + public static void failNotLast() { + try { + dummyMethod(); + Assertions.fail("Not last :("); + dummyMethod(); + } catch (Throwable t) { + dummyRecover(); + } + } + + private static void dummyRecover() {} + + private static void dummyMethod() {} + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java index c06eb2fe5fc..395dfcb6c89 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/collectionincompatibletype/JUnitIncompatibleTypeTest.java @@ -223,4 +223,44 @@ public void test(Map xs) { """) .doTest(); } + + @Test + public void assertEquals_mismatchedJUnit5() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Test { + public void test() { + // BUG: Diagnostic contains: + org.junit.jupiter.api.Assertions.assertEquals(new Test(), ""); + // BUG: Diagnostic contains: + org.junit.jupiter.api.Assertions.assertEquals("msg", new Test(), ""); + // BUG: Diagnostic contains: + org.junit.jupiter.api.Assertions.assertNotEquals(new Test(), ""); + // BUG: Diagnostic contains: + org.junit.jupiter.api.Assertions.assertNotEquals("msg", new Test(), ""); + } + } + """) + .doTest(); + } + + @Test + public void assertEquals_matchedJUnit5() { + compilationHelper + .addSourceLines( + "Test.java", + """ + class Test { + public void test() { + org.junit.jupiter.api.Assertions.assertEquals("a", "b"); + org.junit.jupiter.api.Assertions.assertEquals("msg", "a", "b"); + org.junit.jupiter.api.Assertions.assertNotEquals("a", "b"); + org.junit.jupiter.api.Assertions.assertNotEquals("msg", "a", "b"); + } + } + """) + .doTest(); + } } diff --git a/pom.xml b/pom.xml index fd9e8a7961d..0bed43d243e 100644 --- a/pom.xml +++ b/pom.xml @@ -46,6 +46,7 @@ 1.43.3 1.0.0 1.35.0 + 5.14.4