From 2d9e106685cf057980a64aad17b93583809daa33 Mon Sep 17 00:00:00 2001 From: Tarek Belkahia Date: Thu, 5 Sep 2024 17:21:08 +0100 Subject: [PATCH] Fix device selection (#1953) * Fix coding style * Fix device picking logic * Add some logs * Prompt user for device creation. * Refactor TestCommand file * Fix missing import * Fix issues with shard-all and 1 test * TestCommand: minor formatting improvements, use named args more * Fix initial state of param * Add shard info to logs * Fix single shard case * Remove device creation code * Remove count from --shard-all * Revert "Remove count from --shard-all" This reverts commit 9238f97b2a707afde00b5551eb0cd7824c206b6d. * handle more edge cases --------- Co-authored-by: Bartek Pacia --- .../java/maestro/cli/command/TestCommand.kt | 406 +++++++++--------- .../maestro/cli/device/DeviceCreateUtil.kt | 88 ++-- .../maestro/cli/report/TestDebugReporter.kt | 11 +- .../cli/runner/MaestroCommandRunner.kt | 44 +- .../maestro/cli/runner/TestSuiteInteractor.kt | 75 +--- .../java/maestro/cli/util/ScreenshotUtils.kt | 46 ++ .../maestro/cli/view/TestSuiteStatusView.kt | 34 +- .../main/java/maestro/debuglog/LogConfig.kt | 2 - .../java/maestro/utils/ScreenshotUtils.kt | 6 +- 9 files changed, 348 insertions(+), 364 deletions(-) create mode 100644 maestro-cli/src/main/java/maestro/cli/util/ScreenshotUtils.kt diff --git a/maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt b/maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt index 01e6fba438..5b41714f42 100644 --- a/maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt +++ b/maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt @@ -19,20 +19,17 @@ package maestro.cli.command -import io.ktor.util.collections.ConcurrentSet import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll -import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.sync.Semaphore +import maestro.Maestro import maestro.cli.App import maestro.cli.CliError import maestro.cli.DisableAnsiMixin import maestro.cli.ShowHelpMixin -import maestro.cli.device.DeviceCreateUtil +import maestro.cli.device.Device import maestro.cli.device.DeviceService -import maestro.cli.device.PickDeviceView import maestro.cli.model.TestExecutionSummary import maestro.cli.report.ReportFormat import maestro.cli.report.ReporterFactory @@ -43,22 +40,24 @@ import maestro.cli.runner.resultview.AnsiResultView import maestro.cli.runner.resultview.PlainTextResultView import maestro.cli.session.MaestroSessionManager import maestro.cli.util.PrintUtils +import maestro.cli.view.box import maestro.orchestra.error.ValidationError import maestro.orchestra.util.Env.withInjectedShellEnvVars import maestro.orchestra.workspace.WorkspaceExecutionPlanner import maestro.orchestra.workspace.WorkspaceExecutionPlanner.ExecutionPlan import maestro.orchestra.yaml.YamlCommandReader import okio.sink +import org.slf4j.LoggerFactory import picocli.CommandLine import picocli.CommandLine.Option import java.io.File import java.nio.file.Path import java.util.concurrent.Callable import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.CountDownLatch import kotlin.io.path.absolutePathString +import kotlin.math.roundToInt import kotlin.system.exitProcess -import kotlin.time.Duration.Companion.seconds +import kotlinx.coroutines.withContext import maestro.utils.isSingleFile import maestro.orchestra.util.Env.withDefaultEnvVars @@ -78,11 +77,12 @@ class TestCommand : Callable { private val parent: App? = null @CommandLine.Parameters(description = ["One or more flow files or folders containing flow files"]) - private lateinit var flowFiles: Set + private var flowFiles: Set = emptySet() @Option( - names = ["--config"], - description = ["Optional YAML configuration file for the workspace. If not provided, Maestro will look for a config.yaml file in the workspace's root directory."]) + names = ["--config"], + description = ["Optional YAML configuration file for the workspace. If not provided, Maestro will look for a config.yaml file in the workspace's root directory."] + ) private var configFile: File? = null @Option( @@ -154,10 +154,8 @@ class TestCommand : Callable { @CommandLine.Spec lateinit var commandSpec: CommandLine.Model.CommandSpec - private val deviceCreationSemaphore = Semaphore(1) private val usedPorts = ConcurrentHashMap() - private val initialActiveDevices = ConcurrentSet() - private val currentActiveDevices = ConcurrentSet() + private val logger = LoggerFactory.getLogger(TestCommand::class.java) private fun isWebFlow(): Boolean { if (flowFiles.isSingleFile) { @@ -179,6 +177,7 @@ class TestCommand : Callable { throw CliError("Options --shard-split and --shard-all are mutually exclusive.") } + @Suppress("DEPRECATION") if (legacyShardCount != null) { PrintUtils.warn("--shards option is deprecated and will be removed in the next Maestro version. Use --shard-split or --shard-all instead.") shardSplit = legacyShardCount @@ -213,187 +212,65 @@ class TestCommand : Callable { val onlySequenceFlows = plan.sequence.flows.isNotEmpty() && plan.flowsToRun.isEmpty() // An edge case runCatching { - val deviceIds = (if (isWebFlow()) - "chromium".also { - PrintUtils.warn("Web support is an experimental feature and may be removed in future versions.\n") + val availableDevices = DeviceService.listConnectedDevices().map { it.instanceId }.toSet() + val deviceIds = getPassedOptionsDeviceIds() + .filter { device -> + if (device !in availableDevices) { + throw CliError("Device $device was requested, but it is not connected.") + } else { + true + } } - else parent?.deviceId) - .orEmpty() - .split(",") - .map { it.trim() } - .filter { it.isNotBlank() } - - initialActiveDevices.addAll(DeviceService.listConnectedDevices().map { - it.instanceId - }.toMutableSet()) + .ifEmpty { availableDevices } + .toList() - val availableDevices = if (deviceIds.isNotEmpty()) deviceIds.size else initialActiveDevices.size - val effectiveShards = if (onlySequenceFlows) 1 else requestedShards.coerceAtMost(plan.flowsToRun.size) - - if (requestedShards > plan.flowsToRun.size) { - PrintUtils.warn("Requested $requestedShards shards, but it cannot be higher than the number of flows (${plan.flowsToRun.size}). Will use $effectiveShards shards instead.") + val missingDevices = requestedShards - deviceIds.size + if (missingDevices > 0) { + PrintUtils.warn("Want to use ${deviceIds.size} devices, which is not enough to run $requestedShards shards. Missing $missingDevices device(s).") + throw CliError("Not enough devices connected ($missingDevices) to run the requested number of shards ($requestedShards).") } - val chunkPlans: List = if (onlySequenceFlows) { - // Handle an edge case - // We only want to run sequential flows in this case. - listOf(plan) - } else if (shardAll != null) { - (0 until effectiveShards).map { plan.copy() } - } else { - plan.flowsToRun - .withIndex() - .groupBy { it.index % effectiveShards } - .map { (shardIndex, files) -> - ExecutionPlan( - flowsToRun = files.map { it.value }, - sequence = plan.sequence, - ) - } - } - - PrintUtils.info(buildString { - val flowCount = if (onlySequenceFlows) plan.sequence.flows.size else plan.flowsToRun.size - - val message = when { - shardAll != null -> "Will run $effectiveShards shards, with all $flowCount flows in each shard" + val effectiveShards = when { - shardSplit != null -> { - val flowsPerShard = flowCount / effectiveShards - val isApprox = flowCount % effectiveShards != 0 - val prefix = if (isApprox) "approx. " else "" - "Will split $flowCount flows across $effectiveShards shards (${prefix}$flowsPerShard flows per shard)" - } - - else -> "Will run $flowCount flows in a single shard" - } + onlySequenceFlows -> 1 - appendLine(message) - }) + shardAll == null -> requestedShards.coerceAtMost(plan.flowsToRun.size) - // Collect device configurations for missing shards, if any - val missing = effectiveShards - availableDevices + shardSplit == null -> requestedShards.coerceAtMost(deviceIds.size) - if (missing > 0) { - PrintUtils.warn("$availableDevices device(s) connected, which is not enough to run $effectiveShards shards. Missing $missing device(s).") + else -> 1 } - val allDeviceConfigs = if (shardAll == null) (0 until missing).map { shardIndex -> - PrintUtils.message("------------------ Shard ${shardIndex + 1} ------------------") - // Collect device configurations here, one per shard - PickDeviceView.requestDeviceOptions() - }.toMutableList() else mutableListOf() + val warning = "Requested $requestedShards shards, " + + "but it cannot be higher than the number of flows (${plan.flowsToRun.size}). " + + "Will use $effectiveShards shards instead." + if (shardAll == null && requestedShards > plan.flowsToRun.size) PrintUtils.warn(warning) + + val chunkPlans = makeChunkPlans(plan, effectiveShards, onlySequenceFlows) + + val flowCount = if (onlySequenceFlows) plan.sequence.flows.size else plan.flowsToRun.size + val message = when { + shardAll != null -> "Will run $effectiveShards shards, with all $flowCount flows in each shard" + shardSplit != null -> { + val flowsPerShard = (flowCount.toFloat() / effectiveShards).roundToInt() + val isApprox = flowCount % effectiveShards != 0 + val prefix = if (isApprox) "approx. " else "" + "Will split $flowCount flows across $effectiveShards shards (${prefix}$flowsPerShard flows per shard)" + } - val barrier = CountDownLatch(effectiveShards) + else -> null + } + message?.let { PrintUtils.info(it) } val results = (0 until effectiveShards).map { shardIndex -> async(Dispatchers.IO) { - val driverHostPort = if (effectiveShards == 1) { - parent?.port ?: 7001 - } else { - (7001..7128).shuffled().find { port -> - usedPorts.putIfAbsent(port, true) == null - } ?: error("No available ports found") - } - - // Acquire lock to execute device creation block - deviceCreationSemaphore.acquire() - - val deviceId = - deviceIds.getOrNull(shardIndex) // 1. Reuse existing device if deviceId provided - ?: initialActiveDevices.elementAtOrNull(shardIndex) // 2. Reuse existing device if connected device found - ?: run { // 3. Create a new device - val cfg = allDeviceConfigs.first() - allDeviceConfigs.remove(cfg) - val deviceCreated = DeviceCreateUtil.getOrCreateDevice( - cfg.platform, cfg.osVersion, null, null, true, shardIndex - ) - - DeviceService.startDevice( - deviceCreated, driverHostPort, initialActiveDevices + currentActiveDevices - ).instanceId.also { - currentActiveDevices.add(it) - delay(2.seconds) - } - } - // Release lock if device ID was obtained from the connected devices - deviceCreationSemaphore.release() - // Signal that this thread has reached the barrier - barrier.countDown() - // Wait for all threads/shards to complete device creation before proceeding - barrier.await() - - MaestroSessionManager.newSession( - host = parent?.host, - port = parent?.port, - driverHostPort = driverHostPort, - deviceId = deviceId, - platform = parent?.platform, - ) { session -> - val maestro = session.maestro - val device = session.device - - if (flowFiles.isSingleFile.not() || format != ReportFormat.NOOP) { - // Run multiple flows - if (continuous) { - val error = - if (format != ReportFormat.NOOP) "Format can not be different from NOOP in continuous mode. Passed format is $format." - else "Continuous mode is only supported in case of a single flow file. ${flowFiles.joinToString(", ") { it.absolutePath } } has more that a single flow file." - throw CommandLine.ParameterException(commandSpec.commandLine(), error) - } - - val suiteResult = TestSuiteInteractor( - maestro = maestro, - device = device, - reporter = ReporterFactory.buildReporter(format, testSuiteName), - ).runTestSuite( - executionPlan = chunkPlans[shardIndex], - env = env, - reportOut = null, - debugOutputPath = debugOutputPath - ) - - if (!flattenDebugOutput) { - TestDebugReporter.deleteOldFiles() - } - - return@newSession Triple(suiteResult.passedCount, suiteResult.totalTests, suiteResult) - } else { - // Run a single flow - val flowFile = flowFiles.first() - env = env - .withInjectedShellEnvVars() - .withDefaultEnvVars(flowFile) - - if (continuous) { - if (!flattenDebugOutput) { - TestDebugReporter.deleteOldFiles() - } - TestRunner.runContinuous(maestro, device, flowFile, env) - - } else { - val resultView = - if (DisableAnsiMixin.ansiEnabled && parent?.verbose == false) AnsiResultView() - else PlainTextResultView() - val resultSingle = TestRunner.runSingle( - maestro, - device, - flowFile, - env, - resultView, - debugOutputPath - ) - if (resultSingle == 1) { - printExitDebugMessage() - } - if (!flattenDebugOutput) { - TestDebugReporter.deleteOldFiles() - } - val result = if (resultSingle == 0) 1 else 0 - return@newSession Triple(result, 1, null) - } - } - } + runShardSuite( + effectiveShards = effectiveShards, + deviceIds = deviceIds, + shardIndex = shardIndex, + chunkPlans = chunkPlans, + debugOutputPath = debugOutputPath, + ) } }.awaitAll() @@ -413,6 +290,148 @@ class TestCommand : Callable { }.getOrDefault(0) } + private suspend fun runShardSuite( + effectiveShards: Int, + deviceIds: List, + shardIndex: Int, + chunkPlans: List, + debugOutputPath: Path, + ): Triple = withContext(Dispatchers.IO) { + val driverHostPort = selectPort(effectiveShards) + val deviceId = deviceIds[shardIndex] + + logger.info("[shard ${shardIndex + 1}] Selected device $deviceId using port $driverHostPort") + + return@withContext MaestroSessionManager.newSession( + host = parent?.host, + port = parent?.port, + driverHostPort = driverHostPort, + deviceId = deviceId, + platform = parent?.platform, + ) { session -> + val maestro = session.maestro + val device = session.device + + val isReplicatingSingleFile = shardAll != null && effectiveShards > 1 && flowFiles.isSingleFile + val isMultipleFiles = flowFiles.isSingleFile.not() + val isAskingForReport = format != ReportFormat.NOOP + if (isMultipleFiles || isAskingForReport || isReplicatingSingleFile) { + if (continuous) { + throw CommandLine.ParameterException( + commandSpec.commandLine(), + "Continuous mode is not supported when running multiple flows. (${flowFiles.joinToString(", ")})", + ) + } + runMultipleFlows(maestro, device, chunkPlans, shardIndex, debugOutputPath) + } else { + val flowFile = flowFiles.first() + if (continuous) { + if (!flattenDebugOutput) { + TestDebugReporter.deleteOldFiles() + } + TestRunner.runContinuous(maestro, device, flowFile, env) + } else { + runSingleFlow(maestro, device, flowFile, debugOutputPath) + } + } + } + } + + private fun selectPort(effectiveShards: Int): Int = + if (effectiveShards == 1) parent?.port ?: 7001 + else (7001..7128).shuffled().find { port -> + usedPorts.putIfAbsent(port, true) == null + } ?: error("No available ports found") + + private fun runSingleFlow( + maestro: Maestro, + device: Device?, + flowFile: File, + debugOutputPath: Path, + ): Triple { + val resultView = + if (DisableAnsiMixin.ansiEnabled) AnsiResultView() + else PlainTextResultView() + + env = env + .withInjectedShellEnvVars() + .withDefaultEnvVars(flowFile) + + val resultSingle = TestRunner.runSingle( + maestro = maestro, + device = device, + flowFile = flowFile, + env = env, + resultView = resultView, + debugOutputPath = debugOutputPath, + ) + + if (resultSingle == 1) { + printExitDebugMessage() + } + + if (!flattenDebugOutput) { + TestDebugReporter.deleteOldFiles() + } + + val result = if (resultSingle == 0) 1 else 0 + return Triple(result, 1, null) + } + + private fun runMultipleFlows( + maestro: Maestro, + device: Device?, + chunkPlans: List, + shardIndex: Int, + debugOutputPath: Path + ): Triple { + val suiteResult = TestSuiteInteractor( + maestro = maestro, + device = device, + shardIndex = if (chunkPlans.size == 1) null else shardIndex, + reporter = ReporterFactory.buildReporter(format, testSuiteName), + ).runTestSuite( + executionPlan = chunkPlans[shardIndex], + env = env, + reportOut = null, + debugOutputPath = debugOutputPath + ) + + if (!flattenDebugOutput) { + TestDebugReporter.deleteOldFiles() + } + return Triple(suiteResult.passedCount, suiteResult.totalTests, suiteResult) + } + + private fun makeChunkPlans( + plan: ExecutionPlan, + effectiveShards: Int, + onlySequenceFlows: Boolean, + ) = when { + onlySequenceFlows -> listOf(plan) // We only want to run sequential flows in this case. + shardAll != null -> (0 until effectiveShards).reversed().map { plan.copy() } + else -> plan.flowsToRun + .withIndex() + .groupBy { it.index % effectiveShards } + .map { (_, files) -> + val flowsToRun = files.map { it.value } + ExecutionPlan(flowsToRun, plan.sequence) + } + } + + private fun getPassedOptionsDeviceIds(): List { + val arguments = if (isWebFlow()) { + PrintUtils.warn("Web support is an experimental feature and may be removed in future versions.\n") + "chromium" + } else parent?.deviceId + val deviceIds = arguments + .orEmpty() + .split(",") + .map { it.trim() } + .filter { it.isNotBlank() } + return deviceIds + } + private fun printExitDebugMessage() { println() println("==== Debug output (logs & screenshots) ====") @@ -420,17 +439,11 @@ class TestCommand : Callable { } private fun printShardsMessage(passedTests: Int, totalTests: Int, shardResults: List) { - val box = buildString { - val lines = listOf("Passed: $passedTests/$totalTests") + shardResults.mapIndexed { index, result -> - "[ ${result.suites.first().deviceName} ] - ${result.passedCount ?: 0}/${result.totalTests ?: 0}" - } - - val lineWidth = lines.maxOf(String::length) - append("┌${"─".repeat(lineWidth)}┐\n") - lines.forEach { append("│${it.padEnd(lineWidth)}│\n") } - append("└${"─".repeat(lineWidth)}┘") - } - PrintUtils.message(box) + val lines = listOf("Passed: $passedTests/$totalTests") + + shardResults.mapIndexed { _, result -> + "[ ${result.suites.first().deviceName} ] - ${result.passedCount ?: 0}/${result.totalTests ?: 0}" + } + PrintUtils.message(lines.joinToString("\n").box()) } private fun TestExecutionSummary.saveReport() { @@ -439,17 +452,16 @@ class TestCommand : Callable { format.fileExtension?.let { extension -> (output ?: File("report$extension")).sink() }?.also { sink -> - reporter.report( - this, - sink, - ) + reporter.report(this, sink) } } private fun List.mergeSummaries(): TestExecutionSummary? = reduceOrNull { acc, summary -> - TestExecutionSummary(passed = acc.passed && summary.passed, + TestExecutionSummary( + passed = acc.passed && summary.passed, suites = acc.suites + summary.suites, passedCount = sumOf { it.passedCount ?: 0 }, - totalTests = sumOf { it.totalTests ?: 0 }) + totalTests = sumOf { it.totalTests ?: 0 } + ) } } diff --git a/maestro-cli/src/main/java/maestro/cli/device/DeviceCreateUtil.kt b/maestro-cli/src/main/java/maestro/cli/device/DeviceCreateUtil.kt index 1e0ad83d71..7480247895 100644 --- a/maestro-cli/src/main/java/maestro/cli/device/DeviceCreateUtil.kt +++ b/maestro-cli/src/main/java/maestro/cli/device/DeviceCreateUtil.kt @@ -5,30 +5,23 @@ import maestro.cli.util.* internal object DeviceCreateUtil { - fun getOrCreateDevice(platform: Platform, - osVersion: Int?, - language: String?, - country: String?, - forceCreate: Boolean, - shardIndex: Int? = null): Device.AvailableForLaunch { - return when (platform) { - Platform.ANDROID -> { - getOrCreateAndroidDevice(osVersion, language, country, forceCreate, shardIndex) - } - - Platform.IOS -> { - getOrCreateIosDevice(osVersion, language, country, forceCreate, shardIndex) - } - - else -> throw CliError("Unsupported platform $platform. Please specify one of: android, ios") - } + fun getOrCreateDevice( + platform: Platform, + osVersion: Int? = null, + language: String? = null, + country: String? = null, + forceCreate: Boolean = false, + shardIndex: Int? = null, + ): Device.AvailableForLaunch = when (platform) { + Platform.ANDROID -> getOrCreateAndroidDevice(osVersion, language, country, forceCreate, shardIndex) + Platform.IOS -> getOrCreateIosDevice(osVersion, language, country, forceCreate, shardIndex) + else -> throw CliError("Unsupported platform $platform. Please specify one of: android, ios") } - private fun getOrCreateIosDevice(version: Int?, - language: String?, - country: String?, - forceCreate: Boolean, - shardIndex: Int? = null): Device.AvailableForLaunch { + private fun getOrCreateIosDevice( + version: Int?, language: String?, country: String?, forceCreate: Boolean, shardIndex: Int? = null + ): Device.AvailableForLaunch { + @Suppress("NAME_SHADOWING") val version = version ?: DeviceConfigIos.defaultVersion if (version !in DeviceConfigIos.versions) { throw CliError("Provided iOS version is not supported. Please use one of ${DeviceConfigIos.versions}") } @@ -38,7 +31,7 @@ internal object DeviceCreateUtil { throw CliError("Provided iOS runtime is not supported $runtime") } - val deviceName = DeviceConfigIos.generateDeviceName(version!!) + shardIndex?.let { "_${it + 1}" }.orEmpty() + val deviceName = DeviceConfigIos.generateDeviceName(version) + shardIndex?.let { "_${it + 1}" }.orEmpty() val device = DeviceConfigIos.device // check connected device @@ -63,9 +56,12 @@ internal object DeviceCreateUtil { } catch (e: IllegalStateException) { val error = e.message ?: "" if (error.contains("Invalid runtime")) { - val msg = "Required runtime to create the simulator is not installed: $runtime\n\n" + - "To install additional iOS runtimes checkout this guide:\n" + - "* https://developer.apple.com/documentation/xcode/installing-additional-simulator-runtimes" + val msg = """ + Required runtime to create the simulator is not installed: $runtime + + To install additional iOS runtimes checkout this guide: + * https://developer.apple.com/documentation/xcode/installing-additional-simulator-runtimes + """.trimIndent() throw CliError(msg) } else if (error.contains("Invalid device type")) { throw CliError("Device type $device is either not supported or not found.") @@ -86,11 +82,10 @@ internal object DeviceCreateUtil { } - private fun getOrCreateAndroidDevice(version: Int?, - language: String?, - country: String?, - forceCreate: Boolean, - shardIndex: Int? = null): Device.AvailableForLaunch { + private fun getOrCreateAndroidDevice( + version: Int?, language: String?, country: String?, forceCreate: Boolean, shardIndex: Int? = null + ): Device.AvailableForLaunch { + @Suppress("NAME_SHADOWING") val version = version ?: DeviceConfigAndroid.defaultVersion if (version !in DeviceConfigAndroid.versions) { throw CliError("Provided Android version is not supported. Please use one of ${DeviceConfigAndroid.versions}") } @@ -100,7 +95,7 @@ internal object DeviceCreateUtil { val pixel = DeviceConfigAndroid.choosePixelDevice(pixels) ?: AvdDevice("-1", "Pixel 6", "pixel_6") val config = try { - DeviceConfigAndroid.createConfig(version!!, pixel, architecture) + DeviceConfigAndroid.createConfig(version, pixel, architecture) } catch (e: IllegalStateException) { throw CliError(e.message ?: "Unable to create android device config") } @@ -114,7 +109,8 @@ internal object DeviceCreateUtil { // existing device val existingDevice = - if (forceCreate) null else DeviceService.isDeviceAvailableToLaunch(deviceName, Platform.ANDROID)?.modelId + if (forceCreate) null + else DeviceService.isDeviceAvailableToLaunch(deviceName, Platform.ANDROID)?.modelId // dependencies if (existingDevice == null && !DeviceService.isAndroidSystemImageInstalled(systemImage)) { @@ -125,22 +121,18 @@ internal object DeviceCreateUtil { if (r == "y" || r == "yes") { PrintUtils.message("Attempting to install $systemImage via Android SDK Manager...\n") if (!DeviceService.installAndroidSystemImage(systemImage)) { - throw CliError( - "Unable to install required dependencies. You can install the system image manually by running this command:\n${ - DeviceService.getAndroidSystemImageInstallCommand( - systemImage - ) - }" - ) + val message = """ + Unable to install required dependencies. You can install the system image manually by running this command: + ${DeviceService.getAndroidSystemImageInstallCommand(systemImage)} + """.trimIndent() + throw CliError(message) } } else { - throw CliError( - "To install the system image manually, you can run this command:\n${ - DeviceService.getAndroidSystemImageInstallCommand( - systemImage - ) - }" - ) + val message = """ + To install the system image manually, you can run this command: + ${DeviceService.getAndroidSystemImageInstallCommand(systemImage)} + """.trimIndent() + throw CliError(message) } } @@ -171,4 +163,4 @@ internal object DeviceCreateUtil { country = country, ) } -} \ No newline at end of file +} diff --git a/maestro-cli/src/main/java/maestro/cli/report/TestDebugReporter.kt b/maestro-cli/src/main/java/maestro/cli/report/TestDebugReporter.kt index e9cc9dede3..ad9f85b118 100644 --- a/maestro-cli/src/main/java/maestro/cli/report/TestDebugReporter.kt +++ b/maestro-cli/src/main/java/maestro/cli/report/TestDebugReporter.kt @@ -67,16 +67,19 @@ object TestDebugReporter { /** * Save debug information about a single flow, after it has finished. */ - fun saveFlow(flowName: String, debugOutput: FlowDebugOutput, path: Path) { + fun saveFlow(flowName: String, debugOutput: FlowDebugOutput, path: Path, shardIndex: Int? = null) { // TODO(bartekpacia): Potentially accept a single "FlowPersistentOutput" object // TODO(bartekpacia: Build output incrementally, instead of single-shot on flow completion // Be aware that this goal somewhat conflicts with including links to other flows in the HTML report. + val shardPrefix = shardIndex?.let { "shard-${it + 1}-" }.orEmpty() + val shardLogPrefix = shardIndex?.let { "[shard ${it + 1}] " }.orEmpty() + // commands try { val commandMetadata = debugOutput.commands if (commandMetadata.isNotEmpty()) { - val commandsFilename = "commands-(${flowName.replace("/", "_")}).json" + val commandsFilename = "commands-$shardPrefix(${flowName.replace("/", "_")}).json" val file = File(path.absolutePathString(), commandsFilename) commandMetadata.map { CommandDebugWrapper(it.key, it.value) @@ -85,7 +88,7 @@ object TestDebugReporter { } } } catch (e: JsonMappingException) { - logger.error("Unable to parse commands", e) + logger.error("${shardLogPrefix}Unable to parse commands", e) } // screenshots @@ -96,7 +99,7 @@ object TestDebugReporter { CommandStatus.WARNED -> "⚠️" else -> "﹖" } - val filename = "screenshot-$status-${it.timestamp}-(${flowName}).png" + val filename = "screenshot-$shardPrefix$status-${it.timestamp}-(${flowName}).png" val file = File(path.absolutePathString(), filename) it.screenshot.copyTo(file) diff --git a/maestro-cli/src/main/java/maestro/cli/runner/MaestroCommandRunner.kt b/maestro-cli/src/main/java/maestro/cli/runner/MaestroCommandRunner.kt index bcdfaadc8f..7347150c52 100644 --- a/maestro-cli/src/main/java/maestro/cli/runner/MaestroCommandRunner.kt +++ b/maestro-cli/src/main/java/maestro/cli/runner/MaestroCommandRunner.kt @@ -34,11 +34,9 @@ import maestro.orchestra.MaestroCommand import maestro.orchestra.Orchestra import maestro.orchestra.yaml.YamlCommandReader import maestro.utils.Insight -import okio.Buffer -import okio.sink import org.slf4j.LoggerFactory -import java.io.File import java.util.IdentityHashMap +import maestro.cli.util.ScreenshotUtils /** * Knows how to run a list of Maestro commands and update the UI. @@ -64,40 +62,6 @@ object MaestroCommandRunner { val commandStatuses = IdentityHashMap() val commandMetadata = IdentityHashMap() - fun takeDebugScreenshot(status: CommandStatus): File? { - val containsFailed = debugOutput.screenshots.any { it.status == CommandStatus.FAILED } - - // Avoids duplicate failed images from parent commands - if (containsFailed && status == CommandStatus.FAILED) { - return null - } - - val result = kotlin.runCatching { - val out = File - .createTempFile("screenshot-${System.currentTimeMillis()}", ".png") - .also { it.deleteOnExit() } // save to another dir before exiting - maestro.takeScreenshot(out.sink(), false) - debugOutput.screenshots.add( - FlowDebugOutput.Screenshot( - screenshot = out, - timestamp = System.currentTimeMillis(), - status = status - ) - ) - out - } - - return result.getOrNull() - } - - fun writeAIscreenshot(buffer: Buffer): File { - val out = File - .createTempFile("ai-screenshot-${System.currentTimeMillis()}", ".png") - .also { it.deleteOnExit() } - out.outputStream().use { it.write(buffer.readByteArray()) } - return out - } - fun refreshUi() { view.setState( UiState.Running( @@ -151,7 +115,7 @@ object MaestroCommandRunner { error = e } - takeDebugScreenshot(CommandStatus.FAILED) + ScreenshotUtils.takeDebugScreenshot(maestro, debugOutput, CommandStatus.FAILED) if (e !is MaestroException) { throw e @@ -179,7 +143,7 @@ object MaestroCommandRunner { status = CommandStatus.WARNED } - takeDebugScreenshot(CommandStatus.WARNED) + ScreenshotUtils.takeDebugScreenshot(maestro, debugOutput, CommandStatus.WARNED) refreshUi() }, @@ -198,7 +162,7 @@ object MaestroCommandRunner { }, onCommandGeneratedOutput = { command, defects, screenshot -> logger.info("${command.description()} generated output") - val screenshotPath = writeAIscreenshot(screenshot) + val screenshotPath = ScreenshotUtils.writeAIscreenshot(screenshot) aiOutput.screenOutputs.add( SingleScreenFlowAIOutput( screenshotPath = screenshotPath, diff --git a/maestro-cli/src/main/java/maestro/cli/runner/TestSuiteInteractor.kt b/maestro-cli/src/main/java/maestro/cli/runner/TestSuiteInteractor.kt index 68f9721913..4592e12bba 100644 --- a/maestro-cli/src/main/java/maestro/cli/runner/TestSuiteInteractor.kt +++ b/maestro-cli/src/main/java/maestro/cli/runner/TestSuiteInteractor.kt @@ -21,13 +21,13 @@ import maestro.orchestra.Orchestra import maestro.orchestra.util.Env.withEnv import maestro.orchestra.workspace.WorkspaceExecutionPlanner import maestro.orchestra.yaml.YamlCommandReader -import okio.Buffer import okio.Sink import org.slf4j.LoggerFactory import java.io.File import java.nio.file.Path import kotlin.system.measureTimeMillis import kotlin.time.Duration.Companion.seconds +import maestro.cli.util.ScreenshotUtils import maestro.orchestra.util.Env.withDefaultEnvVars import maestro.orchestra.util.Env.withInjectedShellEnvVars @@ -42,9 +42,11 @@ class TestSuiteInteractor( private val maestro: Maestro, private val device: Device? = null, private val reporter: TestSuiteReporter, + private val shardIndex: Int? = null, ) { private val logger = LoggerFactory.getLogger(TestSuiteInteractor::class.java) + private val shardPrefix = shardIndex?.let { "[shard ${it + 1}] " }.orEmpty() fun runTestSuite( executionPlan: WorkspaceExecutionPlanner.ExecutionPlan, @@ -52,21 +54,20 @@ class TestSuiteInteractor( env: Map, debugOutputPath: Path ): TestExecutionSummary { - if (executionPlan.flowsToRun.isEmpty() && executionPlan.sequence?.flows?.isEmpty() == true) { - throw CliError("No flows returned from the tag filter used") + if (executionPlan.flowsToRun.isEmpty() && executionPlan.sequence.flows.isEmpty()) { + throw CliError("${shardPrefix}No flows returned from the tag filter used") } val flowResults = mutableListOf() - PrintUtils.message("Waiting for flows to complete...") - println() + PrintUtils.message("${shardPrefix}Waiting for flows to complete...") var passed = true val aiOutputs = mutableListOf() // first run sequence of flows if present val flowSequence = executionPlan.sequence - for (flow in flowSequence?.flows ?: emptyList()) { + for (flow in flowSequence.flows) { val flowFile = flow.toFile() val updatedEnv = env .withInjectedShellEnvVars() @@ -77,8 +78,8 @@ class TestSuiteInteractor( if (result.status == FlowStatus.ERROR) { passed = false - if (executionPlan.sequence?.continueOnFailure != true) { - PrintUtils.message("Flow ${result.name} failed and continueOnFailure is set to false, aborting running sequential Flows") + if (executionPlan.sequence.continueOnFailure != true) { + PrintUtils.message("${shardPrefix}Flow ${result.name} failed and continueOnFailure is set to false, aborting running sequential Flows") println() break } @@ -103,6 +104,7 @@ class TestSuiteInteractor( TestSuiteViewModel( status = if (passed) FlowStatus.SUCCESS else FlowStatus.ERROR, duration = suiteDuration, + shardIndex = shardIndex, flows = flowResults .map { TestSuiteViewModel.FlowResult( @@ -162,39 +164,6 @@ class TestSuiteInteractor( flowFile = flowFile, ) - fun takeDebugScreenshot(status: CommandStatus): File? { - val containsFailed = debugOutput.screenshots.any { it.status == CommandStatus.FAILED } - - // Avoids duplicate failed images from parent commands - if (containsFailed && status == CommandStatus.FAILED) { - return null - } - - val result = kotlin.runCatching { - val out = File.createTempFile("screenshot-${System.currentTimeMillis()}", ".png") - .also { it.deleteOnExit() } // save to another dir before exiting - maestro.takeScreenshot(out, false) - debugOutput.screenshots.add( - FlowDebugOutput.Screenshot( - screenshot = out, - timestamp = System.currentTimeMillis(), - status = status - ) - ) - out - } - - return result.getOrNull() - } - - fun writeAIscreenshot(buffer: Buffer): File { - val out = File - .createTempFile("ai-screenshot-${System.currentTimeMillis()}", ".png") - .also { it.deleteOnExit() } - out.outputStream().use { it.write(buffer.readByteArray()) } - return out - } - val flowTimeMillis = measureTimeMillis { try { val commands = YamlCommandReader @@ -206,21 +175,21 @@ class TestSuiteInteractor( val orchestra = Orchestra( maestro = maestro, onCommandStart = { _, command -> - logger.info("${command.description()} RUNNING") + logger.info("${shardPrefix}${command.description()} RUNNING") debugOutput.commands[command] = CommandDebugMetadata( timestamp = System.currentTimeMillis(), status = CommandStatus.RUNNING ) }, onCommandComplete = { _, command -> - logger.info("${command.description()} COMPLETED") + logger.info("${shardPrefix}${command.description()} COMPLETED") debugOutput.commands[command]?.let { it.status = CommandStatus.COMPLETED it.calculateDuration() } }, onCommandFailed = { _, command, e -> - logger.info("${command.description()} FAILED") + logger.info("${shardPrefix}${command.description()} FAILED") if (e is MaestroException) debugOutput.exception = e debugOutput.commands[command]?.let { it.status = CommandStatus.FAILED @@ -228,30 +197,30 @@ class TestSuiteInteractor( it.error = e } - takeDebugScreenshot(CommandStatus.FAILED) + ScreenshotUtils.takeDebugScreenshot(maestro, debugOutput, CommandStatus.FAILED) Orchestra.ErrorResolution.FAIL }, onCommandSkipped = { _, command -> - logger.info("${command.description()} SKIPPED") + logger.info("${shardPrefix}${command.description()} SKIPPED") debugOutput.commands[command]?.let { it.status = CommandStatus.SKIPPED } }, onCommandWarned = { _, command -> - logger.info("${command.description()} WARNED") + logger.info("${shardPrefix}${command.description()} WARNED") debugOutput.commands[command]?.apply { status = CommandStatus.WARNED } }, onCommandReset = { command -> - logger.info("${command.description()} PENDING") + logger.info("${shardPrefix}${command.description()} PENDING") debugOutput.commands[command]?.let { it.status = CommandStatus.PENDING } }, onCommandGeneratedOutput = { command, defects, screenshot -> - logger.info("${command.description()} generated output") - val screenshotPath = writeAIscreenshot(screenshot) + logger.info("${shardPrefix}${command.description()} generated output") + val screenshotPath = ScreenshotUtils.writeAIscreenshot(screenshot) aiOutput.screenOutputs.add( SingleScreenFlowAIOutput( screenshotPath = screenshotPath, @@ -264,7 +233,7 @@ class TestSuiteInteractor( val flowSuccess = orchestra.runFlow(commands) flowStatus = if (flowSuccess) FlowStatus.SUCCESS else FlowStatus.ERROR } catch (e: Exception) { - logger.error("Failed to complete flow", e) + logger.error("${shardPrefix}Failed to complete flow", e) flowStatus = FlowStatus.ERROR errorMessage = ErrorViewUtils.exceptionToMessage(e) } @@ -274,6 +243,7 @@ class TestSuiteInteractor( TestDebugReporter.saveFlow( flowName = flowName, debugOutput = debugOutput, + shardIndex = shardIndex, path = debugOutputPath, ) // FIXME(bartekpacia): Save AI output as well @@ -283,6 +253,7 @@ class TestSuiteInteractor( name = flowName, status = flowStatus, duration = flowDuration, + shardIndex = shardIndex, error = debugOutput.exception?.message, ) ) @@ -294,7 +265,7 @@ class TestSuiteInteractor( status = flowStatus, failure = if (flowStatus == FlowStatus.ERROR) { TestExecutionSummary.Failure( - message = errorMessage ?: debugOutput.exception?.message ?: "Unknown error", + message = shardPrefix + (errorMessage ?: debugOutput.exception?.message ?: "Unknown error"), ) } else null, duration = flowDuration, diff --git a/maestro-cli/src/main/java/maestro/cli/util/ScreenshotUtils.kt b/maestro-cli/src/main/java/maestro/cli/util/ScreenshotUtils.kt new file mode 100644 index 0000000000..c3a6e1a2f6 --- /dev/null +++ b/maestro-cli/src/main/java/maestro/cli/util/ScreenshotUtils.kt @@ -0,0 +1,46 @@ +package maestro.cli.util + +import java.io.File +import maestro.Maestro +import maestro.cli.report.FlowDebugOutput +import maestro.cli.runner.CommandStatus +import okio.Buffer +import okio.sink + +object ScreenshotUtils { + + fun takeDebugScreenshot(maestro: Maestro, debugOutput: FlowDebugOutput, status: CommandStatus): File? { + val containsFailed = debugOutput.screenshots.any { it.status == CommandStatus.FAILED } + + // Avoids duplicate failed images from parent commands + if (containsFailed && status == CommandStatus.FAILED) { + return null + } + + val result = kotlin.runCatching { + val out = File + .createTempFile("screenshot-${System.currentTimeMillis()}", ".png") + .also { it.deleteOnExit() } // save to another dir before exiting + maestro.takeScreenshot(out.sink(), false) + debugOutput.screenshots.add( + FlowDebugOutput.Screenshot( + screenshot = out, + timestamp = System.currentTimeMillis(), + status = status + ) + ) + out + } + + return result.getOrNull() + } + + fun writeAIscreenshot(buffer: Buffer): File { + val out = File + .createTempFile("ai-screenshot-${System.currentTimeMillis()}", ".png") + .also { it.deleteOnExit() } + out.outputStream().use { it.write(buffer.readByteArray()) } + return out + } + +} diff --git a/maestro-cli/src/main/java/maestro/cli/view/TestSuiteStatusView.kt b/maestro-cli/src/main/java/maestro/cli/view/TestSuiteStatusView.kt index 9fb1efafb6..364927e329 100644 --- a/maestro-cli/src/main/java/maestro/cli/view/TestSuiteStatusView.kt +++ b/maestro-cli/src/main/java/maestro/cli/view/TestSuiteStatusView.kt @@ -13,27 +13,22 @@ import kotlin.time.Duration.Companion.seconds object TestSuiteStatusView { fun showFlowCompletion(result: FlowResult) { + val shardPrefix = result.shardIndex?.let { "[shard ${it + 1}] " }.orEmpty() + print(Ansi.ansi().fgCyan().render(shardPrefix).fgDefault()) + printStatus(result.status, result.cancellationReason) val durationString = result.duration?.let { " ($it)" }.orEmpty() print(" ${result.name}$durationString") + if (result.status == FlowStatus.ERROR && result.error != null) { - print( - Ansi.ansi() - .fgRed() - .render(" (${result.error})") - .fgDefault() - ) + val error = " (${result.error})" + print(Ansi.ansi().fgRed().render(error).fgDefault()) } else if (result.status == FlowStatus.WARNING) { - print( - Ansi.ansi() - .fgYellow() - .render(" (Warning)") - .fgDefault() - ) + val warning = " (Warning)" + print(Ansi.ansi().fgYellow().render(warning).fgDefault()) } - println() } @@ -44,18 +39,19 @@ object TestSuiteStatusView { val hasError = suite.flows.find { it.status == FlowStatus.ERROR } != null val canceledFlows = suite.flows .filter { it.status == FlowStatus.CANCELED } + val shardPrefix = suite.shardIndex?.let { "[shard ${it + 1}] " }.orEmpty() if (suite.status == FlowStatus.ERROR || hasError) { val failedFlows = suite.flows .filter { it.status == FlowStatus.ERROR } PrintUtils.err( - "${failedFlows.size}/${suite.flows.size} ${flowWord(failedFlows.size)} Failed", + "${shardPrefix}${failedFlows.size}/${suite.flows.size} ${flowWord(failedFlows.size)} Failed", bold = true, ) if (canceledFlows.isNotEmpty()) { - PrintUtils.warn("${canceledFlows.size} ${flowWord(canceledFlows.size)} Canceled") + PrintUtils.warn("${shardPrefix}${canceledFlows.size} ${flowWord(canceledFlows.size)} Canceled") } } else { @@ -66,16 +62,16 @@ object TestSuiteStatusView { if (passedFlows.isNotEmpty()) { val durationMessage = suite.duration?.let { " in $it" } ?: "" PrintUtils.success( - "${passedFlows.size}/${suite.flows.size} ${flowWord(passedFlows.size)} Passed$durationMessage", + "${shardPrefix}${passedFlows.size}/${suite.flows.size} ${flowWord(passedFlows.size)} Passed$durationMessage", bold = true, ) if (canceledFlows.isNotEmpty()) { - PrintUtils.warn("${canceledFlows.size} ${flowWord(canceledFlows.size)} Canceled") + PrintUtils.warn("${shardPrefix}${canceledFlows.size} ${flowWord(canceledFlows.size)} Canceled") } } else { println() - PrintUtils.err("All flows were canceled") + PrintUtils.err("${shardPrefix}All flows were canceled") } } println() @@ -143,6 +139,7 @@ object TestSuiteStatusView { val status: FlowStatus, val flows: List, val duration: Duration? = null, + val shardIndex: Int? = null, val uploadDetails: UploadDetails? = null, ) { @@ -151,6 +148,7 @@ object TestSuiteStatusView { val status: FlowStatus, val duration: Duration? = null, val error: String? = null, + val shardIndex: Int? = null, val cancellationReason: UploadStatus.CancellationReason? = null ) diff --git a/maestro-client/src/main/java/maestro/debuglog/LogConfig.kt b/maestro-client/src/main/java/maestro/debuglog/LogConfig.kt index 4129f6cce0..d4b1f3a644 100644 --- a/maestro-client/src/main/java/maestro/debuglog/LogConfig.kt +++ b/maestro-client/src/main/java/maestro/debuglog/LogConfig.kt @@ -6,9 +6,7 @@ import ch.qos.logback.classic.encoder.PatternLayoutEncoder import ch.qos.logback.classic.spi.ILoggingEvent import ch.qos.logback.core.FileAppender import ch.qos.logback.core.status.NopStatusListener -import maestro.Driver import org.slf4j.LoggerFactory -import java.util.Properties object LogConfig { private const val LOG_PATTERN = "[%-5level] %logger{36} - %msg%n" diff --git a/maestro-client/src/main/java/maestro/utils/ScreenshotUtils.kt b/maestro-client/src/main/java/maestro/utils/ScreenshotUtils.kt index ed8bb517b7..90128b30e9 100644 --- a/maestro-client/src/main/java/maestro/utils/ScreenshotUtils.kt +++ b/maestro-client/src/main/java/maestro/utils/ScreenshotUtils.kt @@ -14,13 +14,13 @@ class ScreenshotUtils { private val LOGGER = LoggerFactory.getLogger(ScreenshotUtils::class.java) fun takeScreenshot(out: Sink, compressed: Boolean, driver: Driver) { - LOGGER.info("Taking screenshot to output sink") + LOGGER.trace("Taking screenshot to output sink") driver.takeScreenshot(out, compressed) } fun takeScreenshot(compressed: Boolean, driver: Driver): ByteArray { - LOGGER.info("Taking screenshot to byte array") + LOGGER.trace("Taking screenshot to byte array") val buffer = Buffer() takeScreenshot(buffer, compressed, driver) @@ -99,4 +99,4 @@ class ScreenshotUtils { return ViewHierarchy.from(driver, false) } } -} \ No newline at end of file +}