Improve error messages #780

adamfilipow92 · 2020-05-13T15:11:24Z

Fixes #656

Checklist

Documented
Unit tested
release_notes.md updated

bootstraponline · 2020-05-14T05:15:15Z

test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt

    val autoGoogleLogin = cli?.autoGoogleLogin ?: cli?.noAutoGoogleLogin?.not() ?: androidGcloud.autoGoogleLogin

    // We use not() on noUseOrchestrator because if the flag is on, useOrchestrator needs to be false
    val useOrchestrator = cli?.useOrchestrator ?: cli?.noUseOrchestrator?.not() ?: androidGcloud.useOrchestrator
-    val roboDirectives = cli?.roboDirectives?.parseRoboDirectives() ?: androidGcloud.roboDirectives.parseRoboDirectives()
+    val roboDirectives =
+        cli?.roboDirectives?.parseRoboDirectives() ?: androidGcloud.roboDirectives.parseRoboDirectives()


Is there a reason why the formatting is being changed here?

It's auto code format change. Before save file I always use auto code format. If it wrong or I shouldn't commit this changes let me know, I can revert it. :)

Auto wrapping isn't works well in many cases i prefer to do it by hand. Its also good idea to expand setting General -> Code Style -> Hard wrap at to 200 for example, to avoid accidental wrappings.

Thanks! Now i set Hard wrap to 200 i revert changes.

bootstraponline · 2020-05-14T05:17:14Z

test_runner/src/test/kotlin/ftl/args/yml/ErrorParserTest.kt

+  "test" : "./src/test/kotlin/ftl/fixtures/tmp/apk/app-single-success-debug-androidTest.apk",
+  "device" : {
+    "version" : "test"
+  }


The new error format looks awesome!

jan-goral

Pls revert accidental wrappings, intellij is not doing it well automatically. Anyway great job!

jan-goral · 2020-05-14T13:21:21Z

test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt

    val autoGoogleLogin = cli?.autoGoogleLogin ?: cli?.noAutoGoogleLogin?.not() ?: androidGcloud.autoGoogleLogin

    // We use not() on noUseOrchestrator because if the flag is on, useOrchestrator needs to be false
    val useOrchestrator = cli?.useOrchestrator ?: cli?.noUseOrchestrator?.not() ?: androidGcloud.useOrchestrator
-    val roboDirectives = cli?.roboDirectives?.parseRoboDirectives() ?: androidGcloud.roboDirectives.parseRoboDirectives()
+    val roboDirectives =
+        cli?.roboDirectives?.parseRoboDirectives() ?: androidGcloud.roboDirectives.parseRoboDirectives()


Auto wrapping isn't works well in many cases i prefer to do it by hand. Its also good idea to expand setting General -> Code Style -> Hard wrap at to 200 for example, to avoid accidental wrappings.

jan-goral · 2020-05-14T13:27:29Z

test_runner/src/main/kotlin/ftl/args/yml/errors/ConfigurationErrorMessageBuilder.kt

+import com.fasterxml.jackson.databind.JsonNode
+import java.lang.Exception
+
+class ConfigurationErrorMessageBuilder {


Minor suggestion. This class has no state and no constructor arguments, there is no reasons to create new instances. It could be object or even function.

Thanks for this! I made changes :)

jan-goral · 2020-05-14T13:29:45Z

test_runner/src/main/kotlin/ftl/args/yml/errors/ConfigurationErrorParser.kt

@@ -0,0 +1,30 @@
+package ftl.args.yml.errors
+
+internal class ConfigurationErrorParser {


no state, no constructor args, no needs for class.

Thanks for this! I made changes :)

jan-goral · 2020-05-14T13:30:13Z

test_runner/src/main/kotlin/ftl/args/yml/errors/ErrorNodeResolver.kt

+
+import com.fasterxml.jackson.databind.JsonNode
+
+internal class ErrorNodeResolver {


no state, no constructor args, no needs for class :).

Thanks for this! I made changes :)

pawelpasterz · 2020-05-14T20:06:38Z

