Refactor config entities and arguments (#831)

* Refactor config entities and arguments

release_notes.md

@@ -1,5 +1,6 @@
 ## next (unreleased)
 
+- [#831](https://github.com/Flank/flank/pull/831) Refactor config entities and arguments. ([jan-gogo](https://github.com/jan-gogo))
 - [#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))

test_runner/src/main/kotlin/ftl/args/AndroidArgs.kt

@@ -1,162 +1,28 @@
 package ftl.args
 
-import com.google.common.annotations.VisibleForTesting
-import ftl.android.AndroidCatalog
-import ftl.android.IncompatibleModelVersion
-import ftl.android.SupportedDeviceConfig
-import ftl.android.UnsupportedModelId
-import ftl.android.UnsupportedVersionId
-import ftl.args.ArgsHelper.assertCommonProps
-import ftl.args.ArgsHelper.assertFileExists
-import ftl.args.ArgsHelper.assertGcsFileExists
-import ftl.args.ArgsHelper.createGcsBucket
-import ftl.args.ArgsHelper.createJunitBucket
-import ftl.args.ArgsHelper.evaluateFilePath
-import ftl.args.ArgsHelper.mergeYmlMaps
-import ftl.args.ArgsHelper.yamlMapper
-import ftl.args.ArgsToString.apksToString
-import ftl.args.ArgsToString.listToString
-import ftl.args.ArgsToString.mapToString
-import ftl.args.ArgsToString.objectsToString
-import ftl.args.yml.AndroidFlankYml
-import ftl.args.yml.AndroidGcloudYml
-import ftl.args.yml.FlankYml
-import ftl.args.yml.GcloudYml
 import ftl.args.yml.AppTestPair
-import ftl.args.yml.AndroidGcloudYmlParams
-import ftl.args.yml.YamlDeprecated
-import ftl.util.loadFile
-import ftl.cli.firebase.test.android.AndroidRunCommand
-import ftl.config.Device
-import ftl.config.FtlConstants
-import ftl.config.parseRoboDirectives
-import ftl.run.status.asOutputStyle
-import ftl.util.FlankFatalError
-import java.io.File
-import ftl.util.uniqueObjectName
-import java.io.Reader
-import java.nio.file.Path
 
-// set default values, init properties, etc.
-class AndroidArgs(
-    gcloudYml: GcloudYml,
-    androidGcloudYml: AndroidGcloudYml,
-    flankYml: FlankYml,
-    androidFlankYml: AndroidFlankYml,
-    override val data: String,
-    val cli: AndroidRunCommand? = null
-) : IArgs {
+data class AndroidArgs(
+    val commonArgs: CommonArgs,
+    val appApk: String?,
+    val testApk: String?,
+    val additionalApks: List<String>,
+    val autoGoogleLogin: Boolean,
+    val useOrchestrator: Boolean,
+    val roboDirectives: List<FlankRoboDirective>,
+    val roboScript: String?,
+    val environmentVariables: Map<String, String>,
+    val directoriesToPull: List<String>,
+    val otherFiles: Map<String, String>,
+    val performanceMetrics: Boolean,
+    val numUniformShards: Int?,
+    val testRunnerClass: String?,
+    val testTargets: List<String>,
+    val additionalAppTestApks: List<AppTestPair>,
+    override val useLegacyJUnitResult: Boolean
+) : IArgs by commonArgs {
+    companion object : AndroidArgsCompanion()
 
-    private val gcloud = gcloudYml.gcloud
-    override val resultsBucket: String
-    override val resultsDir = (cli?.resultsDir ?: gcloud.resultsDir) ?: uniqueObjectName()
-    override val recordVideo = cli?.recordVideo ?: cli?.noRecordVideo?.not() ?: gcloud.recordVideo
-    override val testTimeout = cli?.timeout ?: gcloud.timeout
-    override val async = cli?.async ?: gcloud.async
-    override val resultsHistoryName = cli?.resultsHistoryName ?: gcloud.resultsHistoryName
-    override val flakyTestAttempts = cli?.flakyTestAttempts ?: gcloud.flakyTestAttempts
-
-    private val androidGcloud = androidGcloudYml.gcloud
-    var appApk: String? = (cli?.app ?: androidGcloud.app)?.processFilePath("from app")
-    var testApk: String? = (cli?.test ?: androidGcloud.test)?.processFilePath("from test")
-    val additionalApks = (cli?.additionalApks ?: androidGcloud.additionalApks).map { it.processFilePath("from additional-apks") }
-    val autoGoogleLogin = cli?.autoGoogleLogin ?: cli?.noAutoGoogleLogin?.not() ?: androidGcloud.autoGoogleLogin
-
-    // We use not() on noUseOrchestrator because if the flag is on, useOrchestrator needs to be false
-    val useOrchestrator = cli?.useOrchestrator ?: cli?.noUseOrchestrator?.not() ?: androidGcloud.useOrchestrator
-    val roboDirectives = cli?.roboDirectives?.parseRoboDirectives() ?: androidGcloud.roboDirectives.parseRoboDirectives()
-    val roboScript = (cli?.roboScript ?: androidGcloud.roboScript)?.processFilePath("from roboScript")
-    val environmentVariables = cli?.environmentVariables ?: androidGcloud.environmentVariables
-    val directoriesToPull = cli?.directoriesToPull ?: androidGcloud.directoriesToPull
-    val otherFiles = (cli?.otherFiles ?: androidGcloud.otherFiles).map { (devicePath, filePath) ->
-        devicePath to filePath.processFilePath("from otherFiles")
-    }.toMap()
-    val performanceMetrics = cli?.performanceMetrics ?: cli?.noPerformanceMetrics?.not() ?: androidGcloud.performanceMetrics
-    val numUniformShards = cli?.numUniformShards ?: androidGcloud.numUniformShards
-    val testRunnerClass = cli?.testRunnerClass ?: androidGcloud.testRunnerClass
-    val testTargets = cli?.testTargets ?: androidGcloud.testTargets.filterNotNull()
-    val devices = cli?.device ?: androidGcloud.device
-
-    private val flank = flankYml.flank
-    override val maxTestShards = convertToShardCount(cli?.maxTestShards ?: flank.maxTestShards)
-    override val shardTime = cli?.shardTime ?: flank.shardTime
-    override val repeatTests = cli?.repeatTests ?: flank.repeatTests
-    override val smartFlankGcsPath = cli?.smartFlankGcsPath ?: flank.smartFlankGcsPath
-    override val smartFlankDisableUpload = cli?.smartFlankDisableUpload ?: flank.smartFlankDisableUpload
-    override val testTargetsAlwaysRun = cli?.testTargetsAlwaysRun ?: flank.testTargetsAlwaysRun
-    override val filesToDownload = cli?.filesToDownload ?: flank.filesToDownload
-    override val disableSharding = cli?.disableSharding ?: flank.disableSharding
-    override val project = cli?.project ?: flank.project
-    override val localResultDir = cli?.localResultsDir ?: flank.localResultsDir
-    override val runTimeout = cli?.runTimeout ?: flank.runTimeout
-    override val useLegacyJUnitResult = cli?.useLegacyJUnitResult ?: flank.useLegacyJUnitResult
-    override val fullJUnitResult = cli?.fullJUnitResult ?: flank.fullJUnitResult
-    override val clientDetails = cli?.clientDetails ?: gcloud.clientDetails
-    override val networkProfile = cli?.networkProfile ?: gcloud.networkProfile
-    override val ignoreFailedTests = cli?.ignoreFailedTests ?: flank.ignoreFailedTests
-    override val keepFilePath = cli?.keepFilePath ?: flank.keepFilePath
-    override val outputStyle by lazy { (cli?.outputStyle ?: flank.outputStyle)?.asOutputStyle() ?: defaultOutputStyle }
-
-    private val androidFlank = androidFlankYml.flank
-    val additionalAppTestApks = (cli?.additionalAppTestApks ?: androidFlank.additionalAppTestApks).map { (app, test) ->
-        AppTestPair(
-            app = app?.processFilePath("from additional-app-test-apks.app"),
-            test = test.processFilePath("from additional-app-test-apks.test")
-        )
-    }
-    override val hasMultipleExecutions: Boolean get() = super.hasMultipleExecutions || additionalAppTestApks.isNotEmpty()
-
-    init {
-        if (appApk == null) additionalAppTestApks
-            .filter { (app, _) -> app == null }
-            .map { File(it.test).name }
-            .run {
-                if (isNotEmpty()) throw FlankFatalError("Cannot resolve app apk pair for $this")
-            }
-
-        resultsBucket = createGcsBucket(project, cli?.resultsBucket ?: gcloud.resultsBucket)
-        createJunitBucket(project, flank.smartFlankGcsPath)
-
-        devices.forEach { device -> assertDeviceSupported(device) }
-
-        if (numUniformShards != null && maxTestShards > 1) throw FlankFatalError(
-            "Option num-uniform-shards cannot be specified along with max-test-shards. Use only one of them."
-        )
-
-        if (!(isRoboTest or isInstrumentationTest)) throw FlankFatalError(
-            "One of following options must be specified [test, robo-directives, robo-script]."
-        )
-
-        // Using both roboDirectives and roboScript may hang test execution on FTL
-        if (roboDirectives.isNotEmpty() && roboScript != null) throw FlankFatalError(
-            "Options robo-directives and robo-script are mutually exclusive, use only one of them."
-        )
-
-        assertCommonProps(this)
-
-        // Call lazy outputStyle inside init to get possible error ASAP
-        outputStyle
-    }
-
-    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)) {
-            SupportedDeviceConfig -> {
-            }
-            UnsupportedModelId -> throw RuntimeException("Unsupported model id, '${device.model}'\nSupported model ids: ${AndroidCatalog.androidModelIds(this.project)}")
-            UnsupportedVersionId -> throw RuntimeException("Unsupported version id, '${device.version}'\nSupported Version ids: ${AndroidCatalog.androidVersionIds(this.project)}")
-            is IncompatibleModelVersion -> throw RuntimeException("Incompatible model, '${device.model}', and version, '${device.version}'\nSupported version ids for '${device.model}': $deviceConfigTest")
-        }
-    }
-
-    // Note: environmentVariables may contain secrets and are not printed for security reasons.
     override fun toString(): String {
         return """
 AndroidArgs
@@ -166,24 +32,24 @@ AndroidArgs
       record-video: $recordVideo
       timeout: $testTimeout
       async: $async
-      client-details: ${mapToString(clientDetails)}
+      client-details: ${ArgsToString.mapToString(clientDetails)}
       network-profile: $networkProfile
       results-history-name: $resultsHistoryName
       # Android gcloud
       app: $appApk
       test: $testApk
-      additional-apks: ${listToString(additionalApks)}
+      additional-apks: ${ArgsToString.listToString(additionalApks)}
       auto-google-login: $autoGoogleLogin
       use-orchestrator: $useOrchestrator
-      directories-to-pull:${listToString(directoriesToPull)}
-      other-files:${mapToString(otherFiles)}
+      directories-to-pull:${ArgsToString.listToString(directoriesToPull)}
+      other-files:${ArgsToString.mapToString(otherFiles)}
       performance-metrics: $performanceMetrics
       num-uniform-shards: $numUniformShards
       test-runner-class: $testRunnerClass
-      test-targets:${listToString(testTargets)}
-      robo-directives:${objectsToString(roboDirectives)}
+      test-targets:${ArgsToString.listToString(testTargets)}
+      robo-directives:${ArgsToString.objectsToString(roboDirectives)}
       robo-script: $roboScript
-      device:${objectsToString(devices)}
+      device:${ArgsToString.objectsToString(devices)}
       num-flaky-test-attempts: $flakyTestAttempts
 
     flank:
@@ -192,62 +58,28 @@ AndroidArgs
       num-test-runs: $repeatTests
       smart-flank-gcs-path: $smartFlankGcsPath
       smart-flank-disable-upload: $smartFlankDisableUpload
-      files-to-download:${listToString(filesToDownload)}
-      test-targets-always-run:${listToString(testTargetsAlwaysRun)}
+      files-to-download:${ArgsToString.listToString(filesToDownload)}
+      test-targets-always-run:${ArgsToString.listToString(testTargetsAlwaysRun)}
       disable-sharding: $disableSharding
       project: $project
       local-result-dir: $localResultDir
       full-junit-result: $fullJUnitResult
       # Android Flank Yml
       keep-file-path: $keepFilePath
-      additional-app-test-apks:${apksToString(additionalAppTestApks)}
+      additional-app-test-apks:${ArgsToString.apksToString(additionalAppTestApks)}
       run-timeout: $runTimeout
       legacy-junit-result: $useLegacyJUnitResult
       ignore-failed-tests: $ignoreFailedTests
       output-style: ${outputStyle.name.toLowerCase()}
    """.trimIndent()
     }
-
-    companion object : IArgsCompanion {
-        override val validArgs by lazy {
-            mergeYmlMaps(GcloudYml, AndroidGcloudYml, FlankYml, AndroidFlankYml)
-        }
-
-        fun load(yamlPath: Path, cli: AndroidRunCommand? = null): AndroidArgs = load(loadFile(yamlPath), cli)
-
-        @VisibleForTesting
-        internal fun load(yamlReader: Reader, cli: AndroidRunCommand? = null): AndroidArgs {
-
-            val data = YamlDeprecated.modifyAndThrow(yamlReader, android = true)
-            val flankYml = yamlMapper.readValue(data, FlankYml::class.java)
-            val gcloudYml = yamlMapper.readValue(data, GcloudYml::class.java)
-            val androidGcloudYml = yamlMapper.readValue(data, AndroidGcloudYml::class.java)
-            val androidFlankYml = yamlMapper.readValue(data, AndroidFlankYml::class.java)
-
-            return AndroidArgs(
-                gcloudYml,
-                androidGcloudYml,
-                flankYml,
-                androidFlankYml,
-                data,
-                cli
-            )
-        }
-
-        fun default(): AndroidArgs {
-            return AndroidArgs(
-                GcloudYml(),
-                AndroidGcloudYml(AndroidGcloudYmlParams(app = ".", test = ".")),
-                FlankYml(),
-                AndroidFlankYml(),
-                "",
-                AndroidRunCommand()
-            )
-        }
-    }
 }
 
-private fun String.processFilePath(name: String): String =
-    if (startsWith(FtlConstants.GCS_PREFIX))
-        this.also { assertGcsFileExists(it) } else
-        evaluateFilePath(this).also { assertFileExists(it, name) }
+val AndroidArgs.isInstrumentationTest
+    get() = appApk != null && testApk != null ||
+            additionalAppTestApks.isNotEmpty() &&
+            (appApk != null || additionalAppTestApks.all { (app, _) -> app != null })
+
+val AndroidArgs.isRoboTest
+    get() = appApk != null &&
+            (roboDirectives.isNotEmpty() || roboScript != null)

test_runner/src/main/kotlin/ftl/args/AndroidArgsCompanion.kt

@@ -0,0 +1,43 @@
+package ftl.args
+
+import com.google.common.annotations.VisibleForTesting
+import ftl.args.yml.mergeYmlKeys
+import ftl.cli.firebase.test.android.AndroidRunCommand
+import ftl.config.android.AndroidFlankConfig
+import ftl.config.android.AndroidGcloudConfig
+import ftl.config.common.CommonFlankConfig
+import ftl.config.common.CommonGcloudConfig
+import ftl.config.defaultAndroidConfig
+import ftl.config.loadAndroidConfig
+import ftl.config.plus
+import ftl.util.loadFile
+import java.io.Reader
+import java.nio.file.Path
+
+open class AndroidArgsCompanion : IArgs.ICompanion {
+    override val validArgs by lazy {
+        mergeYmlKeys(
+            CommonGcloudConfig,
+            AndroidGcloudConfig,
+            CommonFlankConfig,
+            AndroidFlankConfig
+        )
+    }
+
+    fun default() =
+        createAndroidArgs(defaultAndroidConfig())
+
+    fun load(yamlPath: Path, cli: AndroidRunCommand? = null) =
+        load(loadFile(yamlPath), cli)
+
+    @VisibleForTesting
+    internal fun load(yamlReader: Reader, cli: AndroidRunCommand? = null) =
+        createAndroidArgs(
+            config = defaultAndroidConfig() +
+                    loadAndroidConfig(reader = yamlReader) +
+                    cli?.config
+        ).apply {
+            commonArgs.validate()
+            this.validate()
+        }
+}

test_runner/src/main/kotlin/ftl/args/ArgsHelper.kt

@@ -11,7 +11,6 @@ import com.google.cloud.storage.Storage
 import com.google.cloud.storage.StorageClass
 import com.google.cloud.storage.StorageOptions
 import ftl.args.IArgs.Companion.AVAILABLE_SHARD_COUNT_RANGE
-import ftl.args.yml.IYmlMap
 import ftl.args.yml.YamlObjectMapper
 import ftl.config.FtlConstants
 import ftl.config.FtlConstants.GCS_PREFIX
@@ -41,17 +40,6 @@ object ArgsHelper {
         YamlObjectMapper().registerKotlinModule()
     }
 
-    fun mergeYmlMaps(vararg ymlMaps: IYmlMap): Map<String, List<String>> {
-        val result = mutableMapOf<String, List<String>>()
-        ymlMaps.map { it.map }
-            .forEach { map ->
-                map.forEach { (k, v) ->
-                    result.merge(k, v) { a, b -> a + b }
-                }
-            }
-        return result
-    }
-
     fun assertFileExists(file: String, name: String) {
         if (!File(file).exists()) {
             throw FlankFatalError("'$file' $name doesn't exist")
@@ -109,11 +97,7 @@ object ArgsHelper {
         val bucket = gcsURI.authority
         val path = gcsURI.path.drop(1) // Drop leading slash
 
-        val blob = GcStorage.storage.get(bucket, path)
-
-        if (blob == null) {
-            throw FlankFatalError("The file at '$uri' does not exist")
-        }
+        GcStorage.storage.get(bucket, path) ?: throw FlankFatalError("The file at '$uri' does not exist")
     }
 
     fun validateTestMethods(
@@ -258,3 +242,8 @@ object ArgsHelper {
         return shards
     }
 }
+
+fun String.processFilePath(name: String): String =
+    if (startsWith(GCS_PREFIX))
+        this.also { ArgsHelper.assertGcsFileExists(it) } else
+        ArgsHelper.evaluateFilePath(this).also { ArgsHelper.assertFileExists(it, name) }