From 6d6ea181443c0c00cecb3b5434331c590912dde5 Mon Sep 17 00:00:00 2001 From: "Xavier F. Gouchet" Date: Wed, 2 Oct 2024 09:30:46 +0200 Subject: [PATCH] Detekt the api coverage in Integration tests --- .gitlab-ci.yml | 17 ++- .../gradle/config/TestPyramidConfig.kt | 1 - detekt_test_pyramid.yml | 26 ++++- .../tools/detekt/rules/pyramid/ApiSurface.kt | 106 +++++++++++++----- .../tools/detekt/rules/pyramid/ApiUsage.kt | 8 +- 5 files changed, 121 insertions(+), 37 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 63c141144a..b722fee63c 100644 --- 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 index 062149f7f6..247c537863 100644 --- 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 index 3ea20126ef..8241211efd 100644 --- 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 index c11afc3683..07aee8f182 100644 --- 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,12 +13,13 @@ 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 @@ -26,7 +27,6 @@ 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 by config(defaultValue = emptyList()) + private val ignoredClasses: List 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().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() != 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()?.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().firstOrNull() + parameterList?.children?.filterIsInstance()?.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 index 98899a5426..f212efdef1 100644 --- 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