Error message improvements (mobile-dev-inc#1372)

Error message improvements * Proper error messages for empty workspaces * Surface workspace validation errors before connecting to device when running `maestro test` * Lay test fixture groundwork for future error improvements Behavior Change: This PR also changes the behavior when includeTags are specified in both config.yaml and as a command line option. Previously, in order for a flow to be included, it would need to match one of the config.yaml includeTags AND one of the command line includeTags. Even though this might be useful for some specific scenarios, this behavior is confusing and hard to explain. Now, in order for a flow to be included, it needs to match only one includeTag regardless of where the includeTag was specified.
alexandrefavre4 · Aug 22, 2023 · c070cd8 · c070cd8
1 parent 3a45232
commit c070cd8
Show file tree

Hide file tree

Showing 27 changed files with 224 additions and 99 deletions.
diff --git a/maestro-cli/src/main/java/maestro/cli/App.kt b/maestro-cli/src/main/java/maestro/cli/App.kt
@@ -19,6 +19,8 @@
 
 package maestro.cli
 
+import java.util.Properties
+import kotlin.system.exitProcess
 import maestro.cli.command.BugReportCommand
 import maestro.cli.command.CloudCommand
 import maestro.cli.command.DownloadSamplesCommand
@@ -35,14 +37,9 @@ import maestro.cli.update.Updates
 import maestro.cli.util.ErrorReporter
 import maestro.cli.view.box
 import maestro.debuglog.DebugLogStore
-import maestro.debuglog.LogConfig
-import org.slf4j.LoggerFactory
 import picocli.CommandLine
 import picocli.CommandLine.Command
 import picocli.CommandLine.Option
-import java.util.Properties
-import kotlin.io.path.absolutePathString
-import kotlin.system.exitProcess
 
 @Command(
     name = "maestro",
@@ -94,9 +91,6 @@ fun main(args: Array<String>) {
     // https://stackoverflow.com/a/17544259
     System.setProperty("apple.awt.UIElement", "true")
 
-    // logs & debug output
-    val logger = LoggerFactory.getLogger(App::class.java)
-
     Dependencies.install()
     Updates.fetchUpdatesAsync()
 
@@ -113,7 +107,6 @@ fun main(args: Array<String>) {
                 ex.stackTraceToString()
             }
 
-            logger.error(message)
             println()
             cmd.err.println(
                 cmd.colorScheme.errorText(message)

diff --git a/maestro-cli/src/main/java/maestro/cli/command/CloudCommand.kt b/maestro-cli/src/main/java/maestro/cli/command/CloudCommand.kt
@@ -30,7 +30,6 @@ import maestro.orchestra.workspace.WorkspaceExecutionPlanner
 import picocli.CommandLine
 import picocli.CommandLine.Option
 import java.io.File
-import java.nio.file.Files
 import java.util.concurrent.Callable
 import java.util.concurrent.TimeUnit
 
@@ -186,16 +185,11 @@ class CloudCommand : Callable<Int> {
     }
 
     private fun validateWorkSpace() {
-        val flowPath = flowsFile.toPath().toAbsolutePath()
-        if (Files.notExists(flowPath)) {
-            throw CliError("Flow file path $flowsFile does not exist")
-        }
-
         try {
             PrintUtils.message("Evaluating workspace...")
             WorkspaceExecutionPlanner
                 .plan(
-                    input = flowPath,
+                    input = flowsFile.toPath().toAbsolutePath(),
                     includeTags = includeTags,
                     excludeTags = excludeTags,
                 )

diff --git a/maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt b/maestro-cli/src/main/java/maestro/cli/command/TestCommand.kt
@@ -31,9 +31,9 @@ import maestro.cli.runner.resultview.AnsiResultView
 import maestro.cli.runner.resultview.PlainTextResultView
 import maestro.cli.session.MaestroSessionManager
 import maestro.cli.util.PrintUtils
-import maestro.debuglog.DebugLogStore
-import maestro.debuglog.LogConfig
+import maestro.orchestra.error.ValidationError
 import maestro.orchestra.util.Env.withInjectedShellEnvVars
+import maestro.orchestra.workspace.WorkspaceExecutionPlanner
 import maestro.orchestra.yaml.YamlCommandReader
 import okio.buffer
 import okio.sink
@@ -114,17 +114,16 @@ class TestCommand : Callable<Int> {
     }
 
     override fun call(): Int {
-        if (!flowFile.exists()) {
-            throw CommandLine.ParameterException(
-                commandSpec.commandLine(),
-                "File not found: $flowFile"
-            )
-        }
-
         if (parent?.platform != null) {
             throw CliError("--platform option was deprecated. You can remove it to run your test.")
         }
 
+        val executionPlan = try {
+            WorkspaceExecutionPlanner.plan(flowFile.toPath().toAbsolutePath(), includeTags, excludeTags)
+        } catch (e: ValidationError) {
+            throw CliError(e.message)
+        }
+
         val deviceId =
             if (isWebFlow()) "chromium".also { PrintUtils.warn("Web support is an experimental feature and may be removed in future versions.\n") }
             else parent?.deviceId
@@ -150,10 +149,8 @@ class TestCommand : Callable<Int> {
                     maestro = maestro,
                     device = device,
                     reporter = ReporterFactory.buildReporter(format, testSuiteName),
-                    includeTags = includeTags,
-                    excludeTags = excludeTags,
                 ).runTestSuite(
-                    input = flowFile,
+                    executionPlan = executionPlan,
                     env = env,
                     reportOut = format.fileExtension
                         ?.let { extension ->

diff --git a/maestro-cli/src/main/java/maestro/cli/runner/TestSuiteInteractor.kt b/maestro-cli/src/main/java/maestro/cli/runner/TestSuiteInteractor.kt
@@ -6,25 +6,18 @@ import maestro.cli.CliError
 import maestro.cli.device.Device
 import maestro.cli.model.FlowStatus
 import maestro.cli.model.TestExecutionSummary
-import maestro.cli.report.CommandDebugMetadata
-import maestro.cli.report.FlowDebugMetadata
-import maestro.cli.report.ScreenshotDebugMetadata
-import maestro.cli.report.TestDebugReporter
-import maestro.cli.report.TestSuiteReporter
+import maestro.cli.report.*
 import maestro.cli.util.PrintUtils
 import maestro.cli.view.ErrorViewUtils
 import maestro.cli.view.TestSuiteStatusView
 import maestro.cli.view.TestSuiteStatusView.TestSuiteViewModel
-import maestro.debuglog.DebugLogStore
-import maestro.debuglog.LogConfig
 import maestro.orchestra.Orchestra
 import maestro.orchestra.util.Env.withEnv
 import maestro.orchestra.workspace.WorkspaceExecutionPlanner
 import maestro.orchestra.yaml.YamlCommandReader
 import okio.Sink
 import org.slf4j.LoggerFactory
 import java.io.File
-import kotlin.io.path.absolutePathString
 import kotlin.math.roundToLong
 import kotlin.system.measureTimeMillis
 import kotlin.time.Duration.Companion.seconds
@@ -33,53 +26,20 @@ class TestSuiteInteractor(
     private val maestro: Maestro,
     private val device: Device? = null,
     private val reporter: TestSuiteReporter,
-    private val includeTags: List<String> = emptyList(),
-    private val excludeTags: List<String> = emptyList(),
 ) {
 
     private val logger = LoggerFactory.getLogger(TestSuiteInteractor::class.java)
 
     fun runTestSuite(
-        input: File,
+        executionPlan: WorkspaceExecutionPlanner.ExecutionPlan,
         reportOut: Sink?,
         env: Map<String, String>,
         debugOutput: String?
     ): TestExecutionSummary {
-        return if (input.isFile) {
-            runTestSuite(
-                WorkspaceExecutionPlanner.ExecutionPlan(flowsToRun = listOf(input.toPath())),
-                reportOut,
-                env,
-                debugOutput
-            )
-        } else {
-            val plan = WorkspaceExecutionPlanner
-                .plan(
-                    input = input.toPath().toAbsolutePath(),
-                    includeTags = includeTags,
-                    excludeTags = excludeTags,
-                )
-            val flowFiles = plan.flowsToRun
-
-            if (flowFiles.isEmpty() && plan.sequence?.flows?.isEmpty() == true) {
-                throw CliError("No flows returned from the tag filter used")
-            }
-
-            runTestSuite(
-                plan,
-                reportOut,
-                env,
-                debugOutput
-            )
+        if (executionPlan.flowsToRun.isEmpty() && executionPlan.sequence?.flows?.isEmpty() == true) {
+            throw CliError("No flows returned from the tag filter used")
         }
-    }
 
-    private fun runTestSuite(
-        executionPlan: WorkspaceExecutionPlanner.ExecutionPlan,
-        reportOut: Sink?,
-        env: Map<String, String>,
-        debugOutput: String?
-    ): TestExecutionSummary {
         val flowResults = mutableListOf<TestExecutionSummary.FlowResult>()
 
         PrintUtils.message("Waiting for flows to complete...")

diff --git a/maestro-orchestra/build.gradle b/maestro-orchestra/build.gradle
@@ -15,6 +15,7 @@ dependencies {
     api(libs.jackson.dataformat.yaml)
 
     testImplementation(libs.junit.jupiter.api)
+    testImplementation(libs.junit.jupiter.params)
     testRuntimeOnly(libs.junit.jupiter.engine)
 
     testImplementation(libs.google.truth)
@@ -33,4 +34,5 @@ plugins.withId("com.vanniktech.maven.publish") {
 
 test {
     useJUnitPlatform()
+    environment("PROJECT_DIR", projectDir.absolutePath)
 }
diff --git a/maestro-orchestra/src/main/java/maestro/orchestra/error/ValidationError.kt b/maestro-orchestra/src/main/java/maestro/orchestra/error/ValidationError.kt
@@ -0,0 +1,3 @@
+package maestro.orchestra.error
+
+open class ValidationError(override val message: String) : RuntimeException()
diff --git a/maestro-orchestra/src/main/java/maestro/orchestra/workspace/WorkspaceExecutionPlanner.kt b/maestro-orchestra/src/main/java/maestro/orchestra/workspace/WorkspaceExecutionPlanner.kt
@@ -1,15 +1,13 @@
 package maestro.orchestra.workspace
 
+import maestro.orchestra.MaestroCommand
 import maestro.orchestra.MaestroConfig
 import maestro.orchestra.WorkspaceConfig
+import maestro.orchestra.error.ValidationError
 import maestro.orchestra.yaml.YamlCommandReader
 import java.nio.file.Files
 import java.nio.file.Path
-import kotlin.io.path.exists
-import kotlin.io.path.isRegularFile
-import kotlin.io.path.name
-import kotlin.io.path.nameWithoutExtension
-import kotlin.io.path.pathString
+import kotlin.io.path.*
 import kotlin.streams.toList
 
 object WorkspaceExecutionPlanner {
@@ -19,12 +17,30 @@ object WorkspaceExecutionPlanner {
         includeTags: List<String>,
         excludeTags: List<String>,
     ): ExecutionPlan {
+        if (input.notExists()) {
+            throw ValidationError("""
+                Flow path does not exist: ${input.absolutePathString()}
+            """.trimIndent())
+        }
+
         if (input.isRegularFile()) {
+            validateFlowFile(input)
             return ExecutionPlan(
                 flowsToRun = listOf(input),
             )
         }
 
+        // retrieve all Flow files
+
+        val unfilteredFlowFiles = Files.walk(input).filter(this::isFlowFile).toList()
+        if (unfilteredFlowFiles.isEmpty()) {
+            throw ValidationError("""
+                Flow directory does not contain any Flow files: ${input.absolutePathString()}
+            """.trimIndent())
+        }
+
+        // Filter flows based on flows config
+
         val workspaceConfig = findConfigFile(input)
             ?.let { YamlCommandReader.readWorkspaceConfig(it) }
             ?: WorkspaceConfig()
@@ -36,41 +52,48 @@ object WorkspaceExecutionPlanner {
                 input.fileSystem.getPathMatcher("glob:${input.pathString}/$it")
             }
 
-        val globalIncludeTags = workspaceConfig.includeTags?.toList() ?: emptyList()
-        val globalExcludeTags = workspaceConfig.excludeTags?.toList() ?: emptyList()
-
-        // retrieve all Flow files
-        val unsortedFlowFiles = Files.walk(input)
+        val unsortedFlowFiles = unfilteredFlowFiles
             .filter { path ->
                 matchers.any { matcher -> matcher.matches(path) }
             }
             .toList()
-            .filter { it.nameWithoutExtension != "config" }
-            .filter {
-                it.isRegularFile()
-                    && (
-                    it.name.endsWith(".yaml")
-                        || it.name.endsWith(".yml")
-                    )
+
+        if (unsortedFlowFiles.isEmpty()) {
+            if ("*" == globs.singleOrNull()) {
+                throw ValidationError("""
+                    Top-level directory does not contain any Flows: ${input.absolutePathString()}
+                    To configure Maestro to run Flows in subdirectories, check out the following resources:
+                      * https://maestro.mobile.dev/cli/test-suites-and-reports#inclusion-patterns
+                      * https://blog.mobile.dev/maestro-best-practices-structuring-your-test-suite-54ec390c5c82
+                """.trimIndent())
+            } else {
+                throw ValidationError("Flow inclusion pattern(s) did not match any Flow files:\n${toYamlListString(globs)}")
             }
+        }
+
+        // Filter flows based on tags
 
-        // retrieve config for each Flow file
         val configPerFlowFile = unsortedFlowFiles.associateWith {
-            val commands = YamlCommandReader.readCommands(it)
+            val commands = validateFlowFile(it)
             YamlCommandReader.getConfig(commands)
         }
 
-        // filter out all Flows not matching tags if present
+        val allIncludeTags = includeTags + (workspaceConfig.includeTags?.toList() ?: emptyList())
+        val allExcludeTags = excludeTags + (workspaceConfig.excludeTags?.toList() ?: emptyList())
         val allFlows = unsortedFlowFiles.filter {
             val config = configPerFlowFile[it]
             val tags = config?.tags ?: emptyList()
 
-            (includeTags.isEmpty() || tags.any(includeTags::contains))
-                && (globalIncludeTags.isEmpty() || tags.any(globalIncludeTags::contains))
-                && (excludeTags.isEmpty() || !tags.any(excludeTags::contains))
-                && (globalExcludeTags.isEmpty() || !tags.any(globalExcludeTags::contains))
+            (allIncludeTags.isEmpty() || tags.any(allIncludeTags::contains))
+                && (allExcludeTags.isEmpty() || !tags.any(allExcludeTags::contains))
+        }
+
+        if (allFlows.isEmpty()) {
+            throw ValidationError("Include / Exclude tags did not match any Flows:\n\nInclude Tags:\n${toYamlListString(allIncludeTags)}\n\nExclude Tags:\n${toYamlListString(allExcludeTags)}")
         }
 
+        // Handle sequential execution
+
         val flowsToRunInSequence = getFlowsToRunInSequence(allFlows, configPerFlowFile, workspaceConfig) ?: emptyList()
         var normalFlows = allFlows - flowsToRunInSequence.toSet()
 
@@ -89,6 +112,10 @@ object WorkspaceExecutionPlanner {
         )
     }
 
+    private fun validateFlowFile(topLevelFlowPath: Path): List<MaestroCommand> {
+        return YamlCommandReader.readCommands(topLevelFlowPath)
+    }
+
     private fun findConfigFile(input: Path): Path? {
         return input.resolve("config.yaml")
             .takeIf { it.exists() }
@@ -119,6 +146,18 @@ object WorkspaceExecutionPlanner {
         return null
     }
 
+    private fun isFlowFile(path: Path): Boolean {
+        if (!path.isRegularFile()) return false // Not a file
+        val extension = path.extension
+        if (extension != "yaml" && extension != "yml") return false // Not YAML
+        if (path.nameWithoutExtension == "config") return false // Config file
+        return true
+    }
+
+    private fun toYamlListString(strings: List<String>): String {
+        return strings.joinToString("\n") { "- $it" }
+    }
+
     data class FlowSequence(
         val flows: List<Path>,
         val continueOnFailure: Boolean? = true
@@ -128,5 +167,4 @@ object WorkspaceExecutionPlanner {
         val flowsToRun: List<Path>,
         val sequence: FlowSequence? = null
     )
-
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		package maestro.orchestra.error

		open class ValidationError(override val message: String) : RuntimeException()