Merge pull request #37 from Automattic/fix_incorrect_requested_task

Fix incorrect requested task assignment
Automattic · Feb 29, 2024 · bad994f · bad994f
2 parents f84809a + 723fbb3
commit bad994f
Show file tree

Hide file tree

Showing 8 changed files with 47 additions and 28 deletions.
diff --git a/measure-builds/src/main/java/com/automattic/android/measure/BuildTimePlugin.kt b/measure-builds/src/main/java/com/automattic/android/measure/BuildTimePlugin.kt
@@ -8,6 +8,7 @@ import com.automattic.android.measure.providers.BuildDataProvider
 import com.automattic.android.measure.providers.UsernameProvider
 import com.gradle.scan.plugin.BuildScanExtension
 import kotlinx.coroutines.runBlocking
+import org.gradle.StartParameter
 import org.gradle.api.Plugin
 import org.gradle.api.Project
 import org.gradle.api.flow.FlowActionSpec
@@ -45,7 +46,13 @@ class BuildTimePlugin @Inject constructor(
                 ConfigurationPhaseObserver.init()
 
                 prepareBuildData(project, extension, encodedUser)
-                prepareBuildFinishedAction(extension, analyticsReporter, buildInitiatedTime, configurationProvider)
+                prepareBuildFinishedAction(
+                    project.gradle.startParameter,
+                    extension,
+                    analyticsReporter,
+                    buildInitiatedTime,
+                    configurationProvider
+                )
             }
         }
 
@@ -83,6 +90,7 @@ class BuildTimePlugin @Inject constructor(
     }
 
     private fun prepareBuildFinishedAction(
+        startParameter: StartParameter,
         extension: MeasureBuildsExtension,
         analyticsReporter: MetricsReporter,
         buildInitiatedTime: Long,
@@ -97,6 +105,7 @@ class BuildTimePlugin @Inject constructor(
                 this.analyticsReporter.set(analyticsReporter)
                 this.initiationTime.set(buildInitiatedTime)
                 this.configurationPhaseExecuted.set(configurationPhaseObserver)
+                this.startParameter.set(startParameter)
             }
         }
     }

diff --git a/...-builds/src/main/java/com/automattic/android/measure/lifecycle/BuildFinishedFlowAction.kt b/...-builds/src/main/java/com/automattic/android/measure/lifecycle/BuildFinishedFlowAction.kt
@@ -6,6 +6,7 @@ import com.automattic.android.measure.InMemoryReport
 import com.automattic.android.measure.models.ExecutionData
 import com.automattic.android.measure.networking.MetricsReporter
 import kotlinx.coroutines.runBlocking
+import org.gradle.StartParameter
 import org.gradle.api.flow.BuildWorkResult
 import org.gradle.api.flow.FlowAction
 import org.gradle.api.flow.FlowParameters
