From e86746c7fde9274d24e0f3be6d885da8319baad1 Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Thu, 2 Jul 2020 20:13:14 +0200 Subject: [PATCH 1/5] #870 Added --directories-to-pull validation and avoid making request with empty toolStepResult --- .../kotlin/ftl/args/ValidateAndroidArgs.kt | 16 +++ .../ftl/reports/api/CreateJUnitTestResult.kt | 3 + ...alidateDirectoriesToPullAndroidArgsTest.kt | 100 ++++++++++++++++++ 3 files changed, 119 insertions(+) create mode 100644 test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt diff --git a/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt b/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt index 89ff24d1af..a61559389b 100644 --- a/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt +++ b/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt @@ -14,6 +14,7 @@ fun AndroidArgs.validate() { assertShards() assertTestTypes() assertRoboTest() + assertDirectoriesToPull() } private fun AndroidArgs.assertAdditionalAppTestApks() { @@ -57,3 +58,18 @@ private fun AndroidArgs.assertRoboTest() { "Options robo-directives and robo-script are mutually exclusive, use only one of them." ) } + +// Validation is done according to https://cloud.google.com/sdk/gcloud/reference/firebase/test/android/run#--directories-to-pull +private fun AndroidArgs.assertDirectoriesToPull() { + val correctNameRegex = "(/[a-zA-Z0-9_\\-.+]+)+".toRegex() + directoriesToPull + .filter { !it.startsWith("/sdcard") && !it.startsWith("/data/local/tmp") || !correctNameRegex.matches(it) } + .takeIf { it.isNotEmpty() } + ?.also { + throw FlankFatalError( + "Invalid value for [directories-to-pull]: Invalid path $it.\n" + + "Path must be absolute paths under /sdcard or /data/local/tmp (for example, --directories-to-pull /sdcard/tempDir1,/data/local/tmp/tempDir2).\n" + + "Path names are restricted to the characters [a-zA-Z0-9_-./+]. " + ) + } +} diff --git a/test_runner/src/main/kotlin/ftl/reports/api/CreateJUnitTestResult.kt b/test_runner/src/main/kotlin/ftl/reports/api/CreateJUnitTestResult.kt index 3d1f8feb81..7e7144421f 100644 --- a/test_runner/src/main/kotlin/ftl/reports/api/CreateJUnitTestResult.kt +++ b/test_runner/src/main/kotlin/ftl/reports/api/CreateJUnitTestResult.kt @@ -7,6 +7,7 @@ internal fun List.createJUnitTestResult( withStackTraces: Boolean = false ) = JUnitTestResult( testsuites = this + .filterNullToolResultsStep() .createTestExecutionDataListAsync() .prepareForJUnitResult() .let { executionDataList -> @@ -16,3 +17,5 @@ internal fun List.createJUnitTestResult( .createJUnitTestSuites() .toMutableList() ) + +private fun List.filterNullToolResultsStep() = filter { it.toolResultsStep != null } diff --git a/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt b/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt new file mode 100644 index 0000000000..7c05e8b3c1 --- /dev/null +++ b/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt @@ -0,0 +1,100 @@ +package ftl.args + +import ftl.test.util.TestHelper +import org.junit.Assert.assertEquals +import org.junit.Test +import java.io.StringReader + +class ValidateDirectoriesToPullAndroidArgsTest { + + @Test + fun `should not throw any error with valid directoriesToPull`() { + // given + val testYaml = """ + 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 + directories-to-pull: + - /sdcard/data/sample + - /data/local/tmp/sample + flank: + disable-sharding: true + """.trimIndent() + + // when + AndroidArgs.load(StringReader(testYaml)).validate() + + // then + // no issues + } + + @Test + fun `should not throw any error without directoriesToPull specifiec`() { + // given + val testYaml = """ + 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 + flank: + disable-sharding: true + """.trimIndent() + + // when + AndroidArgs.load(StringReader(testYaml)).validate() + + // then + // no issues + } + + @Test + fun `should throw error with bad prefixed paths in directoriesToPull`() { + // given + val expectedErrorMessage = getExpectedMessageForPaths(listOf("/sdcard/data/sample!", "/data/local/tmp/sample#")) + val testYaml = """ + 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 + directories-to-pull: + - /sdcard/data/sample! + - /data/local/tmp/sample# + flank: + disable-sharding: true + """.trimIndent() + + // when + val exception = TestHelper.getThrowable { AndroidArgs.load(StringReader(testYaml)).validate() } + + // then + assertEquals(expectedErrorMessage, exception.message) + } + + + @Test + fun `should throw error with bad characters in paths in directoriesToPull`() { + // given + val expectedErrorMessage = getExpectedMessageForPaths(listOf("/app/", "/data/sample")) + val testYaml = """ + 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 + directories-to-pull: + - /app/ + - /data/sample + - /sdcard/test + flank: + disable-sharding: true + """.trimIndent() + + // when + val exception = TestHelper.getThrowable { AndroidArgs.load(StringReader(testYaml)).validate() } + + // then + assertEquals(expectedErrorMessage, exception.message) + } + + private fun getExpectedMessageForPaths(badPaths: List) = + "Invalid value for [directories-to-pull]: Invalid path $badPaths.\n" + + "Path must be absolute paths under /sdcard or /data/local/tmp (for example, --directories-to-pull /sdcard/tempDir1,/data/local/tmp/tempDir2).\n" + + "Path names are restricted to the characters [a-zA-Z0-9_-./+]. " + +} From e02e7a3cd6b0b9ef980d69ae805fa7e83e3a16fd Mon Sep 17 00:00:00 2001 From: piotradamczyk5 <65554637+piotradamczyk5@users.noreply.github.com> Date: Thu, 2 Jul 2020 20:25:49 +0200 Subject: [PATCH 2/5] Update release_notes.md --- release_notes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/release_notes.md b/release_notes.md index e01794134d..ab8fb5dfe5 100644 --- a/release_notes.md +++ b/release_notes.md @@ -6,6 +6,7 @@ - [#865](https://github.com/Flank/flank/pull/865) Flank needs to respect the timeout value as that's a cap for billing purposes. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - [#866](https://github.com/Flank/flank/pull/866) Fix printing all matrix links. ([piotradamczyk5](https://github.com/piotradamczyk5)) - [#862](https://github.com/Flank/flank/pull/862) Added printing outcome details. ([piotradamczyk5](https://github.com/piotradamczyk5), [jan-gogo](https://github.com/jan-gogo)) +- [#876](https://github.com/Flank/flank/pull/876) Added --directories-to-pull validation and avoid making request with empty toolStepResult. ([piotradamczyk5](https://github.com/piotradamczyk5)) - - From 8af6e5282661ed01e91d4cab9bb788da23db277a Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Thu, 2 Jul 2020 20:36:17 +0200 Subject: [PATCH 3/5] #870 Detekt and tests fixes --- .../test/kotlin/ftl/args/AndroidArgsTest.kt | 25 ++++++++++++++++--- ...alidateDirectoriesToPullAndroidArgsTest.kt | 2 -- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt b/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt index 82d0c2a296..d68565e571 100644 --- a/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt @@ -733,7 +733,7 @@ AndroidArgs @Test fun `cli directoriesToPull`() { val cli = AndroidRunCommand() - CommandLine(cli).parseArgs("--directories-to-pull=a,b") + CommandLine(cli).parseArgs("--directories-to-pull=/sdcard/test,/data/local/tmp/test") val yaml = """ gcloud: @@ -743,7 +743,22 @@ AndroidArgs assertThat(AndroidArgs.load(yaml).directoriesToPull).isEmpty() val androidArgs = AndroidArgs.load(yaml, cli) - assertThat(androidArgs.directoriesToPull).isEqualTo(listOf("a", "b")) + assertThat(androidArgs.directoriesToPull).isEqualTo(listOf("/sdcard/test", "/data/local/tmp/test")) + } + + @Test(expected = FlankFatalError::class) + fun `should throw FlankFatalError when invalid cli directoriesToPull`() { + val cli = AndroidRunCommand() + CommandLine(cli).parseArgs("--directories-to-pull=a,b") + + val yaml = """ + gcloud: + app: $appApk + test: $testApk + """ + assertThat(AndroidArgs.load(yaml).directoriesToPull).isEmpty() + + AndroidArgs.load(yaml, cli) } @Test @@ -1455,6 +1470,8 @@ AndroidArgs } } -private fun AndroidArgs.Companion.load(yamlData: String, cli: AndroidRunCommand? = null): AndroidArgs = load(StringReader(yamlData), cli) +private fun AndroidArgs.Companion.load(yamlData: String, cli: AndroidRunCommand? = null): AndroidArgs = + load(StringReader(yamlData), cli) -fun getAndroidShardChunks(args: AndroidArgs): ShardChunks = runBlocking { (args.createAndroidTestContexts().first() as InstrumentationTestContext).shards } +fun getAndroidShardChunks(args: AndroidArgs): ShardChunks = + runBlocking { (args.createAndroidTestContexts().first() as InstrumentationTestContext).shards } diff --git a/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt b/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt index 7c05e8b3c1..d0ac5a8a87 100644 --- a/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt @@ -68,7 +68,6 @@ class ValidateDirectoriesToPullAndroidArgsTest { assertEquals(expectedErrorMessage, exception.message) } - @Test fun `should throw error with bad characters in paths in directoriesToPull`() { // given @@ -96,5 +95,4 @@ class ValidateDirectoriesToPullAndroidArgsTest { "Invalid value for [directories-to-pull]: Invalid path $badPaths.\n" + "Path must be absolute paths under /sdcard or /data/local/tmp (for example, --directories-to-pull /sdcard/tempDir1,/data/local/tmp/tempDir2).\n" + "Path names are restricted to the characters [a-zA-Z0-9_-./+]. " - } From ab808f6ca0970cfeb4ae75a868041381797646d9 Mon Sep 17 00:00:00 2001 From: Adam Date: Fri, 3 Jul 2020 15:00:01 +0200 Subject: [PATCH 4/5] Fix problem with paths contains / on last char, update tests --- test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt | 2 +- test_runner/src/test/kotlin/Debug.kt | 4 ++-- .../ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt b/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt index a61559389b..6fb240173b 100644 --- a/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt +++ b/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt @@ -61,7 +61,7 @@ private fun AndroidArgs.assertRoboTest() { // Validation is done according to https://cloud.google.com/sdk/gcloud/reference/firebase/test/android/run#--directories-to-pull private fun AndroidArgs.assertDirectoriesToPull() { - val correctNameRegex = "(/[a-zA-Z0-9_\\-.+]+)+".toRegex() + val correctNameRegex = "(/[a-zA-Z0-9_\\-.+]+)+/?".toRegex() directoriesToPull .filter { !it.startsWith("/sdcard") && !it.startsWith("/data/local/tmp") || !correctNameRegex.matches(it) } .takeIf { it.isNotEmpty() } diff --git a/test_runner/src/test/kotlin/Debug.kt b/test_runner/src/test/kotlin/Debug.kt index 8a2c275d10..d87dbd4904 100644 --- a/test_runner/src/test/kotlin/Debug.kt +++ b/test_runner/src/test/kotlin/Debug.kt @@ -10,8 +10,8 @@ fun main() { // run "gradle check" to generate required fixtures val projectId = System.getenv("GOOGLE_CLOUD_PROJECT") ?: "YOUR PROJECT ID" - val quantity = "single" - val type = "robo" + val quantity = "multiple" + val type = "success" // Bugsnag keeps the process alive so we must call exitProcess // https://github.com/bugsnag/bugsnag-java/issues/151 diff --git a/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt b/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt index d0ac5a8a87..1f5be748eb 100644 --- a/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt @@ -17,6 +17,7 @@ class ValidateDirectoriesToPullAndroidArgsTest { directories-to-pull: - /sdcard/data/sample - /data/local/tmp/sample + - /sdcard/ flank: disable-sharding: true """.trimIndent() @@ -29,7 +30,7 @@ class ValidateDirectoriesToPullAndroidArgsTest { } @Test - fun `should not throw any error without directoriesToPull specifiec`() { + fun `should not throw any error without directoriesToPull specific`() { // given val testYaml = """ gcloud: From af17830c3b7383f67b72d4b21dca05c29980bf18 Mon Sep 17 00:00:00 2001 From: Piotr Adamczyk Date: Mon, 6 Jul 2020 18:20:25 +0200 Subject: [PATCH 5/5] #870 added info for table about invalid matrix; remove duplicate tables --- .../src/main/kotlin/ftl/json/SavedMatrix.kt | 15 ++++++++++++-- test_runner/src/main/kotlin/ftl/util/Utils.kt | 3 +-- .../test/kotlin/ftl/json/SavedMatrixTest.kt | 20 +++++++++++++++++++ .../src/test/kotlin/ftl/util/UtilsTest.kt | 17 +--------------- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/json/SavedMatrix.kt b/test_runner/src/main/kotlin/ftl/json/SavedMatrix.kt index 079d092b48..d194bd7332 100644 --- a/test_runner/src/main/kotlin/ftl/json/SavedMatrix.kt +++ b/test_runner/src/main/kotlin/ftl/json/SavedMatrix.kt @@ -11,6 +11,7 @@ import ftl.reports.api.data.TestSuiteOverviewData import ftl.reports.api.prepareForJUnitResult import ftl.util.MatrixState.ERROR import ftl.util.MatrixState.FINISHED +import ftl.util.MatrixState.INVALID import ftl.util.StepOutcome.failure import ftl.util.StepOutcome.flaky import ftl.util.StepOutcome.inconclusive @@ -67,8 +68,7 @@ class SavedMatrix(matrix: TestMatrix) { val changedLink = webLink != newLink if (changedState) { - this.state = newState - if (this.state == FINISHED) finished(matrix) + updateState(newState, matrix) } if (changedLink) { @@ -78,6 +78,17 @@ class SavedMatrix(matrix: TestMatrix) { return changedState || changedLink } + private fun updateState(newState: String, testMatrix: TestMatrix) { + state = newState + when (state) { + FINISHED -> finished(testMatrix) + INVALID -> { + outcomeDetails = "Matrix is invalid" + outcome = "---" + } + } + } + private fun finished(matrix: TestMatrix) { if (matrix.state != FINISHED) { throw RuntimeException("Matrix ${matrix.testMatrixId} ${matrix.state} != $FINISHED") diff --git a/test_runner/src/main/kotlin/ftl/util/Utils.kt b/test_runner/src/main/kotlin/ftl/util/Utils.kt index aa35ef16cb..add52d8c17 100644 --- a/test_runner/src/main/kotlin/ftl/util/Utils.kt +++ b/test_runner/src/main/kotlin/ftl/util/Utils.kt @@ -169,8 +169,7 @@ fun withGlobalExceptionHandling(block: () -> Int) { } private fun SavedMatrix.logError() { - println("More details are available at [${this.webLink}]") - println(this.asPrintableTable()) + println("Matrix is $state") } fun , T> mutableMapProperty( diff --git a/test_runner/src/test/kotlin/ftl/json/SavedMatrixTest.kt b/test_runner/src/test/kotlin/ftl/json/SavedMatrixTest.kt index 82e256743d..91eedf7ed2 100644 --- a/test_runner/src/test/kotlin/ftl/json/SavedMatrixTest.kt +++ b/test_runner/src/test/kotlin/ftl/json/SavedMatrixTest.kt @@ -12,6 +12,7 @@ import ftl.gc.GcAndroidDevice import ftl.test.util.FlankTestRunner import ftl.util.MatrixState.ERROR import ftl.util.MatrixState.FINISHED +import ftl.util.MatrixState.INVALID import ftl.util.MatrixState.PENDING import ftl.util.webLink import org.junit.Assert @@ -183,4 +184,23 @@ class SavedMatrixTest { savedMatrix.update(testMatrix) Assert.assertEquals(1, savedMatrix.billableVirtualMinutes) } + + @Test + fun `savedMatrix should have outcome and outcome details properly filled when state is INVALID`() { + val expectedOutcome = "---" + val expectedOutcomeDetails = "Matrix is invalid" + val testMatrix = TestMatrix() + testMatrix.testMatrixId = "123" + testMatrix.state = PENDING + testMatrix.resultStorage = createResultsStorage() + + val savedMatrix = SavedMatrix(testMatrix) + savedMatrix.update(testMatrix) + + testMatrix.state = INVALID + savedMatrix.update(testMatrix) + Assert.assertEquals(expectedOutcome, savedMatrix.outcome) + Assert.assertEquals(expectedOutcomeDetails, savedMatrix.outcomeDetails) + Assert.assertEquals(INVALID, savedMatrix.state) + } } diff --git a/test_runner/src/test/kotlin/ftl/util/UtilsTest.kt b/test_runner/src/test/kotlin/ftl/util/UtilsTest.kt index 31dd6f599f..d24db0b8ef 100644 --- a/test_runner/src/test/kotlin/ftl/util/UtilsTest.kt +++ b/test_runner/src/test/kotlin/ftl/util/UtilsTest.kt @@ -197,8 +197,7 @@ class UtilsTest { // will exit.expectSystemExitWithStatus(3) exit.checkAssertionAfterwards { - assertTrue(output.log.contains("More details are available at [${testMatrix1.webLink}]")) - assertOutputIsEqualReferenceTable(output.log, testMatrix1.matrixId) + assertTrue(output.log.contains("Matrix is ${testMatrix1.state}")) } // when @@ -276,20 +275,6 @@ class UtilsTest { withGlobalExceptionHandling(block) } - private fun assertOutputIsEqualReferenceTable(outputLog: String, matrixId: String) { - val referenceTable = - "┌─────────┬───────────┬────────────────────┐\n" + - "│ OUTCOME │ MATRIX ID │ TEST DETAILS │\n" + - "├─────────┼───────────┼────────────────────┤\n" + - "│ Failed │ $matrixId │ Test failed to run │\n" + - "└─────────┴───────────┴────────────────────┘" - referenceTable.split("\n") - .forEach { - println(it) - assertTrue(outputLog.contains(it)) - } - } - @Test fun `should terminate process with exit code 1 if there is not tests to run overall`() { // given