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

Add AndroidTestContext as base data for dump shards & test execution #817

Merged
merged 9 commits into from
Jun 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions release_notes.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## next (unreleased)

- [#817](https://github.com/Flank/flank/pull/817) Add AndroidTestContext as base data for dump shards & test execution. ([jan-gogo](https://github.com/jan-gogo))
- [#801](https://github.com/Flank/flank/pull/801) Omit missing app apk if additional-app-test-apks specified. ([jan-gogo](https://github.com/jan-gogo))
- [#784](https://github.com/Flank/flank/pull/784) Add output-style option. ([jan-gogo](https://github.com/jan-gogo))
- [#779](https://github.com/Flank/flank/pull/779) Print retries & display additional info. ([jan-gogo](https://github.com/jan-gogo))
Expand Down
11 changes: 8 additions & 3 deletions test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class AndroidArgs(
"Option num-uniform-shards cannot be specified along with max-test-shards. Use only one of them."
)

if (!(isRoboTest xor isInstrumentationTest)) throw FlankFatalError(
if (!(isRoboTest or isInstrumentationTest)) throw FlankFatalError(
"One of following options must be specified [test, robo-directives, robo-script]."
)

Expand All @@ -138,8 +138,13 @@ class AndroidArgs(
outputStyle
}

val isInstrumentationTest get() = testApk != null || additionalAppTestApks.run { isNotEmpty() && all { (app, _) -> app != null } }
val isRoboTest get() = roboDirectives.isNotEmpty() || roboScript != null
val isInstrumentationTest
get() = appApk != null && testApk != null ||
additionalAppTestApks.isNotEmpty() &&
(appApk != null || additionalAppTestApks.all { (app, _) -> app != null })
private val isRoboTest
get() = appApk != null &&
(roboDirectives.isNotEmpty() || roboScript != null)

private fun assertDeviceSupported(device: Device) {
when (val deviceConfigTest = AndroidCatalog.supportedDeviceConfig(device.model, device.version, this.project)) {
Expand Down
12 changes: 0 additions & 12 deletions test_runner/src/main/kotlin/ftl/args/yml/AndroidFlankYml.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,6 @@ data class AppTestPair(
val test: String
)

data class ResolvedApks(
val app: String,
val test: String?,
val additionalApks: List<String> = emptyList()
)

data class UploadedApks(
val app: String,
val test: String?,
val additionalApks: List<String> = emptyList()
)

/** Flank specific parameters for Android */
@JsonIgnoreProperties(ignoreUnknown = true)
class AndroidFlankYmlParams(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ class AndroidRunCommand : CommonRunCommand(), Runnable {

val config = AndroidArgs.load(Paths.get(configPath), cli = this)

if (dumpShards) {
dumpShards(config)
} else runBlocking {
newTestRun(config)
runBlocking {
if (dumpShards)
dumpShards(config) else
newTestRun(config)
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion keeping curly braces is more redable

runBlocking {
            if (dumpShards)
                dumpShards(config) else
                newTestRun(config)
      }

vs

runBlocking {
            if (dumpShards) {
                dumpShards(config)
            } else {
                newTestRun(config)
            }
        }

or

override fun run() {
        if (dryRun) {
            MockServer.start()
        }
        val config = AndroidArgs.load(Paths.get(configPath), cli = this)
        runProcess(config)
    }

    private fun runProcess(config: AndroidArgs) = runBlocking {
        if (dumpShards) {
            dumpShards(config)
        } else {
            newTestRun(config)
        }
    }

Tell me what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curly braces make sense when u need to open new scope for more than one operation. Otherwise I prefer clear declarative expressions where curly braces are redundant.

}
}

Expand Down
16 changes: 11 additions & 5 deletions test_runner/src/main/kotlin/ftl/run/DumpShards.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,27 @@ import ftl.util.FlankFatalError
import java.nio.file.Files
import java.nio.file.Paths

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

fun dumpShards(args: IosArgs) {
fun dumpShards(
args: IosArgs,
shardFilePath: String = IOS_SHARD_FILE
) {
saveShardChunks(
shardFilePath = IOS_SHARD_FILE,
shardFilePath = shardFilePath,
shards = args.testShardChunks,
size = args.testShardChunks.size
)
Expand Down
17 changes: 17 additions & 0 deletions test_runner/src/main/kotlin/ftl/run/model/AndroidTestContext.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package ftl.run.model

import ftl.args.ShardChunks
import ftl.util.FileReference

sealed class AndroidTestContext

data class InstrumentationTestContext(
val app: FileReference,
val test: FileReference,
val shards: ShardChunks = emptyList()
) : AndroidTestContext()

data class RoboTestContext(
val app: FileReference,
val roboScript: FileReference
) : AndroidTestContext()

This file was deleted.

64 changes: 20 additions & 44 deletions test_runner/src/main/kotlin/ftl/run/platform/RunAndroidTests.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,16 @@ 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.ShardChunks
import ftl.run.platform.android.getAndroidShardChunks
import ftl.args.yml.ResolvedApks
import ftl.gc.GcAndroidDevice
import ftl.gc.GcAndroidTestMatrix
import ftl.gc.GcToolResults
import ftl.http.executeWithRetry
import ftl.run.model.InstrumentationTestContext
import ftl.run.model.TestResult
import ftl.run.platform.android.createAndroidTestConfig
import ftl.run.platform.android.resolveApks
import ftl.run.platform.android.uploadApks
import ftl.run.platform.android.createAndroidTestContexts
import ftl.run.platform.android.upload
import ftl.run.platform.android.uploadAdditionalApks
import ftl.run.platform.android.uploadOtherFiles
import ftl.run.platform.common.afterRunTests
import ftl.run.platform.common.beforeRunMessage
Expand All @@ -37,47 +36,26 @@ internal suspend fun runAndroidTests(args: AndroidArgs): TestResult = coroutineS

val history = GcToolResults.createToolResultsHistory(args)
val otherGcsFiles = args.uploadOtherFiles(runGcsPath)
val additionalApks = args.uploadAdditionalApks(runGcsPath)

args.resolveApks().forEachIndexed { index: Int, apks: ResolvedApks ->
val testShards = apks.test?.let { test ->
getAndroidShardChunks(args, test)
}
// We can't return if testShards is null since it can be a robo test.
testShards?.let {
val shardsWithAtLeastOneTest = testShards.filterAtLeastOneTest()
if (shardsWithAtLeastOneTest.isEmpty()) {
// No tests to run, skipping the execution.
return@forEachIndexed
args.createAndroidTestContexts()
.upload(args.resultsBucket, runGcsPath)
.forEachIndexed { index, context ->
if (context is InstrumentationTestContext) allTestShardChunks += context.shards
val androidTestConfig = args.createAndroidTestConfig(context)
testMatrices += executeAndroidTestMatrix(runCount = args.repeatTests) {
GcAndroidTestMatrix.build(
androidTestConfig = androidTestConfig,
runGcsPath = "$runGcsPath/matrix_$index/",
additionalApkGcsPaths = additionalApks,
androidDeviceList = androidDeviceList,
args = args,
otherFiles = otherGcsFiles,
toolResultsHistory = history
)
}
allTestShardChunks += shardsWithAtLeastOneTest
}

val uploadedApks = uploadApks(
apks = apks,
args = args,
runGcsPath = runGcsPath
)

val androidTestConfig = args.createAndroidTestConfig(
uploadedApks = uploadedApks,
testShards = testShards,
runGcsPath = runGcsPath,
keepTestTargetsEmpty = args.disableSharding && args.testTargets.isEmpty()
)

testMatrices += executeAndroidTestMatrix(runCount = args.repeatTests) {
GcAndroidTestMatrix.build(
androidTestConfig = androidTestConfig,
runGcsPath = "$runGcsPath/matrix_$index/",
additionalApkGcsPaths = uploadedApks.additionalApks,
androidDeviceList = androidDeviceList,
args = args,
otherFiles = otherGcsFiles,
toolResultsHistory = history
)
}
}

if (testMatrices.isEmpty()) throw FlankCommonException("There are no tests to run.")

println(beforeRunMessage(args, allTestShardChunks))
Expand All @@ -95,5 +73,3 @@ private suspend fun executeAndroidTestMatrix(
}
}
}

private fun ShardChunks.filterAtLeastOneTest(): ShardChunks = filter { chunk -> chunk.isNotEmpty() }
Original file line number Diff line number Diff line change
@@ -1,25 +1,13 @@
package ftl.run.platform.android

import ftl.args.AndroidArgs
import ftl.args.ShardChunks
import ftl.args.yml.UploadedApks
import ftl.util.FlankFatalError
import ftl.run.model.AndroidTestContext
import ftl.run.model.InstrumentationTestContext
import ftl.run.model.RoboTestContext

internal fun AndroidArgs.createAndroidTestConfig(
uploadedApks: UploadedApks,
testShards: ShardChunks? = null,
runGcsPath: String? = null,
keepTestTargetsEmpty: Boolean = false
): AndroidTestConfig = when {
isInstrumentationTest -> createInstrumentationConfig(
uploadedApks = uploadedApks,
keepTestTargetsEmpty = keepTestTargetsEmpty,
testShards = testShards ?: throw FlankFatalError("Arg testShards is required for instrumentation test.")
)
isRoboTest
-> createRoboConfig(
uploadedApks = uploadedApks,
runGcsPath = runGcsPath ?: throw FlankFatalError("Arg runGcsPath is required for robo test.")
)
else -> throw FlankFatalError("Cannot create AndroidTestConfig, invalid AndroidArgs.")
testContext: AndroidTestContext
): AndroidTestConfig = when (testContext) {
is InstrumentationTestContext -> createInstrumentationConfig(testContext)
is RoboTestContext -> createRoboConfig(testContext)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package ftl.run.platform.android

import com.linkedin.dex.parser.DexParser
import ftl.args.AndroidArgs
import ftl.args.ArgsHelper
import ftl.config.FtlConstants
import ftl.filter.TestFilter
import ftl.filter.TestFilters
import ftl.run.model.AndroidTestContext
import ftl.run.model.InstrumentationTestContext
import ftl.util.FlankTestMethod
import ftl.util.downloadIfNeeded
import kotlinx.coroutines.async
import kotlinx.coroutines.awaitAll
import kotlinx.coroutines.coroutineScope
import java.io.File

suspend fun AndroidArgs.createAndroidTestContexts(): List<AndroidTestContext> = resolveApks().setupShards(this)

private suspend fun List<AndroidTestContext>.setupShards(
args: AndroidArgs,
testFilter: TestFilter = TestFilters.fromTestTargets(args.testTargets)
): List<AndroidTestContext> = coroutineScope {
map { testContext ->
async {
if (testContext !is InstrumentationTestContext) testContext
else testContext.downloadApks().calculateShards(
args = args,
testFilter = testFilter
)
}
}.awaitAll().dropEmptyInstrumentationTest()
}

private fun InstrumentationTestContext.downloadApks(): InstrumentationTestContext = copy(
app = app.downloadIfNeeded(),
test = test.downloadIfNeeded()
)

private fun InstrumentationTestContext.calculateShards(
args: AndroidArgs,
testFilter: TestFilter = TestFilters.fromTestTargets(args.testTargets)
): InstrumentationTestContext = copy(
shards = ArgsHelper.calculateShards(
filteredTests = getFlankTestMethods(testFilter),
args = args,
forcedShardCount = args.numUniformShards
).filter { it.isNotEmpty() }
)

private fun InstrumentationTestContext.getFlankTestMethods(
testFilter: TestFilter
): List<FlankTestMethod> =
DexParser.findTestMethods(test.local).asSequence().distinct().filter(testFilter.shouldRun).map { testMethod ->
FlankTestMethod(
testName = "class ${testMethod.testName}",
ignored = testMethod.annotations.any { it.name == "org.junit.Ignore" }
)
}.toList()

private fun List<AndroidTestContext>.dropEmptyInstrumentationTest(): List<AndroidTestContext> =
filterIsInstance<InstrumentationTestContext>().filter { it.shards.isEmpty() }.let { withoutTests ->
if (withoutTests.isNotEmpty())
printNoTests(withoutTests)
minus(withoutTests)
}

private fun printNoTests(testApks: List<InstrumentationTestContext>) {
val testApkNames = testApks.joinToString(", ") { pathname -> File(pathname.test.local).name }
println("${FtlConstants.indent}No tests for $testApkNames")
println()
}
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
package ftl.run.platform.android

import ftl.args.AndroidArgs
import ftl.args.ShardChunks
import ftl.args.yml.UploadedApks
import ftl.run.model.InstrumentationTestContext

internal fun AndroidArgs.createInstrumentationConfig(
uploadedApks: UploadedApks,
keepTestTargetsEmpty: Boolean,
testShards: ShardChunks
testApk: InstrumentationTestContext
) = AndroidTestConfig.Instrumentation(
appApkGcsPath = uploadedApks.app,
testApkGcsPath = uploadedApks.test!!,
appApkGcsPath = testApk.app.gcs,
testApkGcsPath = testApk.test.gcs,
testRunnerClass = testRunnerClass,
orchestratorOption = "USE_ORCHESTRATOR".takeIf { useOrchestrator },
disableSharding = disableSharding,
numUniformShards = numUniformShards,
testShards = testShards,
keepTestTargetsEmpty = keepTestTargetsEmpty
testShards = testApk.shards,
keepTestTargetsEmpty = disableSharding && testTargets.isEmpty()
)
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
package ftl.run.platform.android

import ftl.args.AndroidArgs
import ftl.args.yml.UploadedApks
import ftl.gc.GcStorage
import ftl.run.model.RoboTestContext

internal fun AndroidArgs.createRoboConfig(
uploadedApks: UploadedApks,
runGcsPath: String
testApk: RoboTestContext
) = AndroidTestConfig.Robo(
appApkGcsPath = uploadedApks.app,
appApkGcsPath = testApk.app.gcs,
flankRoboDirectives = roboDirectives,
roboScriptGcsPath = roboScript?.let {
GcStorage.upload(it, resultsBucket, runGcsPath)
}
roboScriptGcsPath = testApk.roboScript.gcs
)
Loading