Improvements for TwistedAssertionInspection to no longer report .class types as constant to bother, nor matches() and doesNotMatch() for regexps. If both sides are constants, they will only show as weak problems.

This commit is contained in:
Chris Hodges 2020-02-17 20:17:33 +01:00
parent e8ce8ce2c6
commit 9b25e50183
6 changed files with 73 additions and 21 deletions

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. 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 - BogusAssertion
Sometimes programmers make copy and paste or logical errors writing down assertions Sometimes programmers make copy and paste or logical errors writing down assertions
@ -785,6 +794,11 @@ Feel free to use the code (in package ```de.platon42.intellij.jupiter```) for yo
## Changelog ## Changelog
#### V1.9 (unreleased) #### V1.9 (unreleased)
- 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. - 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', 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. the warning will be reduced to information level as the assertion may be testing ```equals()``` or ```hashCode()``` for validity.

View File

@ -45,6 +45,11 @@ patchPluginXml {
changeNotes """ changeNotes """
<h4>V1.9 (unreleased)</h4> <h4>V1.9 (unreleased)</h4>
<ul> <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. <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', 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. the warning will be reduced to information level as the assertion may be testing equals() or hashCode() for validity.

View File

@ -1,10 +1,9 @@
package de.platon42.intellij.plugins.cajon.inspections package de.platon42.intellij.plugins.cajon.inspections
import com.intellij.codeInspection.ProblemHighlightType
import com.intellij.codeInspection.ProblemsHolder import com.intellij.codeInspection.ProblemsHolder
import com.intellij.psi.JavaElementVisitor import com.intellij.psi.*
import com.intellij.psi.PsiElementVisitor import com.intellij.psi.search.GlobalSearchScope
import com.intellij.psi.PsiExpressionStatement
import com.intellij.psi.PsiMethodCallExpression
import com.intellij.psi.util.PsiTreeUtil import com.intellij.psi.util.PsiTreeUtil
import com.siyeh.ig.callMatcher.CallMatcher import com.siyeh.ig.callMatcher.CallMatcher
import de.platon42.intellij.plugins.cajon.* 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_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( private val CALL_MATCHER_TO_REPLACEMENT_MAP = mapOf(
GENERIC_IS_EQUAL_TO to MethodNames.IS_EQUAL_TO, GENERIC_IS_EQUAL_TO to MethodNames.IS_EQUAL_TO,
GENERIC_IS_NOT_EQUAL_TO to MethodNames.IS_NOT_EQUAL_TO, GENERIC_IS_NOT_EQUAL_TO to MethodNames.IS_NOT_EQUAL_TO,
@ -60,15 +61,28 @@ class TwistedAssertionInspection : AbstractAssertJInspection() {
actualExpression.calculateConstantValue() ?: return actualExpression.calculateConstantValue() ?: return
val allCalls = assertThatCall.collectMethodCallsUpToStatement().toList() val allCalls = assertThatCall.collectMethodCallsUpToStatement().toList()
val tooComplex = allCalls.find(USING_COMPARATOR::test) != null 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) { if (!tooComplex) {
val onlyAssertionCalls = allCalls val onlyAssertionCalls = allCalls
.filterNot(NOT_ACTUAL_ASSERTIONS::test) .filterNot(NOT_ACTUAL_ASSERTIONS::test)
.toList() .toList()
if (onlyAssertionCalls.size == 1) { if (onlyAssertionCalls.size == 1) {
val originalMethodCall = onlyAssertionCalls.first() val expectedMethodCall = onlyAssertionCalls.first()
val matchedMethod = CALL_MATCHER_TO_REPLACEMENT_MAP.asSequence().firstOrNull { it.key.test(originalMethodCall) } 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) { if (matchedMethod != null) {
val originalMethodName = getOriginalMethodName(originalMethodCall) val originalMethodName = getOriginalMethodName(expectedMethodCall)
val replacementMethod = matchedMethod.value val replacementMethod = matchedMethod.value
val description = if (originalMethodName == replacementMethod) { val description = if (originalMethodName == replacementMethod) {
SWAP_ACTUAL_AND_EXPECTED_DESCRIPTION SWAP_ACTUAL_AND_EXPECTED_DESCRIPTION
@ -82,9 +96,12 @@ class TwistedAssertionInspection : AbstractAssertJInspection() {
) )
return return
} }
} else {
severity = ProblemHighlightType.WEAK_WARNING
} }
} }
holder.registerProblem(statement, ACTUAL_IS_A_CONSTANT_MESSAGE) }
holder.registerProblem(statement, ACTUAL_IS_A_CONSTANT_MESSAGE, severity)
} }
} }
} }

View File

@ -13,7 +13,7 @@ internal class TwistedAssertionInspectionTest : AbstractCajonTest() {
internal fun hint_twisted_actual_and_expected_and_provide_quickfix_where_possible(@MyFixture myFixture: JavaCodeInsightTestFixture) { internal fun hint_twisted_actual_and_expected_and_provide_quickfix_where_possible(@MyFixture myFixture: JavaCodeInsightTestFixture) {
myFixture.enableInspections(TwistedAssertionInspection::class.java) myFixture.enableInspections(TwistedAssertionInspection::class.java)
myFixture.configureByFile("TwistedAssertionBefore.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") assertHighlightings(myFixture, 10, "Twisted actual and expected expressions")
executeQuickFixes(myFixture, Regex.fromLiteral("Swap actual and expected expressions in assertion"), 6) executeQuickFixes(myFixture, Regex.fromLiteral("Swap actual and expected expressions in assertion"), 6)

View File

@ -1,4 +1,5 @@
import java.util.*; import java.util.*;
import java.util.regex.Pattern;
import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail; import static org.assertj.core.api.Assertions.fail;
@ -32,6 +33,13 @@ public class TwistedAssertions {
assertThat(4).isEqualTo(number).isNotEqualTo(number * 2); assertThat(4).isEqualTo(number).isNotEqualTo(number * 2);
assertThat(4).usingComparator(Comparator.reverseOrder()).isGreaterThanOrEqualTo(number); 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); org.junit.Assert.assertThat(list, null);
fail("oh no!"); fail("oh no!");
} }

View File

@ -1,4 +1,5 @@
import java.util.*; import java.util.*;
import java.util.regex.Pattern;
import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.fail; import static org.assertj.core.api.Assertions.fail;
@ -32,6 +33,13 @@ public class TwistedAssertions {
assertThat(4).isEqualTo(number).isNotEqualTo(number * 2); assertThat(4).isEqualTo(number).isNotEqualTo(number * 2);
assertThat(4).usingComparator(Comparator.reverseOrder()).isGreaterThanOrEqualTo(number); 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); org.junit.Assert.assertThat(list, null);
fail("oh no!"); fail("oh no!");
} }