From 7ba733553f5fe35cfd902d61ae100e128df05d8f Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Thu, 30 Jul 2020 20:54:00 +0200 Subject: [PATCH 1/3] #911 Added preload configuration to detekt .yml issues by Doctor --- test_runner/src/main/kotlin/ftl/doctor/Doctor.kt | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt b/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt index ad2689f80b..3cf19d67fc 100644 --- a/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt +++ b/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt @@ -1,8 +1,11 @@ package ftl.doctor import com.google.common.annotations.VisibleForTesting +import ftl.args.AndroidArgsCompanion import ftl.args.ArgsHelper import ftl.args.IArgs +import ftl.config.loadAndroidConfig +import ftl.config.loadIosConfig import ftl.util.loadFile import java.io.Reader import java.nio.file.Path @@ -10,7 +13,7 @@ import java.nio.file.Path object Doctor { fun validateYaml(args: IArgs.ICompanion, data: Path): String { if (!data.toFile().exists()) return "Skipping yaml validation. No file at path $data" - return validateYaml(args, loadFile(data)) + return validateYaml(args, loadFile(data)) + preloadConfiguration(data, args is AndroidArgsCompanion) } @VisibleForTesting @@ -41,4 +44,11 @@ object Doctor { return result } + + private fun preloadConfiguration(data: Path, isAndroid: Boolean) = try { + if (isAndroid) loadAndroidConfig(data) else loadIosConfig(data) + "" + } catch (e: Exception) { + e.message + } } From 013bcb9ef41ee11a17f69d97c59a7bdc8b0399a2 Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Fri, 31 Jul 2020 16:55:55 +0200 Subject: [PATCH 2/3] #911 Refactor Doctor and add json tree validation --- release_notes.md | 1 + .../test/android/AndroidDoctorCommand.kt | 2 +- .../cli/firebase/test/ios/IosDoctorCommand.kt | 2 +- .../src/main/kotlin/ftl/doctor/Doctor.kt | 72 +++++++++++-------- .../src/test/kotlin/ftl/doctor/DoctorTest.kt | 59 +++++++++++++-- .../flank_android_failed_configuration.yml | 24 +++++++ .../fixtures/flank_android_failed_tree.yml | 24 +++++++ 7 files changed, 144 insertions(+), 40 deletions(-) create mode 100644 test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_configuration.yml create mode 100644 test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_tree.yml diff --git a/release_notes.md b/release_notes.md index 61681d9f99..72b241b1e7 100644 --- a/release_notes.md +++ b/release_notes.md @@ -14,6 +14,7 @@ - [#916](https://github.com/Flank/flank/pull/916) Test artifacts monorepo. ([jan-gogo](https://github.com/jan-gogo)) - [#910](https://github.com/Flank/flank/pull/910) Migrate Bitrise release workflow into GitHub actions. ([piotradamczyk5](https://github.com/piotradamczyk5)) - [#915](https://github.com/Flank/flank/pull/915) Update virtual devices sharding limit. ([adamfilipow92](https://github.com/adamfilipow92)) +- [#920](https://github.com/Flank/flank/pull/920) Improve .yml validation on `doctor` command. ([piotradamczyk5](https://github.com/piotradamczyk5)) - - diff --git a/test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidDoctorCommand.kt b/test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidDoctorCommand.kt index 372ce3c4b3..18e6253f9c 100644 --- a/test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidDoctorCommand.kt +++ b/test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidDoctorCommand.kt @@ -3,7 +3,7 @@ package ftl.cli.firebase.test.android import ftl.args.AndroidArgs import ftl.cli.firebase.test.processValidation import ftl.config.FtlConstants -import ftl.doctor.Doctor.validateYaml +import ftl.doctor.validateYaml import java.nio.file.Paths import picocli.CommandLine.Command import picocli.CommandLine.Option diff --git a/test_runner/src/main/kotlin/ftl/cli/firebase/test/ios/IosDoctorCommand.kt b/test_runner/src/main/kotlin/ftl/cli/firebase/test/ios/IosDoctorCommand.kt index c15856f7a0..bf15628d58 100644 --- a/test_runner/src/main/kotlin/ftl/cli/firebase/test/ios/IosDoctorCommand.kt +++ b/test_runner/src/main/kotlin/ftl/cli/firebase/test/ios/IosDoctorCommand.kt @@ -3,7 +3,7 @@ package ftl.cli.firebase.test.ios import ftl.args.IosArgs import ftl.cli.firebase.test.processValidation import ftl.config.FtlConstants -import ftl.doctor.Doctor.validateYaml +import ftl.doctor.validateYaml import java.nio.file.Paths import picocli.CommandLine.Command import picocli.CommandLine.Option diff --git a/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt b/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt index 3cf19d67fc..ed76f711c7 100644 --- a/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt +++ b/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt @@ -1,54 +1,64 @@ package ftl.doctor +import com.fasterxml.jackson.databind.JsonNode import com.google.common.annotations.VisibleForTesting import ftl.args.AndroidArgsCompanion import ftl.args.ArgsHelper import ftl.args.IArgs import ftl.config.loadAndroidConfig import ftl.config.loadIosConfig +import ftl.util.FlankFatalError import ftl.util.loadFile import java.io.Reader +import java.lang.StringBuilder import java.nio.file.Path -object Doctor { - fun validateYaml(args: IArgs.ICompanion, data: Path): String { - if (!data.toFile().exists()) return "Skipping yaml validation. No file at path $data" - return validateYaml(args, loadFile(data)) + preloadConfiguration(data, args is AndroidArgsCompanion) - } - - @VisibleForTesting - internal fun validateYaml(args: IArgs.ICompanion, data: Reader): String { - var result = "" - val parsed = ArgsHelper.yamlMapper.readTree(data) +fun validateYaml(args: IArgs.ICompanion, data: Path): String { + if (!data.toFile().exists()) return "Skipping yaml validation. No file at path $data" + return validateYaml(args, loadFile(data)) + preloadConfiguration(data, args is AndroidArgsCompanion) +} - val validArgs = args.validArgs - val parsedArgs = mutableMapOf>() +@VisibleForTesting +internal fun validateYaml(args: IArgs.ICompanion, data: Reader) = + runCatching { ArgsHelper.yamlMapper.readTree(data) } + .onFailure { return it.message ?: "Unknown error when parsing tree" } + .getOrNull() + ?.run { validateYamlKeys(args) } + .orEmpty() - for (child in parsed.fields()) { - val key = child.key - val values = mutableListOf() - child.value.fields().forEach { values.add(it.key) } +private fun JsonNode.validateYamlKeys(args: IArgs.ICompanion) = StringBuilder().apply { + append(validateTopLevelKeys(args)) + args.validArgs.forEach { (topLevelKey, validArgsKeys) -> + append(validateNestedKeys(topLevelKey, validArgsKeys)) + } +}.toString() - parsedArgs[key] = values - } +private fun JsonNode.validateTopLevelKeys(args: IArgs.ICompanion) = + (parseArgs().keys - args.validArgs.keys) + .takeIf { it.isNotEmpty() } + ?.let { unknownKeys -> "Unknown top level keys: $unknownKeys\n" } + .orEmpty() - val unknownTopLevelKeys = parsedArgs.keys - validArgs.keys - if (unknownTopLevelKeys.isNotEmpty()) result += "Unknown top level keys: $unknownTopLevelKeys\n" +private fun JsonNode.parseArgs() = mutableMapOf>().apply { + for (child in fields()) { + this[child.key] = child.value.fields().asSequence().map { it.key }.toList() + } +} - validArgs.forEach { (topLevelKey, keyList) -> - val parsedKeys = mutableListOf() - parsed[topLevelKey]?.fields()?.forEach { parsedKeys.add(it.key) } - val unknownKeys = parsedKeys - keyList - if (unknownKeys.isNotEmpty()) result += "Unknown keys in $topLevelKey -> $unknownKeys\n" - } +private fun JsonNode.validateNestedKeys(topLevelKey: String, validArgsKeys: List) = + nestedKeysFor(topLevelKey) + .minus(validArgsKeys) + .takeIf { it.isNotEmpty() } + ?.let { "Unknown keys in $topLevelKey -> $it\n" } + .orEmpty() - return result - } +private fun JsonNode.nestedKeysFor(topLevelKey: String) = + this[topLevelKey]?.fields()?.asSequence()?.map { it.key }?.toList().orEmpty() - private fun preloadConfiguration(data: Path, isAndroid: Boolean) = try { +private fun preloadConfiguration(data: Path, isAndroid: Boolean) = + try { if (isAndroid) loadAndroidConfig(data) else loadIosConfig(data) "" - } catch (e: Exception) { + } catch (e: FlankFatalError) { e.message } -} diff --git a/test_runner/src/test/kotlin/ftl/doctor/DoctorTest.kt b/test_runner/src/test/kotlin/ftl/doctor/DoctorTest.kt index a9e20265f4..686b9df08d 100644 --- a/test_runner/src/test/kotlin/ftl/doctor/DoctorTest.kt +++ b/test_runner/src/test/kotlin/ftl/doctor/DoctorTest.kt @@ -14,13 +14,13 @@ import java.nio.file.Paths class DoctorTest { @Test fun androidDoctorTest() { - val lint = Doctor.validateYaml(AndroidArgs, Paths.get("src/test/kotlin/ftl/fixtures/flank.local.yml")) + val lint = validateYaml(AndroidArgs, Paths.get("src/test/kotlin/ftl/fixtures/flank.local.yml")) assertThat(lint).isEmpty() } @Test fun androidDoctorTest2() { - val lint = Doctor.validateYaml( + val lint = validateYaml( AndroidArgs, """ hi: . foo: @@ -71,7 +71,7 @@ Unknown keys in flank -> [three] @Test fun androidDoctorTest3() { - val lint = Doctor.validateYaml( + val lint = validateYaml( AndroidArgs, """ gcloud: app: . @@ -83,15 +83,60 @@ flank: assertThat(lint).isEqualTo("") } + @Test + fun androidDoctorTestWithFailedConfiguration() { + // given + val expectedErrorMessage = """ +Error on parse config: flank->additional-app-test-apks +At line: 20, column: 5 +Error node: { + "additional-app-test-apks" : { + "app" : "../sample/app/build/output/apk/debug/sample-app.apk", + "test" : "../library/databases/build/output/apk/androidTest/debug/sample-databases-test.apk" + } +} + """.trimIndent() + + // when + val actual = validateYaml( + AndroidArgs, + Paths.get("src/test/kotlin/ftl/fixtures/flank_android_failed_configuration.yml") + ) + assertThat(actual).isEqualTo(expectedErrorMessage) + } + + @Test + fun androidDoctorTestWithFailedTree() { + // given + val expectedErrorMessage = """ + Error on parse config: expected , but found '?' + At line: 19, column: 4 + Error node: + test: ../main/app/build/output/a ... + ^Error on parse config: expected , but found '?' + At line: 19, column: 4 + Error node: + test: ../main/app/build/output/a ... + ^ + """.trimIndent() + + // when + val actual = validateYaml( + AndroidArgs, + Paths.get("src/test/kotlin/ftl/fixtures/flank_android_failed_tree.yml") + ) + assertThat(actual).isEqualTo(expectedErrorMessage) + } + @Test fun iosDoctorTest() { - val lint = Doctor.validateYaml(IosArgs, Paths.get("src/test/kotlin/ftl/fixtures/flank.ios.yml")) + val lint = validateYaml(IosArgs, Paths.get("src/test/kotlin/ftl/fixtures/flank.ios.yml")) assertThat(lint).isEmpty() } @Test fun iosDoctorTest2() { - val lint = Doctor.validateYaml( + val lint = validateYaml( IosArgs, """ hi: . foo: @@ -135,7 +180,7 @@ Unknown keys in flank -> [three] @Test fun iosDoctorTest3() { - val lint = Doctor.validateYaml( + val lint = validateYaml( IosArgs, """ gcloud: test: . @@ -148,4 +193,4 @@ flank: } } -private fun Doctor.validateYaml(args: IArgs.ICompanion, data: String): String = validateYaml(args, StringReader(data)) +private fun validateYaml(args: IArgs.ICompanion, data: String): String = validateYaml(args, StringReader(data)) diff --git a/test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_configuration.yml b/test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_configuration.yml new file mode 100644 index 0000000000..0398c5d538 --- /dev/null +++ b/test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_configuration.yml @@ -0,0 +1,24 @@ +gcloud: + app: /Users/no/workspace/fladle/sample/build/outputs/apk/debug/sample-debug.apk + test: /Users/no/workspace/fladle/sample/build/outputs/apk/androidTest/debug/sample-debug-androidTest.apk + device: + - model: Nexus5 + version: 23 + use-orchestrator: true + auto-google-login: false + record-video: true + performance-metrics: true + timeout: 15m + environment-variables: + clearPackageData: true + test-targets: + - class com.osacky.flank.gradle.sample.ExampleInstrumentedTest#seeView + num-flaky-test-attempts: 1 +flank: + additional-app-test-apks: + app: ../main/app/build/output/apk/debug/app.apk + test: ../main/app/build/output/apk/androidTest/debug/app-test.apk + app: ../sample/app/build/output/apk/debug/sample-app.apk + test: ../sample/app/build/output/apk/androidTest/debug/sample-app-test.apk + test: ../feature/room/build/output/apk/androidTest/debug/feature-room-test.apk + test: ../library/databases/build/output/apk/androidTest/debug/sample-databases-test.apk diff --git a/test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_tree.yml b/test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_tree.yml new file mode 100644 index 0000000000..861e95864a --- /dev/null +++ b/test_runner/src/test/kotlin/ftl/fixtures/flank_android_failed_tree.yml @@ -0,0 +1,24 @@ +gcloud: + app: /Users/no/workspace/fladle/sample/build/outputs/apk/debug/sample-debug.apk + test: /Users/no/workspace/fladle/sample/build/outputs/apk/androidTest/debug/sample-debug-androidTest.apk + device: + - model: Nexus5 + version: 23 + use-orchestrator: true + auto-google-login: false + record-video: true + performance-metrics: true + timeout: 15m + environment-variables: + clearPackageData: true + test-targets: + - class com.osacky.flank.gradle.sample.ExampleInstrumentedTest#seeView + num-flaky-test-attempts: 1 +flank: + additional-app-test-apks: + - app: ../main/app/build/output/apk/debug/app.apk + test: ../main/app/build/output/apk/androidTest/debug/app-test.apk + app: ../sample/app/build/output/apk/debug/sample-app.apk + test: ../sample/app/build/output/apk/androidTest/debug/sample-app-test.apk + test: ../feature/room/build/output/apk/androidTest/debug/feature-room-test.apk + test: ../library/databases/build/output/apk/androidTest/debug/sample-databases-test.apk From 5dd7bea0dcd88805a2179eb9c459f14a78594c27 Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Mon, 3 Aug 2020 16:32:22 +0200 Subject: [PATCH 3/3] fixes PR comments --- test_runner/src/main/kotlin/ftl/doctor/Doctor.kt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt b/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt index ed76f711c7..652b88cbb3 100644 --- a/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt +++ b/test_runner/src/main/kotlin/ftl/doctor/Doctor.kt @@ -13,10 +13,9 @@ import java.io.Reader import java.lang.StringBuilder import java.nio.file.Path -fun validateYaml(args: IArgs.ICompanion, data: Path): String { - if (!data.toFile().exists()) return "Skipping yaml validation. No file at path $data" - return validateYaml(args, loadFile(data)) + preloadConfiguration(data, args is AndroidArgsCompanion) -} +fun validateYaml(args: IArgs.ICompanion, data: Path) = + if (!data.toFile().exists()) "Skipping yaml validation. No file at path $data" + else validateYaml(args, loadFile(data)) + preloadConfiguration(data, args is AndroidArgsCompanion) @VisibleForTesting internal fun validateYaml(args: IArgs.ICompanion, data: Reader) =