From c9f361fa093a3ce89a690625167f06cccb3ada96 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Mon, 27 Jan 2025 07:49:52 -0500 Subject: [PATCH] Don't generate a broken test when a constructor has a value class parameter (#94) An implementation detail of the Kotlin compiler gets in our way. Closes: https://github.com/cashapp/burst/issues/93 Co-authored-by: Jesse Wilson --- CHANGELOG.md | 3 ++ .../burst/gradle/BurstGradlePluginTest.kt | 25 +++++++++++++ .../valueClassConstructor/build.gradle.kts | 37 +++++++++++++++++++ .../lib/build.gradle.kts | 8 ++++ .../lib/src/test/kotlin/CoffeeTest.kt | 27 ++++++++++++++ .../valueClassConstructor/settings.gradle.kts | 9 +++++ .../kotlin/app/cash/burst/kotlin/BurstApis.kt | 6 +++ .../app/cash/burst/kotlin/ClassSpecializer.kt | 20 +++++++++- 8 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 burst-gradle-plugin/src/test/projects/valueClassConstructor/build.gradle.kts create mode 100644 burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/build.gradle.kts create mode 100644 burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/src/test/kotlin/CoffeeTest.kt create mode 100644 burst-gradle-plugin/src/test/projects/valueClassConstructor/settings.gradle.kts diff --git a/CHANGELOG.md b/CHANGELOG.md index ba2e3a1..57f5b55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,9 @@ ## [Unreleased] [Unreleased]: https://github.com/cashapp/burst/compare/2.3.0...HEAD +**Fixed** + + * Don't fail for constructors that have value class parameters. Unfortunately we can't run such tests from the IDE. ## [2.4.0] *(2025-01-24)* diff --git a/burst-gradle-plugin/src/test/kotlin/app/cash/burst/gradle/BurstGradlePluginTest.kt b/burst-gradle-plugin/src/test/kotlin/app/cash/burst/gradle/BurstGradlePluginTest.kt index a14c92d..e4113a9 100644 --- a/burst-gradle-plugin/src/test/kotlin/app/cash/burst/gradle/BurstGradlePluginTest.kt +++ b/burst-gradle-plugin/src/test/kotlin/app/cash/burst/gradle/BurstGradlePluginTest.kt @@ -329,6 +329,31 @@ class BurstGradlePluginTest { } } + @Test + fun valueClassConstructor() { + val projectDir = File("src/test/projects/valueClassConstructor") + + val taskName = ":lib:test" + val result = createRunner(projectDir, "clean", taskName).build() + assertThat(result.task(taskName)!!.outcome).isIn(*SUCCESS_OUTCOMES) + + val testResults = projectDir.resolve("lib/build/test-results") + + // There's no default specialization. + assertThat(testResults.resolve("test/TEST-CoffeeTest.xml").exists()).isFalse() + + val sampleTest = readTestSuite(testResults.resolve("test/TEST-CoffeeTest_Decaf.xml")) + val sampleTestTest = sampleTest.testCases.single() + assertThat(sampleTestTest.name).isEqualTo("test") + assertThat(sampleTestTest.skipped).isFalse() + assertThat(sampleTest.systemOut).isEqualTo( + """ + |running Decaf + | + """.trimMargin(), + ) + } + private fun createRunner( projectDir: File, vararg taskNames: String, diff --git a/burst-gradle-plugin/src/test/projects/valueClassConstructor/build.gradle.kts b/burst-gradle-plugin/src/test/projects/valueClassConstructor/build.gradle.kts new file mode 100644 index 0000000..66fc2bb --- /dev/null +++ b/burst-gradle-plugin/src/test/projects/valueClassConstructor/build.gradle.kts @@ -0,0 +1,37 @@ +import org.jetbrains.kotlin.gradle.dsl.JvmTarget +import org.jetbrains.kotlin.gradle.tasks.KotlinJvmCompile + +buildscript { + repositories { + maven { + url = file("$rootDir/../../../../../build/testMaven").toURI() + } + mavenCentral() + google() + } + dependencies { + classpath("app.cash.burst:burst-gradle-plugin:${project.property("burstVersion")}") + classpath(libs.kotlin.gradlePlugin) + } +} + +allprojects { + repositories { + maven { + url = file("$rootDir/../../../../../build/testMaven").toURI() + } + mavenCentral() + google() + } + + tasks.withType(JavaCompile::class.java).configureEach { + sourceCompatibility = "1.8" + targetCompatibility = "1.8" + } + + tasks.withType(KotlinJvmCompile::class.java).configureEach { + compilerOptions { + jvmTarget.set(JvmTarget.JVM_1_8) + } + } +} diff --git a/burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/build.gradle.kts b/burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/build.gradle.kts new file mode 100644 index 0000000..36cb092 --- /dev/null +++ b/burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/build.gradle.kts @@ -0,0 +1,8 @@ +plugins { + kotlin("jvm") + id("app.cash.burst") +} + +dependencies { + testImplementation(kotlin("test")) +} diff --git a/burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/src/test/kotlin/CoffeeTest.kt b/burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/src/test/kotlin/CoffeeTest.kt new file mode 100644 index 0000000..c1a2eca --- /dev/null +++ b/burst-gradle-plugin/src/test/projects/valueClassConstructor/lib/src/test/kotlin/CoffeeTest.kt @@ -0,0 +1,27 @@ +import app.cash.burst.Burst +import app.cash.burst.burstValues +import kotlin.test.Test + +@Burst +class CoffeeTest( + private val espresso: Espresso = burstValues(Decaf, Regular, Double), +) { + @Test + fun test() { + println("running $espresso") + } +} + +@JvmInline +value class Espresso(val ordinal: Int) { + override fun toString() = when (this) { + Decaf -> "Decaf" + Regular -> "Regular" + Double -> "Double" + else -> "$ordinal" + } +} + +val Decaf = Espresso(0) +val Regular = Espresso(1) +val Double = Espresso(2) diff --git a/burst-gradle-plugin/src/test/projects/valueClassConstructor/settings.gradle.kts b/burst-gradle-plugin/src/test/projects/valueClassConstructor/settings.gradle.kts new file mode 100644 index 0000000..78eeed4 --- /dev/null +++ b/burst-gradle-plugin/src/test/projects/valueClassConstructor/settings.gradle.kts @@ -0,0 +1,9 @@ +dependencyResolutionManagement { + versionCatalogs { + create("libs") { + from(files("../../../../../gradle/libs.versions.toml")) + } + } +} + +include(":lib") diff --git a/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/BurstApis.kt b/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/BurstApis.kt index 212feb1..d382682 100644 --- a/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/BurstApis.kt +++ b/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/BurstApis.kt @@ -70,5 +70,11 @@ private val junit5TestClassId = junit5Package.classId("Test") private val kotlinTestPackage = FqPackageName("kotlin.test") private val kotlinTestClassId = kotlinTestPackage.classId("Test") +private val kotlinJvmFqPackage = FqPackageName("kotlin.jvm") +private val jvmInlineAnnotationId = kotlinJvmFqPackage.classId("JvmInline") + internal val IrAnnotationContainer.hasAtBurst: Boolean get() = hasAnnotation(burstAnnotationId) + +internal val IrAnnotationContainer.hasAtJvmInline: Boolean + get() = hasAnnotation(jvmInlineAnnotationId) diff --git a/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/ClassSpecializer.kt b/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/ClassSpecializer.kt index bb0672c..df88d9a 100644 --- a/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/ClassSpecializer.kt +++ b/burst-kotlin-plugin/src/main/kotlin/app/cash/burst/kotlin/ClassSpecializer.kt @@ -26,6 +26,7 @@ import org.jetbrains.kotlin.ir.declarations.IrConstructor import org.jetbrains.kotlin.ir.declarations.IrFile import org.jetbrains.kotlin.ir.symbols.UnsafeDuringIrConstructionAPI import org.jetbrains.kotlin.ir.types.IrTypeSystemContextImpl +import org.jetbrains.kotlin.ir.types.getClass import org.jetbrains.kotlin.ir.util.addFakeOverrides import org.jetbrains.kotlin.ir.util.constructors import org.jetbrains.kotlin.ir.util.createImplicitParameterDeclarationWithWrappedDescriptor @@ -89,7 +90,10 @@ internal class ClassSpecializer( if (valueParameters.isEmpty()) return // Nothing to do. val specializations = specializations(pluginContext, burstApis, valueParameters) - val indexOfDefaultSpecialization = specializations.indexOfFirst { it.isDefault } + val indexOfDefaultSpecialization = when { + onlyConstructor.defaultSpecializationIsBroken() -> -1 + else -> specializations.indexOfFirst { it.isDefault } + } // Make sure the constructor we're using is accessible. Drop the default arguments to prevent // JUnit from using it. @@ -180,4 +184,18 @@ internal class ClassSpecializer( } pluginContext.metadataDeclarationRegistrar.registerConstructorAsMetadataVisible(constructor) } + + /** + * Returns true if the default specialization would be broken for this constructor. + * + * We don't support the default specialization if any constructor parameter is a JVM value class. + * For each constructor that declares a value class parameter, the Kotlin compiler generates an + * extra (not-user-visible) constructor. But JUnit requires each test class to have exactly one + * public constructor. + * + * https://github.com/cashapp/burst/issues/93 + */ + private fun IrConstructor.defaultSpecializationIsBroken(): Boolean { + return valueParameters.any { it.type.getClass()?.hasAtJvmInline == true } + } }