From 42429c0f72b5f142a7199549566c3c63e08467a3 Mon Sep 17 00:00:00 2001 From: chrisly42 Date: Sun, 17 Nov 2019 21:21:45 +0100 Subject: [PATCH] Added AssertThatComparableExpression for funny compareTo() uses. --- README.md | 33 ++++- build.gradle | 1 + .../inspections/AbstractAssertJInspection.kt | 2 + .../AssertThatComparableInspection.kt | 115 ++++++++++++++++++ src/main/resources/META-INF/plugin.xml | 2 + .../AssertThatComparable.html | 5 + .../AssertThatComparableInspectionTest.kt | 24 ++++ .../Comparable/ComparableAfter.java | 41 +++++++ .../Comparable/ComparableBefore.java | 41 +++++++ 9 files changed, 262 insertions(+), 2 deletions(-) create mode 100644 src/main/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspection.kt create mode 100644 src/main/resources/inspectionDescriptions/AssertThatComparable.html create mode 100644 src/test/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspectionTest.kt create mode 100644 src/test/resources/inspections/Comparable/ComparableAfter.java create mode 100644 src/test/resources/inspections/Comparable/ComparableBefore.java diff --git a/README.md b/README.md index f42a673..79441b2 100644 --- a/README.md +++ b/README.md @@ -234,6 +234,35 @@ You can toggle the various inspections in the Settings/Editor/Inspections in the to: assertThat(objActual).hasSameHashCodeAs(objExpected); ``` +- AssertThatComparableExpression + + Handles ```compareTo()``` inside an expected expression. + + ``` + from: assertThat(obj1.compareTo(obj2)).isEqualTo(0); + to: assertThat(obj1).isEqualByComparingTo(obj2); + + from: assertThat(obj1.compareTo(obj2)).isNotZero(); + to: assertThat(obj1).isNotEqualByComparingTo(obj2); + + from: assertThat(obj1.compareTo(obj2)).isNotEqualTo(-1); + from: assertThat(obj1.compareTo(obj2)).isGreaterThanOrEqualTo(0); + from: assertThat(obj1.compareTo(obj2)).isGreaterThan(-1); + from: assertThat(obj1.compareTo(obj2)).isNotNegative(); + to: assertThat(obj1).isGreaterThanOrEqualTo(obj2); + + from: assertThat(obj1.compareTo(obj2)).isOne(); + to: assertThat(obj1).isGreaterThan(obj2); + + from: assertThat(obj1.compareTo(obj2)).isNotPositive(); + to: assertThat(obj1).isLessThanOrEqualTo(obj2); + + from: assertThat(obj1.compareTo(obj2)).isLessThan(0); + to: assertThat(obj1).isLessThan(obj2); + ``` + + Several more combinations omitted... + - AssertThatCollectionOrMapExpression Moves ```Collection``` and ```Map``` operations inside ```assertThat()``` out. @@ -657,8 +686,7 @@ Feel free to use the code (in package ```de.platon42.intellij.jupiter```) for yo ## Planned features - More Optional fixes such as ```opt1.get() == opt2.get()``` etc. -- More moving out of methods for Path, LocalDate/Time etc. -- Converting ```foo.compareTo(bar) == 0``` to ```isEqualTo()``` (yes, I've *really* seen code like that) +- More moving out of methods for LocalDate/Time etc. - Extraction with property names to lambda/method reference with Java 8 ``` @@ -672,6 +700,7 @@ Feel free to use the code (in package ```de.platon42.intellij.jupiter```) for yo - Fixed a lapsuus in AssertThatFileExpression also transforming ```.listFiles()``` with a filter argument. - Added first version of AssertThatPathExpression for a limited number transformations (more stuff is possible, but requires detection and transformation of static ```Files```-methods). +- Added AssertThatComparableExpression for funny ```compareTo()``` uses. #### V1.6 (30-Sep-19) - Really fixed AssertThatGuavaOptional inspections to avoid conversions from ```.get()``` to ```.contains()``` diff --git a/build.gradle b/build.gradle index 7e28525..3db4a40 100644 --- a/build.gradle +++ b/build.gradle @@ -48,6 +48,7 @@ patchPluginXml {
  • Fixed a lapsuus in AssertThatFileExpression also transforming listFiles() with a filter argument.
  • Added first version of AssertThatPathExpression for a limited number transformations (more stuff is possible, but requires detection and transformation of static Files-methods). +
  • Added AssertThatComparableExpression for funny compareTo() uses.

    Full changelog available at Github project site.

    """ diff --git a/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AbstractAssertJInspection.kt b/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AbstractAssertJInspection.kt index 723f044..1f963de 100644 --- a/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AbstractAssertJInspection.kt +++ b/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AbstractAssertJInspection.kt @@ -141,6 +141,8 @@ abstract class AbstractAssertJInspection : AbstractBaseJavaLocalInspectionTool() .parameterCount(0)!! val IS_NOT_ZERO = CallMatcher.instanceCall(ABSTRACT_INTEGER_ASSERT_CLASSNAME, MethodNames.IS_NOT_ZERO) .parameterCount(0)!! + val IS_ONE = CallMatcher.instanceCall(ABSTRACT_INTEGER_ASSERT_CLASSNAME, "isOne") + .parameterCount(0)!! val IS_NEGATIVE = CallMatcher.instanceCall(ABSTRACT_INTEGER_ASSERT_CLASSNAME, "isNegative") .parameterCount(0)!! val IS_NOT_NEGATIVE = CallMatcher.instanceCall(ABSTRACT_INTEGER_ASSERT_CLASSNAME, "isNotNegative") diff --git a/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspection.kt b/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspection.kt new file mode 100644 index 0000000..163dcdb --- /dev/null +++ b/src/main/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspection.kt @@ -0,0 +1,115 @@ +package de.platon42.intellij.plugins.cajon.inspections + +import com.intellij.codeInspection.ProblemsHolder +import com.intellij.psi.* +import com.siyeh.ig.callMatcher.CallMatcher +import de.platon42.intellij.plugins.cajon.MethodNames +import de.platon42.intellij.plugins.cajon.calculateConstantValue +import de.platon42.intellij.plugins.cajon.firstArg + +class AssertThatComparableInspection : AbstractMoveOutInspection() { + + companion object { + private const val DISPLAY_NAME = "Asserting a compareTo() expression" + + private val ARG_IS_ZERO_CONST: (PsiExpressionStatement, PsiMethodCallExpression) -> Boolean = { _, call -> call.firstArg.calculateConstantValue() == 0 } + private val ARG_IS_PLUS_ONE_CONST: (PsiExpressionStatement, PsiMethodCallExpression) -> Boolean = { _, call -> call.firstArg.calculateConstantValue() == 1 } + private val ARG_IS_MINUS_ONE_CONST: (PsiExpressionStatement, PsiMethodCallExpression) -> Boolean = { _, call -> call.firstArg.calculateConstantValue() == -1 } + + private val COMPARABLE_COMPARE_TO = + CallMatcher.instanceCall(CommonClassNames.JAVA_LANG_COMPARABLE, "compareTo").parameterCount(1) + + private val MAPPINGS = listOf( + MoveOutMapping( + COMPARABLE_COMPARE_TO, + "isEqualByComparingTo", expectedMatcher = IS_EQUAL_TO_INT, replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_ZERO_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + "isEqualByComparingTo", expectedMatcher = IS_ZERO, replaceFromOriginalMethod = true + ), + + MoveOutMapping( + COMPARABLE_COMPARE_TO, + "isNotEqualByComparingTo", expectedMatcher = IS_NOT_EQUAL_TO_INT, replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_ZERO_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + "isNotEqualByComparingTo", expectedMatcher = IS_NOT_ZERO, replaceFromOriginalMethod = true + ), + + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_GREATER_THAN_OR_EQUAL_TO, expectedMatcher = IS_GREATER_THAN_OR_EQUAL_TO_INT, replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_ZERO_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_GREATER_THAN_OR_EQUAL_TO, expectedMatcher = CallMatcher.anyOf(IS_NOT_EQUAL_TO_INT, IS_GREATER_THAN_INT), replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_MINUS_ONE_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_GREATER_THAN_OR_EQUAL_TO, expectedMatcher = IS_NOT_NEGATIVE, replaceFromOriginalMethod = true + ), + + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_GREATER_THAN, expectedMatcher = CallMatcher.anyOf(IS_EQUAL_TO_INT, IS_GREATER_THAN_OR_EQUAL_TO_INT), replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_PLUS_ONE_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_GREATER_THAN, expectedMatcher = IS_GREATER_THAN_INT, replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_ZERO_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_GREATER_THAN, expectedMatcher = CallMatcher.anyOf(IS_POSITIVE, IS_ONE), replaceFromOriginalMethod = true + ), + + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_LESS_THAN_OR_EQUAL_TO, expectedMatcher = IS_LESS_THAN_OR_EQUAL_TO_INT, replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_ZERO_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_LESS_THAN_OR_EQUAL_TO, expectedMatcher = CallMatcher.anyOf(IS_NOT_EQUAL_TO_INT, IS_LESS_THAN_INT), replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_PLUS_ONE_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_LESS_THAN_OR_EQUAL_TO, expectedMatcher = IS_NOT_POSITIVE, replaceFromOriginalMethod = true + ), + + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_LESS_THAN, expectedMatcher = CallMatcher.anyOf(IS_EQUAL_TO_INT, IS_LESS_THAN_OR_EQUAL_TO_INT), replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_MINUS_ONE_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_LESS_THAN, expectedMatcher = IS_LESS_THAN_INT, replaceFromOriginalMethod = true, + additionalCondition = ARG_IS_ZERO_CONST + ), + MoveOutMapping( + COMPARABLE_COMPARE_TO, + MethodNames.IS_LESS_THAN, expectedMatcher = IS_NEGATIVE, replaceFromOriginalMethod = true + ) + ) + } + + override fun getDisplayName() = DISPLAY_NAME + + override fun buildVisitor(holder: ProblemsHolder, isOnTheFly: Boolean): PsiElementVisitor { + return object : JavaElementVisitor() { + override fun visitExpressionStatement(statement: PsiExpressionStatement) { + super.visitExpressionStatement(statement) + createInspectionsForMappings(statement, holder, MAPPINGS) + } + } + } +} \ No newline at end of file diff --git a/src/main/resources/META-INF/plugin.xml b/src/main/resources/META-INF/plugin.xml index 76d686c..8a200f8 100644 --- a/src/main/resources/META-INF/plugin.xml +++ b/src/main/resources/META-INF/plugin.xml @@ -41,6 +41,8 @@ implementationClass="de.platon42.intellij.plugins.cajon.inspections.AssertThatBinaryExpressionInspection"/> + + +Turns assertThat(obj1.compareTo(obj2)) into assertThat(obj1).someMethod(obj2). + + \ No newline at end of file diff --git a/src/test/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspectionTest.kt b/src/test/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspectionTest.kt new file mode 100644 index 0000000..3108141 --- /dev/null +++ b/src/test/java/de/platon42/intellij/plugins/cajon/inspections/AssertThatComparableInspectionTest.kt @@ -0,0 +1,24 @@ +package de.platon42.intellij.plugins.cajon.inspections + +import com.intellij.testFramework.fixtures.JavaCodeInsightTestFixture +import de.platon42.intellij.jupiter.MyFixture +import de.platon42.intellij.jupiter.TestDataSubPath +import de.platon42.intellij.plugins.cajon.AbstractCajonTest +import org.junit.jupiter.api.Test + +internal class AssertThatComparableInspectionTest : AbstractCajonTest() { + + @Test + @TestDataSubPath("inspections/Comparable") + internal fun assertThat_with_compareTo_method(@MyFixture myFixture: JavaCodeInsightTestFixture) { + myFixture.enableInspections(AssertThatComparableInspection::class.java) + myFixture.configureByFile("ComparableBefore.java") + executeQuickFixes(myFixture, Regex.fromLiteral("Remove compareTo() of actual expression and use assertThat().isEqualByComparingTo() instead"), 2) + executeQuickFixes(myFixture, Regex.fromLiteral("Remove compareTo() of actual expression and use assertThat().isNotEqualByComparingTo() instead"), 2) + executeQuickFixes(myFixture, Regex.fromLiteral("Remove compareTo() of actual expression and use assertThat().isGreaterThanOrEqualTo() instead"), 4) + executeQuickFixes(myFixture, Regex.fromLiteral("Remove compareTo() of actual expression and use assertThat().isGreaterThan() instead"), 5) + executeQuickFixes(myFixture, Regex.fromLiteral("Remove compareTo() of actual expression and use assertThat().isLessThanOrEqualTo() instead"), 4) + executeQuickFixes(myFixture, Regex.fromLiteral("Remove compareTo() of actual expression and use assertThat().isLessThan() instead"), 4) + myFixture.checkResultByFile("ComparableAfter.java") + } +} \ No newline at end of file diff --git a/src/test/resources/inspections/Comparable/ComparableAfter.java b/src/test/resources/inspections/Comparable/ComparableAfter.java new file mode 100644 index 0000000..520059a --- /dev/null +++ b/src/test/resources/inspections/Comparable/ComparableAfter.java @@ -0,0 +1,41 @@ +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; + +public class CompareTo { + + private void comparable() { + String string = "string"; + assertThat(string).isEqualByComparingTo("foo"); + assertThat(string).isEqualByComparingTo("foo"); + + assertThat(string).isNotEqualByComparingTo("foo"); + assertThat(string).isNotEqualByComparingTo("foo"); + + assertThat(string).isGreaterThanOrEqualTo("foo"); + assertThat(string).isGreaterThanOrEqualTo("foo"); + assertThat(string).isGreaterThanOrEqualTo("foo"); + assertThat(string).isGreaterThanOrEqualTo("foo"); + + assertThat(string).isGreaterThan("foo"); + assertThat(string).isGreaterThan("foo"); + assertThat(string).isGreaterThan("foo"); + assertThat(string).isGreaterThan("foo"); + assertThat(string).isGreaterThan("foo"); + + assertThat(string).isLessThanOrEqualTo("foo"); + assertThat(string).isLessThanOrEqualTo("foo"); + assertThat(string).isLessThanOrEqualTo("foo"); + assertThat(string).isLessThanOrEqualTo("foo"); + + assertThat(string).isLessThan("foo"); + assertThat(string).isLessThan("foo"); + assertThat(string).isLessThan("foo"); + assertThat(string).isLessThan("foo"); + + assertThat(string.compareTo("foo")).isNotEqualTo(2); + assertThat(string.compareTo("foo")).isEqualTo(2); + + org.junit.Assert.assertThat(string, null); + fail("oh no!"); + } +} diff --git a/src/test/resources/inspections/Comparable/ComparableBefore.java b/src/test/resources/inspections/Comparable/ComparableBefore.java new file mode 100644 index 0000000..ae3b1c6 --- /dev/null +++ b/src/test/resources/inspections/Comparable/ComparableBefore.java @@ -0,0 +1,41 @@ +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.fail; + +public class CompareTo { + + private void comparable() { + String string = "string"; + assertThat(string.compareTo("foo")).isEqualTo(0); + assertThat(string.compareTo("foo")).isZero(); + + assertThat(string.compareTo("foo")).isNotEqualTo(0); + assertThat(string.compareTo("foo")).isNotZero(); + + assertThat(string.compareTo("foo")).isNotEqualTo(-1); + assertThat(string.compareTo("foo")).isGreaterThanOrEqualTo(0); + assertThat(string.compareTo("foo")).isGreaterThan(-1); + assertThat(string.compareTo("foo")).isNotNegative(); + + assertThat(string.compareTo("foo")).isEqualTo(1); + assertThat(string.compareTo("foo")).isOne(); + assertThat(string.compareTo("foo")).isGreaterThan(0); + assertThat(string.compareTo("foo")).isPositive(); + assertThat(string.compareTo("foo")).isGreaterThanOrEqualTo(1); + + assertThat(string.compareTo("foo")).isNotEqualTo(1); + assertThat(string.compareTo("foo")).isLessThanOrEqualTo(0); + assertThat(string.compareTo("foo")).isLessThan(1); + assertThat(string.compareTo("foo")).isNotPositive(); + + assertThat(string.compareTo("foo")).isEqualTo(-1); + assertThat(string.compareTo("foo")).isLessThan(0); + assertThat(string.compareTo("foo")).isNegative(); + assertThat(string.compareTo("foo")).isLessThanOrEqualTo(-1); + + assertThat(string.compareTo("foo")).isNotEqualTo(2); + assertThat(string.compareTo("foo")).isEqualTo(2); + + org.junit.Assert.assertThat(string, null); + fail("oh no!"); + } +}