diff --git a/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt b/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt index aea4c2b0e0..445a19d48f 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/build/AbstractCancellableRunAction.kt @@ -12,6 +12,7 @@ import com.itsaky.androidide.lookup.Lookup import com.itsaky.androidide.projects.builder.BuildService import com.itsaky.androidide.resources.R import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.requestBuildCancellation import org.slf4j.Logger import org.slf4j.LoggerFactory @@ -77,34 +78,13 @@ abstract class AbstractCancellableRunAction( protected abstract fun doExec(data: ActionData): Any protected fun cancelBuild(): Boolean { - log.info("Sending build cancellation request...") val builder = Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE) if (builder?.isToolingServerStarted() != true) { flashError(com.itsaky.androidide.projects.R.string.msg_tooling_server_unavailable) return false } - builder.cancelCurrentBuild().whenComplete { - result, - error, - -> - if (error != null) { - log.error("Failed to send build cancellation request", error) - return@whenComplete - } - - if (!result.wasEnqueued) { - log.warn( - "Unable to enqueue cancellation request reason={} reason.message={}", - result.failureReason, - result.failureReason!!.message, - ) - return@whenComplete - } - - log.info("Build cancellation request was successfully enqueued...") - } - + requestBuildCancellation(builder) return true } diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt index afc6707577..716a225cf6 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/ProjectHandlerActivity.kt @@ -80,6 +80,7 @@ import com.itsaky.androidide.tooling.api.messages.BuildRunType import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams import com.itsaky.androidide.tooling.api.messages.result.InitializeResult import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.BUILD_CANCELLED import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.CACHE_READ_ERROR import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.PROJECT_DIRECTORY_INACCESSIBLE import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.PROJECT_NOT_DIRECTORY @@ -95,6 +96,7 @@ import com.itsaky.androidide.utils.DialogUtils.showRestartPrompt import com.itsaky.androidide.utils.RecursiveFileSearcher import com.itsaky.androidide.utils.dpToPx import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.utils.flashbarBuilder import com.itsaky.androidide.utils.onLongPress @@ -797,6 +799,18 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() { manager.projectDir.name } + // A sync the user stopped is not a failure, and arrives here through the same callback + // as one. ADFA-5542 fixed that for builds and missed this path, which is the one a + // cancelled *sync* takes: the user pressed Stop and got an indefinite red "Project + // initialization failed" for doing so. + if (failure == BUILD_CANCELLED) { + val cancelled = getString(string.info_build_cancelled) + setStatus(cancelled) + flashInfo(cancelled) + editorViewModel.isInitializing = false + return + } + val initFailed = if (projectName.isNotEmpty()) { getString(string.msg_project_initialization_failed_with_name, projectName) diff --git a/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt b/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt index c095b08320..f216a7e0ac 100644 --- a/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt +++ b/app/src/main/java/com/itsaky/androidide/analytics/gradle/BuildCompletedMetric.kt @@ -20,5 +20,11 @@ class BuildCompletedMetric( putString("build_type", buildType) putBoolean("success", isSuccess) putLong("duration_ms", buildResult.durationMs) + // Why it was not successful, which the metric used to drop. A build the user stopped + // and a build that broke both arrive as success=false, so without this the two are + // indistinguishable and every build-success rate counts deliberate cancels as + // failures. isSuccess keeps its meaning -- a cancelled build did not succeed -- and a + // consumer that wants the rate excluding cancels can now compute it. + buildResult.failure?.let { putString("failure_reason", it.name) } } } diff --git a/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt b/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt index cee0b2ad74..918a2ef2f2 100644 --- a/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt +++ b/app/src/main/java/com/itsaky/androidide/editor/floating/EditorPanelDockableContent.kt @@ -19,6 +19,7 @@ import com.itsaky.androidide.models.Position import com.itsaky.androidide.models.Range import com.itsaky.androidide.projects.builder.BuildService import com.itsaky.androidide.ui.CodeEditorView +import com.itsaky.androidide.utils.requestBuildCancellation import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -114,7 +115,9 @@ class EditorPanelDockableContent( private fun cancelBuild() { val builder = Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE) if (builder?.isToolingServerStarted() == true) { - builder.cancelCurrentBuild() + // Through the shared reporter, because this copy threw the result away entirely: a + // Stop the server refused here said nothing at all, not even to the log. + requestBuildCancellation(builder) } } diff --git a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt index c40e68d7b2..685b859113 100644 --- a/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt +++ b/app/src/main/java/com/itsaky/androidide/handlers/EditorBuildEventListener.kt @@ -27,12 +27,14 @@ import com.itsaky.androidide.projects.builder.LaunchResult import com.itsaky.androidide.resources.R.string import com.itsaky.androidide.services.builder.GradleBuildService import com.itsaky.androidide.tooling.api.messages.result.BuildInfo +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.configuration.ProjectConfigurationStartEvent import com.itsaky.androidide.tooling.events.task.TaskFinishEvent import com.itsaky.androidide.tooling.events.task.TaskStartEvent import com.itsaky.androidide.utils.MetricsAnnotationStore import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashInfo import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.viewmodel.BuildOutputViewModel import org.slf4j.LoggerFactory @@ -49,13 +51,6 @@ class EditorBuildEventListener : GradleBuildService.EventListener { private var buildStartTimeMs: Long = System.currentTimeMillis() private var lastOutputTimeMs: Long = SystemClock.elapsedRealtime() - /** - * Set when the user asks for the running build to stop, so [onBuildFailed] can tell a cancel - * from a real failure. Cleared as each build is prepared. - */ - @VisibleForTesting - internal var cancelRequested = false - /** * Whether the build now running drew a "Build started" marker. * @@ -100,10 +95,8 @@ class EditorBuildEventListener : GradleBuildService.EventListener { override fun prepareBuild(buildInfo: BuildInfo) { // Before the activity check, not after: this listener outlives any one activity, so a - // build whose outcome arrived with none attached would otherwise leave both flags set for - // the next build to inherit -- a stale cancel mislabelling a real failure, or a stale - // pairing drawing a finish for a build that never started. - cancelRequested = false + // build whose outcome arrived with none attached would otherwise leave the flag set for + // the next build to inherit and draw a finish for a build that never started. annotatedBuild = false val act = checkActivity("prepareBuild") ?: return @@ -143,6 +136,54 @@ class EditorBuildEventListener : GradleBuildService.EventListener { } } + /** + * Whether [failure] is the user's own Stop rather than something going wrong. + * + * One definition, because this callback used to ask the same question three times -- once in + * [failureMessage], once in [outcomeKind] and once inline -- which is how the chart and the + * messages beside it came to disagree in the first place. + */ + @VisibleForTesting + internal fun isCancelled(failure: TaskExecutionResult.Failure?): Boolean = failure == TaskExecutionResult.Failure.BUILD_CANCELLED + + /** + * What a failed build is reported as, to the plugins and in the result the editor posts. + * + * [cancelledText] is passed in rather than resolved here so this can be asserted without an + * activity, for the same reason [outcomeKind] is separate: [onBuildFailed] returns early + * without one, so anything decided inside it is unreachable from a test. + */ + @VisibleForTesting + internal fun failureMessage( + failure: TaskExecutionResult.Failure?, + cancelledText: String, + ): String = + when { + isCancelled(failure) -> cancelledText + lastStatusLine.contains("BUILD FAILED") -> lastStatusLine + else -> "Build failed. Check build output for details." + } + + /** + * Which marker a failed build gets: the user's own cancel, or a real failure (ADFA-5542). + * + * [failure] is the server's own classification of the throwable Gradle raised. The listener + * used to answer this from a flag it set when the cancel was requested, which meant deciding + * from the order two main-thread runnables happened to run in -- and a cancel that overtook + * [prepareBuild] was cleared by it, so the build the user stopped was reported back to them as + * an error. + * + * Separated from [onBuildFailed] so the decision can be tested: that method needs a live + * activity before it reaches this point, and returns early without one. + */ + @VisibleForTesting + internal fun outcomeKind(failure: TaskExecutionResult.Failure?): MetricsAnnotationStore.Kind = + if (isCancelled(failure)) { + MetricsAnnotationStore.Kind.BUILD_CANCELLED + } else { + MetricsAnnotationStore.Kind.BUILD_FAILED + } + private fun resetBuildTimers() { buildStartTimeMs = System.currentTimeMillis() lastOutputTimeMs = SystemClock.elapsedRealtime() @@ -183,10 +224,6 @@ class EditorBuildEventListener : GradleBuildService.EventListener { lastStatusLine = "" } - override fun onBuildCancelRequested() { - cancelRequested = true - } - override fun onProgressEvent(event: ProgressEvent) { val act = checkActivity("onProgressEvent") ?: return @@ -212,31 +249,44 @@ class EditorBuildEventListener : GradleBuildService.EventListener { @VisibleForTesting internal fun isAnnotated(event: ProgressEvent): Boolean = event is TaskStartEvent || event is TaskFinishEvent - override fun onBuildFailed(tasks: List) { + override fun onBuildFailed( + tasks: List, + failure: TaskExecutionResult.Failure?, + ) { val act = checkActivity("onBuildFailed") ?: return + val cancelled = isCancelled(failure) + if (annotatedBuild) { // A build the user stopped arrives through this same callback. Marking it as a failure // would report their own deliberate action back to them in the error colour. - act.recordBuildAnnotation( - if (cancelRequested) { - MetricsAnnotationStore.Kind.BUILD_CANCELLED - } else { - MetricsAnnotationStore.Kind.BUILD_FAILED - }, - ) + act.recordBuildAnnotation(outcomeKind(failure)) } annotatedBuild = false - cancelRequested = false analyzeCurrentFile() GeneralPreferences.isFirstBuild = false act.editorViewModel.isBuildInProgress = false - act.flashError(R.string.build_status_failed) + // Everything this method says, not only the chart marker. The annotation was fixed first + // and the three reports beside it were not, so a user who pressed Stop still got a red + // "Build failed" bar, a "Build failed" notification and an isSuccess=false result -- their + // own action read back to them as an error in every place but one. + val cancelledText = act.getString(R.string.info_build_cancelled) + if (cancelled) { + act.flashInfo(R.string.info_build_cancelled) + // The status line under the output too. Gradle prints "BUILD FAILED" for a cancelled + // build like any other, and [onOutput] copies that line into the label, so the label + // sat there contradicting the bar that had just said the build was stopped. This runs + // after onOutput, so it has the last word. + act.setStatus(cancelledText) + } else { + act.flashError(R.string.build_status_failed) + } - val message = - if (lastStatusLine.contains("BUILD FAILED")) lastStatusLine else "Build failed. Check build output for details." + val message = failureMessage(failure, cancelledText) + // The plugin API has no way to say "cancelled" -- IdeServices.onBuildFailed takes an error + // string and nothing else -- so the message is the whole of what a plugin can be told. pluginBuildService?.notifyBuildFailed(message) act.notifyBuildResult(BuildResult(isSuccess = false, message = message, launchResult = null)) diff --git a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt index 437698d64d..e5feb398ef 100644 --- a/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt +++ b/app/src/main/java/com/itsaky/androidide/services/builder/GradleBuildService.kt @@ -181,10 +181,6 @@ class GradleBuildService : null } else { object : EventListener { - override fun onBuildCancelRequested() { - runOnUiThread { listener.onBuildCancelRequested() } - } - override fun prepareBuild(buildInfo: BuildInfo) { runOnUiThread { listener.prepareBuild(buildInfo) } } @@ -197,8 +193,11 @@ class GradleBuildService : runOnUiThread { listener.onProgressEvent(event) } } - override fun onBuildFailed(tasks: List) { - runOnUiThread { listener.onBuildFailed(tasks) } + override fun onBuildFailed( + tasks: List, + failure: TaskExecutionResult.Failure?, + ) { + runOnUiThread { listener.onBuildFailed(tasks, failure) } } override fun onOutput(line: String?) { @@ -460,11 +459,29 @@ class GradleBuildService : eventListener?.onBuildSuccessful(result.tasks) } + /** + * What the notification in the shade says about a build that did not succeed. + * + * Extracted so it can be asserted: [onBuildFailed] needs a live service before it reaches this + * point, which is the same reason [EditorBuildEventListener] separates its own two decisions. + * Without a test here, deleting the cancelled arm left every test green while a build the user + * stopped went back to saying "Build failed" in the shade. + */ + @VisibleForTesting + internal fun notificationStatusFor(failure: TaskExecutionResult.Failure?): Int = + if (failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { + R.string.info_build_cancelled + } else { + R.string.build_status_failed + } + override fun onBuildFailed(result: BuildResult) { - updateNotification(getString(R.string.build_status_failed), false) + // The notification too, not only what reaches the listener: a build the user stopped left + // "Build failed" in the shade whatever the chart said (ADFA-5542). + updateNotification(getString(notificationStatusFor(result.failure)), false) dispatchBuildResult(result, false) - eventListener?.onBuildFailed(result.tasks) + eventListener?.onBuildFailed(result.tasks, result.failure) } private fun dispatchBuildResult( @@ -665,9 +682,6 @@ class GradleBuildService : override fun cancelCurrentBuild(): CompletableFuture { checkServerStarted() - // Before delegating: the cancellation surfaces as a build failure, and the listener needs - // to know it was asked for rather than reporting the user's own action as an error. - eventListener?.onBuildCancelRequested() return server!!.cancelCurrentBuild() } @@ -816,19 +830,6 @@ class GradleBuildService : /** Handles events received from a Gradle build. */ interface EventListener { - /** - * Called when the user asks for the running build to stop. - * - * The tooling API reports a cancelled build through [onBuildFailed], so a listener that - * wants to tell the two apart has to be told here. - * - * Deliberately not defaulted. It was, and the forwarding wrapper in [GradleBuildService] - * then quietly inherited the no-op instead of passing it on -- so the cancel never reached - * the real listener, and a build the user stopped went on being annotated as a failure. A - * member with no default cannot be forgotten by a wrapper; the compiler asks for it. - */ - fun onBuildCancelRequested() - /** * Called just before a build is started. * @@ -855,10 +856,21 @@ class GradleBuildService : /** * Called when a build fails. * + * A build the user cancelled arrives here too, and [failure] is what tells the two apart. + * It comes from the server, which classifies the throwable Gradle raised -- the only place + * the answer is known rather than inferred (ADFA-5542). + * * @param tasks The tasks that were run. + * @param failure Why the build failed. Never null from this server, which classifies every + * failure before reporting it; nullable because the wire type allows a server that does + * not. A null is treated as an ordinary failure, which is the safe reading -- reporting + * a real failure as a cancel would hide it. * @see IToolingApiClient.onBuildFailed */ - fun onBuildFailed(tasks: List) + fun onBuildFailed( + tasks: List, + failure: TaskExecutionResult.Failure?, + ) /** * Called when the output line is received. diff --git a/app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt b/app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt new file mode 100644 index 0000000000..af0b83cb2c --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/BuildCancellation.kt @@ -0,0 +1,63 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.itsaky.androidide.projects.builder.BuildService +import com.itsaky.androidide.resources.R +import org.slf4j.LoggerFactory + +private val log = LoggerFactory.getLogger("BuildCancellation") + +/** + * Asks [service] to stop the running build, and says so when it will not. + * + * A Stop the server turns down -- there is no build to cancel, or Gradle refused the request -- + * reached a log line at one call site and nothing at all at the other, so the button appeared to + * do nothing and the build carried on. ADFA-5542 removed `EventListener.onBuildCancelRequested`, + * which was the only request-time signal, because it guessed the outcome rather than reporting + * one; nothing replaced the half of it that mattered. This is that half, in one place, because + * two call sites written separately are how they came to disagree. + * + * Success is deliberately silent: the build stopping is its own feedback, and + * [com.itsaky.androidide.handlers.EditorBuildEventListener.onBuildFailed] reports the outcome the + * server actually reached. + */ +fun requestBuildCancellation(service: BuildService) { + log.info("Sending build cancellation request...") + service.cancelCurrentBuild().whenComplete { result, error -> + if (error != null) { + log.error("Failed to send build cancellation request", error) + flashError(R.string.msg_build_cancel_failed) + return@whenComplete + } + + if (!result.wasEnqueued) { + // failureReason is nullable on the wire, so this reads it rather than asserting it. + // A refusal with no reason still has to reach the user. + log.warn( + "Unable to enqueue cancellation request reason={} reason.message={}", + result.failureReason, + result.failureReason?.message, + ) + flashError(R.string.msg_build_cancel_failed) + return@whenComplete + } + + log.info("Build cancellation request was successfully enqueued...") + } +} diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt index 027670f55a..1d56122c16 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/BuildViewModel.kt @@ -15,6 +15,7 @@ import com.itsaky.androidide.projects.models.assembleTaskOutputListingFile import com.itsaky.androidide.tooling.api.messages.BuildRunType import com.itsaky.androidide.tooling.api.messages.GradleBuildParams import com.itsaky.androidide.tooling.api.messages.TaskExecutionMessage +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -91,7 +92,17 @@ class BuildViewModel( }.await() if (result == null || !result.isSuccessful) { - throw RuntimeException("Task execution failed: ${result.failure}") + // A build the user stopped is not a failure, and it does not arrive as a + // CancellationException -- the catch below only recognises the coroutine kind. + // It comes back as a result carrying BUILD_CANCELLED, so without this the + // user's own Stop finished as BuildState.Error("Task execution failed: + // BUILD_CANCELLED"), with the enum name shown to them. + if (result?.failure == TaskExecutionResult.Failure.BUILD_CANCELLED) { + log.info("Build was cancelled by the user.") + reporter.finish(BuildState.Idle) + return@launch + } + throw RuntimeException("Task execution failed: ${result?.failure}") } if (isPluginProject) { diff --git a/app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt b/app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt new file mode 100644 index 0000000000..7a4671d0eb --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/analytics/BuildCompletedMetricTest.kt @@ -0,0 +1,78 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.analytics + +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.analytics.gradle.BuildCompletedMetric +import com.itsaky.androidide.tooling.api.messages.BuildId +import com.itsaky.androidide.tooling.api.messages.result.BuildResult +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * What build telemetry says about a build that did not succeed (ADFA-5542). + * + * A build the user stopped and a build that broke both report `success=false`. Without the reason + * beside it the two are indistinguishable, so every build-success rate counts deliberate cancels + * as failures -- and the cancel is the one the IDE deliberately makes easy to press. + */ +@RunWith(RobolectricTestRunner::class) +class BuildCompletedMetricTest { + private fun metric( + isSuccess: Boolean, + failure: TaskExecutionResult.Failure?, + ) = BuildCompletedMetric( + buildId = BuildId.Unknown, + buildType = "assemble", + isSuccess = isSuccess, + buildResult = + BuildResult( + buildId = BuildId.Unknown, + tasks = listOf(":app:assembleDebug"), + durationMs = 1_234L, + failure = failure, + ), + ) + + @Test + fun `a cancelled build carries the reason that says so`() { + val bundle = metric(isSuccess = false, failure = TaskExecutionResult.Failure.BUILD_CANCELLED).asBundle() + + assertThat(bundle.getString("failure_reason")).isEqualTo("BUILD_CANCELLED") + // isSuccess keeps its plain meaning: the build did not succeed. The reason is what lets a + // consumer separate the user's own Stop from a broken build. + assertThat(bundle.getBoolean("success")).isFalse() + } + + @Test + fun `a build that really failed carries its own reason`() { + val bundle = metric(isSuccess = false, failure = TaskExecutionResult.Failure.BUILD_FAILED).asBundle() + + assertThat(bundle.getString("failure_reason")).isEqualTo("BUILD_FAILED") + } + + @Test + fun `a successful build carries no reason at all`() { + val bundle = metric(isSuccess = true, failure = null).asBundle() + + assertThat(bundle.containsKey("failure_reason")).isFalse() + assertThat(bundle.getBoolean("success")).isTrue() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt index 5fe152155e..186cfa6873 100644 --- a/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt +++ b/app/src/test/java/com/itsaky/androidide/handlers/EditorBuildEventListenerAnnotationTest.kt @@ -18,8 +18,10 @@ package com.itsaky.androidide.handlers import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.result.BuildInfo +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.tooling.events.ProgressEvent import com.itsaky.androidide.tooling.events.internal.DefaultOperationDescriptor import com.itsaky.androidide.tooling.events.internal.DefaultProgressEvent @@ -28,15 +30,19 @@ import com.itsaky.androidide.tooling.events.task.TaskFinishEvent import com.itsaky.androidide.tooling.events.task.TaskOperationDescriptor import com.itsaky.androidide.tooling.events.task.TaskStartEvent import com.itsaky.androidide.tooling.model.PluginIdentifier +import com.itsaky.androidide.utils.MetricsAnnotationStore import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner /** - * Which Gradle progress events the metrics charts annotate (ADFA-5486). + * What the metrics charts annotate, and which build each annotation belongs to. * - * Task starts and stops, and nothing else. Asserted against the predicate rather than through - * `onProgressEvent`, which needs a live activity before it gets this far. + * Two decisions, both asserted against the function that makes them rather than through the + * callback that acts on it -- those need a live activity before they get this far. Which progress + * events are marked at all (ADFA-5486), and whether a build that failed was really the user + * stopping it (ADFA-5542). The second is now a reading of what the server said rather than a + * conclusion drawn on this side, so what is worth pinning is which answers are *not* a cancel. */ @RunWith(RobolectricTestRunner::class) class EditorBuildEventListenerAnnotationTest { @@ -74,25 +80,66 @@ class EditorBuildEventListenerAnnotationTest { ) @Test - fun `preparing a build clears a stale cancel, even with no activity attached`() { - listener.cancelRequested = true + fun `the server saying a build was cancelled is what marks it cancelled`() { + // The listener used to answer this from a flag it set when the cancel was asked for, which + // meant deciding from the order two main-thread runnables happened to run in -- and a + // cancel that overtook prepareBuild was cleared by it. Nothing here depends on order any + // more: the server classifies the throwable Gradle raised and this reads the answer. + assertThat(listener.outcomeKind(TaskExecutionResult.Failure.BUILD_CANCELLED)) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_CANCELLED) + } - // No activity is attached here, so prepareBuild returns early -- which is the point. This - // listener outlives any one activity, and a cancel whose onBuildFailed arrived without one - // would otherwise leave the flag set for the next build to inherit and be mislabelled. - listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) + @Test + fun `every other reason the server gives is a failure`() { + // The substance of the mapping, and the half worth pinning: a connection that dropped or a + // Gradle version that is not supported is not the user stopping anything, and reporting it + // as one would tell them their own action broke a build they never touched. + val notCancels = + TaskExecutionResult.Failure.entries.filterNot { it == TaskExecutionResult.Failure.BUILD_CANCELLED } + notCancels.forEach { failure -> + assertWithMessage(failure.name) + .that(listener.outcomeKind(failure)) + .isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) + } + } - assertThat(listener.cancelRequested).isFalse() + @Test + fun `a failure the server did not classify is a failure`() { + // Null reaches here from any path that reports a build result without a reason. Treating + // an absent answer as a cancel would put the user's name on something they did not do. + assertThat(listener.outcomeKind(null)).isEqualTo(MetricsAnnotationStore.Kind.BUILD_FAILED) } @Test - fun `preparing a build clears a stale pairing`() { + fun `the message for a build the user stopped is the cancelled text`() { + // One of the four places a cancel used to be reported as an error. The others are pinned + // separately -- the notification by GradleBuildServiceNotificationStatusTest, the chart + // marker by the outcomeKind cases above. The bar itself (flashInfo rather than flashError) + // and the cancelled-sync branch in ProjectHandlerActivity are not pinned: both need a live + // activity, which is what onBuildFailed returns early without. + // + // This test was previously named for all four and asserted only this one, so deleting the + // flashInfo branch or the notification branch left it green. + assertThat(listener.failureMessage(TaskExecutionResult.Failure.BUILD_CANCELLED, CANCELLED_TEXT)) + .isEqualTo(CANCELLED_TEXT) + } + + @Test + fun `a build that really failed still says so`() { + assertThat(listener.failureMessage(TaskExecutionResult.Failure.BUILD_FAILED, CANCELLED_TEXT)) + .isNotEqualTo(CANCELLED_TEXT) + assertThat(listener.failureMessage(null, CANCELLED_TEXT)).isNotEqualTo(CANCELLED_TEXT) + } + + @Test + fun `preparing a build clears a stale pairing, even with no activity attached`() { listener.annotatedBuild = true - // The flag means "a start marker was drawn for the build now running", so a new build - // must not inherit it: the outcome callbacks read it to decide whether to draw the other - // half of the pair, and they are handed a different task list from this one. Cleared - // before the activity check for the same reason as the cancel flag. + // No activity is attached here, so prepareBuild returns early -- which is the point. The + // flag means "a start marker was drawn for the build now running", and this listener + // outlives any one activity, so a build whose outcome arrived without one would otherwise + // leave it set for the next build to inherit and draw a finish for a build that never + // started. listener.prepareBuild(BuildInfo(BuildId.Unknown, listOf(":app:assembleDebug"))) assertThat(listener.annotatedBuild).isFalse() @@ -114,4 +161,9 @@ class EditorBuildEventListenerAnnotationTest { // that matter under configuration noise. assertThat(listener.isAnnotated(plainEvent())).isFalse() } + + private companion object { + /** Stands in for the string the activity would resolve, which a test has no activity for. */ + const val CANCELLED_TEXT = "Build was cancelled by the user." + } } diff --git a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt index d515017ffa..5eac7a65cb 100644 --- a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt +++ b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceListenerWrapperTest.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.services.builder import com.google.common.truth.Truth.assertThat import com.itsaky.androidide.services.builder.GradleBuildService.EventListener +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -27,12 +28,16 @@ import java.lang.reflect.Proxy /** * Pins that the build service's listener wrapper forwards every callback it is given. * - * It did not. `onBuildCancelRequested` was declared with a `= Unit` default so that only listeners - * that cared had to implement it; the wrapper then inherited that no-op rather than passing the - * call on, so the cancel never reached the real listener and a build the user had stopped went on - * being annotated as a failure -- which is what BUILD_CANCELLED exists to prevent. The feature was - * unreachable, and the store-level test for it passed the whole time, because it called the store - * directly and nothing exercised the path to it. + * It did not. A now-removed `onBuildCancelRequested` was declared with a `= Unit` default so that + * only listeners that cared had to implement it; the wrapper then inherited that no-op rather than + * passing the call on, so the cancel never reached the real listener and a build the user had + * stopped went on being annotated as a failure -- which is what BUILD_CANCELLED exists to prevent. + * The feature was unreachable, and the store-level test for it passed the whole time, because it + * called the store directly and nothing exercised the path to it. + * + * That callback is gone: the server classifies the throwable Gradle raised and says so on the + * BuildResult, so nothing on this side has to be told separately (ADFA-5542). The invariant it + * left behind outlives it and still guards every remaining member. */ @RunWith(RobolectricTestRunner::class) class GradleBuildServiceListenerWrapperTest { @@ -40,12 +45,15 @@ class GradleBuildServiceListenerWrapperTest { private class Recorder { val calls = mutableListOf() + var lastArgs: List = emptyList() + val listener: EventListener = Proxy.newProxyInstance( EventListener::class.java.classLoader, arrayOf(EventListener::class.java), - ) { _, method, _ -> + ) { _, method, args -> calls += method.name + lastArgs = args.orEmpty().toList() null } as EventListener } @@ -71,14 +79,21 @@ class GradleBuildServiceListenerWrapperTest { } @Test - fun `a cancel request reaches the listener`() { + fun `a failure reaches the listener with the server's reason for it`() { val recorder = Recorder() val wrapped = GradleBuildService.wrap(recorder.listener)!! - wrapped.onBuildCancelRequested() + wrapped.onBuildFailed(listOf(":app:assembleDebug"), TaskExecutionResult.Failure.BUILD_CANCELLED) + + assertThat(recorder.calls).containsExactly("onBuildFailed") - // The one this went wrong on, kept as its own case so the reason is legible in a report. - assertThat(recorder.calls).containsExactly("onBuildCancelRequested") + // The reason is the whole of what tells a cancel from a failure (ADFA-5542), and it is the + // server's answer rather than one this side worked out. A wrapper that forwarded the call + // and dropped the argument would be the earlier defect one layer in, with no signature to + // complain about it. + assertThat(recorder.lastArgs) + .containsExactly(listOf(":app:assembleDebug"), TaskExecutionResult.Failure.BUILD_CANCELLED) + .inOrder() } @Test diff --git a/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt new file mode 100644 index 0000000000..4327102618 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/services/builder/GradleBuildServiceNotificationStatusTest.kt @@ -0,0 +1,62 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.services.builder + +import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.assertWithMessage +import com.itsaky.androidide.R +import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * What the shade says about a build that did not succeed (ADFA-5542). + * + * The notification is one of the four places a cancelled build used to be reported as a failure, + * and the only one of them with no test: the chart marker and the message both had one, so the + * notification could have been reverted without a single test noticing. + */ +@RunWith(RobolectricTestRunner::class) +class GradleBuildServiceNotificationStatusTest { + private val service = GradleBuildService() + + @Test + fun `a build the user stopped says so in the shade`() { + assertThat(service.notificationStatusFor(TaskExecutionResult.Failure.BUILD_CANCELLED)) + .isEqualTo(R.string.info_build_cancelled) + } + + @Test + fun `every other failure says the build failed`() { + TaskExecutionResult.Failure.entries + .filter { it != TaskExecutionResult.Failure.BUILD_CANCELLED } + .forEach { failure -> + assertWithMessage(failure.name) + .that(service.notificationStatusFor(failure)) + .isEqualTo(R.string.build_status_failed) + } + } + + @Test + fun `a failure the server did not classify says the build failed`() { + // Reporting an unclassified failure as a cancel would put the user's name on something + // they did not do. + assertThat(service.notificationStatusFor(null)).isEqualTo(R.string.build_status_failed) + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 1f2990d32a..7e2f5b77bf 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -1167,6 +1167,7 @@ No APK found in output listing file. APK file specified does not exist: %1$s Build was cancelled by the user. + Could not stop the build. Quick Run failed. Building… Installing plugin… diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index 9a05bacaac..c9549d8985 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.tooling.impl import com.itsaky.androidide.tooling.api.IToolingApiClient import com.itsaky.androidide.tooling.api.IToolingApiServer +import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.ClientGradleBuildConfig import com.itsaky.androidide.tooling.api.messages.GradleDistributionParams import com.itsaky.androidide.tooling.api.messages.GradleDistributionType @@ -143,14 +144,9 @@ internal class ToolingApiServerImpl : IToolingApiServer { return@runBuild doInitialize(params, start) } catch (err: Throwable) { log.error("Failed to initialize project", err) - notifyBuildFailure( - BuildResult( - tasks = emptyList(), - buildId = params.buildId, - durationMs = System.currentTimeMillis() - start, - ), + return@runBuild InitializeResult.Failure( + notifyBuildFailure(params.buildId, emptyList(), start, err), ) - return@runBuild InitializeResult.Failure(getTaskFailureType(err)) } } } @@ -218,7 +214,17 @@ internal class ToolingApiServerImpl : IToolingApiServer { clientConfig = clientConfig, ) - RootModelBuilder.build(params, modelBuilderParams) + try { + RootModelBuilder.build(params, modelBuilderParams) + } finally { + // The sync path never cleared this on any outcome -- only shutdown() and an actual + // Stop did. So after every sync a token for a finished build sat here: the next + // Stop cancelled that dead source and answered wasEnqueued = true with no build + // running, and the check at the top of this method cancelled it again on the next + // initialize. The sibling of the same omission in executeTasks. + buildCancellationToken = null + } + notifyBuildSuccess( BuildResult( tasks = emptyList(), @@ -305,28 +311,30 @@ internal class ToolingApiServerImpl : IToolingApiServer { try { builder.run() - this.buildCancellationToken = null - notifyBuildSuccess( - result = - BuildResult( - tasks = message.tasks, - buildId = message.buildId, - durationMs = System.currentTimeMillis() - start, - ), - ) - return@runBuild TaskExecutionResult.SUCCESS } catch (error: Throwable) { log.error("Failed to run tasks: {}", message.tasks, error) - notifyBuildFailure( - result = - BuildResult( - tasks = message.tasks, - buildId = message.buildId, - durationMs = System.currentTimeMillis() - start, - ), + return@runBuild TaskExecutionResult( + false, + notifyBuildFailure(message.buildId, message.tasks, start, error), ) - return@runBuild TaskExecutionResult(false, getTaskFailureType(error)) + } finally { + // On both paths. Only the success path cleared it, so every failed build left a + // token behind for a source that was already finished. The next Stop then + // cancelled that dead source and answered wasEnqueued = true while the live build + // ran on, and [initialize] -- which cancels first whenever one is set -- paid for + // a build that had ended long before. + this.buildCancellationToken = null } + + notifyBuildSuccess( + result = + BuildResult( + tasks = message.tasks, + buildId = message.buildId, + durationMs = System.currentTimeMillis() - start, + ), + ) + return@runBuild TaskExecutionResult.SUCCESS } } @@ -357,8 +365,31 @@ internal class ToolingApiServerImpl : IToolingApiServer { } } - private fun notifyBuildFailure(result: BuildResult) { - client?.onBuildFailed(result) + /** + * Tells the client a build failed, and answers with why. + * + * Both in one call on purpose. The classification and the notification used to be written + * separately at each failure site, which is how the notified [BuildResult] came to carry + * everything except the answer while the caller of the request got it (ADFA-5542). A site + * cannot now report a failure without saying which, or say one thing to the client and another + * to its caller. + */ + private fun notifyBuildFailure( + buildId: BuildId, + tasks: List, + startedAtMillis: Long, + error: Throwable, + ): Failure { + val failure = getTaskFailureType(error) + client?.onBuildFailed( + BuildResult( + buildId = buildId, + tasks = tasks, + durationMs = System.currentTimeMillis() - startedAtMillis, + failure = failure, + ), + ) + return failure } private fun notifyBuildSuccess(result: BuildResult) { diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index 21a346df7e..9f1c4501ba 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -1,7 +1,11 @@ package com.itsaky.androidide.tooling.impl import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.tooling.api.IToolingApiClient +import com.itsaky.androidide.tooling.api.messages.BuildId import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams +import com.itsaky.androidide.tooling.api.messages.result.BuildCancellationRequestResult +import com.itsaky.androidide.tooling.api.messages.result.BuildResult import com.itsaky.androidide.tooling.api.messages.result.InitializeResult import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.tooling.api.messages.result.isSuccessful @@ -10,8 +14,10 @@ import com.itsaky.androidide.tooling.impl.sync.RootModelBuilder import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject +import io.mockk.slot import io.mockk.spyk import io.mockk.verify +import org.gradle.tooling.BuildCancelledException import org.gradle.tooling.GradleConnector import org.gradle.tooling.ProjectConnection import org.junit.Test @@ -25,18 +31,21 @@ import java.util.concurrent.TimeUnit */ @RunWith(JUnit4::class) class ToolingApiServerImplTest { - private fun testInitParams( directory: String = "/does/not/exist", forceSync: Boolean = false, ) = InitializeProjectParams( - directory = directory, needsGradleSync = forceSync + // Required since ADFA-2784 added it, and never supplied here: this file has not compiled + // on stage since, and no workflow runs :subprojects: tests, so nothing said so. + buildId = BuildId.Unknown, + directory = directory, + needsGradleSync = forceSync, ) private data class MockServer( val server: ToolingApiServerImpl, val connector: GradleConnector, - val connection: ProjectConnection + val connection: ProjectConnection, ) private fun mockkToolingServer(): MockServer { @@ -47,7 +56,10 @@ class ToolingApiServerImplTest { // ensure that we do not start actual Gradle build every { server.getOrConnectProject( - projectDir = any(), forceConnect = true, initParams = any(), gradleDist = any() + projectDir = any(), + forceConnect = true, + initParams = any(), + gradleDist = any(), ) } returns (connector to connection) @@ -56,12 +68,12 @@ class ToolingApiServerImplTest { @Test fun `GIVEN any initialization params WHEN project init fails THEN report as failure`() { - mockkObject(RootModelBuilder) every { // Simulate a Gradle sync failure RootModelBuilder.build( - any(), any() + any(), + any(), ) } throws RuntimeException("intentional failure") @@ -82,8 +94,81 @@ class ToolingApiServerImplTest { } @Test - fun `GIVEN force sync not requested WHEN sync files are unreadable THEN sync anyway`() { + fun `GIVEN a build the user stopped WHEN it fails THEN the client is told it was a cancel`() { + mockkObject(RootModelBuilder) + every { + // Gradle raises this, and only this, for a build that was cancelled. + RootModelBuilder.build(any(), any()) + } throws BuildCancelledException("stopped by the user") + + val (server) = mockkToolingServer() + + every { + server.validateProjectDirectory(any()) + } returns null + + val client = mockk(relaxed = true) + server.connect(client) + + val result = server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + assertThat((result as InitializeResult.Failure).failure) + .isEqualTo(TaskExecutionResult.Failure.BUILD_CANCELLED) + + // The same verdict has to reach the client, not only the caller of initialize. It did not, + // and the editor was left reconstructing "was that a cancel?" from the order its own + // callbacks happened to arrive in -- which it got wrong, annotating a build the user had + // stopped as a failure (ADFA-5542). + // + // This drives the sync path. The task-run path is the one the ticket is really about, and + // standing it up needs a live ProjectConnection; instead of testing the two separately, + // notifyBuildFailure now classifies and notifies in one call and hands the answer back, so + // neither site can report a failure without saying which, or tell the client one thing and + // its caller another. There is one place left to get this wrong and this covers it. + val reported = slot() + verify { client.onBuildFailed(capture(reported)) } + assertThat(reported.captured.failure).isEqualTo(TaskExecutionResult.Failure.BUILD_CANCELLED) + } + + @Test + fun `GIVEN a sync that finished WHEN a Stop arrives THEN there is no build to cancel`() { + mockkObject(RootModelBuilder) + every { RootModelBuilder.build(any(), any()) } returns File("/does/not/exist/cache") + val (server) = mockkToolingServer() + every { server.validateProjectDirectory(any()) } returns null + server.connect(mockk(relaxed = true)) + + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + // The token for the sync's own build was never cleared on any outcome, so it outlived the + // build it belonged to. A Stop pressed afterwards cancelled that dead source and answered + // "enqueued" -- telling the user a build was being stopped when none was running, and, once + // a real build had started, leaving it running while claiming otherwise. + val result = server.cancelCurrentBuild().get(5, TimeUnit.SECONDS) + + assertThat(result.wasEnqueued).isFalse() + assertThat(result.failureReason).isEqualTo(BuildCancellationRequestResult.Reason.NO_RUNNING_BUILD) + } + + @Test + fun `GIVEN a sync that failed WHEN a Stop arrives THEN there is no build to cancel`() { + mockkObject(RootModelBuilder) + every { RootModelBuilder.build(any(), any()) } throws RuntimeException("intentional failure") + + val (server) = mockkToolingServer() + every { server.validateProjectDirectory(any()) } returns null + server.connect(mockk(relaxed = true)) + + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + // The failing path leaked it the same way the succeeding one did. + val result = server.cancelCurrentBuild().get(5, TimeUnit.SECONDS) + + assertThat(result.wasEnqueued).isFalse() + } + + @Test + fun `GIVEN force sync not requested WHEN sync files are unreadable THEN sync anyway`() { val initParams = testInitParams(forceSync = false) val cacheFile = ProjectSyncHelper.cacheFileForProject(File(initParams.directory)) @@ -91,7 +176,8 @@ class ToolingApiServerImplTest { every { // simulate a successful cache write RootModelBuilder.build( - any(), any() + any(), + any(), ) } returns cacheFile diff --git a/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt b/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt index 46fb9babba..baf6359b9d 100644 --- a/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt +++ b/subprojects/tooling-api/src/main/java/com/itsaky/androidide/tooling/api/messages/result/BuildResult.kt @@ -28,4 +28,20 @@ data class BuildResult( val buildId: BuildId, val tasks: List, val durationMs: Long, + /** + * Why the build failed. + * + * `null` on a successful build, which this type also carries. On a failed one this server + * always fills it in -- `notifyBuildFailure` classifies the throwable and returns a + * non-null [TaskExecutionResult.Failure], and both catch paths go through it -- so a client + * seeing `null` here alongside a failure is talking to a server that does not classify, not + * to this one. Nullable on the wire for exactly that case. + * + * The server is the only party that can answer this: Gradle raises a + * `BuildCancelledException` for a build the user stopped, and the same throwable that decides + * the [TaskExecutionResult] decides this. Without it a client had to reconstruct "was that a + * cancel?" from the order its own callbacks happened to arrive in, and got it wrong + * (ADFA-5542). + */ + val failure: TaskExecutionResult.Failure? = null, )