Hey @adamfilipow92 great job with! Error messages look much better now.
I am wondering if we could cover few more cases within this PR (all of them are still related with yaml parsing. (for clarity _ means space white char.)

gcloud:
  app: /my/path/to/app-debug.apk
  _test: /my/path/to/app-debug-androidTest.apk

results with nasty error with stack trace like

com.fasterxml.jackson.dataformat.yaml.snakeyaml.error.MarkedYAMLException: mapping values are not allowed here
 in 'reader', line 3, column 8:
       test: /my/path/to/app-debug-androidT ...
           ^

 at [Source: (BufferedReader); line: 3, column: 8]
	at com.fasterxml.jackson.dataformat.yaml.snakeyaml.error.MarkedYAMLException.from(MarkedYAMLException.java:27)
	at com.fasterxml.jackson.dataformat.yaml.YAMLParser.nextToken(YAMLParser.java:359)
	at com.fasterxml.jackson.core.JsonParser.nextFieldName(JsonParser.java:861)
	at com.fasterxml.jackson.databind.deser.std.BaseNodeDeserializer.deserializeObject(JsonNodeDeserializer.java:250)
	at com.fasterxml.jackson.databind.deser.std.BaseNodeDeserializer.deserializeObject(JsonNodeDeserializer.java:258)
	at com.fasterxml.jackson.databind.deser.std.JsonNodeDeserializer.deserialize(JsonNodeDeserializer.java:68)
	at com.fasterxml.jackson.databind.deser.std.JsonNodeDeserializer.deserialize(JsonNodeDeserializer.java:15)
	at com.fasterxml.jackson.databind.ObjectMapper._readTreeAndClose(ObjectMapper.java:4254)
	at com.fasterxml.jackson.databind.ObjectMapper.readTree(ObjectMapper.java:2701)
	at ftl.args.yml.YamlDeprecated.modify$flank(YamlDeprecated.kt:150)
	at ftl.args.yml.YamlDeprecated.modifyAndThrow(YamlDeprecated.kt:138)
	at ftl.args.AndroidArgs$Companion.load$flank(AndroidArgs.kt:210)
	at ftl.args.AndroidArgs$Companion.load(AndroidArgs.kt:205)
	at ftl.cli.firebase.test.android.AndroidRunCommand.run(AndroidRunCommand.kt:45)
	at picocli.CommandLine.executeUserObject(CommandLine.java:1769)
	at picocli.CommandLine.access$900(CommandLine.java:145)
	at picocli.CommandLine$RunLast.executeUserObjectOfLastSubcommandWithSameParent(CommandLine.java:2150)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2144)
	at picocli.CommandLine$RunLast.handle(CommandLine.java:2108)
	at picocli.CommandLine$AbstractParseResultHandler.execute(CommandLine.java:1975)
	at picocli.CommandLine.execute(CommandLine.java:1904)
	at ftl.Main$Companion$main$1.invoke(Main.kt:53)
	at ftl.Main$Companion$main$1.invoke(Main.kt:43)
	at ftl.util.Utils.withGlobalExceptionHandling(Utils.kt:132)
	at ftl.Main$Companion.main(Main.kt:49)
	at ftl.Main.main(Main.kt)
Caused by: mapping values are not allowed here
 in 'reader', line 3, column 8:
       test: /my/path/to/app-debug-androidT ...
           ^

	at org.yaml.snakeyaml.scanner.ScannerImpl.fetchValue(ScannerImpl.java:869)
	at org.yaml.snakeyaml.scanner.ScannerImpl.fetchMoreTokens(ScannerImpl.java:358)
	at org.yaml.snakeyaml.scanner.ScannerImpl.checkToken(ScannerImpl.java:227)
	at org.yaml.snakeyaml.parser.ParserImpl$ParseBlockMappingKey.produce(ParserImpl.java:558)
	at org.yaml.snakeyaml.parser.ParserImpl.peekEvent(ParserImpl.java:158)
	at org.yaml.snakeyaml.parser.ParserImpl.getEvent(ParserImpl.java:168)
	at com.fasterxml.jackson.dataformat.yaml.YAMLParser.nextToken(YAMLParser.java:355)

Error with wrong type of data like

gcloud:
  app: /my/path/to/app-debug.apk
  tests: /my/path/to/app-debug-androidTest.apk
  environment-variables: "123, 12312, 12312"

results with

ftl.util.FlankConfigurationException: Error on parse config: gcloud
At line: 5, column: 26
Error node: {
  "gcloud" : {
    "app" : "/my/path/to/app-debug.apk",
    "tests" : "/my/path/to/app-debug-androidTest.apk",
    "environment-variables" : "123, 12312, 12312"
  },
  "flank" : { }
}

which is not very useful for me since I still don't know what is wrong (yeah, in this particular example it's rather clear)

On the other hand flank is not for casual users and every dev should now basics of yml files...
Those are my loose thoughts, not change request. Again, great job!

cc
@bootstraponline @jan-gogo

Let me all know what you think.

bootstraponline · 2020-05-14T20:17:52Z