@@ -37,6 +38,9 @@ class BuildFinishedFlowAction : FlowAction<BuildFinishedFlowAction.Parameters> {
 
         @get:ServiceReference
         val buildTaskService: Property<BuildTaskService>
+
+        @get:Input
+        val startParameter: Property<StartParameter>
     }
 
     override fun execute(parameters: Parameters) {
@@ -57,9 +61,10 @@ class BuildFinishedFlowAction : FlowAction<BuildFinishedFlowAction.Parameters> {
             buildTime = buildPhaseDuration + configurationTime,
             failed = result.failure.isPresent,
             failure = result.failure.getOrNull()?.message,
-            tasks = parameters.buildTaskService.get().tasks,
+            executedTasks = parameters.buildTaskService.get().tasks,
             buildFinishedTimestamp = finish,
-            configurationPhaseDuration = configurationTime
+            configurationPhaseDuration = configurationTime,
+            requestedTasks = parameters.startParameter.get().taskNames.toList()
         )
 
         InMemoryReport.setExecutionData(executionData)

diff --git a/measure-builds/src/main/java/com/automattic/android/measure/models/BuildData.kt b/measure-builds/src/main/java/com/automattic/android/measure/models/BuildData.kt
@@ -14,7 +14,6 @@ import kotlinx.serialization.Serializable
 data class BuildData(
     val forProject: MeasureBuildsExtension.AutomatticProject,
     val user: String,
-    val tasks: List<String>,
     val gradleVersion: String,
     val operatingSystem: String,
     val environment: Environment,
@@ -28,7 +27,8 @@ data class ExecutionData(
     val buildTime: Long,
     val failed: Boolean,
     val failure: String?,
-    val tasks: List<MeasuredTask>,
+    val executedTasks: List<MeasuredTask>,
+    val requestedTasks: List<String>,
     val buildFinishedTimestamp: Long,
     val configurationPhaseDuration: Long,
 )

diff --git a/measure-builds/src/main/java/com/automattic/android/measure/networking/MetricsReporter.kt b/measure-builds/src/main/java/com/automattic/android/measure/networking/MetricsReporter.kt
@@ -152,10 +152,11 @@ class MetricsReporter(
         if (report.executionData.buildTime == 0L) {
             return
         }
-
         val slowTasks =
-            report.executionData.tasks.sortedByDescending { it.duration }.chunked(atMostLoggedTasks)
-                .first()
+            report.executionData.executedTasks.sortedByDescending { it.duration }
+                .chunked(atMostLoggedTasks).firstOrNull()
+                ?: return
+
         logger.lifecycle("\n$TURTLE_ICON ${slowTasks.size} slowest tasks were: ")
 
         logger.lifecycle(

diff --git a/measure-builds/src/main/java/com/automattic/android/measure/networking/ToAppsInfraPayload.kt b/measure-builds/src/main/java/com/automattic/android/measure/networking/ToAppsInfraPayload.kt
@@ -16,10 +16,10 @@ fun InMemoryReport.toAppsMetricsPayload(gradleScanId: String?): GroupedAppsMetri
         "operating-system" to buildData.operatingSystem,
     )
 
-    val taskGroups = executionData.tasks.groupBy { it.state }
+    val taskGroups = executionData.executedTasks.groupBy { it.state }
 
     val metrics = mapOf(
-        "requested-tasks" to buildData.tasks.joinToString(separator = ","),
+        "requested-tasks" to executionData.requestedTasks.joinToString(separator = ","),
         "build-time-ms" to executionData.buildTime.toString(),
         "build-status" to if (executionData.failed) "Failure" else "Success",
         "failure-message" to executionData.failure.toString(),
@@ -35,7 +35,7 @@ fun InMemoryReport.toAppsMetricsPayload(gradleScanId: String?): GroupedAppsMetri
         "configuration-duration" to executionData.configurationPhaseDuration.toString()
     )
 
-    val tasks = executionData.tasks.map {
+    val tasks = executionData.executedTasks.map {
         "task-${it.name}" to it.duration.inWholeMilliseconds.toString()
     }
 

diff --git a/measure-builds/src/main/java/com/automattic/android/measure/providers/BuildDataProvider.kt b/measure-builds/src/main/java/com/automattic/android/measure/providers/BuildDataProvider.kt
@@ -19,7 +19,6 @@ object BuildDataProvider {
         @Suppress("UnstableApiUsage")
         return BuildData(
             forProject = automatticProject,
-            tasks = startParameter.taskNames,
             environment = gradle.environment(),
             gradleVersion = gradle.gradleVersion,
             operatingSystem = System.getProperty("os.name").lowercase(),

diff --git a/...ds/src/test/java/com/automattic/android/measure/BuildTimePluginConfigurationCacheTests.kt b/...ds/src/test/java/com/automattic/android/measure/BuildTimePluginConfigurationCacheTests.kt
@@ -41,23 +41,19 @@ class BuildTimePluginConfigurationCacheTests {
     }
 
     @Test
-    fun `given a project utilizes configuration cache, when build finishes, then assert that build or execution data was not reused`() {
-        fun runTaskAndGetDetails(vararg arguments: String): Pair<List<String>, Long> {
+    fun `given a project utilizes configuration cache, when build finishes, then assert that execution data was not reused`() {
+        fun runTaskAndGetDetails(vararg arguments: String): Long {
             runner(*arguments).build()
-            return buildData.tasks to executionData.buildFinishedTimestamp
+            return executionData.buildFinishedTimestamp
         }
 
-        val (tasksA, timestampA) = runTaskAndGetDetails("help")
-        val (tasksB, timestampB) = runTaskAndGetDetails("outgoingVariants")
-        val (tasksC, timestampC) = runTaskAndGetDetails("tasks")
+        val timestampA = runTaskAndGetDetails("help")
+        val timestampB = runTaskAndGetDetails("outgoingVariants")
+        val timestampC = runTaskAndGetDetails("tasks")
 
         assertThat(timestampA).isNotEqualTo(timestampB)
         assertThat(timestampA).isNotEqualTo(timestampC)
         assertThat(timestampB).isNotEqualTo(timestampC)
-
-        assertThat(tasksA).isNotEqualTo(tasksB)
-        assertThat(tasksA).isNotEqualTo(tasksC)
-        assertThat(tasksB).isNotEqualTo(tasksC)
     }
 
     @Test
@@ -79,6 +75,18 @@ class BuildTimePluginConfigurationCacheTests {
         assertThat(buildData.environment).isEqualTo(Environment.IDE)
     }
 
+    @Test
+    fun `given a task which configuration is cached, when calling it again, then the requested task is correct`() {
+        runner("help").build()
+        assertThat(executionData.requestedTasks).contains("help")
+
+        runner("outgoingVariants").build()
+        assertThat(executionData.requestedTasks).contains("outgoingVariants")
+
+        runner("help").build()
+        assertThat(executionData.requestedTasks).contains("help")
+    }
+
     private fun runner(vararg arguments: String): GradleRunner {
         val projectDir = File("build/functionalTest").apply {
             mkdirs()
@@ -97,11 +105,8 @@ class BuildTimePluginConfigurationCacheTests {
             )
         }
 
-        return GradleRunner.create()
-            .forwardOutput()
-            .withPluginClasspath()
-            .withArguments(*arguments, "--configuration-cache")
-            .withProjectDir(projectDir)
+        return GradleRunner.create().forwardOutput().withPluginClasspath()
+            .withArguments(*arguments, "--configuration-cache").withProjectDir(projectDir)
     }
 
     private val executionData: ExecutionData

diff --git a/measure-builds/src/test/java/com/automattic/android/measure/BuildTimePluginTest.kt b/measure-builds/src/test/java/com/automattic/android/measure/BuildTimePluginTest.kt
@@ -135,7 +135,7 @@ class BuildTimePluginTest {
         File("build/functionalTest/build/reports/measure_builds/execution_data.json").let {
             val executionData = Json.decodeFromString<ExecutionData>(it.readText())
 
-            assertThat(executionData.tasks).hasSize(1).first().satisfies({ task ->
+            assertThat(executionData.executedTasks).hasSize(1).first().satisfies({ task ->
                 assertThat(task.name).isEqualTo(":help")
                 assertThat(task.state).isEqualTo(MeasuredTask.State.EXECUTED)
             })