Merge pull request #2300 from DataDog/xgouchet/reliability/it_api_cov…

…erage Detekt the api coverage in Integration tests
DataDog · Oct 9, 2024 · 4b83197 · 4b83197
2 parents 4fce2ac + 6d6ea18
commit 4b83197
Show file tree

Hide file tree

Showing 5 changed files with 121 additions and 37 deletions.
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
@@ -370,6 +370,22 @@ test-pyramid:legacy-integration-instrumented-median-api:
     - !reference [.snippets, install-android-sdk]
     - !reference [.snippets, run-legacy-integration-instrumented]
 
+test-pyramid:detekt-api-coverage:
+  tags: [ "arch:amd64" ]
+  image: $CI_IMAGE_DOCKER
+  stage: test-pyramid
+  timeout: 1h
+  script:
+    - mkdir -p ./config/
+    - aws ssm get-parameter --region us-east-1 --name ci.dd-sdk-android.gradle-properties --with-decryption --query "Parameter.Value" --out text >> ./gradle.properties
+    - GRADLE_OPTS="-Xmx4096M" ./gradlew assembleLibrariesDebug --stacktrace --no-daemon
+    - GRADLE_OPTS="-Xmx4096M" ./gradlew printSdkDebugRuntimeClasspath --stacktrace --no-daemon
+    - GRADLE_OPTS="-Xmx4096M" ./gradlew :tools:detekt:jar --stacktrace --no-daemon
+    - curl -sSLO https://github.com/detekt/detekt/releases/download/v1.23.4/detekt-cli-1.23.4-all.jar
+    - java -jar detekt-cli-1.23.4-all.jar --config detekt_test_pyramid.yml --plugins tools/detekt/build/libs/detekt.jar -ex "**/*.kts" --jvm-target 11 -cp $(cat sdk_classpath)
+    # For now we just print the uncovered apis, eventually we will fail if it's not empty
+    - grep -v -f apiUsage.log apiSurface.log
+
 test-pyramid:publish-e2e-synthetics:
   tags: [ "arch:amd64" ]
   image: $CI_IMAGE_DOCKER
@@ -424,7 +440,6 @@ test-pyramid:publish-webview-synthetics:
     paths:
       - sample/kotlin/build/outputs/apk/us1/release/kotlin-us1-release.apk
 
-
 test-pyramid:publish-staging-synthetics:
   tags: [ "arch:amd64" ]
   image: $CI_IMAGE_DOCKER

