Added a first version of a new inspection that tries to detect bogus uses of return statements in test methods and replaces them by assumeThat() calls. Removed oraclejdk8 from travis-ci. Added custom Jupiter DisplayNameGenerator for user friendly test names in report. Extended documentation. Removed Playground.

2019-05-05 19:02:35 +02:00
parent 66f1467b23
commit eab50f590b
18 changed files with 681 additions and 474 deletions
@@ -7,6 +7,9 @@ class AssertJClassNames {
        @NonNls
        const val ASSERTIONS_CLASSNAME = "org.assertj.core.api.Assertions"

+        @NonNls
+        const val ASSUMPTIONS_CLASSNAME = "org.assertj.core.api.Assumptions"
+
        @NonNls
        const val DESCRIPTABLE_INTERFACE = "org.assertj.core.api.Descriptable"
        @NonNls
@@ -12,7 +12,10 @@ import de.platon42.intellij.plugins.cajon.inspections.AbstractAssertJInspection
 val PsiMethodCallExpression.qualifierExpression: PsiExpression get() = methodExpression.qualifierExpression!!
 val PsiMethodCallExpression.firstArg: PsiExpression get() = getArg(0)

-fun PsiElement.hasAssertThat() = text.contains("assertThat")
+fun PsiElement.hasAssertThat(): Boolean {
+    val elementText = text
+    return elementText.startsWith("${MethodNames.ASSERT_THAT}(") || elementText.contains(".${MethodNames.ASSERT_THAT}(")
+}

 fun PsiMethodCallExpression.replaceQualifier(qualifier: PsiElement) {
    qualifierExpression.replace(qualifier)
@@ -12,6 +12,9 @@ class MethodNames {
        @NonNls
        const val ASSERT_THAT = "assertThat"

+        @NonNls
+        const val ASSUME_THAT = "assumeThat"
+
        @NonNls
        const val AS = "as"
        @NonNls
@@ -0,0 +1,146 @@
+package de.platon42.intellij.plugins.cajon.inspections
+
+import com.intellij.codeInspection.ProblemHighlightType
+import com.intellij.codeInspection.ProblemsHolder
+import com.intellij.psi.*
+import de.platon42.intellij.plugins.cajon.hasAssertThat
+import de.platon42.intellij.plugins.cajon.quickfixes.ReplaceIfByAssumeThatQuickFix
+
+class AssumeThatInsteadOfReturnInspection : AbstractAssertJInspection() {
+
+    companion object {
+        private const val DISPLAY_NAME = "Replace conditional test exits by assumeThat() statements with same actual expression"
+        private const val REPLACE_RETURN_BY_ASSUME_THAT_DESCRIPTION = "Conditional return should probably be an assumeThat() statement instead"
+
+        private const val MAX_RECURSION_DEPTH = 5
+        private const val MAX_STATEMENTS_COUNT = 50
+
+        private val TEST_ANNOTATIONS = listOf(
+            "org.junit.Test",
+            "org.junit.jupiter.api.Test",
+            "org.junit.jupiter.api.TestTemplate",
+            "org.junit.jupiter.api.params.ParameterizedTest"
+        )
+
+        private fun hasEmptyReturn(statement: PsiStatement): Boolean {
+            return when (statement) {
+                is PsiBlockStatement -> {
+                    val psiReturnStatement = (statement.firstChild as? PsiCodeBlock)?.statements?.singleOrNull() as? PsiReturnStatement
+                    (psiReturnStatement != null) && (psiReturnStatement.returnValue == null)
+                }
+                is PsiReturnStatement -> statement.returnValue == null
+                else -> false
+            }
+        }
+
+        private fun registerProblem(holder: ProblemsHolder, isOnTheFly: Boolean, statement: PsiStatement, removeElse: Boolean) {
+            val problemDescriptor = holder.manager.createProblemDescriptor(
+                statement,
+                statement,
+                REPLACE_RETURN_BY_ASSUME_THAT_DESCRIPTION,
+                ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+                isOnTheFly,
+                ReplaceIfByAssumeThatQuickFix(removeElse)
+            )
+            holder.registerProblem(problemDescriptor)
+        }
+    }
+
+    override fun getDisplayName() = DISPLAY_NAME
+
+    override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor {
+        return object : JavaElementVisitor() {
+            override fun visitMethod(method: PsiMethod) {
+                super.visitMethod(method)
+                if (TEST_ANNOTATIONS.none(method::hasAnnotation)) {
+                    return
+                }
+                val containingClass = method.containingClass ?: return
+                val visitor: PsiElementVisitor = TestMethodVisitor(holder, isOnTheFly, containingClass)
+                method.accept(visitor)
+            }
+        }
+    }
+
+    class TestMethodVisitor(
+        private val holder: ProblemsHolder,
+        private val isOnTheFly: Boolean,
+        private val containingClass: PsiClass
+    ) : JavaRecursiveElementWalkingVisitor() {
+
+        private var contSearch = true
+
+        override fun visitExpressionStatement(statement: PsiExpressionStatement) {
+            if (contSearch) {
+                val methodCallExpression = statement.expression as? PsiMethodCallExpression
+                if (methodCallExpression != null) {
+                    if (methodCallExpression.hasAssertThat()) {
+                        contSearch = false
+                    } else {
+                        val method = methodCallExpression.resolveMethod()
+                        if (method?.containingClass == containingClass) {
+                            val recursionVisitor = CheckForAssertThatCallsVisitor(containingClass, 1)
+                            method.accept(recursionVisitor)
+                            if (recursionVisitor.aborted || recursionVisitor.foundAssertThat) {
+                                contSearch = false
+                            }
+                        }
+                    }
+                }
+            }
+            if (contSearch) {
+                super.visitExpressionStatement(statement)
+            }
+        }
+
+        override fun visitIfStatement(statement: PsiIfStatement) {
+            if (contSearch) {
+                checkBranch(statement, statement.thenBranch, false)
+                checkBranch(statement, statement.elseBranch, true)
+            }
+        }
+
+        private fun checkBranch(statement: PsiIfStatement, branch: PsiStatement?, removeElse: Boolean) {
+            if (branch != null) {
+                if (hasEmptyReturn(branch)) {
+                    registerProblem(holder, isOnTheFly, statement, removeElse)
+                } else {
+                    branch.accept(TestMethodVisitor(holder, isOnTheFly, containingClass))
+                }
+            }
+        }
+    }
+
+    class CheckForAssertThatCallsVisitor(private val containingClass: PsiClass, private var depth: Int) : JavaRecursiveElementWalkingVisitor() {
+        var foundAssertThat = false
+        private var statementCount = 0
+        var aborted = false
+
+        override fun visitExpressionStatement(statement: PsiExpressionStatement) {
+            if (foundAssertThat || aborted) {
+                return
+            }
+            if (++statementCount > MAX_STATEMENTS_COUNT) {
+                aborted = true
+                return
+            }
+            super.visitExpressionStatement(statement)
+            val methodCallExpression = statement.expression as? PsiMethodCallExpression
+            if (methodCallExpression != null) {
+                foundAssertThat = methodCallExpression.hasAssertThat()
+                val method = methodCallExpression.resolveMethod()
+                if (method?.containingClass == containingClass) {
+                    if (depth < MAX_RECURSION_DEPTH) {
+                        val recursionVisitor = CheckForAssertThatCallsVisitor(containingClass, depth + 1)
+                        method.accept(recursionVisitor)
+                        foundAssertThat = recursionVisitor.foundAssertThat
+                        statementCount += recursionVisitor.statementCount
+                        aborted = recursionVisitor.aborted
+                    } else {
+                        aborted = true
+                    }
+                }
+            }
+        }
+    }
+}
@@ -11,7 +11,7 @@ import de.platon42.intellij.plugins.cajon.quickfixes.JoinStatementsQuickFix
 class JoinAssertThatStatementsInspection : AbstractAssertJInspection() {

    companion object {
-        private const val DISPLAY_NAME = "Joining multiple assertThat() statements with same actual expression"
+        private const val DISPLAY_NAME = "Join multiple assertThat() statements with same actual expression"
        private const val CAN_BE_JOINED_DESCRIPTION = "Multiple assertThat() statements can be joined together"
    }

@@ -53,7 +53,7 @@ class JoinAssertThatStatementsInspection : AbstractAssertJInspection() {
                    }
                    if (reset) {
                        if (sameCount > 1) {
-                            registerProblem(firstStatement, lastStatement)
+                            registerProblem(holder, isOnTheFly, firstStatement!!, lastStatement!!)
                        }
                        firstStatement = statement
                        lastStatement = null
@@ -62,22 +62,10 @@ class JoinAssertThatStatementsInspection : AbstractAssertJInspection() {
                    }
                }
                if (sameCount > 1) {
-                    registerProblem(firstStatement, lastStatement)
+                    registerProblem(holder, isOnTheFly, firstStatement!!, lastStatement!!)
                }
            }

-            private fun registerProblem(firstStatement: PsiStatement?, lastStatement: PsiStatement?) {
-                val problemDescriptor = holder.manager.createProblemDescriptor(
-                    firstStatement!!,
-                    lastStatement!!,
-                    CAN_BE_JOINED_DESCRIPTION,
-                    ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
-                    isOnTheFly,
-                    JoinStatementsQuickFix()
-                )
-                holder.registerProblem(problemDescriptor)
-            }
-
            private fun isLegitAssertThatCall(statement: PsiStatement?): PsiMethodCallExpression? {
                if ((statement is PsiExpressionStatement) && (statement.expression is PsiMethodCallExpression)) {
                    if (!statement.hasAssertThat()) {
@@ -90,4 +78,16 @@ class JoinAssertThatStatementsInspection : AbstractAssertJInspection() {
            }
        }
    }
+
+    private fun registerProblem(holder: ProblemsHolder, isOnTheFly: Boolean, firstStatement: PsiStatement, lastStatement: PsiStatement) {
+        val problemDescriptor = holder.manager.createProblemDescriptor(
+            firstStatement,
+            lastStatement,
+            CAN_BE_JOINED_DESCRIPTION,
+            ProblemHighlightType.GENERIC_ERROR_OR_WARNING,
+            isOnTheFly,
+            JoinStatementsQuickFix()
+        )
+        holder.registerProblem(problemDescriptor)
+    }
 }
@@ -3,9 +3,9 @@ package de.platon42.intellij.plugins.cajon.quickfixes
 import com.intellij.codeInspection.ProblemDescriptor
 import com.intellij.openapi.project.Project
 import com.intellij.psi.*
-import com.intellij.psi.codeStyle.CodeStyleManager
 import com.intellij.psi.util.PsiTreeUtil
 import de.platon42.intellij.plugins.cajon.findStaticMethodCall
+import de.platon42.intellij.plugins.cajon.shortenAndReformat

 class JoinStatementsQuickFix : AbstractCommonQuickFix(JOIN_STATEMENTS_MESSAGE) {

@@ -48,6 +48,6 @@ class JoinStatementsQuickFix : AbstractCommonQuickFix(JOIN_STATEMENTS_MESSAGE) {
            previousStatement.delete()
        } while (previousStatement !== firstStatement)
        val codeBlock = PsiTreeUtil.getParentOfType(lastStatement, PsiCodeBlock::class.java) ?: return
-        CodeStyleManager.getInstance(project).reformat(codeBlock)
+        codeBlock.shortenAndReformat()
    }
 }
@@ -0,0 +1,49 @@
+package de.platon42.intellij.plugins.cajon.quickfixes
+
+import com.intellij.codeInspection.ProblemDescriptor
+import com.intellij.openapi.project.Project
+import com.intellij.psi.JavaPsiFacade
+import com.intellij.psi.PsiBlockStatement
+import com.intellij.psi.PsiDeclarationStatement
+import com.intellij.psi.PsiIfStatement
+import de.platon42.intellij.plugins.cajon.*
+
+class ReplaceIfByAssumeThatQuickFix(private val removeElse: Boolean) : AbstractCommonQuickFix(REPLACE_IF_MESSAGE) {
+
+    companion object {
+        private const val REPLACE_IF_MESSAGE = "Replace if statement by assumeTrue()"
+    }
+
+    override fun applyFix(project: Project, descriptor: ProblemDescriptor) {
+        val ifStatement = descriptor.startElement as PsiIfStatement
+
+        val condition = ifStatement.condition ?: return
+        val factory = JavaPsiFacade.getElementFactory(ifStatement.project)
+        val assumptionExpression = if (removeElse) MethodNames.IS_TRUE else MethodNames.IS_FALSE
+        val assumeThatStatement = factory.createStatementFromText(
+            "${AssertJClassNames.ASSUMPTIONS_CLASSNAME}.${MethodNames.ASSUME_THAT}(true).$assumptionExpression();",
+            ifStatement
+        )
+        val assumeThatMethodCall = assumeThatStatement.findStaticMethodCall() ?: return
+        assumeThatMethodCall.firstArg.replace(condition)
+        assumeThatMethodCall.resolveMethod()?.addAsStaticImport(ifStatement)
+
+        val branchToKeep = (if (removeElse) ifStatement.thenBranch else ifStatement.elseBranch)?.copy()
+        val parentBlock = ifStatement.parent
+        if (branchToKeep != null) {
+            val anchorElement = ifStatement.nextSibling
+            if (branchToKeep is PsiBlockStatement) {
+                val codeBlock = branchToKeep.codeBlock
+                val hasDeclarations = codeBlock.statements.any { it is PsiDeclarationStatement }
+                if (hasDeclarations) {
+                    parentBlock.addAfter(branchToKeep, anchorElement)
+                } else {
+                    parentBlock.addRangeAfter(codeBlock.firstBodyElement, codeBlock.lastBodyElement, anchorElement)
+                }
+            } else {
+                parentBlock.addAfter(branchToKeep, anchorElement)
+            }
+        }
+        ifStatement.replace(assumeThatStatement).shortenAndReformat()
+    }
+}
@@ -45,6 +45,8 @@

        <localInspection groupPath="Java" shortName="JoinAssertThatStatements" enabledByDefault="true" level="WARNING"
                         implementationClass="de.platon42.intellij.plugins.cajon.inspections.JoinAssertThatStatementsInspection"/>
+        <localInspection groupPath="Java" shortName="AssumeThatInsteadOfReturnInspection" enabledByDefault="true" level="WARNING"
+                         implementationClass="de.platon42.intellij.plugins.cajon.inspections.AssumeThatInsteadOfReturnInspection"/>

        <localInspection groupPath="Java" shortName="AssertThatJava8Optional" enabledByDefault="true" level="WARNING"
                         implementationClass="de.platon42.intellij.plugins.cajon.inspections.AssertThatJava8OptionalInspection"/>
@@ -0,0 +1,18 @@
+<html>
+<body>
+Tries to detect bogus uses of return statements in test methods and replaces them by assumeThat() calls.
+<!-- tooltip end -->
+<br>
+Novices will use these to skip test execution by bailing out early on some preconditions not met.
+However, this suggests that the test has actually been run and passed instead of showing the test
+as being skipped.
+<p>Return statements in if statements in main test methods
+    (must be annotated with JUnit 4 or Jupiter @Test annotations)
+    will be verified to have at least one assertThat() statement in the code flow.
+    Method calls within the same class will be examined for assertThat() statements, too.
+    However, at most 50 statements and down to five recursions will be tolerated before giving up.
+</p>
+<p>Currently, the quickfix may lose some comments during operation. The other branch of the if statement
+    will be inlined (blocks with declarations will remain a code block due to variable scope).</p>
+</body>
+</html>
@@ -1,11 +1,11 @@
 <html>
 <body>
-Tries to convert most of the JUnit 4 assertions to AssertJ-Format.
+Tries to convert most of the JUnit 4 assertions to AssertJ format.
 <!-- tooltip end -->
 <br>Works for assertTrue(), assertFalse(), assertNull(), assertNotNull(), assertEquals(), assertNotEquals(), assertSame() assertNotSame(), assertArrayEquals().
 Copes with variants with message and without, handles special versions for double and float types (including arrays).
 <p>
-Does not support Hamcrest-Matchers. If you need that kind of conversion, you might want to check out the Assertions2AssertJ plugin by Ric Emery.
+    Does not support Hamcrest-Matchers. If you need that kind of conversion, you might want to check out the Assertions2AssertJ plugin by Ric Emery.
 </p>
 </body>
 </html>