Improve AnnotationExcluder tests (#4368)

* Add missing deprecation * Simplify AnnotationExcluder * Improve tests making them more exhaustive * Handle the globbing inside AnnotationExcluder * Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt Co-authored-by: Chao Zhang <zhangchao6865@gmail.com> Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>
detekt · Jan 23, 2022 · a6c8435 · a6c8435
1 parent ca9a3fa
commit a6c8435
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 47 deletions.
diff --git a/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt b/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt
@@ -10,8 +10,11 @@ import org.jetbrains.kotlin.psi.KtFile
  */
 class AnnotationExcluder(
     root: KtFile,
-    private val excludes: List<String>
+    excludes: List<String>,
 ) {
+    private val excludes = excludes.map {
+        it.removePrefix("*").removeSuffix("*")
+    }
 
     private val resolvedAnnotations = root.importList?.run {
         imports
@@ -22,6 +25,7 @@ class AnnotationExcluder(
             .toMap()
     }.orEmpty()
 
+    @Deprecated("Use AnnotationExcluder(KtFile, List<String>) instead")
     constructor(root: KtFile, excludes: SplitPattern) : this(root, excludes.mapAll { it })
 
     /**
@@ -31,8 +35,8 @@ class AnnotationExcluder(
     fun shouldExclude(annotations: List<KtAnnotationEntry>): Boolean = annotations.any(::isExcluded)
 
     private fun isExcluded(annotation: KtAnnotationEntry): Boolean {
-        val annotationText = annotation.typeReference?.text
+        val annotationText = annotation.typeReference?.text ?: return false
         val value = resolvedAnnotations[annotationText] ?: annotationText
-        return if (value == null) false else excludes.any { value.contains(it, ignoreCase = true) }
+        return excludes.any { value.contains(it, ignoreCase = true) }
     }
 }
diff --git a/detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluderSpec.kt b/detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluderSpec.kt
@@ -12,50 +12,101 @@ class AnnotationExcluderSpec : Spek({
     val psiFactory by memoized(CachingMode.SCOPE) { createPsiFactory() }
 
     describe("a kt file with some imports") {
-
-        val jvmFieldAnnotation by memoized { psiFactory.createAnnotationEntry("@JvmField") }
-        val fullyQualifiedJvmFieldAnnotation by memoized { psiFactory.createAnnotationEntry("@kotlin.jvm.JvmField") }
-        val sinceKotlinAnnotation by memoized { psiFactory.createAnnotationEntry("@SinceKotlin") }
-
         val file by memoized {
             compileContentForTest(
                 """
                 package foo
 
-                import kotlin.jvm.JvmField
+                import dagger.Component
+                import dagger.Component.Factory
                 """.trimIndent()
             )
         }
 
-        it("should exclude when the annotation was found") {
-            val excluder = AnnotationExcluder(file, listOf("JvmField"))
-            assertThat(excluder.shouldExclude(listOf(jvmFieldAnnotation))).isTrue()
-        }
+        context("All cases") {
+            data class Case(val excludes: String, val annotation: String)
 
-        it("should not exclude when the annotation was not found") {
-            val excluder = AnnotationExcluder(file, listOf("Jvm Field"))
-            assertThat(excluder.shouldExclude(listOf(jvmFieldAnnotation))).isFalse()
-        }
+            mapOf(
+                Case("Component", "@Component") to true,
+                Case("Component", "@dagger.Component") to true,
+                Case("Component", "@Factory") to true, // false positive
+                Case("Component", "@Component.Factory") to true, // false positive
+                Case("Component", "@dagger.Component.Factory") to true, // false positive
+                Case("dagger.Component", "@Component") to true,
+                Case("dagger.Component", "@dagger.Component") to true,
+                Case("dagger.Component", "@Factory") to true, // false positive
+                Case("dagger.Component", "@Component.Factory") to false,
+                Case("dagger.Component", "@dagger.Component.Factory") to true, // false positive
+                Case("Component.Factory", "@Component") to false,
+                Case("Component.Factory", "@dagger.Component") to false,
+                Case("Component.Factory", "@Factory") to true,
+                Case("Component.Factory", "@Component.Factory") to true,
+                Case("Component.Factory", "@dagger.Component.Factory") to true,
+                Case("dagger.Component.Factory", "@Component") to false,
+                Case("dagger.Component.Factory", "@dagger.Component") to false,
+                Case("dagger.Component.Factory", "@Factory") to true,
+                Case("dagger.Component.Factory", "@Component.Factory") to false, // false negative
+                Case("dagger.Component.Factory", "@dagger.Component.Factory") to true,
+                Case("Factory", "@Component") to false,
+                Case("Factory", "@dagger.Component") to false,
+                Case("Factory", "@Factory") to true,
+                Case("Factory", "@Component.Factory") to true,
+                Case("Factory", "@dagger.Component.Factory") to true,
+                Case("dagger.*", "@Component") to true,
+                Case("dagger.*", "@dagger.Component") to true,
+                Case("dagger.*", "@Factory") to true,
+                Case("dagger.*", "@Component.Factory") to false, // false positive
+                Case("dagger.*", "@dagger.Component.Factory") to true,
+                Case("*.Component.Factory", "@Component") to false,
+                Case("*.Component.Factory", "@dagger.Component") to false,
+                Case("*.Component.Factory", "@Factory") to true,
+                Case("*.Component.Factory", "@Component.Factory") to false, // false positive
+                Case("*.Component.Factory", "@dagger.Component.Factory") to true,
+                Case("*.Component.*", "@Component") to false,
+                Case("*.Component.*", "@dagger.Component") to false,
+                Case("*.Component.*", "@Factory") to true,
+                Case("*.Component.*", "@Component.Factory") to false, // false positive
+                Case("*.Component.*", "@dagger.Component.Factory") to true,
+                Case("foo.Component", "@Component") to false,
+                Case("foo.Component", "@dagger.Component") to false,
+                Case("foo.Component", "@Factory") to false,
+                Case("foo.Component", "@Component.Factory") to false,
+                Case("foo.Component", "@dagger.Component.Factory") to false,
+            ).forEach { (case, expected) ->
+                val (exclude, annotation) = case
+                it("With exclude $exclude and annotation $annotation") {
+                    val excluder = AnnotationExcluder(file, listOf(exclude))
 
-        it("should not exclude when no annotations should be excluded") {
-            val excluder = AnnotationExcluder(file, emptyList())
-            assertThat(excluder.shouldExclude(listOf(jvmFieldAnnotation))).isFalse()
+                    val ktAnnotation = psiFactory.createAnnotationEntry(annotation)
+                    assertThat(excluder.shouldExclude(listOf(ktAnnotation))).isEqualTo(expected)
+                }
+            }
         }
 
-        it("should exclude when the annotation was found with its fully qualified name") {
-            val excluder = AnnotationExcluder(file, listOf("JvmField"))
-            assertThat(excluder.shouldExclude(listOf(fullyQualifiedJvmFieldAnnotation))).isTrue()
-        }
+        context("special cases") {
+            val annotation by memoized { psiFactory.createAnnotationEntry("@Component") }
+            val sinceKotlinAnnotation by memoized { psiFactory.createAnnotationEntry("@SinceKotlin") }
 
-        it("should also exclude an annotation that is not imported") {
-            val excluder = AnnotationExcluder(file, listOf("SinceKotlin"))
-            assertThat(excluder.shouldExclude(listOf(sinceKotlinAnnotation))).isTrue()
-        }
+            it("should not exclude when the annotation was not found") {
+                val excluder = AnnotationExcluder(file, listOf("SinceKotlin"))
+                assertThat(excluder.shouldExclude(listOf(annotation))).isFalse()
+            }
+
+            it("should not exclude when no annotations should be excluded") {
+                val excluder = AnnotationExcluder(file, emptyList())
+                assertThat(excluder.shouldExclude(listOf(annotation))).isFalse()
+            }
+
+            it("should also exclude an annotation that is not imported") {
+                val excluder = AnnotationExcluder(file, listOf("SinceKotlin"))
+                assertThat(excluder.shouldExclude(listOf(sinceKotlinAnnotation))).isTrue()
+            }
 
-        it("should exclude when the annotation was found with SplitPattern") {
-            @Suppress("Deprecation")
-            val excluder = AnnotationExcluder(file, SplitPattern("JvmField"))
-            assertThat(excluder.shouldExclude(listOf(jvmFieldAnnotation))).isTrue()
+            it("should exclude when the annotation was found with SplitPattern") {
+                @Suppress("DEPRECATION")
+                val excluder = AnnotationExcluder(file, SplitPattern("SinceKotlin"))
+                assertThat(excluder.shouldExclude(listOf(sinceKotlinAnnotation))).isTrue()
+            }
         }
     }
 })
diff --git a/...plexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterList.kt b/...plexity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/LongParameterList.kt
@@ -62,9 +62,7 @@ class LongParameterList(config: Config = Config.empty) : Rule(config) {
     @Configuration(
         "ignore the annotated parameters for the count (e.g. `fun foo(@Value bar: Int)` would not be counted"
     )
-    private val ignoreAnnotatedParameter: List<String> by config(emptyList<String>()) { list ->
-        list.map { it.removePrefix("*").removeSuffix("*") }
-    }
+    private val ignoreAnnotatedParameter: List<String> by config(emptyList())
 
     private lateinit var annotationExcluder: AnnotationExcluder
 

diff --git a/...-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/LateinitUsage.kt b/...-rules-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/LateinitUsage.kt
@@ -40,9 +40,7 @@ class LateinitUsage(config: Config = Config.empty) : Rule(config) {
 
     @Configuration("Allows you to provide a list of annotations that disable this check.")
     @Deprecated("Use `ignoreAnnotated` instead")
-    private val excludeAnnotatedProperties: List<String> by config(emptyList<String>()) { list ->
-        list.map { it.removePrefix("*").removeSuffix("*") }
-    }
+    private val excludeAnnotatedProperties: List<String> by config(emptyList())
 
     @Configuration("Allows you to disable the rule for a list of classes")
     private val ignoreOnClassesPattern: Regex by config("", String::toRegex)

diff --git a/.../src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/FunctionOnlyReturningConstant.kt b/.../src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/FunctionOnlyReturningConstant.kt
@@ -56,9 +56,7 @@ class FunctionOnlyReturningConstant(config: Config = Config.empty) : Rule(config
 
     @Configuration("allows to provide a list of annotations that disable this check")
     @Deprecated("Use `ignoreAnnotated` instead")
-    private val excludeAnnotatedFunction: List<String> by config(emptyList<String>()) { functions ->
-        functions.map { it.removePrefix("*").removeSuffix("*") }
-    }
+    private val excludeAnnotatedFunction: List<String> by config(emptyList())
 
     private lateinit var annotationExcluder: AnnotationExcluder
 

diff --git a/...style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClass.kt b/...style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClass.kt
@@ -60,9 +60,7 @@ class UnnecessaryAbstractClass(config: Config = Config.empty) : Rule(config) {
 
     @Configuration("Allows you to provide a list of annotations that disable this check.")
     @Deprecated("Use `ignoreAnnotated` instead")
-    private val excludeAnnotatedClasses: List<String> by config(emptyList<String>()) { classes ->
-        classes.map { it.removePrefix("*").removeSuffix("*") }
-    }
+    private val excludeAnnotatedClasses: List<String> by config(emptyList())
 
     private lateinit var annotationExcluder: AnnotationExcluder
 

diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseDataClass.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseDataClass.kt
@@ -56,9 +56,7 @@ class UseDataClass(config: Config = Config.empty) : Rule(config) {
 
     @Configuration("allows to provide a list of annotations that disable this check")
     @Deprecated("Use `ignoreAnnotated` instead")
-    private val excludeAnnotatedClasses: List<String> by config(emptyList<String>()) { classes ->
-        classes.map { it.removePrefix("*").removeSuffix("*") }
-    }
+    private val excludeAnnotatedClasses: List<String> by config(emptyList())
 
     @Configuration("allows to relax this rule in order to exclude classes that contains one (or more) vars")
     private val allowVars: Boolean by config(false)