Skip to content

Commit

Permalink
Improve error messages (#780)
Browse files Browse the repository at this point in the history
* Improve error messages

* No state no class, fix wrong auto formating

* Add support for get full chain with properties contains '-' in name
  • Loading branch information
adamfilipow92 authored May 15, 2020
1 parent 2b82053 commit 9eb491d
Show file tree
Hide file tree
Showing 19 changed files with 353 additions and 10 deletions.
2 changes: 2 additions & 0 deletions release_notes.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
## next (unreleased)

- [#656](https://github.com/Flank/flank/issues/656) Improve error message reporting. ([adamfilipow92](https://github.com/adamfilipow92))
- [#783](https://github.com/Flank/flank/pull/783) Use legacy results for iOS by default. ([pawelpasterz](https://github.com/pawelpasterz))

## v20.05.1
Expand Down
7 changes: 4 additions & 3 deletions test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ import ftl.args.ArgsToString.mapToString
import ftl.args.ArgsToString.objectsToString
import ftl.args.yml.AndroidFlankYml
import ftl.args.yml.AndroidGcloudYml
import ftl.args.yml.AndroidGcloudYmlParams
import ftl.args.yml.AppTestPair
import ftl.args.yml.FlankYml
import ftl.args.yml.GcloudYml
import ftl.args.yml.AppTestPair
import ftl.args.yml.AndroidGcloudYmlParams
import ftl.args.yml.YamlDeprecated
import ftl.cli.firebase.test.android.AndroidRunCommand
import ftl.config.Device
Expand All @@ -43,6 +43,7 @@ class AndroidArgs(
override val data: String,
val cli: AndroidRunCommand? = null
) : IArgs {

private val gcloud = gcloudYml.gcloud
override val resultsBucket: String
override val resultsDir = (cli?.resultsDir ?: gcloud.resultsDir)?.also { assertFileExists(it, "from results-dir") }
Expand Down Expand Up @@ -193,8 +194,8 @@ AndroidArgs

@VisibleForTesting
internal fun load(yamlReader: Reader, cli: AndroidRunCommand? = null): AndroidArgs {
val data = YamlDeprecated.modifyAndThrow(yamlReader, android = true)

val data = YamlDeprecated.modifyAndThrow(yamlReader, android = true)
val flankYml = yamlMapper.readValue(data, FlankYml::class.java)
val gcloudYml = yamlMapper.readValue(data, GcloudYml::class.java)
val androidGcloudYml = yamlMapper.readValue(data, AndroidGcloudYml::class.java)
Expand Down
10 changes: 6 additions & 4 deletions test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
package ftl.args

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory
import com.fasterxml.jackson.module.kotlin.KotlinModule
import com.fasterxml.jackson.module.kotlin.registerKotlinModule
import com.google.api.client.json.GenericJson
import com.google.api.client.json.JsonObjectParser
import com.google.api.client.util.Charsets
Expand All @@ -12,6 +11,7 @@ import com.google.cloud.storage.Storage
import com.google.cloud.storage.StorageClass
import com.google.cloud.storage.StorageOptions
import ftl.args.yml.IYmlMap
import ftl.args.yml.YamlObjectMapper
import ftl.config.FtlConstants
import ftl.config.FtlConstants.GCS_PREFIX
import ftl.config.FtlConstants.JSON_FACTORY
Expand All @@ -23,9 +23,9 @@ import ftl.reports.xml.model.JUnitTestResult
import ftl.shard.Shard
import ftl.shard.StringShards
import ftl.shard.stringShards
import ftl.util.FlankFatalError
import ftl.util.FlankTestMethod
import ftl.util.assertNotEmpty
import ftl.util.FlankFatalError
import java.io.File
import java.net.URI
import java.nio.file.Files
Expand All @@ -35,7 +35,9 @@ import java.util.regex.Pattern

object ArgsHelper {

val yamlMapper: ObjectMapper by lazy { ObjectMapper(YAMLFactory()).registerModule(KotlinModule()) }
val yamlMapper: ObjectMapper by lazy {
YamlObjectMapper().registerKotlinModule()
}

fun mergeYmlMaps(vararg ymlMaps: IYmlMap): Map<String, List<String>> {
val result = mutableMapOf<String, List<String>>()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package ftl.args.yml

import com.fasterxml.jackson.databind.JsonNode
import ftl.args.yml.errors.ConfigurationErrorMessageBuilder
import ftl.util.FlankConfigurationException

fun convertConfigurationErrorExceptions(missingParameterError: Exception, yaml: JsonNode): Throwable {
val errorMessageBuilder = ConfigurationErrorMessageBuilder
val errorMessage = missingParameterError.message
return if (errorMessage != null) {
FlankConfigurationException(errorMessageBuilder(errorMessage, yaml))
} else {
missingParameterError
}
}
18 changes: 18 additions & 0 deletions test_runner/src/main/kotlin/ftl/args/yml/YamlObjectMapper.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package ftl.args.yml

import com.fasterxml.jackson.databind.ObjectMapper
import com.fasterxml.jackson.databind.exc.MismatchedInputException
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory
import com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException

class YamlObjectMapper : ObjectMapper(YAMLFactory()) {
override fun <T> readValue(content: String?, valueType: Class<T>?): T {
try {
return readValue(content, _typeFactory.constructType(valueType))
} catch (missingParameterError: MissingKotlinParameterException) {
throw convertConfigurationErrorExceptions(missingParameterError, readTree(content))
} catch (mismatchedInputException: MismatchedInputException) {
throw convertConfigurationErrorExceptions(mismatchedInputException, readTree(content))
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package ftl.args.yml.errors

import com.fasterxml.jackson.databind.JsonNode
import java.lang.Exception

object ConfigurationErrorMessageBuilder {

private val parseMessage = ConfigurationErrorParser
private val resolveErrorNode = ErrorNodeResolver

//region error message elements
private const val messageHeader = "Error on parse config: "
private const val missingElementMessage = "Missing element or value for: '%s'"
private const val atMessage = "At line: %s, column: %s"
private const val errorNodeMessage = "Error node: %s"
//endregion

private const val exceptionTemplate = "Parse message error: %s"

operator fun invoke(errorMessage: String, yamlTreeNode: JsonNode? = null) =
try {
val errorModel = parseMessage(errorMessage)
val errorMessageBuilder = StringBuilder(messageHeader)
errorMessageBuilder.appendln(createReferenceChain(errorModel.referenceChain))
if (errorModel.propertyName != "") {
errorMessageBuilder.appendln(missingElementMessage.format(errorModel.propertyName))
}
errorMessageBuilder.appendln(atMessage.format(errorModel.line, errorModel.column))
yamlTreeNode?.let {
errorMessageBuilder.appendln(errorNodeMessage.format(resolveErrorNode(yamlTreeNode, errorModel)))
}
errorMessageBuilder.toString().trim()
} catch (error: Exception) {
exceptionTemplate.format(errorMessage)
}

private fun createReferenceChain(referenceChain: List<String>): String {
val chainBuilder = StringBuilder()
referenceChain.forEachIndexed { index, chainPart ->
chainBuilder.append(appendChainElement(chainPart, index > 0))
}
return chainBuilder.toString()
}

private fun appendChainElement(chainPart: String, withSeparator: Boolean): String = when {
chainPart.toIntOrNull() != null -> "[$chainPart]"
withSeparator -> "->$chainPart"
else -> chainPart
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package ftl.args.yml.errors

internal data class ConfigurationErrorModel(val propertyName: String, val line: Int, val column: Int, val referenceChain: List<String>)
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package ftl.args.yml.errors

internal object ConfigurationErrorParser {

//region regex patterns
private val propertyNameRegex = "(?<=property\\s)[a-z]*".toRegex()
private val referenceChainRegex = "(?<=chain:\\s).*(?=[)])".toRegex()
private val referenceChainCleanUpRegex = "(?<=[\\[])\"?[\\w]*\"?(?=])".toRegex()
private val lineAndColumnRegex = "((?<=line:\\s)\\d*), column:\\s(\\d*)".toRegex()
//endregion

operator fun invoke(errorMessage: String): ConfigurationErrorModel {
val (line, column) = parseErrorPositionLine(errorMessage)
return ConfigurationErrorModel(
parsePropertyName(errorMessage),
line.toInt(),
column.toInt(),
parseReferenceChain(errorMessage)
)
}

private fun parsePropertyName(errorMessage: String) = propertyNameRegex.find(errorMessage)?.value ?: ""
private fun parseErrorPositionLine(errorMessage: String) = lineAndColumnRegex.find(errorMessage)!!.destructured

private fun parseReferenceChain(errorMessage: String) =
cleanUpReferenceChain(referenceChainRegex.find(errorMessage)!!.value)

private fun cleanUpReferenceChain(referenceChain: String): List<String> =
referenceChainCleanUpRegex.findAll(referenceChain).map { it.value.replace("\"", "") }.toList()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package ftl.args.yml.errors

import com.fasterxml.jackson.databind.JsonNode

internal object ErrorNodeResolver {
operator fun invoke(treeNode: JsonNode, errorModel: ConfigurationErrorModel): String {
var currentNode: JsonNode = treeNode

val lastNode = errorModel.referenceChain.last()
for (chainNode in errorModel.referenceChain) {
if (chainNode == lastNode) {
break
}
val nodeAsIntValue = chainNode.toIntOrNull()
currentNode = if (nodeAsIntValue != null) {
currentNode[nodeAsIntValue]
} else {
currentNode[chainNode]
}
}
return currentNode.toPrettyString()
}
}
9 changes: 9 additions & 0 deletions test_runner/src/main/kotlin/ftl/util/FlankException.kt
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,12 @@ class FlankFatalError(message: String) : FlankException(message)
* @param message [String] message to be printed to [System.err]
*/
class FlankCommonException(message: String) : FlankException(message)

/**
* Exception throws when required parameters is missing
*
* Exit code: 1
*
* @param message [String] message to be printed to [System.err]
*/
class FlankConfigurationException(message: String) : FlankException(message)
12 changes: 10 additions & 2 deletions test_runner/src/main/kotlin/ftl/util/Utils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ fun withGlobalExceptionHandling(block: () -> Int) {
}
exitProcess(1)
}
is FlankConfigurationException -> {
System.err.println(t)
exitProcess(1)
}
is FTLError -> {
t.matrix.logError("not finished")
exitProcess(3)
Expand All @@ -159,6 +163,7 @@ fun withGlobalExceptionHandling(block: () -> Int) {
System.err.println(t.message)
exitProcess(2)
}

// We need to cover the case where some component in the call stack starts a non-daemon
// thread, and then throws an Error that kills the main thread. This is extra safe implementation
else -> {
Expand All @@ -183,6 +188,9 @@ fun <R : MutableMap<String, Any>, T> mutableMapProperty(
defaultValue: () -> T
) = object : ReadWriteProperty<R, T> {
@Suppress("UNCHECKED_CAST")
override fun getValue(thisRef: R, property: KProperty<*>): T = thisRef.getOrElse(name ?: property.name, defaultValue) as T
override fun setValue(thisRef: R, property: KProperty<*>, value: T) = thisRef.set(name ?: property.name, value as Any)
override fun getValue(thisRef: R, property: KProperty<*>): T =
thisRef.getOrElse(name ?: property.name, defaultValue) as T

override fun setValue(thisRef: R, property: KProperty<*>, value: T) =
thisRef.set(name ?: property.name, value as Any)
}
2 changes: 1 addition & 1 deletion test_runner/src/test/kotlin/Debug.kt
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ fun main() {
val projectId = System.getenv("GOOGLE_CLOUD_PROJECT")
?: "YOUR PROJECT ID"
val quantity = "multiple"
val type = "error"
val type = "parse-error"

// Bugsnag keeps the process alive so we must call exitProcess
// https://github.com/bugsnag/bugsnag-java/issues/151
Expand Down
90 changes: 90 additions & 0 deletions test_runner/src/test/kotlin/ftl/args/yml/ErrorParserTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package ftl.args.yml

import ftl.args.AndroidArgs
import ftl.args.yml.errors.ConfigurationErrorMessageBuilder
import ftl.test.util.TestHelper
import ftl.test.util.TestHelper.getThrowable
import ftl.util.FlankConfigurationException
import org.junit.Assert
import org.junit.Test

class ErrorParserTest {
private val yamlWithoutDeviceVersion =
TestHelper.getPath("src/test/kotlin/ftl/args/yml/test_error_yaml_cases/flank-no-device-version.yml")
private val yamlNoModelName =
TestHelper.getPath("src/test/kotlin/ftl/args/yml/test_error_yaml_cases/flank-no-model-name.yml")
private val yamlNoModelNode =
TestHelper.getPath("src/test/kotlin/ftl/args/yml/test_error_yaml_cases/flank-no-model-node.yml")

@Test
fun `parse json mapping error`() {
val instantionError =
"Instantiation of [simple type, class ftl.config.Device] value failed for JSON property version due to missing (therefore NULL) value for creator parameter version which is a non-nullable type\n" +
" at [Source: (StringReader); line: 23, column: 3] (through reference chain: ftl.args.yml.AndroidGcloudYml[\"gcloud\"]->ftl.args.yml.AndroidGcloudYmlParams[\"device\"]->java.util.ArrayList[4]->ftl.config.Device[\"version\"])"

val expected = """
Error on parse config: gcloud->device[4]->version
Missing element or value for: 'version'
At line: 23, column: 3
""".trimIndent()
val buildErrorMessage = ConfigurationErrorMessageBuilder
Assert.assertEquals(expected, buildErrorMessage(instantionError))
}

@Test
fun `return exception with inner message on parse error`() {
val instantionError =
"Instantiation oflParams[\"device\"]->java.util.A"
val expected = "Parse message error: Instantiation oflParams[\"device\"]->java.util.A".trimIndent()
val buildErrorMessage = ConfigurationErrorMessageBuilder

Assert.assertEquals(expected, buildErrorMessage(instantionError))
}

@Test(expected = FlankConfigurationException::class)
fun `should throw FlankConfigException without device version`() {
AndroidArgs.load(yamlWithoutDeviceVersion)
}

@Test
fun `without model name should have message`() {
val actualMessage = getThrowable { AndroidArgs.load(yamlNoModelName) }.message
val exceptedMessage = """
Error on parse config: gcloud->device[0]->model
Missing element or value for: 'model'
At line: 8, column: 1
Error node: {
"model" : null,
"version" : "test"
}
""".trimIndent()
Assert.assertEquals(exceptedMessage, actualMessage)
}

@Test
fun `without model node should have message`() {
val actualMessage = getThrowable { AndroidArgs.load(yamlNoModelNode) }.message
val exceptedMessage = """
Error on parse config: gcloud->device
At line: 6, column: 5
Error node: {
"app" : "./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk",
"test" : "./src/test/kotlin/ftl/fixtures/tmp/apk/app-single-success-debug-androidTest.apk",
"device" : {
"version" : "test"
}
}
""".trimIndent()
Assert.assertEquals(exceptedMessage, actualMessage)
}

@Test(expected = FlankConfigurationException::class)
fun `should throw FlankConfigException without model name`() {
AndroidArgs.load(yamlNoModelName)
}

@Test(expected = FlankConfigurationException::class)
fun `should throw FlankConfigException without model node`() {
AndroidArgs.load(yamlNoModelNode)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
gcloud:
app: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug.apk
test: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-single-success-debug-androidTest.apk
device:
- model: NexusLowRes
version: 23
- model: NexusLowRes
version: 23
orientation: landscape
- model: shamu
version: 22
locale: zh_CN
orientation: default
# Google Pixel 3
- model: blueline
version: 28
locale: en
orientation: portrait
# Samsung Galaxy S9 SM-G9600
- model: starqltechn
versxion: 28
locale: en
orientation: portrait
# LG Nexus 5
- model: hammerhead
version: 21
locale: en
orientation: portrait
flank:
disable-sharding: true

Loading

0 comments on commit 9eb491d

Please sign in to comment.