From a4b20de8dfd72714c74f032bf4c87d109bfba0b9 Mon Sep 17 00:00:00 2001 From: takahirom Date: Sat, 18 May 2024 12:34:25 +0900 Subject: [PATCH 1/5] Check integrationt tests when removing isskiptest in Plugin --- .../takahirom/roborazzi/RoborazziPlugin.kt | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt index c7b175cc..e2e39d7c 100644 --- a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt +++ b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt @@ -261,15 +261,16 @@ abstract class RoborazziPlugin : Plugin { currentIsTestSkipped } finalizeTestTask.infoln("Roborazzi: roborazziTestFinalizer.doLast isTestSkipped:$isTestSkipped") - if (isTestSkipped) { - // If the test is skipped, we need to use cached files - finalizeTestTask.infoln("Roborazzi: finalizeTestRoborazziTask isTestSkipped:$isTestSkipped Copy files from ${intermediateDir.get()} to ${outputDir.get()}") - intermediateDir.get().asFile.mkdirs() - intermediateDir.get().asFile.copyRecursively( - target = outputDir.get().asFile, - overwrite = true - ) - } + // if (isTestSkipped) { + // If the test is skipped, we need to use cached files + val startCopy = System.currentTimeMillis() + intermediateDir.get().asFile.mkdirs() + intermediateDir.get().asFile.copyRecursively( + target = outputDir.get().asFile, + overwrite = true + ) + finalizeTestTask.infoln("Roborazzi: finalizeTestRoborazziTask Copy files from ${intermediateDir.get()} to ${outputDir.get()} end ${System.currentTimeMillis() - startCopy}ms") + // } val results: List = resultDirFileTree.get().mapNotNull { if (it.name.endsWith(".json")) { From 75a1380fec6eaf2e55af842da980a6757184744d Mon Sep 17 00:00:00 2001 From: takahirom Date: Sat, 18 May 2024 13:08:13 +0900 Subject: [PATCH 2/5] Try to use TestListener instead of doLast of task --- .../takahirom/roborazzi/RoborazziPlugin.kt | 42 ++++++++++++------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt index e2e39d7c..12bf3f39 100644 --- a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt +++ b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt @@ -26,6 +26,9 @@ import org.gradle.api.tasks.TaskAction import org.gradle.api.tasks.options.Option import org.gradle.api.tasks.testing.AbstractTestTask import org.gradle.api.tasks.testing.Test +import org.gradle.api.tasks.testing.TestDescriptor +import org.gradle.api.tasks.testing.TestListener +import org.gradle.api.tasks.testing.TestResult import org.gradle.build.event.BuildEventsListenerRegistry import org.gradle.language.base.plugins.LifecycleBasePlugin.VERIFICATION_GROUP import org.gradle.tooling.events.FinishEvent @@ -299,7 +302,7 @@ abstract class RoborazziPlugin : Plugin { } }) testTaskProvider - .configureEach { test -> + .configureEach { test: AbstractTestTask -> val resultsDir = resultDirFileProperty.get().asFile if (restoreOutputDirRoborazziTaskProvider.isPresent) { test.inputs.dir(restoreOutputDirRoborazziTaskProvider.map { @@ -382,19 +385,30 @@ abstract class RoborazziPlugin : Plugin { resultsDir.deleteRecursively() resultsDir.mkdirs() } - test.doLast { - // Copy all files from outputDir to intermediateDir - // so that we can use Gradle's output caching - it.infoln("Roborazzi: test.doLast Copy files from ${outputDir.get()} to ${intermediateDir.get()}") - // outputDir.get().asFileTree.forEach { - // println("Copy file ${finalizeTask.absolutePath} to ${intermediateDir.get()}") - // } - outputDir.get().asFile.mkdirs() - outputDir.get().asFile.copyRecursively( - target = intermediateDir.get().asFile, - overwrite = true - ) - } + test.addTestListener(object : TestListener { + override fun beforeSuite(suite: TestDescriptor?) { + } + + override fun afterSuite(suite: TestDescriptor?, result: TestResult?) { + // Copy all files from outputDir to intermediateDir + // so that we can use Gradle's output caching + test.infoln("Roborazzi: test.doLast Copy files from ${outputDir.get()} to ${intermediateDir.get()}") + // outputDir.get().asFileTree.forEach { + // println("Copy file ${finalizeTask.absolutePath} to ${intermediateDir.get()}") + // } + outputDir.get().asFile.mkdirs() + outputDir.get().asFile.copyRecursively( + target = intermediateDir.get().asFile, + overwrite = true + ) + } + + override fun beforeTest(testDescriptor: TestDescriptor?) { + } + + override fun afterTest(testDescriptor: TestDescriptor?, result: TestResult?) { + } + }) test.finalizedBy(finalizeTestRoborazziTask) } From d984b7fab8f58cac01278810e87ea4abed8323f3 Mon Sep 17 00:00:00 2001 From: takahirom Date: Sat, 18 May 2024 13:25:24 +0900 Subject: [PATCH 3/5] Remove isTestSkipped completely --- .../takahirom/roborazzi/RoborazziPlugin.kt | 118 +----------------- 1 file changed, 3 insertions(+), 115 deletions(-) diff --git a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt index 12bf3f39..b47a9843 100644 --- a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt +++ b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt @@ -13,12 +13,8 @@ import org.gradle.api.Plugin import org.gradle.api.Project import org.gradle.api.Task import org.gradle.api.file.DirectoryProperty -import org.gradle.api.logging.Logging import org.gradle.api.model.ObjectFactory import org.gradle.api.provider.Property -import org.gradle.api.provider.Provider -import org.gradle.api.services.BuildService -import org.gradle.api.services.BuildServiceParameters import org.gradle.api.tasks.InputDirectory import org.gradle.api.tasks.Optional import org.gradle.api.tasks.OutputDirectory @@ -31,12 +27,6 @@ import org.gradle.api.tasks.testing.TestListener import org.gradle.api.tasks.testing.TestResult import org.gradle.build.event.BuildEventsListenerRegistry import org.gradle.language.base.plugins.LifecycleBasePlugin.VERIFICATION_GROUP -import org.gradle.tooling.events.FinishEvent -import org.gradle.tooling.events.OperationCompletionListener -import org.gradle.tooling.events.task.TaskFailureResult -import org.gradle.tooling.events.task.TaskFinishEvent -import org.gradle.tooling.events.task.TaskSkippedResult -import org.gradle.tooling.events.task.TaskSuccessResult import org.jetbrains.kotlin.gradle.dsl.KotlinMultiplatformExtension import org.jetbrains.kotlin.gradle.plugin.ExecutionTaskHolder import org.jetbrains.kotlin.gradle.plugin.mpp.KotlinNativeTargetWithTests @@ -44,8 +34,6 @@ import org.jetbrains.kotlin.gradle.targets.jvm.KotlinJvmTarget import org.jetbrains.kotlin.gradle.targets.native.KotlinNativeBinaryTestRun import org.jetbrains.kotlin.gradle.targets.native.tasks.KotlinNativeTest import java.util.Locale -import java.util.concurrent.CountDownLatch -import java.util.concurrent.TimeUnit import javax.inject.Inject import kotlin.reflect.KClass @@ -132,21 +120,8 @@ abstract class RoborazziPlugin : Plugin { fun configureRoborazziTasks( variantSlug: String, testTaskName: String, - testTaskSkipEventsServiceProvider: Provider, testTaskClass: KClass = Test::class ) { - try { - testTaskSkipEventsServiceProvider.get().addExpectingTestTaskName(testTaskName) - } catch (e: ClassCastException) { - throw IllegalStateException( - """You should use `id("io.github.takahirom.roborazzi") version "[version]" apply false` in the root project - |to ensure the build cache property functions correctly. - |This is a temporary workaround, - |and we are awaiting a permanent fix from the Gradle core. - |https://github.com/takahirom/roborazzi/issues/266""".trimMargin(), - e - ) - } val testTaskOutputDirForEachVariant: DirectoryProperty = project.objects.directoryProperty() val intermediateDirForEachVariant = testTaskOutputDirForEachVariant.convention( @@ -247,23 +222,6 @@ abstract class RoborazziPlugin : Plugin { project.path + ":" } finalizeTestTask.doLast { - val taskSkipEventsService = testTaskSkipEventsServiceProvider.get() - val buildFinishCountDownLatch = taskSkipEventsService.countDownLatch - val currentIsTestSkipped = - taskSkipEventsService.skippedTestTaskMap[taskPath + testTaskName] - val isTestSkipped = if (currentIsTestSkipped == null) { - // BuildService could cause race condition - // https://github.com/gradle/gradle/issues/24887 - finalizeTestTask.infoln("Roborazzi: roborazziTestFinalizer.doLast test task result doesn't exits. currentIsTestSkipped:$currentIsTestSkipped currentTimeMillis:${System.currentTimeMillis()}") - val waitSuccess = buildFinishCountDownLatch.await(200, TimeUnit.MILLISECONDS) - val new = - taskSkipEventsService.skippedTestTaskMap[taskPath + testTaskName] - finalizeTestTask.infoln("Roborazzi: roborazziTestFinalizer.doLast wait end currentIsTestSkipped:$currentIsTestSkipped new:$new waitSuccess:$waitSuccess currentTimeMillis:${System.currentTimeMillis()}") - new ?: false - } else { - currentIsTestSkipped - } - finalizeTestTask.infoln("Roborazzi: roborazziTestFinalizer.doLast isTestSkipped:$isTestSkipped") // if (isTestSkipped) { // If the test is skipped, we need to use cached files val startCopy = System.currentTimeMillis() @@ -387,6 +345,7 @@ abstract class RoborazziPlugin : Plugin { } test.addTestListener(object : TestListener { override fun beforeSuite(suite: TestDescriptor?) { + // Do nothing } override fun afterSuite(suite: TestDescriptor?, result: TestResult?) { @@ -404,9 +363,11 @@ abstract class RoborazziPlugin : Plugin { } override fun beforeTest(testDescriptor: TestDescriptor?) { + // Do nothing } override fun afterTest(testDescriptor: TestDescriptor?, result: TestResult?) { + // Do nothing } }) test.finalizedBy(finalizeTestRoborazziTask) @@ -418,14 +379,6 @@ abstract class RoborazziPlugin : Plugin { verifyAndRecordTaskProvider.configure { it.dependsOn(testTaskProvider) } } - val testTaskSkipEventsServiceProvider: Provider = - project.gradle.sharedServices.registerIfAbsent( - "roborazziTestTaskEvents", TestTaskSkipEventsServiceProvider::class.java - ) { spec -> - // do nothing - } - getEventsListenerRegistry().onTaskCompletion(testTaskSkipEventsServiceProvider) - fun AndroidComponentsExtension<*, *, *>.configureComponents() { onVariants { variant -> val unitTest = variant.unitTest ?: return@onVariants @@ -436,7 +389,6 @@ abstract class RoborazziPlugin : Plugin { configureRoborazziTasks( variantSlug = variantSlug, testTaskName = "test$testVariantSlug", - testTaskSkipEventsServiceProvider = testTaskSkipEventsServiceProvider ) } } @@ -454,7 +406,6 @@ abstract class RoborazziPlugin : Plugin { configureRoborazziTasks( variantSlug = "Jvm", testTaskName = "test", - testTaskSkipEventsServiceProvider = testTaskSkipEventsServiceProvider ) } project.pluginManager.withPlugin("org.jetbrains.kotlin.multiplatform") { @@ -470,7 +421,6 @@ abstract class RoborazziPlugin : Plugin { configureRoborazziTasks( variantSlug = target.name.capitalizeUS(), testTaskName = testRun.executionTask.name, - testTaskSkipEventsServiceProvider = testTaskSkipEventsServiceProvider ) } } @@ -480,7 +430,6 @@ abstract class RoborazziPlugin : Plugin { configureRoborazziTasks( variantSlug = target.name.capitalizeUS(), testTaskName = (testRun as ExecutionTaskHolder<*>).executionTask.name, - testTaskSkipEventsServiceProvider = testTaskSkipEventsServiceProvider, testTaskClass = KotlinNativeTest::class ) } @@ -524,66 +473,5 @@ abstract class RoborazziPlugin : Plugin { } } -/** - * We can't get whether the test is skipped or not from the test task itself - * because of the configuration cache - */ -abstract class TestTaskSkipEventsServiceProvider : BuildService, - OperationCompletionListener { - val skippedTestTaskMap = mutableMapOf() - var countDownLatch = CountDownLatch(1) - private set - private val logger = Logging.getLogger(this::class.java) - private val expectingTestNames = mutableListOf() - fun addExpectingTestTaskName(testName: String) { - expectingTestNames.add(testName) - } - - override fun onFinish(finishEvent: FinishEvent) { - val displayName = finishEvent.displayName -// println( -// "Roborazzi: onFinish " + -// "expectingTestNames:$expectingTestNames" + -// "displayName:$displayName " + -// "finishEvent:$finishEvent " + -// "finishEvent.descriptor:${finishEvent.descriptor}" + -// "finishEvent.descriptor.name:${finishEvent.descriptor.name}" -// ) - if (finishEvent !is TaskFinishEvent) { - return - } - if (!expectingTestNames.any { - displayName.contains(it, ignoreCase = true) - }) { - return - } - val result = finishEvent.result - if (when (result) { - is TaskSuccessResult -> { - if (result.isFromCache) { - true - } else if (result.isUpToDate) { - true - } else { - false - } - } - - is TaskFailureResult -> false - is TaskSkippedResult -> true - else -> false - } - ) { - logger.info("Roborazzi: Skip test ${finishEvent.descriptor.name} ${System.currentTimeMillis()}") - skippedTestTaskMap[finishEvent.descriptor.name] = true - } else { - logger.info("Roborazzi: Don't skip test ${finishEvent.descriptor.name} ${System.currentTimeMillis()}") - skippedTestTaskMap[finishEvent.descriptor.name] = false - } - countDownLatch.countDown() - countDownLatch = CountDownLatch(1) - } -} - fun Task.infoln(format: String) = logger.info(format) From d84be7111a600cc8c7a2bd308aaeb5e812bc9ae3 Mon Sep 17 00:00:00 2001 From: takahirom Date: Sat, 18 May 2024 13:34:13 +0900 Subject: [PATCH 4/5] Add comments --- .../java/io/github/takahirom/roborazzi/RoborazziPlugin.kt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt index b47a9843..8b4847b7 100644 --- a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt +++ b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt @@ -207,6 +207,8 @@ abstract class RoborazziPlugin : Plugin { val reportFileProperty = project.layout.buildDirectory.file(RoborazziReportConst.reportFilePathFromBuildDir) + // The difference between finalizedTask and afterSuite is that + // finalizedTask is called even if the test is skipped. val finalizeTestRoborazziTask = project.tasks.register( /* name = */ "finalizeTestRoborazzi$variantSlug", /* configurationAction = */ object : Action { @@ -348,6 +350,10 @@ abstract class RoborazziPlugin : Plugin { // Do nothing } + // The reason why we use afterSuite() instead of task.doLast is that + // task.doLast is not called if the test is failed. + // And why we use afterSuite() instead of finalizedBy is that + // we want to change the tasks' output in the task execution phase. override fun afterSuite(suite: TestDescriptor?, result: TestResult?) { // Copy all files from outputDir to intermediateDir // so that we can use Gradle's output caching From 5392f08559f73e1f88a2f237b9980b3115d1c542 Mon Sep 17 00:00:00 2001 From: takahirom Date: Sat, 18 May 2024 13:36:17 +0900 Subject: [PATCH 5/5] Remove some comments --- .../main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt | 3 --- 1 file changed, 3 deletions(-) diff --git a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt index 8b4847b7..73fe9335 100644 --- a/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt +++ b/include-build/roborazzi-gradle-plugin/src/main/java/io/github/takahirom/roborazzi/RoborazziPlugin.kt @@ -224,8 +224,6 @@ abstract class RoborazziPlugin : Plugin { project.path + ":" } finalizeTestTask.doLast { - // if (isTestSkipped) { - // If the test is skipped, we need to use cached files val startCopy = System.currentTimeMillis() intermediateDir.get().asFile.mkdirs() intermediateDir.get().asFile.copyRecursively( @@ -233,7 +231,6 @@ abstract class RoborazziPlugin : Plugin { overwrite = true ) finalizeTestTask.infoln("Roborazzi: finalizeTestRoborazziTask Copy files from ${intermediateDir.get()} to ${outputDir.get()} end ${System.currentTimeMillis() - startCopy}ms") - // } val results: List = resultDirFileTree.get().mapNotNull { if (it.name.endsWith(".json")) {