#870 Added --directories-to-pull validation and avoid making request with empty toolStepResult

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))
 -
 -
 

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_-./+]. "
+            )
+        }
+}

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")

test_runner/src/main/kotlin/ftl/reports/api/CreateJUnitTestResult.kt

@@ -7,6 +7,7 @@ internal fun List<TestExecution>.createJUnitTestResult(
     withStackTraces: Boolean = false
 ) = JUnitTestResult(
     testsuites = this
+        .filterNullToolResultsStep()
         .createTestExecutionDataListAsync()
         .prepareForJUnitResult()
         .let { executionDataList ->
@@ -16,3 +17,5 @@ internal fun List<TestExecution>.createJUnitTestResult(
         .createJUnitTestSuites()
         .toMutableList()
 )
+
+private fun List<TestExecution>.filterNullToolResultsStep() = filter { it.toolResultsStep != null }

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 <R : MutableMap<String, Any>, T> mutableMapProperty(

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

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 }

test_runner/src/test/kotlin/ftl/args/ValidateDirectoriesToPullAndroidArgsTest.kt

@@ -0,0 +1,99 @@
+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
+                - /sdcard/
+            flank:
+              disable-sharding: true
+        """.trimIndent()
+
+        // when
+        AndroidArgs.load(StringReader(testYaml)).validate()
+
+        // then
+        // no issues
+    }
+
+    @Test
+    fun `should not throw any error without directoriesToPull specific`() {
+        // 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<String>) =
+        "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_-./+]. "
+}

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)
+    }
 }

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