Compare commits

...
3 Commits
9 changed files with 159 additions and 27 deletions
+27
View File
@@ -503,6 +503,15 @@ You can toggle the various inspections in the Settings/Editor/Inspections in the
There are, of course, more variations of the theme.
If both sides of an assertion are constant expressions, the problem will only appear as
a weak warning without a quick fix.
Constants used on the actual side of ```.matches()``` and ```doesNotMatch()``` will not be
reported for regular expression testing.
Neither will a ```Class``` type be considered a constant in the classic sense, so
```assertThat(SomeClass.class).isAssignableFrom(SomeOtherClass.class)``` will not be reported.
- BogusAssertion
Sometimes programmers make copy and paste or logical errors writing down assertions
@@ -549,6 +558,14 @@ You can toggle the various inspections in the Settings/Editor/Inspections in the
assertThat(map).hasSameSizeAs(map);
```
Note that expressions with method calls will not cause a warning as the method call might have side effects
that result in the assertion not being bogus at all.
If the assertions is either ```isEqualTo()``` or ```hasSameHashCodeAs()``` it may be checking custom
```equals()``` or ```hashCode()``` behavior. If the test method name containing the statement has a
name that contains 'equal' or 'hashcode' (case insensitive), the warning will be weakened to information
level.
- ImplicitAssertion
Detects and removes implicit use of ```isNotNull()```, ```isNotEmpty()``` and
@@ -776,6 +793,16 @@ Feel free to use the code (in package ```de.platon42.intellij.jupiter```) for yo
## Changelog
#### V1.9 (25-Feb-20) Mardi Gras Edition
- TwistedAssertion inspection will no longer warn for ```.matches()``` and ```doesNotMatch()``` for regular expressions.
Apparently, ```assertThat("somestring").matches(regex)``` is a valid test if the regex is what needs to be tested.
If the actual expression is of ```Class``` type, this will no longer be reported.
- If the expected expression in TwistedAssertion is also a constant, the warning will be weakened and
no quick fix will be available.
- BogusAssertion inspection will no longer warn if the expression contains method calls.
Moreover, for assertions of ```isEqualTo()``` and ```hasSameHashCodeAs()```, AND if the containing method name contains 'equal' or 'hashcode',
the warning will be reduced to information level as the assertion may be testing ```equals()``` or ```hashCode()``` for validity.
#### V1.8 (14-Feb-20) Valentine Edition
- Maintenance. Removed experimental API use. Updated dependencies. Fixed testing problems introduced with IntelliJ IDEA 2019.3
- Added new TwistedAssertion inspection that will warn about assertions with the actual expression being a constant indicating
+12 -1
View File
@@ -7,7 +7,7 @@ plugins {
}
group 'de.platon42'
version '1.8'
version '1.9'
repositories {
mavenCentral()
@@ -43,6 +43,17 @@ intellij {
patchPluginXml {
changeNotes """
<h4>V1.9 (25-Feb-20) Mardi Gras Edition</h4>
<ul>
<li>TwistedAssertion inspection will no longer warn for .matches() and doesNotMatch() for regular expressions.
Apparently, assertThat("somestring").matches(regex) is a valid test if the regex is what needs to be tested.
If the actual expression is of Class type, this will no longer be reported.
<li>If the expected expression in TwistedAssertion is also a constant, the warning will be weakened and
no quick fix will be available.
<li>BogusAssertion inspection will no longer warn if the expression contains method calls.
Moreover, for assertions of isEqualTo() and hasSameHashCodeAs(), AND if the containing method name contains 'equal' or 'hashcode',
the warning will be reduced to information level as the assertion may be testing equals() or hashCode() for validity.
</ul>
<h4>V1.8 (14-Feb-20) Valentine Edition</h4>
<ul>
<li>Maintenance. Removed experimental API use. Updated dependencies. Fixed testing problems introduced with IntelliJ IDEA 2019.3.x
@@ -1,10 +1,8 @@
package de.platon42.intellij.plugins.cajon.inspections
import com.intellij.codeInspection.ProblemHighlightType
import com.intellij.codeInspection.ProblemsHolder
import com.intellij.psi.JavaElementVisitor
import com.intellij.psi.PsiElementVisitor
import com.intellij.psi.PsiExpressionStatement
import com.intellij.psi.PsiMethodCallExpression
import com.intellij.psi.*
import com.intellij.psi.util.PsiTreeUtil
import com.siyeh.ig.callMatcher.CallMatcher
import com.siyeh.ig.psiutils.EquivalenceChecker
@@ -15,6 +13,7 @@ class BogusAssertionInspection : AbstractAssertJInspection() {
companion object {
private const val DISPLAY_NAME = "Bogus assertion due to same actual and expected expressions"
private const val ACTUAL_IS_EQUAL_TO_EXPECTED_MESSAGE = "Actual expression in assertThat() is the same as expected"
private const val WEAK_ACTUAL_IS_EQUAL_TO_EXPECTED_MESSAGE = "Same actual and expected expression, but may be testing equals() or hashCode()"
private val SAME_OBJECT =
CallMatcher.instanceCall(
@@ -57,6 +56,12 @@ class BogusAssertionInspection : AbstractAssertJInspection() {
private val SAME_OBJECT_ARRAY_CONTENTS =
CallMatcher.instanceCall(AssertJClassNames.ABSTRACT_OBJECT_ARRAY_ASSERT_CLASSNAME, *ARRAY_METHODS).parameterCount(1)
private val HASHCODE_OR_IS_EQUAL_TO =
CallMatcher.instanceCall(
AssertJClassNames.ASSERT_INTERFACE,
MethodNames.IS_EQUAL_TO, "hasSameHashCodeAs"
).parameterCount(1)
private val SAME_ENUMERABLE_CONTENTS =
CallMatcher.instanceCall(
AssertJClassNames.ENUMERABLE_ASSERT_INTERFACE,
@@ -130,9 +135,42 @@ class BogusAssertionInspection : AbstractAssertJInspection() {
.filter(SAME_ACTUAL_AND_EXPECTED_MATCHERS::test)
.any { equivalenceChecker.expressionsAreEquivalent(actualExpression, it.firstArg) }
if (isSameExpression) {
holder.registerProblem(statement, ACTUAL_IS_EQUAL_TO_EXPECTED_MESSAGE)
if (!hasExpressionWithSideEffects(actualExpression)) {
if (allCalls.any(HASHCODE_OR_IS_EQUAL_TO::test)) {
val method = PsiTreeUtil.getParentOfType(statement, PsiMethod::class.java, true)
val methodName = method?.name
if ((methodName != null)
&& ((methodName.contains("equal", ignoreCase = true) || methodName.contains("hashcode", ignoreCase = true)))
) {
if (isOnTheFly) {
holder.registerProblem(statement, WEAK_ACTUAL_IS_EQUAL_TO_EXPECTED_MESSAGE, ProblemHighlightType.INFORMATION)
}
return
}
}
holder.registerProblem(statement, ACTUAL_IS_EQUAL_TO_EXPECTED_MESSAGE)
}
}
}
private fun hasExpressionWithSideEffects(actualExpression: PsiExpression): Boolean {
var result = false
PsiTreeUtil.processElements(actualExpression) { element ->
val matched = when (element) {
is PsiUnaryExpression -> (element.operationTokenType == JavaTokenType.PLUSPLUS)
|| (element.operationTokenType == JavaTokenType.MINUSMINUS)
is PsiMethodCallExpression -> true
else -> false
}
if (matched) {
result = true
false
} else {
true
}
}
return result
}
}
}
}
@@ -1,10 +1,9 @@
package de.platon42.intellij.plugins.cajon.inspections
import com.intellij.codeInspection.ProblemHighlightType
import com.intellij.codeInspection.ProblemsHolder
import com.intellij.psi.JavaElementVisitor
import com.intellij.psi.PsiElementVisitor
import com.intellij.psi.PsiExpressionStatement
import com.intellij.psi.PsiMethodCallExpression
import com.intellij.psi.*
import com.intellij.psi.search.GlobalSearchScope
import com.intellij.psi.util.PsiTreeUtil
import com.siyeh.ig.callMatcher.CallMatcher
import de.platon42.intellij.plugins.cajon.*
@@ -32,6 +31,8 @@ class TwistedAssertionInspection : AbstractAssertJInspection() {
private val STRING_IS_EQUAL_TO_IC = CallMatcher.instanceCall(AssertJClassNames.ABSTRACT_CHAR_SEQUENCE_ASSERT_CLASSNAME, MethodNames.IS_EQUAL_TO_IC).parameterCount(1)
private val STRING_REGEX_MATCHING = CallMatcher.instanceCall(AssertJClassNames.ABSTRACT_CHAR_SEQUENCE_ASSERT_CLASSNAME, "matches", "doesNotMatch").parameterCount(1)
private val CALL_MATCHER_TO_REPLACEMENT_MAP = mapOf(
GENERIC_IS_EQUAL_TO to MethodNames.IS_EQUAL_TO,
GENERIC_IS_NOT_EQUAL_TO to MethodNames.IS_NOT_EQUAL_TO,
@@ -60,31 +61,47 @@ class TwistedAssertionInspection : AbstractAssertJInspection() {
actualExpression.calculateConstantValue() ?: return
val allCalls = assertThatCall.collectMethodCallsUpToStatement().toList()
val tooComplex = allCalls.find(USING_COMPARATOR::test) != null
var severity = ProblemHighlightType.GENERIC_ERROR_OR_WARNING
if (actualExpression.type is PsiClassType) {
val psiManager = PsiManager.getInstance(statement.project)
val javaLangClass = PsiType.getJavaLangClass(psiManager, GlobalSearchScope.allScope(statement.project))
if (actualExpression.type!!.isAssignableFrom(javaLangClass)) {
return
}
}
if (!tooComplex) {
val onlyAssertionCalls = allCalls
.filterNot(NOT_ACTUAL_ASSERTIONS::test)
.toList()
if (onlyAssertionCalls.size == 1) {
val originalMethodCall = onlyAssertionCalls.first()
val matchedMethod = CALL_MATCHER_TO_REPLACEMENT_MAP.asSequence().firstOrNull { it.key.test(originalMethodCall) }
if (matchedMethod != null) {
val originalMethodName = getOriginalMethodName(originalMethodCall)
val replacementMethod = matchedMethod.value
val description = if (originalMethodName == replacementMethod) {
SWAP_ACTUAL_AND_EXPECTED_DESCRIPTION
} else {
SWAP_ACTUAL_AND_EXPECTED_AND_REPLACE_DESCRIPTION_TEMPLATE.format(originalMethodName, replacementMethod)
}
holder.registerProblem(
statement,
TWISTED_ACTUAL_AND_EXPECTED_MESSAGE,
SwapActualAndExpectedExpressionMethodCallQuickFix(description, replacementMethod)
)
val expectedMethodCall = onlyAssertionCalls.first()
if (STRING_REGEX_MATCHING.test(expectedMethodCall)) {
return
}
if (expectedMethodCall.getArgOrNull(0)?.calculateConstantValue() == null) {
val matchedMethod = CALL_MATCHER_TO_REPLACEMENT_MAP.asSequence().firstOrNull { it.key.test(expectedMethodCall) }
if (matchedMethod != null) {
val originalMethodName = getOriginalMethodName(expectedMethodCall)
val replacementMethod = matchedMethod.value
val description = if (originalMethodName == replacementMethod) {
SWAP_ACTUAL_AND_EXPECTED_DESCRIPTION
} else {
SWAP_ACTUAL_AND_EXPECTED_AND_REPLACE_DESCRIPTION_TEMPLATE.format(originalMethodName, replacementMethod)
}
holder.registerProblem(
statement,
TWISTED_ACTUAL_AND_EXPECTED_MESSAGE,
SwapActualAndExpectedExpressionMethodCallQuickFix(description, replacementMethod)
)
return
}
} else {
severity = ProblemHighlightType.WEAK_WARNING
}
}
}
holder.registerProblem(statement, ACTUAL_IS_A_CONSTANT_MESSAGE)
holder.registerProblem(statement, ACTUAL_IS_A_CONSTANT_MESSAGE, severity)
}
}
}
@@ -14,5 +14,6 @@ internal class BogusAssertionInspectionTest : AbstractCajonTest() {
myFixture.enableInspections(BogusAssertionInspection::class.java)
myFixture.configureByFile("BogusAssertionBefore.java")
assertHighlightings(myFixture, 14 * 9 + 10 + 12 + 8, "Actual expression in assertThat() is the same as expected")
assertHighlightings(myFixture, 3, "Same actual and expected expression, but may be testing equals() or hashCode()")
}
}
@@ -13,7 +13,7 @@ internal class TwistedAssertionInspectionTest : AbstractCajonTest() {
internal fun hint_twisted_actual_and_expected_and_provide_quickfix_where_possible(@MyFixture myFixture: JavaCodeInsightTestFixture) {
myFixture.enableInspections(TwistedAssertionInspection::class.java)
myFixture.configureByFile("TwistedAssertionBefore.java")
assertHighlightings(myFixture, 4, "Actual expression in assertThat() is a constant")
assertHighlightings(myFixture, 5, "Actual expression in assertThat() is a constant")
assertHighlightings(myFixture, 10, "Twisted actual and expected expressions")
executeQuickFixes(myFixture, Regex.fromLiteral("Swap actual and expected expressions in assertion"), 6)
@@ -1,3 +1,4 @@
import java.io.File;
import java.util.*;
import static org.assertj.core.api.Assertions.assertThat;
@@ -190,7 +191,28 @@ public class BogusAssertions {
assertThat(bar).isEqualTo(string);
assertThat(new Random().nextBoolean()).isEqualTo(new Random().nextBoolean());
assertThat(generateString()).isEqualTo(generateString());
int number = 4;
assertThat(number++).isEqualTo(number++);
assertThat(number++).isEqualTo(number++);
org.junit.Assert.assertThat(list, null);
fail("oh no!");
}
private void test_equals() {
assertThat("foo").isEqualTo("foo");
assertThat(new File("foo")).isEqualTo(new File("foo"));
}
private void test_HasHCode() {
assertThat("foo").hasSameHashCodeAs("foo");
}
private String generateString()
{
return "foo";
}
}
@@ -1,4 +1,5 @@
import java.util.*;
import java.util.regex.Pattern;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail;
@@ -32,6 +33,13 @@ public class TwistedAssertions {
assertThat(4).isEqualTo(number).isNotEqualTo(number * 2);
assertThat(4).usingComparator(Comparator.reverseOrder()).isGreaterThanOrEqualTo(number);
assertThat(String.class).isEqualTo(Class.forName("java.lang.String"));
assertThat("XX").matches(Pattern.compile(".."));
assertThat("XX").matches(".."));
assertThat("XX").doesNotMatch(Pattern.compile(".."));
assertThat("XX").doesNotMatch(".."));
assertThat(SOME_CONST).isEqualTo(10);
org.junit.Assert.assertThat(list, null);
fail("oh no!");
}
@@ -1,4 +1,5 @@
import java.util.*;
import java.util.regex.Pattern;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail;
@@ -32,6 +33,13 @@ public class TwistedAssertions {
assertThat(4).isEqualTo(number).isNotEqualTo(number * 2);
assertThat(4).usingComparator(Comparator.reverseOrder()).isGreaterThanOrEqualTo(number);
assertThat(String.class).isEqualTo(Class.forName("java.lang.String"));
assertThat("XX").matches(Pattern.compile(".."));
assertThat("XX").matches(".."));
assertThat("XX").doesNotMatch(Pattern.compile(".."));
assertThat("XX").doesNotMatch(".."));
assertThat(SOME_CONST).isEqualTo(10);
org.junit.Assert.assertThat(list, null);
fail("oh no!");
}