Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dump shards #794

Merged
merged 11 commits into from
May 25, 2020
1 change: 1 addition & 0 deletions release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- [#781](https://github.com/Flank/flank/pull/781) Remove local exists check on cloud results-dir. Fixes crash when results-dir is set by the user. ([adamfilipow92](https://github.com/adamfilipow92))
- [#656](https://github.com/Flank/flank/issues/656) Improve error message reporting. ([adamfilipow92](https://github.com/adamfilipow92))
- [#783](https://github.com/Flank/flank/pull/783) Use legacy results for iOS by default. ([pawelpasterz](https://github.com/pawelpasterz))
- [#794](https://github.com/Flank/flank/pull/794) Enhance `--dump-shards` to dump shards from all test apks ([bootstraponline](https://github.com/bootstraponline), [jan-gogo](https://github.com/jan-gogo))

## v20.05.1

Expand Down
51 changes: 0 additions & 51 deletions test_runner/src/main/kotlin/ftl/args/AndroidTestShard.kt

This file was deleted.

8 changes: 6 additions & 2 deletions test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,11 @@ object ArgsHelper {
return ArgsFileVisitor("glob:$filePath").walk(searchDir)
}

fun calculateShards(filteredTests: List<FlankTestMethod>, args: IArgs): ShardChunks {
fun calculateShards(
filteredTests: List<FlankTestMethod>,
args: IArgs,
forcedShardCount: Int? = null
): ShardChunks {
if (filteredTests.isEmpty()) {
// Avoid unnecessary computing if we already know there aren't tests to run.
return listOf(emptyList())
Expand All @@ -234,7 +238,7 @@ object ArgsHelper {
listOf(filteredTests.map { it.testName }.toMutableList())
} else {
val oldTestResult = GcStorage.downloadJunitXml(args) ?: JUnitTestResult(mutableListOf())
val shardCount = Shard.shardCountByTime(filteredTests, oldTestResult, args)
val shardCount = forcedShardCount ?: Shard.shardCountByTime(filteredTests, oldTestResult, args)
Shard.createShardsByShardCount(filteredTests, oldTestResult, args, shardCount).stringShards()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ internal object ConfigurationErrorParser {
//region regex patterns
private val propertyNameRegex = "(?<=property\\s)[a-z]*".toRegex()
private val referenceChainRegex = "(?<=chain:\\s).*(?=[)])".toRegex()
private val referenceChainCleanUpRegex = "(?<=[\\[])\"?[\\w]*\"?(?=])".toRegex()
private val referenceChainCleanUpRegex = "(?<=[\\[])\"?(\\w|-)*\"?(?=])".toRegex()
private val lineAndColumnRegex = "((?<=line:\\s)\\d*), column:\\s(\\d*)".toRegex()
//endregion

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package ftl.cli.firebase.test.android

import ftl.args.AndroidArgs
import ftl.args.AndroidTestShard
import ftl.args.ShardChunks
import ftl.args.yml.AppTestPair
import ftl.cli.firebase.test.CommonRunCommand
import ftl.config.Device
Expand All @@ -12,12 +10,12 @@ import ftl.config.FtlConstants.defaultAndroidVersion
import ftl.config.FtlConstants.defaultLocale
import ftl.config.FtlConstants.defaultOrientation
import ftl.mock.MockServer
import ftl.run.common.prettyPrint
import ftl.run.ANDROID_SHARD_FILE
import ftl.run.dumpShards
import ftl.run.newTestRun
import kotlinx.coroutines.runBlocking
import picocli.CommandLine.Command
import picocli.CommandLine.Option
import java.nio.file.Files
import java.nio.file.Paths

@Command(
Expand Down Expand Up @@ -45,25 +43,18 @@ class AndroidRunCommand : CommonRunCommand(), Runnable {
val config = AndroidArgs.load(Paths.get(configPath), cli = this)

if (dumpShards) {
val testShardChunks: ShardChunks = AndroidTestShard.getTestShardChunks(config, config.testApk!!)
val testShardChunksJson: String = prettyPrint.toJson(testShardChunks)

Files.write(Paths.get(shardFile), testShardChunksJson.toByteArray())
println("Saved shards to $shardFile")
} else {
runBlocking {
newTestRun(config)
}
dumpShards(config)
} else runBlocking {
newTestRun(config)
}
}

companion object {
private const val shardFile = "android_shards.json"
}

// Flank debug

@Option(names = ["--dump-shards"], description = ["Dumps the shards to $shardFile for debugging"])
@Option(
names = ["--dump-shards"],
description = ["Measures test shards from given test apks and writes them into $ANDROID_SHARD_FILE file instead of executing."]
)
var dumpShards: Boolean = false

// Flank specific
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ import ftl.config.FtlConstants
import ftl.config.FtlConstants.defaultIosModel
import ftl.config.FtlConstants.defaultIosVersion
import ftl.mock.MockServer
import ftl.run.common.prettyPrint
import ftl.run.IOS_SHARD_FILE
import ftl.run.dumpShards
import ftl.run.newTestRun
import kotlinx.coroutines.runBlocking
import picocli.CommandLine.Command
import picocli.CommandLine.Option
import java.nio.file.Files
import java.nio.file.Paths

@Command(
Expand All @@ -39,23 +39,18 @@ class IosRunCommand : CommonRunCommand(), Runnable {
val config = IosArgs.load(Paths.get(configPath), cli = this)

if (dumpShards) {
val testShardChunksJson: String = prettyPrint.toJson(config.testShardChunks)
Files.write(Paths.get(shardFile), testShardChunksJson.toByteArray())
println("Saved shards to $shardFile")
} else {
runBlocking {
newTestRun(config)
}
dumpShards(config)
} else runBlocking {
newTestRun(config)
}
}

companion object {
private const val shardFile = "ios_shards.json"
}

// Flank debug

@Option(names = ["--dump-shards"], description = ["Dumps the shards to $shardFile for debugging"])
@Option(
names = ["--dump-shards"],
description = ["Measures test shards from given test apks and writes them into $IOS_SHARD_FILE file instead of executing."]
)
var dumpShards: Boolean = false

// Flank specific
Expand Down
45 changes: 45 additions & 0 deletions test_runner/src/main/kotlin/ftl/run/DumpShards.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package ftl.run

import ftl.args.AndroidArgs
import ftl.args.IosArgs
import ftl.run.common.prettyPrint
import ftl.run.model.AndroidMatrixTestShards
import ftl.run.platform.android.getAndroidMatrixShards
import ftl.util.FlankFatalError
import java.nio.file.Files
import java.nio.file.Paths

fun dumpShards(args: AndroidArgs) {
if (!args.isInstrumentationTest) throw FlankFatalError(
"Cannot dump shards for non instrumentation test, ensure test apk has been set."
)
val shards: AndroidMatrixTestShards = getAndroidMatrixShards(args)
saveShardChunks(
shardFilePath = ANDROID_SHARD_FILE,
shards = shards,
size = shards.size
)
}

fun dumpShards(args: IosArgs) {
saveShardChunks(
shardFilePath = IOS_SHARD_FILE,
shards = args.testShardChunks,
size = args.testShardChunks.size
)
}

private fun saveShardChunks(
shardFilePath: String,
shards: Any,
size: Int
) {
Files.write(
Paths.get(shardFilePath),
prettyPrint.toJson(shards).toByteArray()
)
println("Saved $size shards to $shardFilePath")
}

const val ANDROID_SHARD_FILE = "android_shards.json"
const val IOS_SHARD_FILE = "ios_shards.json"
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package ftl.run.model

typealias AndroidMatrixTestShards = Map<String, AndroidTestShards>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package ftl.run.model

data class AndroidTestShards(
val app: String,
val test: String,
val shards: Map<String, List<String>> = emptyMap()
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package ftl.run.model

import ftl.util.FileReference

data class InstrumentationTestApk(
val app: FileReference = FileReference(),
val test: FileReference = FileReference()
)
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ package ftl.run.platform
import com.google.api.services.testing.Testing
import com.google.api.services.testing.model.TestMatrix
import ftl.args.AndroidArgs
import ftl.args.AndroidTestShard
import ftl.args.ShardChunks
import ftl.run.platform.android.getAndroidShardChunks
import ftl.args.yml.ResolvedApks
import ftl.gc.GcAndroidDevice
import ftl.gc.GcAndroidTestMatrix
Expand Down Expand Up @@ -40,7 +40,7 @@ internal suspend fun runAndroidTests(args: AndroidArgs): TestResult = coroutineS

args.resolveApks().forEachIndexed { index: Int, apks: ResolvedApks ->
val testShards = apks.test?.let { test ->
AndroidTestShard.getTestShardChunks(args, test)
getAndroidShardChunks(args, test)
}
// We can't return if testShards is null since it can be a robo test.
testShards?.let {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package ftl.run.platform.android

import ftl.args.AndroidArgs
import ftl.args.ShardChunks
import ftl.run.model.AndroidMatrixTestShards
import ftl.run.model.AndroidTestShards
import ftl.run.model.InstrumentationTestApk
import ftl.util.asFileReference

fun getAndroidMatrixShards(
args: AndroidArgs
): AndroidMatrixTestShards =
getInstrumentationShardChunks(
args = args,
testApks = args.createInstrumentationTestApks()
).asMatrixTestShards()

private fun AndroidArgs.createInstrumentationTestApks(): List<InstrumentationTestApk> =
listOfNotNull(
testApk?.let { testApk ->
InstrumentationTestApk(
app = appApk.asFileReference(),
test = testApk.asFileReference()
)
}
) + additionalAppTestApks.map {
InstrumentationTestApk(
app = (it.app ?: appApk).asFileReference(),
test = it.test.asFileReference()
)
}

private fun Map<InstrumentationTestApk, ShardChunks>.asMatrixTestShards(): AndroidMatrixTestShards =
map { (testApks, shards: List<List<String>>) ->
AndroidTestShards(
app = testApks.app.local,
test = testApks.test.local,
shards = shards.mapIndexed { index, testCases ->
"shard-$index" to testCases
}.toMap()
)
}.mapIndexed { index, androidTestShards ->
"matrix-$index" to androidTestShards
}.toMap()
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package ftl.run.platform.android

import ftl.args.AndroidArgs
import ftl.args.ShardChunks
import ftl.run.model.InstrumentationTestApk
import ftl.util.asFileReference

fun getAndroidShardChunks(
args: AndroidArgs,
testApk: String
): ShardChunks =
getInstrumentationShardChunks(
args = args,
testApks = listOf(InstrumentationTestApk(test = testApk.asFileReference()))
).flatMap { (_, shardChunks) -> shardChunks }
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package ftl.run.platform.android

import com.linkedin.dex.parser.DexParser
import ftl.args.AndroidArgs
import ftl.args.ArgsHelper
import ftl.args.ShardChunks
import ftl.config.FtlConstants
import ftl.filter.TestFilter
import ftl.filter.TestFilters
import ftl.run.model.InstrumentationTestApk
import ftl.util.FlankTestMethod
import ftl.util.downloadIfNeeded
import java.io.File

fun getInstrumentationShardChunks(
args: AndroidArgs,
testApks: List<InstrumentationTestApk>
): Map<InstrumentationTestApk, ShardChunks> =
getFlankTestMethods(
testApks = testApks.download(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have concurrent download but I guess it's for another PR :)
cc @bootstraponline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about concurrent download, but decided to leave as is due to additional changes in tests. But I can add it easily. @bootstraponline ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think concurrent downloads would be nice

testFilter = TestFilters.fromTestTargets(args.testTargets)
).mapValues { (_, testMethods: List<FlankTestMethod>) ->
if (testMethods.isNotEmpty()) {
ArgsHelper.calculateShards(testMethods, args, args.numUniformShards)
} else {
printNoTests(testApks)
emptyList()
}
}

private fun getFlankTestMethods(
testApks: List<InstrumentationTestApk>,
testFilter: TestFilter
): Map<InstrumentationTestApk, List<FlankTestMethod>> =
testApks.associateWith { testApk ->
DexParser.findTestMethods(testApk.test.local).asSequence().distinct().filter(testFilter.shouldRun).map { testMethod ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about more functional styling here, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only function calls, no value assignments, no ordinary loops, even no conditions so I have no idea how to do it more functional :). Pls explain me by example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well my comment was just cosmetic one. Here we have invocation chain in one line, I was thinking about splitting it to new line each. So as I said 100% cosmetic

FlankTestMethod(
testName = "class ${testMethod.testName}",
ignored = testMethod.annotations.any { it.name == "org.junit.Ignore" }
)
}.toList()
}

private fun List<InstrumentationTestApk>.download(): List<InstrumentationTestApk> =
map { reference ->
reference.copy(
app = reference.app.downloadIfNeeded(),
test = reference.test.downloadIfNeeded()
)
}

private fun printNoTests(testApks: List<InstrumentationTestApk>) {
val testApkNames = testApks.joinToString(", ") { pathname -> File(pathname.test.local).name }
println("${FtlConstants.indent}No tests for $testApkNames")
}
Loading