diff --git a/buildSrc/src/main/kotlin/com/datadog/gradle/config/TestPyramidConfig.kt b/buildSrc/src/main/kotlin/com/datadog/gradle/config/TestPyramidConfig.kt
@@ -19,7 +19,6 @@ fun Project.registerSubModuleAggregationTask(
 ) {
     tasks.register(taskName) {
         project.subprojects.forEach { subProject ->
-            println("SubProject ${subProject.name} / ${subProject.path}")
             if (!exceptions.contains(subProject.name) &&
                 subProject.name.startsWith(subModuleNamePrefix) &&
                 subProject.path.startsWith(subModulePathPrefix)

diff --git a/detekt_test_pyramid.yml b/detekt_test_pyramid.yml
@@ -82,9 +82,27 @@ datadog-test-pyramid:
   active: true
   ApiUsage:
     active: true
-    includes: ['**/reliability/**']
+    includes: [ '**/reliability/**' ]
+    internalPackagePrefix: 'com.datadog'
   ApiSurface:
     active: true
-    includes: ['**/dd-sdk-android-*/**']
-    excludes: ['**/build/**', '**/test/**', '**/testDebug/**','**/testRelease/**', '**/androidTest/**', '**/testFixtures/**', '**/buildSrc/**', '**/*.kts', '**/instrumented/**', '**/sample/**', '**/tools/**']
-
+    includes: [ '**/dd-sdk-android-*/**' ]
+    excludes:
+      - '**/build/**'
+      - '**/test/**'
+      - '**/testDebug/**'
+      - '**/testRelease/**'
+      - '**/androidTest/**'
+      - '**/testFixtures/**'
+      - '**/buildSrc/**'
+      - '**/*.kts'
+      - '**/instrumented/**'
+      - '**/sample/**'
+      - '**/tools/**'
+      - '**/dd-sdk-android-internal/**'
+    internalPackagePrefix: 'com.datadog'
+    ignoredAnnotations:
+      - "com.datadog.android.lint.InternalApi"
+    ignoredClasses:
+      - "com.datadog.android._InternalProxy"
+      - "com.datadog.android.rum._RumInternalProxy"
diff --git a/tools/detekt/src/main/kotlin/com/datadog/tools/detekt/rules/pyramid/ApiSurface.kt b/tools/detekt/src/main/kotlin/com/datadog/tools/detekt/rules/pyramid/ApiSurface.kt
@@ -13,20 +13,20 @@ import io.gitlab.arturbosch.detekt.api.Issue
 import io.gitlab.arturbosch.detekt.api.Severity
 import io.gitlab.arturbosch.detekt.api.config
 import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
+import io.gitlab.arturbosch.detekt.rules.isOverride
 import org.jetbrains.kotlin.lexer.KtTokens
 import org.jetbrains.kotlin.psi.KtClass
 import org.jetbrains.kotlin.psi.KtFunctionType
 import org.jetbrains.kotlin.psi.KtNamedFunction
 import org.jetbrains.kotlin.psi.KtNullableType
-import org.jetbrains.kotlin.psi.KtObjectLiteralExpression
+import org.jetbrains.kotlin.psi.KtObjectDeclaration
 import org.jetbrains.kotlin.psi.KtParameter
 import org.jetbrains.kotlin.psi.KtParameterList
 import org.jetbrains.kotlin.psi.KtPrimaryConstructor
 import org.jetbrains.kotlin.psi.KtTypeElement
 import org.jetbrains.kotlin.psi.KtUserType
 import org.jetbrains.kotlin.psi.psiUtil.containingClassOrObject
 import org.jetbrains.kotlin.psi.psiUtil.getChildrenOfType
-import org.jetbrains.kotlin.psi.psiUtil.getStrictParentOfType
 import org.jetbrains.kotlin.psi.psiUtil.isExtensionDeclaration
 import java.io.File
 
@@ -40,6 +40,9 @@ class ApiSurface(
 
     private val outputFileName: String by config(defaultValue = "apiSurface.log")
     private val outputFile: File by lazy { File(outputFileName) }
+    private val internalPackagePrefix: String by config(defaultValue = "")
+    private val ignoredAnnotations: List<String> by config(defaultValue = emptyList())
+    private val ignoredClasses: List<String> by config(defaultValue = emptyList())
 
     // region Rule
 
@@ -50,20 +53,45 @@ class ApiSurface(
         Debt.FIVE_MINS
     )
 
-    override fun visitClass(klass: KtClass) {
-        if (klass.hasModifier(KtTokens.PRIVATE_KEYWORD) || klass.hasModifier(KtTokens.INTERNAL_KEYWORD)) {
+    override fun visitObjectDeclaration(declaration: KtObjectDeclaration) {
+        val hasIgnoredKeyword = ignoredKeywords.any { declaration.hasModifier(it) }
+        val isIgnoredClass = declaration.fqName?.asString() in ignoredClasses
+        val hasIgnoredAnnotation = declaration.annotationEntries.any {
+            it.shortName?.asString()?.resolveFullType() in ignoredAnnotations
+        }
+
+        @Suppress("ComplexCondition")
+        if (isIgnoredClass || hasIgnoredKeyword || hasIgnoredAnnotation) {
             return
         }
-        if (klass.isInterface()) {
+
+        super.visitObjectDeclaration(declaration)
+    }
+
+    override fun visitClass(klass: KtClass) {
+        val hasIgnoredKeyword = ignoredKeywords.any { klass.hasModifier(it) }
+        val isInterface = klass.isInterface()
+        val isEnum = klass.isEnum()
+        val isIgnoredClass = klass.fqName?.asString() in ignoredClasses
+        val hasIgnoredAnnotation = klass.annotationEntries.any {
+            it.shortName?.asString()?.resolveFullType() in ignoredAnnotations
+        }
+
+        @Suppress("ComplexCondition")
+        if (isIgnoredClass || hasIgnoredKeyword || isInterface || isEnum || hasIgnoredAnnotation) {
             return
         }
+
         super.visitClass(klass)
     }
 
     override fun visitPrimaryConstructor(constructor: KtPrimaryConstructor) {
-        if (constructor.hasModifier(KtTokens.PRIVATE_KEYWORD) || constructor.hasModifier(KtTokens.INTERNAL_KEYWORD)) {
+        val hasIgnoredKeyword = ignoredKeywords.any { constructor.hasModifier(it) }
+
+        if (hasIgnoredKeyword) {
             return
         }
+
         val parentName = constructor.containingClassOrObject?.fqName
             ?: constructor.containingKtFile.packageFqName
         outputFile.appendText("$parentName.constructor(")
@@ -78,37 +106,47 @@ class ApiSurface(
         outputFile.appendText(")\n")
     }
 
-    @Suppress("ReturnCount")
     override fun visitNamedFunction(function: KtNamedFunction) {
-        if (function.hasModifier(KtTokens.PRIVATE_KEYWORD) || function.hasModifier(KtTokens.INTERNAL_KEYWORD)) {
-            return
-        }
-        val parameterList = function.getChildrenOfType<KtParameterList>().firstOrNull()
-        if (function.name == "toString" && parameterList?.children.isNullOrEmpty()) {
-            return
+        val hasIgnoredKeyword = ignoredKeywords.any { function.hasModifier(it) }
+        val hasIgnoredName = function.name in ignoredFunctionNames
+        val hasIgnoredAnnotation = function.annotationEntries.any {
+            it.shortName?.asString()?.resolveFullType() in ignoredAnnotations
         }
-        if (function.getStrictParentOfType<KtObjectLiteralExpression>() != null) {
-            // Function is overriding something in an anonymous object
-            // e.g.: val x = object : Interface { override fun foo() {} }
+        val isOverride = function.isOverride()
+
+        @Suppress("ComplexCondition")
+        if (hasIgnoredKeyword || hasIgnoredName || hasIgnoredAnnotation || isOverride) {
             return
         }
-        if (function.isExtensionDeclaration()) {
-            val target = function.receiverTypeReference?.nameForReceiverLabel()
-            val fqName = target?.resolveFullType()
-            outputFile.appendText("$fqName.${function.nameAsSafeName}(")
+
+        val containerFqName = if (function.isExtensionDeclaration()) {
+            val receiverType = function.receiverTypeReference?.typeElement
+            val target = if (receiverType is KtNullableType) {
+                receiverType.innerType?.fullType()
+            } else {
+                receiverType?.fullType()
+            }
+            val parentType = function.typeParameters.filter { it.name == target }
+                .map { it.extendsBound?.typeElement?.fullType() }
+                .firstOrNull()
+            parentType ?: target ?: "null"
         } else {
-            val parentName = function.containingClassOrObject?.fqName
-                ?: function.containingKtFile.packageFqName
-            outputFile.appendText("$parentName.${function.nameAsSafeName}(")
+            function.containingClassOrObject?.fqName?.asString()
+                ?: function.containingKtFile.packageFqName.asString()
         }
 
-        parameterList?.children?.filterIsInstance<KtParameter>()?.forEachIndexed { idx, p ->
-            val typeElement = p.typeReference?.typeElement
-            if (idx > 0) outputFile.appendText(", ")
-            outputFile.appendText(typeElement?.fullType() ?: "???")
-        }
+        if (internalPackagePrefix.isBlank() || containerFqName.startsWith(internalPackagePrefix)) {
+            outputFile.appendText("$containerFqName.${function.nameAsSafeName}(")
 
-        outputFile.appendText(")\n")
+            val parameterList = function.getChildrenOfType<KtParameterList>().firstOrNull()
+            parameterList?.children?.filterIsInstance<KtParameter>()?.forEachIndexed { idx, p ->
+                val typeElement = p.typeReference?.typeElement
+                if (idx > 0) outputFile.appendText(", ")
+                outputFile.appendText(typeElement?.fullType() ?: "???")
+            }
+
+            outputFile.appendText(")\n")
+        }
     }
 
     // endregion
@@ -137,4 +175,14 @@ class ApiSurface(
     }
 
     // endregion
+
+    companion object {
+        private val ignoredFunctionNames = setOf("toString", "equals", "hashCode")
+
+        private val ignoredKeywords = setOf(
+            KtTokens.PRIVATE_KEYWORD,
+            KtTokens.PROTECTED_KEYWORD,
+            KtTokens.INTERNAL_KEYWORD
+        )
+    }
 }
diff --git a/tools/detekt/src/main/kotlin/com/datadog/tools/detekt/rules/pyramid/ApiUsage.kt b/tools/detekt/src/main/kotlin/com/datadog/tools/detekt/rules/pyramid/ApiUsage.kt
@@ -28,6 +28,8 @@ class ApiUsage(
     private val outputFileName: String by config(defaultValue = "apiUsage.log")
     private val outputFile: File by lazy { File(outputFileName) }
 
+    private val internalPackagePrefix: String by config(defaultValue = "")
+
     private var visitingTestFunction = false
 
     // region Rule
@@ -63,8 +65,10 @@ class ApiUsage(
         expression: KtCallExpression,
         resolvedCall: ResolvedFunCall
     ) {
-        outputFile.appendText(resolvedCall.call)
-        outputFile.appendText("\n")
+        if (internalPackagePrefix.isBlank() || resolvedCall.containerFqName.startsWith(internalPackagePrefix)) {
+            outputFile.appendText(resolvedCall.call)
+            outputFile.appendText("\n")
+        }
     }
 
     // endregion