I am wondering if we could cover few more cases within this PR (all of them are still related with yaml parsing. (for clarity _ means space white char.)

I think these would be great to address in follow up PRs. They're definitely improvements we want to make. I like to have small pull requests that get merged quickly. The longer a pull request stays open, the higher the cost to maintain and deal with merge conflicts.

https://google.github.io/eng-practices/review/developer/small-cls.html

adamfilipow92 · 2020-05-15T06:44:39Z

environment-variables: "123, 12312, 12312"

Really thanks for tests! My regex to for get "clean" reference chain was ignoring properties with '-' in name. I fix it and now in case with that property we have better informations. Like this:
ftl.util.FlankConfigurationException: Error on parse config: gcloud->environment-variables At line: 5, column: 26 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", "environment-variables" : "123, 12312, 12312", ...

adamfilipow92 · 2020-05-15T07:01:47Z

I am wondering if we could cover few more cases within this PR (all of them are still related with yaml parsing. (for clarity _ means space white char.)

I think these would be great to address in follow up PRs. They're definitely improvements we want to make. I like to have small pull requests that get merged quickly. The longer a pull request stays open, the higher the cost to maintain and deal with merge conflicts.

https://google.github.io/eng-practices/review/developer/small-cls.html

@bootstraponline I want make some more improvments to this issue. Based of @pawelpasterz suggestion handle more cases. Even if messages of them are better we could make some changes and convert them to one generic excepion for parsing errors like FlankConfigurationException.
Next potential case to handle is not displaying full node on error in top nodes like gcloud. We can do something like:

`
ftl.util.FlankConfigurationException: Error on parse config: gcloud->environment-variables At line: 5, column: 26 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", "environment-variables" : "123, 12312, 12312"...
^

`
cc
@jan-gogo @pawelpasterz
Let me all know what you think.

pawelpasterz · 2020-05-15T11:23:43Z

@adamfilipow92 Fine for me, let's create separate issue to track progress

Improve error messages

d0e4e34

adamfilipow92 requested a review from jan-goral May 13, 2020 15:25

bootstraponline reviewed May 14, 2020

View reviewed changes

bootstraponline requested a review from pawelpasterz May 14, 2020 05:17

jan-goral reviewed May 14, 2020

View reviewed changes

adamfilipow92 and others added 2 commits May 14, 2020 16:07

No state no class, fix wrong auto formating

75ad119

Merge branch 'master' into #656-improve-error-message-reporting

c20af7f

jan-goral previously approved these changes May 14, 2020

View reviewed changes

Add support for get full chain with properties contains '-' in name

2be1c47

adamfilipow92 dismissed jan-goral’s stale review via 2be1c47 May 15, 2020 06:40

adamfilipow92 requested review from bootstraponline and jan-goral May 15, 2020 06:45

pawelpasterz approved these changes May 15, 2020

View reviewed changes

jan-goral approved these changes May 15, 2020

View reviewed changes

adamfilipow92 merged commit 9eb491d into master May 15, 2020

adamfilipow92 deleted the #656-improve-error-message-reporting branch May 15, 2020 14:04

adamfilipow92 mentioned this pull request May 15, 2020

More improvments in error messages #787

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error messages #780

Improve error messages #780

adamfilipow92 commented May 13, 2020

bootstraponline May 14, 2020

adamfilipow92 May 14, 2020

jan-goral May 14, 2020

adamfilipow92 May 14, 2020

bootstraponline May 14, 2020

jan-goral left a comment

jan-goral May 14, 2020

jan-goral May 14, 2020 •

edited

Loading

adamfilipow92 May 14, 2020

jan-goral May 14, 2020

adamfilipow92 May 14, 2020

jan-goral May 14, 2020

adamfilipow92 May 14, 2020

pawelpasterz commented May 14, 2020

bootstraponline commented May 14, 2020

adamfilipow92 commented May 15, 2020

adamfilipow92 commented May 15, 2020

pawelpasterz commented May 15, 2020

		@@ -0,0 +1,30 @@
		package ftl.args.yml.errors

		internal class ConfigurationErrorParser {


		import com.fasterxml.jackson.databind.JsonNode

		internal class ErrorNodeResolver {

Improve error messages #780

Improve error messages #780

Conversation

adamfilipow92 commented May 13, 2020

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-goral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-goral May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelpasterz commented May 14, 2020

bootstraponline commented May 14, 2020

adamfilipow92 commented May 15, 2020

adamfilipow92 commented May 15, 2020

pawelpasterz commented May 15, 2020

jan-goral May 14, 2020 •

edited

Loading