Merge branch 'master' into #296-new-java-support

Flank · Sep 9, 2020 · e4a7755 · e4a7755
2 parents 18e8012 + 5783b5d
commit e4a7755
Show file tree

Hide file tree

Showing 45 changed files with 595 additions and 123 deletions.
diff --git a/docs/bugs/986_test_count_and_smart_sharding_ count_dont_match.md b/docs/bugs/986_test_count_and_smart_sharding_ count_dont_match.md
@@ -0,0 +1,36 @@
+# Test count and smart sharding count don't match
+
+Bug reported in [this issue](https://github.com/Flank/flank/issues/986)
+
+## The problem
+
+Flank does not support parameterized tests sharding. Every class with parameterized is considered as one test during shard calculation.
+
+Flank is using [DEX parser](https://github.com/linkedin/dex-test-parser) to decompile apks and gather info about all the tests inside. As for now, Flank is unable to determine how many times a test in a parameterized class is invoked. Due to this fact scans apks for any class with an annotation that contains `JUnitParamsRunner` or `Parameterized`:
+
+```kotlin
+
+@RunWith(JUnitParamsRunner::class)
+...
+@RunWith(Parameterized::class)
+
+```
+
+## Solution
+
+1. Flank knows how many tests and classes are being sent to Firebase. So we can inform the user of how many classes we have. Example:
+
+```txt
+
+ Smart Flank cache hit: 0% (0 / 9)
+  Shard times: 240s, 240s, 240s, 360s
+
+  Uploading app-debug.apk .
+  Uploading app-multiple-flaky-debug-androidTest.apk .
+  5 tests + 4 parameterized classes / 4 shards
+
+```
+
+1. Default test time for classes should be different from the default time for test
+   1. You can set default test time for class with ```--default-class-test-time``` command
+   2. If you did not set this time, the default value is 240s
diff --git a/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt b/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt
@@ -20,12 +20,12 @@ import ftl.config.FtlConstants.useMock
 import ftl.gc.GcStorage
 import ftl.gc.GcToolResults
 import ftl.reports.xml.model.JUnitTestResult
-import ftl.shard.StringShards
 import ftl.shard.createShardsByShardCount
 import ftl.shard.shardCountByTime
-import ftl.shard.stringShards
 import ftl.run.exception.FlankConfigurationError
 import ftl.run.exception.FlankGeneralError
+import ftl.shard.Chunk
+import ftl.shard.TestMethod
 import ftl.util.FlankTestMethod
 import ftl.util.assertNotEmpty
 import java.io.File
@@ -222,30 +222,25 @@ object ArgsHelper {
     ): CalculateShardsResult {
         if (filteredTests.isEmpty()) {
             // Avoid unnecessary computing if we already know there aren't tests to run.
-            return CalculateShardsResult(listOf(emptyList()), emptyList())
+            return CalculateShardsResult(emptyList(), emptyList())
         }
         val (ignoredTests, testsToExecute) = filteredTests.partition { it.ignored }
         val shards = if (args.disableSharding) {
-            // Avoid to cast it to MutableList<String> to be agnostic from the caller.
-            listOf(testsToExecute.map { it.testName }.toMutableList())
+            listOf(Chunk(testsToExecute.map { TestMethod(name = it.testName, isParameterized = it.isParameterizedClass, time = 0.0) }))
         } else {
             val oldTestResult = GcStorage.downloadJunitXml(args) ?: JUnitTestResult(mutableListOf())
             val shardCount = forcedShardCount ?: shardCountByTime(testsToExecute, oldTestResult, args)
-            createShardsByShardCount(testsToExecute, oldTestResult, args, shardCount).stringShards()
+            createShardsByShardCount(testsToExecute, oldTestResult, args, shardCount).map { Chunk(it.testMethods) }
         }
 
         return CalculateShardsResult(testMethodsAlwaysRun(shards, args), ignoredTestCases = ignoredTests.map { it.testName })
     }
 
-    private fun testMethodsAlwaysRun(shards: StringShards, args: IArgs): StringShards {
+    private fun testMethodsAlwaysRun(shards: List<Chunk>, args: IArgs): List<Chunk> {
         val alwaysRun = args.testTargetsAlwaysRun
+        val find = shards.flatMap { it.testMethods }.filter { alwaysRun.contains(it.name) }
 
-        shards.forEach { shard ->
-            shard.removeAll(alwaysRun)
-            shard.addAll(0, alwaysRun)
-        }
-
-        return shards
+        return shards.map { Chunk(find + it.testMethods.filterNot { method -> find.contains(method) }) }
     }
 }
 

diff --git a/test_runner/src/main/kotlin/ftl/args/CalculateShardsResult.kt b/test_runner/src/main/kotlin/ftl/args/CalculateShardsResult.kt
@@ -1,3 +1,5 @@
 package ftl.args
 
-data class CalculateShardsResult(val shardChunks: ShardChunks, val ignoredTestCases: IgnoredTestCases)
+import ftl.shard.Chunk
+
+data class CalculateShardsResult(val shardChunks: List<Chunk>, val ignoredTestCases: IgnoredTestCases)
diff --git a/test_runner/src/main/kotlin/ftl/args/CommonArgs.kt b/test_runner/src/main/kotlin/ftl/args/CommonArgs.kt
@@ -36,5 +36,6 @@ data class CommonArgs(
     override val outputStyle: OutputStyle,
     override val disableResultsUpload: Boolean,
     override val defaultTestTime: Double,
+    override val defaultClassTestTime: Double,
     override val useAverageTestTimeForNewTests: Boolean,
 ) : IArgs
diff --git a/test_runner/src/main/kotlin/ftl/args/CreateCommonArgs.kt b/test_runner/src/main/kotlin/ftl/args/CreateCommonArgs.kt
@@ -43,6 +43,7 @@ fun CommonConfig.createCommonArgs(
     localResultDir = flank.localResultsDir!!,
     disableResultsUpload = flank.disableResultsUpload!!,
     defaultTestTime = flank.defaultTestTime!!,
+    defaultClassTestTime = flank.defaultClassTestTime!!,
     useAverageTestTimeForNewTests = flank.useAverageTestTimeForNewTests!!
 ).apply {
     ArgsHelper.createJunitBucket(project, smartFlankGcsPath)

diff --git a/test_runner/src/main/kotlin/ftl/args/IArgs.kt b/test_runner/src/main/kotlin/ftl/args/IArgs.kt
@@ -60,6 +60,7 @@ interface IArgs {
         get() = maxTestShards in AVAILABLE_VIRTUAL_SHARD_COUNT_RANGE
 
     val defaultTestTime: Double
+    val defaultClassTestTime: Double
     val useAverageTestTimeForNewTests: Boolean
 
     fun useLocalResultDir() = localResultDir != defaultLocalResultsDir

diff --git a/test_runner/src/main/kotlin/ftl/args/IosArgs.kt b/test_runner/src/main/kotlin/ftl/args/IosArgs.kt
@@ -3,6 +3,7 @@ package ftl.args
 import com.google.common.annotations.VisibleForTesting
 import ftl.ios.Xctestrun.findTestNames
 import ftl.run.exception.FlankConfigurationError
+import ftl.shard.Chunk
 import ftl.util.FlankTestMethod
 
 data class IosArgs(
@@ -14,7 +15,7 @@ data class IosArgs(
 ) : IArgs by commonArgs {
 
     override val useLegacyJUnitResult = true
-    val testShardChunks: ShardChunks by lazy { calculateShardChunks() }
+    val testShardChunks: List<Chunk> by lazy { calculateShardChunks() }
 
     companion object : IosArgsCompanion()
 
@@ -62,7 +63,7 @@ IosArgs
 }
 
 private fun IosArgs.calculateShardChunks() = if (disableSharding)
-    listOf(emptyList()) else
+    emptyList() else
     ArgsHelper.calculateShards(
         filteredTests = filterTests(findTestNames(xctestrunFile), testTargets)
             .distinct()

diff --git a/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt b/test_runner/src/main/kotlin/ftl/args/ValidateAndroidArgs.kt
@@ -21,6 +21,7 @@ fun AndroidArgs.validate() = apply {
     assertParametersConflict()
     assertTestFiles()
     assertOtherFiles()
+    checkResultsDirUnique()
 }
 
 private fun AndroidArgs.assertDevicesSupported() = devices

diff --git a/test_runner/src/main/kotlin/ftl/args/ValidateCommonArgs.kt b/test_runner/src/main/kotlin/ftl/args/ValidateCommonArgs.kt
@@ -2,6 +2,7 @@ package ftl.args
 
 import ftl.config.Device
 import ftl.config.FtlConstants
+import ftl.gc.GcStorage
 import ftl.reports.FullJUnitReport
 import ftl.reports.JUnitReport
 import ftl.run.exception.FlankConfigurationError
@@ -28,8 +29,8 @@ private fun List<Device>.throwIfAnyMisspelt() =
 private fun CommonArgs.assertProjectId() {
     if (project.isEmpty()) throw FlankConfigurationError(
         "The project is not set. Define GOOGLE_CLOUD_PROJECT, set project in flank.yml\n" +
-            "or save service account credential to ${FtlConstants.defaultCredentialPath}\n" +
-            " See https://github.com/GoogleCloudPlatform/google-cloud-java#specifying-a-project-id"
+                "or save service account credential to ${FtlConstants.defaultCredentialPath}\n" +
+                " See https://github.com/GoogleCloudPlatform/google-cloud-java#specifying-a-project-id"
     )
 }
 
@@ -66,3 +67,8 @@ private fun CommonArgs.assertSmartFlankGcsPath() = with(smartFlankGcsPath) {
         )
     }
 }
+
+fun IArgs.checkResultsDirUnique() {
+    if (useLegacyJUnitResult && GcStorage.exist(resultsBucket, resultsDir))
+        println("WARNING: Google cloud storage result directory should be unique, otherwise results from multiple test matrices will be overwritten or intermingled\n")
+}
diff --git a/test_runner/src/main/kotlin/ftl/args/ValidateIosArgs.kt b/test_runner/src/main/kotlin/ftl/args/ValidateIosArgs.kt
@@ -11,6 +11,7 @@ fun IosArgs.validate() = apply {
     assertTestTypes()
     assertMaxTestShards()
     assertTestFiles()
+    checkResultsDirUnique()
 }
 
 fun IosArgs.validateRefresh() = apply {

diff --git a/test_runner/src/main/kotlin/ftl/config/common/CommonFlankConfig.kt b/test_runner/src/main/kotlin/ftl/config/common/CommonFlankConfig.kt
@@ -7,6 +7,7 @@ import ftl.args.ArgsHelper
 import ftl.args.yml.IYmlKeys
 import ftl.config.Config
 import ftl.config.FtlConstants
+import ftl.shard.DEFAULT_CLASS_TEST_TIME_SEC
 import ftl.shard.DEFAULT_TEST_TIME_SEC
 import picocli.CommandLine
 
@@ -148,6 +149,9 @@ data class CommonFlankConfig @JsonIgnore constructor(
     @set:JsonProperty("default-test-time")
     var defaultTestTime: Double? by data
 
+    @set:JsonProperty("default-class-test-time")
+    var defaultClassTestTime: Double? by data
+
     @set:CommandLine.Option(
         names = ["--use-average-test-time-for-new-tests"],
         description = ["Enable using average time from previous tests duration when using SmartShard and tests did not run before."]
@@ -178,6 +182,9 @@ data class CommonFlankConfig @JsonIgnore constructor(
             "output-style",
             "disable-results-upload",
             "full-junit-result",
+            "local-result-dir",
+            "default-test-time",
+            "default-class-test-time",
             "local-result-dir"
         )
 
@@ -201,6 +208,7 @@ data class CommonFlankConfig @JsonIgnore constructor(
             outputStyle = null
             disableResultsUpload = false
             defaultTestTime = DEFAULT_TEST_TIME_SEC
+            defaultClassTestTime = DEFAULT_CLASS_TEST_TIME_SEC
             useAverageTestTimeForNewTests = false
         }
     }

diff --git a/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt b/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt
@@ -3,6 +3,8 @@ package ftl.gc
 import com.google.api.client.http.GoogleApiLogger
 import com.google.cloud.storage.BlobInfo
 import com.google.cloud.storage.Storage
+import com.google.cloud.storage.Storage.BlobListOption.pageSize
+import com.google.cloud.storage.Storage.BlobListOption.prefix
 import com.google.cloud.storage.StorageOptions
 import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper
 import com.google.common.annotations.VisibleForTesting
@@ -172,4 +174,10 @@ object GcStorage {
             outputFile.path
         }
     }
+
+    fun exist(
+        rootGcsBucket: String,
+        runGcsPath: String,
+        storage: Storage = GcStorage.storage
+    ) = storage.list(rootGcsBucket, pageSize(1), prefix("$runGcsPath/")).values.count() > 0
 }
diff --git a/test_runner/src/main/kotlin/ftl/reports/util/MatrixPath.kt b/test_runner/src/main/kotlin/ftl/reports/util/MatrixPath.kt
@@ -3,9 +3,16 @@ package ftl.reports.util
 import java.io.File
 import java.nio.file.Paths
 
-fun File.getMatrixPath(objectName: String) = getShardName()?.asObjectPath(objectName)
+fun File.getMatrixPath(objectName: String) = getShardName(objectName)?.asObjectPath()
 
-private fun File.getShardName() = shardNameRegex.find(toString())?.value?.removePrefix("/")?.removeSuffix("/")
-private val shardNameRegex = "/.*(shard_|matrix_)\\d+(-rerun_\\d+)?/".toRegex()
+private fun File.getShardName(
+    objectName: String
+) = shardNameRegex(objectName)
+    .find(toString())
+    ?.value
+    ?.removePrefix("/")
+    ?.removeSuffix("/")
 
-private fun String.asObjectPath(objectName: String) = Paths.get(objectName, this).toString()
+private fun shardNameRegex(objectName: String) = "/($objectName)/(shard_|matrix_)\\d+(-rerun_\\d+)?/".toRegex()
+
+private fun String.asObjectPath() = Paths.get(this).toString()
diff --git a/test_runner/src/main/kotlin/ftl/reports/util/ReportManager.kt b/test_runner/src/main/kotlin/ftl/reports/util/ReportManager.kt
@@ -8,7 +8,6 @@ import ftl.args.ShardChunks
 import ftl.gc.GcStorage
 import ftl.json.MatrixMap
 import ftl.json.isAllSuccessful
-import ftl.json.validate
 import ftl.reports.CostReport
 import ftl.reports.FullJUnitReport
 import ftl.reports.HtmlErrorReport
@@ -28,21 +27,19 @@ import kotlin.math.roundToInt
 
 object ReportManager {
 
-    private fun findXmlFiles(matrices: MatrixMap, args: IArgs): List<File> {
-        val xmlFiles = mutableListOf<File>()
-        val rootFolder = File(resolveLocalRunPath(matrices, args))
-
-        rootFolder.walk().forEach {
-            if (it.name.matches(Artifacts.testResultRgx)) {
-                xmlFiles.add(it)
-            }
+    private fun findXmlFiles(
+        matrices: MatrixMap,
+        args: IArgs
+    ) = File(resolveLocalRunPath(matrices, args)).walk()
+        .filter { it.name.matches(Artifacts.testResultRgx) }
+        .fold(listOf<File>()) { xmlFiles, file ->
+            xmlFiles + file
         }
 
-        return xmlFiles
-    }
-
-    private fun getWebLink(matrices: MatrixMap, xmlFile: File): String = xmlFile.getMatrixPath(matrices.runPath)
-        ?.findMatrixPath(matrices) ?: "".also { println("WARNING: Matrix path not found in JSON.") }
+    private fun getWebLink(matrices: MatrixMap, xmlFile: File): String =
+        xmlFile.getMatrixPath(matrices.runPath)
+            ?.findMatrixPath(matrices)
+            ?: "".also { println("WARNING: Matrix path not found in JSON.") }
 
     private val deviceStringRgx = Regex("([^-]+-[^-]+-[^-]+-[^-]+).*")
 
@@ -119,7 +116,6 @@ object ReportManager {
             args.useLegacyJUnitResult -> processJunitXml(testSuite, args, testShardChunks)
             else -> processJunitXml(testSuite, args, testShardChunks)
         }
-        matrices.validate(args.ignoreFailedTests)
     }
 
     private fun IgnoredTestCases.toJunitTestsResults() = getSkippedJUnitTestSuite(map {
@@ -201,6 +197,10 @@ object ReportManager {
     }
 }
 
-private fun String.findMatrixPath(matrices: MatrixMap) =
-    matrices.map.values.firstOrNull { savedMatrix -> savedMatrix.gcsPath.endsWith(this) }?.webLink
-        ?: "".also { println("WARNING: Matrix path not found in JSON. $this") }
+private fun String.findMatrixPath(matrices: MatrixMap) = matrices.map.values
+    .firstOrNull { savedMatrix -> savedMatrix.gcsPath.endsWithTextWithOptionalSlashAtTheEnd(this) }
+    ?.webLink
+    ?: "".also { println("WARNING: Matrix path not found in JSON. $this") }
+
+@VisibleForTesting
+internal fun String.endsWithTextWithOptionalSlashAtTheEnd(text: String) = "($text)/*$".toRegex().containsMatchIn(this)
diff --git a/test_runner/src/main/kotlin/ftl/run/DumpShards.kt b/test_runner/src/main/kotlin/ftl/run/DumpShards.kt
@@ -7,6 +7,7 @@ import ftl.run.common.prettyPrint
 import ftl.run.exception.FlankConfigurationError
 import ftl.run.model.AndroidMatrixTestShards
 import ftl.run.platform.android.getAndroidMatrixShards
+import ftl.shard.testCases
 import ftl.util.obfuscatePrettyPrinter
 import java.nio.file.Files
 import java.nio.file.Paths
@@ -35,7 +36,7 @@ fun dumpShards(
 ) {
     saveShardChunks(
         shardFilePath = shardFilePath,
-        shards = args.testShardChunks,
+        shards = args.testShardChunks.testCases,
         size = args.testShardChunks.size,
         obfuscatedOutput = obfuscatedOutput
     )

diff --git a/test_runner/src/main/kotlin/ftl/run/NewTestRun.kt b/test_runner/src/main/kotlin/ftl/run/NewTestRun.kt
@@ -5,12 +5,14 @@ import ftl.args.IArgs
 import ftl.args.IosArgs
 import ftl.json.SavedMatrix
 import ftl.json.updateMatrixMap
+import ftl.json.validate
 import ftl.reports.util.ReportManager
 import ftl.run.common.fetchArtifacts
 import ftl.run.common.pollMatrices
 import ftl.run.exception.FlankGeneralError
 import ftl.run.exception.FlankTimeoutError
 import ftl.run.model.TestResult
+import ftl.run.platform.common.printMatricesWebLinks
 import ftl.run.platform.runAndroidTests
 import ftl.run.platform.runIosTests
 import kotlinx.coroutines.TimeoutCancellationException
@@ -25,6 +27,11 @@ suspend fun newTestRun(args: IArgs) = withTimeoutOrNull(args.parsedTimeout) {
         cancelTestsOnTimeout(args.project, matrixMap.map) { fetchArtifacts(matrixMap, args) }
 
         ReportManager.generate(matrixMap, args, testShardChunks, ignoredTests)
+
+        println()
+        matrixMap.printMatricesWebLinks(args.project)
+
+        matrixMap.validate(args.ignoreFailedTests)
     }
 }
 

diff --git a/test_runner/src/main/kotlin/ftl/run/RefreshLastRun.kt b/test_runner/src/main/kotlin/ftl/run/RefreshLastRun.kt
@@ -15,6 +15,7 @@ import ftl.run.common.updateMatrixFile
 import ftl.args.ShardChunks
 import ftl.json.needsUpdate
 import ftl.json.updateWithMatrix
+import ftl.json.validate
 import ftl.util.MatrixState
 import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
@@ -33,6 +34,8 @@ suspend fun refreshLastRun(currentArgs: IArgs, testShardChunks: ShardChunks) {
 
     // Must generate reports *after* fetching xml artifacts since reports require xml
     ReportManager.generate(matrixMap, lastArgs, testShardChunks)
+
+    matrixMap.validate(lastArgs.ignoreFailedTests)
 }
 
 /** Refresh all in progress matrices in parallel **/