From 32ca13b95f13b08c1c94e82664d3db07310f4333 Mon Sep 17 00:00:00 2001 From: Carlos Sessa Date: Fri, 16 Nov 2018 16:26:11 -0600 Subject: [PATCH] SmartFlank algo (#397) * SmartFlank * SmarkFlank code review --- .../src/main/kotlin/ftl/args/AndroidArgs.kt | 13 +-- .../src/main/kotlin/ftl/args/ArgsHelper.kt | 41 ++------- .../src/main/kotlin/ftl/args/IosArgs.kt | 7 +- .../src/main/kotlin/ftl/gc/GcStorage.kt | 18 +++- .../kotlin/ftl/reports/util/ReportManager.kt | 6 +- .../src/main/kotlin/ftl/shard/Shard.kt | 85 +++++++++-------- .../kotlin/ftl/args/AndroidArgsFileTest.kt | 4 +- .../test/kotlin/ftl/args/ArgsHelperTest.kt | 71 ++++---------- .../src/test/kotlin/ftl/shard/ShardTest.kt | 92 ++++++++++++++----- 9 files changed, 160 insertions(+), 177 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt b/test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt index 0d3f1d82d2..5f93fe7dcf 100644 --- a/test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt +++ b/test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt @@ -9,8 +9,7 @@ import ftl.android.UnsupportedModelId import ftl.android.UnsupportedVersionId import ftl.args.ArgsHelper.assertFileExists import ftl.args.ArgsHelper.assertGcsFileExists -import ftl.args.ArgsHelper.calculateShards -import ftl.args.ArgsHelper.evaluateFilePath +import ftl.args.ArgsHelper.convertShards import ftl.args.ArgsHelper.createGcsBucket import ftl.args.ArgsHelper.createJunitBucket import ftl.args.ArgsHelper.evaluateFilePath @@ -28,6 +27,8 @@ import ftl.config.FtlConstants import ftl.config.FtlConstants.useMock import ftl.filter.TestFilters import ftl.gc.GcStorage +import ftl.reports.xml.model.JUnitTestResult +import ftl.shard.Shard import ftl.util.Utils import kotlinx.coroutines.runBlocking import java.nio.file.Files @@ -79,12 +80,8 @@ class AndroidArgs( } val filteredTests = getTestMethods(testLocalApk) - - calculateShards( - filteredTests, - testTargetsAlwaysRun, - testShards - ) + val oldTestResult = GcStorage.downloadJunitXml(this) ?: JUnitTestResult(mutableListOf()) + convertShards(Shard.calculateShardsByTime(filteredTests, oldTestResult, testShards)).toMutableList() } init { diff --git a/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt b/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt index da753fc941..a28aa77844 100644 --- a/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt +++ b/test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt @@ -11,16 +11,15 @@ import com.google.cloud.storage.BucketInfo import com.google.cloud.storage.Storage import com.google.cloud.storage.StorageClass import com.google.cloud.storage.StorageOptions -import com.google.common.math.IntMath import ftl.args.yml.IYmlMap import ftl.config.FtlConstants import ftl.config.FtlConstants.GCS_PREFIX import ftl.config.FtlConstants.JSON_FACTORY import ftl.config.FtlConstants.defaultCredentialPath import ftl.gc.GcStorage +import ftl.shard.TestShard import ftl.util.Utils import java.io.File -import java.math.RoundingMode import java.net.URI import java.nio.file.Files import java.nio.file.Path @@ -95,39 +94,10 @@ object ArgsHelper { if (validTestMethods.isEmpty()) Utils.fatalError("$from has no tests") } - fun calculateShards( - testMethodsToShard: Collection, - testMethodsAlwaysRun: Collection, - testShards: Int - ): List> { - val testShardMethods = testMethodsToShard.distinct().toMutableList() - testShardMethods.removeAll(testMethodsAlwaysRun) - - val oneTestPerChunk = testShards == -1 - var chunkSize = IntMath.divide(testShardMethods.size, testShards, RoundingMode.UP) - - if (oneTestPerChunk || chunkSize < 1) { - chunkSize = 1 - } - - val testShardChunks = testShardMethods.asSequence() - .chunked(chunkSize) - .map { testMethodsAlwaysRun + it } - .toList() - - // Ensure we don't create more VMs than requested. VM count per run should be <= testShards - if (!oneTestPerChunk && testShardChunks.size > testShards) { - Utils.fatalError("Calculated chunks $testShardChunks is > requested $testShards testShards.") - } - if (testShardChunks.isEmpty()) Utils.fatalError("Failed to populate test shard chunks") - - return testShardChunks - } - fun createJunitBucket(projectId: String, junitGcsPath: String) { if (FtlConstants.useMock || junitGcsPath.isEmpty()) return val bucket = junitGcsPath.drop(GCS_PREFIX.length).substringBefore('/') - createGcsBucket(projectId, bucket) + createGcsBucket(projectId, bucket) } fun createGcsBucket(projectId: String, bucket: String): String { @@ -182,6 +152,7 @@ object ArgsHelper { // https://stackoverflow.com/a/2821201/2450315 private val envRegex = Pattern.compile("\\$([a-zA-Z_]+[a-zA-Z0-9_]*)") + private fun evaluateEnvVars(text: String): String { val buffer = StringBuffer() val matcher = envRegex.matcher(text) @@ -202,4 +173,10 @@ object ArgsHelper { return ArgsFileVisitor("glob:$filePath").walk(searchDir) } + + fun convertShards(shards: List): List> { + return shards.map { shard -> + shard.testMethods.map { it.name } + } + } } diff --git a/test_runner/src/main/kotlin/ftl/args/IosArgs.kt b/test_runner/src/main/kotlin/ftl/args/IosArgs.kt index 63280f28e8..36c531c4bc 100644 --- a/test_runner/src/main/kotlin/ftl/args/IosArgs.kt +++ b/test_runner/src/main/kotlin/ftl/args/IosArgs.kt @@ -63,11 +63,8 @@ class IosArgs( testTargets } - ArgsHelper.calculateShards( - testMethodsToShard = testsToShard, - testMethodsAlwaysRun = testTargetsAlwaysRun, - testShards = testShards - ) + // TODO: Use Shard.calculateShardsByTime + emptyList>() } init { diff --git a/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt b/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt index ac6c74c5a8..08b939cb33 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcStorage.kt @@ -9,6 +9,9 @@ import ftl.args.IArgs import ftl.args.IosArgs import ftl.config.FtlConstants import ftl.config.FtlConstants.GCS_PREFIX +import ftl.reports.xml.model.JUnitTestResult +import ftl.reports.xml.parseIosXml +import ftl.reports.xml.xmlToString import ftl.util.Utils.fatalError import ftl.util.Utils.join import java.io.File @@ -41,9 +44,8 @@ object GcStorage { runGcsPath = runGcsPath ) - fun uploadJunitXml(localJunitXml: String, args: IArgs) { + fun uploadJunitXml(testResult: JUnitTestResult, args: IArgs) { if (args.junitGcsPath.isEmpty()) return - if (File(localJunitXml).exists().not()) return // bucket/path/to/object val rawPath = args.junitGcsPath.drop(GCS_PREFIX.length) @@ -53,7 +55,7 @@ object GcStorage { val fileBlob = BlobInfo.newBuilder(bucket, name).build() try { - storage.create(fileBlob, Files.readAllBytes(Paths.get(localJunitXml))) + storage.create(fileBlob, testResult.xmlToString().toByteArray()) } catch (e: Exception) { fatalError(e) } @@ -80,8 +82,14 @@ object GcStorage { download(args.testApk) // junit xml may not exist. ignore error if it doesn't exist - fun downloadJunitXml(args: IArgs): String = - download(args.junitGcsPath, ignoreError = true) + fun downloadJunitXml(args: IArgs): JUnitTestResult? { + val oldXmlPath = download(args.junitGcsPath, ignoreError = true) + if (oldXmlPath.isNotEmpty()) { + return parseIosXml(Paths.get(oldXmlPath)) + } + + return null + } private fun upload(file: String, fileBytes: ByteArray, rootGcsBucket: String, runGcsPath: String): String { val fileName = Paths.get(file).fileName.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 index faa651787b..3db6dfb96b 100644 --- a/test_runner/src/main/kotlin/ftl/reports/util/ReportManager.kt +++ b/test_runner/src/main/kotlin/ftl/reports/util/ReportManager.kt @@ -11,7 +11,6 @@ import ftl.reports.MatrixResultsReport import ftl.reports.xml.model.JUnitTestResult import ftl.reports.xml.parseAndroidXml import ftl.reports.xml.parseIosXml -import ftl.reports.xml.xmlToString import ftl.util.ArtifactRegex import ftl.util.resolveLocalRunPath import java.io.File @@ -100,11 +99,10 @@ object ReportManager { private fun processJunitXml(newTestResult: JUnitTestResult?, args: IArgs) { if (newTestResult == null) return - val oldXmlPath = GcStorage.downloadJunitXml(args) - val oldTestResult = if (oldXmlPath.isNotEmpty()) parseIosXml(oldXmlPath) else null + val oldTestResult = GcStorage.downloadJunitXml(args) newTestResult.mergeTestTimes(oldTestResult) - GcStorage.uploadJunitXml(newTestResult.xmlToString(), args) + GcStorage.uploadJunitXml(newTestResult, args) } } diff --git a/test_runner/src/main/kotlin/ftl/shard/Shard.kt b/test_runner/src/main/kotlin/ftl/shard/Shard.kt index 5a2cfbc3a8..bbcac93216 100644 --- a/test_runner/src/main/kotlin/ftl/shard/Shard.kt +++ b/test_runner/src/main/kotlin/ftl/shard/Shard.kt @@ -1,64 +1,61 @@ package ftl.shard +import ftl.reports.xml.model.JUnitTestResult + data class TestMethod( val name: String, val time: Double ) data class TestShard( - val time: Double, + var time: Double, val testMethods: MutableList ) -data class TestShards( - val shards: MutableList = mutableListOf() -) - object Shard { - /** - * Build shard by removing remaining methods that total to targetShardDuration - * At least one method per shard will always be returned, regardless of targetShardDuration. - * - * remainingMethods must be sorted in order of fastest execution time to slowest. - * remainingMethods.sortBy { it.time } - * - * Based on createConfigurableShard from Flank Java - * https://github.com/TestArmada/flank/blob/ceda6d2c3d9eb2a366f19b826e04289cd24bddf3/Flank/src/main/java/com/walmart/otto/shards/ShardCreator.java#L99 - */ - fun build(remainingMethods: MutableList, targetShardDuration: Double): TestShard { - var timeBudget = targetShardDuration - var shardTime = 0.0 - - val testMethods = remainingMethods.iterator() - val shardTests = mutableListOf() - - while (testMethods.hasNext()) { - val test = testMethods.next() - val testWithinBudget = timeBudget - test.time >= 0 - - if (testWithinBudget) { - timeBudget -= test.time - - shardTime += test.time - shardTests.add(test) - testMethods.remove() - - continue + // take in the XML with timing info then return list of shards + fun calculateShardsByTime(runningTests: List, testResult: JUnitTestResult, maxShards: Int): List { + + val junitMap = mutableMapOf() + + // Create a map with information from previous junit run + testResult.testsuites?.forEach { testsuite -> + testsuite.testcases?.forEach { testcase -> + val key = "${testsuite.name}${testcase.classname}#${testcase.name}".trim() + junitMap[key] = testcase.time.toDouble() } + } + + val testcases = mutableListOf() + runningTests.forEach { + // junitMap doesn't include `class `, we remove it to search in the map + val key = it.replaceFirst("class ", "") + val time = junitMap[key] ?: 10.0 + testcases.add(TestMethod(it, time)) + } - val noTestsAdded = timeBudget == targetShardDuration - val testOverBudget = test.time >= timeBudget + val testCount = testcases.size - if (noTestsAdded && testOverBudget) { - // Always add at least 1 test per shard regardless of budget - shardTime += test.time - shardTests.add(test) - testMethods.remove() + // If maxShards is infinite or we have more shards than tests, let's match it + val shardsCount = if (maxShards == -1 || maxShards > testCount) testCount else maxShards - return TestShard(shardTime, shardTests) - } + // Create the list of shards we will return + var shards = List(shardsCount) { TestShard(0.0, mutableListOf()) } + + // We want to iterate over testcase going from slowest to fastest + testcases.sortByDescending { it.time } + + testcases.forEach { testMethod -> + val shard = shards.first() + + shard.testMethods.add(testMethod) + shard.time += testMethod.time + + // Sort the shards to keep the most empty shard first + shards = shards.sortedBy { it.time } } - return TestShard(shardTime, shardTests) + + return shards } } diff --git a/test_runner/src/test/kotlin/ftl/args/AndroidArgsFileTest.kt b/test_runner/src/test/kotlin/ftl/args/AndroidArgsFileTest.kt index 6a8152886d..92e6d43c18 100644 --- a/test_runner/src/test/kotlin/ftl/args/AndroidArgsFileTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/AndroidArgsFileTest.kt @@ -143,8 +143,8 @@ class AndroidArgsFileTest { val config = configWithTestMethods(155, testShards = 40) with(config) { assert(testShards, 40) - assert(testShardChunks.size, 39) - assert(testShardChunks.first().size, 4) + assert(testShardChunks.size, 40) + assert(testShardChunks.first().size, 3) } } diff --git a/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt b/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt index 6a119ac5fd..8851617fea 100644 --- a/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt +++ b/test_runner/src/test/kotlin/ftl/args/ArgsHelperTest.kt @@ -3,12 +3,14 @@ package ftl.args import com.google.common.truth.Truth.assertThat import ftl.args.ArgsHelper.assertFileExists import ftl.args.ArgsHelper.assertGcsFileExists -import ftl.args.ArgsHelper.calculateShards +import ftl.args.ArgsHelper.convertShards import ftl.args.ArgsHelper.createGcsBucket import ftl.args.ArgsHelper.mergeYmlMaps import ftl.args.ArgsHelper.validateTestMethods import ftl.args.yml.GcloudYml import ftl.args.yml.IosGcloudYml +import ftl.shard.TestMethod +import ftl.shard.TestShard import ftl.test.util.FlankTestRunner import ftl.test.util.TestHelper.absolutePath import org.junit.Rule @@ -100,58 +102,6 @@ class ArgsHelperTest { validateTestMethods(testTargets, validTestMethods, "", skipValidation) } - @Test - fun calculateShards_fails_emptyShardChunks() { - exceptionRule.expectMessage("Failed to populate test shard chunks") - calculateShards( - testMethodsToShard = listOf(""), - testMethodsAlwaysRun = listOf(""), - testShards = 1 - ) - } - - @Test - fun calculateShards_succeeds() { - calculateShards( - testMethodsToShard = listOf("a", "b", "c"), - testMethodsAlwaysRun = listOf("c"), - testShards = -1 - ) - } - - @Test - fun calculateShards_emptyTestTargets() { - val tests = listOf( - "class com.example.profile.ProfileTest#testOne", - "class com.example.profile.ProfileTest#testTwo" - ) - val shards = calculateShards( - testMethodsToShard = tests, - testMethodsAlwaysRun = emptyList(), - testShards = -1 - ) - val expectedShards = listOf( - listOf(tests[0]), - listOf(tests[1]) - ) - assertThat(shards).isEqualTo(expectedShards) - } - - @Test - fun calculateShards_packageTarget() { - val shards = calculateShards( - testMethodsToShard = listOf("a", "b", "c"), - testMethodsAlwaysRun = listOf("c"), - testShards = 2 - ) - - val expectedShards = listOf( - listOf("c", "a"), - listOf("c", "b") - ) - assertThat(shards).isEqualTo(expectedShards) - } - @Test fun yamlMapper_exists() { assertThat(ArgsHelper.yamlMapper).isNotNull() @@ -215,5 +165,20 @@ class ArgsHelperTest { fun evaluateInvalidFilePath() { val testApkPath = "~/flank_test_app/invalid_path/app-debug-*.xctestrun" ArgsHelper.evaluateFilePath(testApkPath) + fun convertShardsTest() { + val shards = listOf( + TestShard(3.0, mutableListOf(TestMethod("a", 1.0), TestMethod("b", 2.0))), + TestShard(4.0, mutableListOf(TestMethod("c", 4.0))), + TestShard(5.0, mutableListOf(TestMethod("d", 2.0), TestMethod("e", 3.0))) + ) + + val expected = listOf( + listOf("a", "b"), + listOf("c"), + listOf("d", "e") + ) + + assertThat(convertShards(shards)).isEqualTo(expected) + } } } diff --git a/test_runner/src/test/kotlin/ftl/shard/ShardTest.kt b/test_runner/src/test/kotlin/ftl/shard/ShardTest.kt index 4c1bf24551..c1279e0166 100644 --- a/test_runner/src/test/kotlin/ftl/shard/ShardTest.kt +++ b/test_runner/src/test/kotlin/ftl/shard/ShardTest.kt @@ -1,6 +1,9 @@ package ftl.shard import com.google.common.truth.Truth.assertThat +import ftl.reports.xml.model.JUnitTestCase +import ftl.reports.xml.model.JUnitTestResult +import ftl.reports.xml.model.JUnitTestSuite import ftl.test.util.FlankTestRunner import org.junit.Test import org.junit.runner.RunWith @@ -8,43 +11,84 @@ import org.junit.runner.RunWith @RunWith(FlankTestRunner::class) class ShardTest { - private val a1 = TestMethod("a", 1.0) - private val b2 = TestMethod("b", 2.0) - private val c3 = TestMethod("c", 3.0) - private val d4 = TestMethod("d", 4.0) - private val e999 = TestMethod("d", 999.0) - private val rawData = mutableListOf(a1, b2, c3, d4) + private fun sample(): JUnitTestResult { + + val testCases = mutableListOf( + JUnitTestCase("a", "a", "1.0"), + JUnitTestCase("b", "b", "2.0"), + JUnitTestCase("c", "c", "4.0"), + JUnitTestCase("d", "d", "6.0"), + JUnitTestCase("e", "e", "0.5"), + JUnitTestCase("f", "f", "2.0"), + JUnitTestCase("g", "g", "1.0") + ) + + val suite1 = JUnitTestSuite("", "-1", "-1", "-1", "-1", "-1", "-1", "-1", testCases, null, null, null) + val suite2 = JUnitTestSuite("", "-1", "-1", "-1", "-1", "-1", "-1", "-1", mutableListOf(), null, null, null) + + return JUnitTestResult(mutableListOf(suite1, suite2)) + } + + @Test + fun oneTestPerShard() { + val reRunTestsToRun = listOf("a", "b", "c", "d", "e", "f", "g") + val suite = sample() + val result = Shard.calculateShardsByTime(reRunTestsToRun, suite, 100) + + assertThat(result.size).isEqualTo(7) + result.forEach { + assertThat(it.testMethods.size).isEqualTo(1) + } + } @Test - fun fillShard_1234() { - val testMethods = mutableListOf().apply { addAll(rawData) } - val startSize = testMethods.size - var index = 1 + fun sampleTest() { + val reRunTestsToRun = listOf("a#a", "b#b", "c#c", "d#d", "e#e", "f#f", "g#g") + val suite = sample() + val result = Shard.calculateShardsByTime(reRunTestsToRun, suite, 3) - while (testMethods.iterator().hasNext()) { - val testMethod = testMethods.iterator().next() + assertThat(result.size).isEqualTo(3) + result.forEach { + assertThat(it.testMethods).isNotEmpty() + } - assertThat(Shard.build(testMethods, testMethod.time).testMethods).isEqualTo(listOf(testMethod)) - assertThat(testMethods.size).isEqualTo(startSize - index) + assertThat(result.sumByDouble { it.time }).isEqualTo(16.5) - index += 1 + val testNames = mutableListOf() + result.forEach { + it.testMethods.forEach { + testNames.add(it.name) + } } - assertThat(testMethods).isEmpty() + testNames.sort() + assertThat(testNames).isEqualTo(listOf("a#a", "b#b", "c#c", "d#d", "e#e", "f#f", "g#g")) } @Test - fun fillShard_10() { - val testMethods = mutableListOf().apply { addAll(rawData) } + fun firstRun() { + val testsToRun = listOf("a", "b", "c") + val result = Shard.calculateShardsByTime(testsToRun, JUnitTestResult(null), 2) + + assertThat(result.size).isEqualTo(2) + assertThat(result.sumByDouble { it.time }).isEqualTo(30.0) - val totalTime = testMethods.sumByDouble { it.time } - val actual = Shard.build(testMethods, totalTime).testMethods - assertThat(actual).isEqualTo(listOf(a1, b2, c3, d4)) + val ordered = result.sortedBy { it.testMethods.size } + assertThat(ordered[0].testMethods.size).isEqualTo(1) + assertThat(ordered[1].testMethods.size).isEqualTo(2) } @Test - fun fillShard_999() { - val actual = Shard.build(mutableListOf(e999), 0.0).testMethods - assertThat(actual).isEqualTo(listOf(e999)) + fun mixedNewAndOld() { + val testsToRun = listOf("a#a", "b#b", "c#c", "w", "y", "z") + val result = Shard.calculateShardsByTime(testsToRun, sample(), 4) + assertThat(result.size).isEqualTo(4) + assertThat(result.sumByDouble { it.time }).isEqualTo(37.0) + + val ordered = result.sortedBy { it.testMethods.size } + assertThat(ordered[0].testMethods.size).isEqualTo(1) + assertThat(ordered[1].testMethods.size).isEqualTo(1) + assertThat(ordered[2].testMethods.size).isEqualTo(1) + assertThat(ordered[3].testMethods.size).isEqualTo(3) } }