Repository: runningcode/fladle (kotlin)

Issue:
maxTestShards missing from configuration
It appears in the documentation a few times but ins't in the FladleConfig file.

Pull request:
Deprecate `testShards` configuration feature.
Fixes #204 

What was done?
* add `@Deprecated` annotation (deprecation should be visible in gradle file in IDE) 
    <img width="152" alt="Screenshot 2021-01-19 at 08 39 43" src="https://user-images.githubusercontent.com/32893017/105002638-e8c42c80-5a31-11eb-9bd9-65f5c9f403fe.png">
* add `maxTestShards` option
* add logic to verify if mutually exclusive options are not used. In case they do, an error is thrown in the configuration phase

Patch:
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfig.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfig.kt
index 9f0c3260..2d961565 100644
--- a/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfig.kt
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfig.kt
@@ -8,6 +8,7 @@ import org.gradle.api.provider.Property
 import org.gradle.api.tasks.Input
 import org.gradle.api.tasks.InputFile
 import org.gradle.api.tasks.Optional
+import kotlin.reflect.full.memberProperties
 
 interface FladleConfig {
   // Project id is automatically discovered by default. Use this to override the project id.
@@ -46,10 +47,24 @@ interface FladleConfig {
   @get:Input
   val testTargets: ListProperty<String>
 
+  @Deprecated(
+    message = "testShards is deprecated. Use maxTestShards instead",
+    replaceWith = ReplaceWith("maxTestShards")
+  )
+  /**
+   * The maximum number of shards. Value will be overwritten by [maxTestShards] if both used in configuration
+   */
   @get:Input
   @get:Optional
   val testShards: Property<Int>
 
+  /**
+   * The maximum number of shards
+   */
+  @get:Input
+  @get:Optional
+  val maxTestShards: Property<Int>
+
   /**
    * shardTime - the amount of time tests within a shard should take
    * when set to > 0, the shard count is dynamically set based on time up to the maximmum limit defined by maxTestShards
@@ -402,4 +417,14 @@ interface FladleConfig {
   @get:Input
   @get:Optional
   val failFast: Property<Boolean>
+
+  fun getPresentProperties() = this::class.memberProperties
+    .filter {
+      when (val prop = it.call(this)) {
+        is Property<*> -> prop.isPresent
+        is MapProperty<*, *> -> prop.isPresent && prop.get().isNotEmpty()
+        is ListProperty<*> -> prop.isPresent && prop.get().isNotEmpty()
+        else -> false
+      }
+    }
 }
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfigImpl.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfigImpl.kt
index 5e62f063..91e0b047 100644
--- a/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfigImpl.kt
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/FladleConfigImpl.kt
@@ -60,7 +60,8 @@ data class FladleConfigImpl(
   override val scenarioNumbers: ListProperty<Int>,
   override val obbFiles: ListProperty<String>,
   override val obbNames: ListProperty<String>,
-  override val failFast: Property<Boolean>
+  override val failFast: Property<Boolean>,
+  override val maxTestShards: Property<Int>
 ) : FladleConfig {
   /**
    * Prepare config to run sanity robo.
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/FladlePluginDelegate.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/FladlePluginDelegate.kt
index 94dc57b9..7e1f95c1 100644
--- a/buildSrc/src/main/java/com/osacky/flank/gradle/FladlePluginDelegate.kt
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/FladlePluginDelegate.kt
@@ -2,6 +2,7 @@ package com.osacky.flank.gradle
 
 import com.android.build.gradle.AppExtension
 import com.android.builder.model.TestOptions
+import com.osacky.flank.gradle.validation.checkForExclusionUsage
 import com.osacky.flank.gradle.validation.validateOptionsUsed
 import org.gradle.api.GradleException
 import org.gradle.api.Project
@@ -68,6 +69,7 @@ class FladlePluginDelegate {
   private fun TaskContainer.createTasksForConfig(base: FlankGradleExtension, config: FladleConfig, project: Project, name: String) {
     checkIfSanityAndValidateConfigs(config)
     validateOptionsUsed(config = config, flank = base.flankVersion.get())
+    checkForExclusionUsage(config)
     val configName = name.toLowerCase()
     // we want to use default dir only if user did not set own `localResultsDir`
     val useDefaultDir = config.localResultsDir.isPresent.not()
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/FlankGradleExtension.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/FlankGradleExtension.kt
index b078b051..33cd4062 100644
--- a/buildSrc/src/main/java/com/osacky/flank/gradle/FlankGradleExtension.kt
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/FlankGradleExtension.kt
@@ -134,6 +134,8 @@ open class FlankGradleExtension @Inject constructor(objects: ObjectFactory) : Fl
 
   override val failFast: Property<Boolean> = objects.property()
 
+  override val maxTestShards: Property<Int> = objects.property()
+
   @Internal
   val configs: NamedDomainObjectContainer<FladleConfigImpl> = objects.domainObjectContainer(FladleConfigImpl::class.java) {
     FladleConfigImpl(
@@ -191,7 +193,8 @@ open class FlankGradleExtension @Inject constructor(objects: ObjectFactory) : Fl
       scenarioNumbers = objects.listProperty<Int>().convention(scenarioNumbers),
       obbFiles = objects.listProperty<String>().convention(obbFiles),
       obbNames = objects.listProperty<String>().convention(obbNames),
-      failFast = objects.property<Boolean>().convention(failFast)
+      failFast = objects.property<Boolean>().convention(failFast),
+      maxTestShards = objects.property<Int>().convention(maxTestShards)
     )
   }
 
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/YamlWriter.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/YamlWriter.kt
index 9ee3f40b..900f2d29 100644
--- a/buildSrc/src/main/java/com/osacky/flank/gradle/YamlWriter.kt
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/YamlWriter.kt
@@ -45,7 +45,10 @@ internal class YamlWriter {
   internal fun writeFlankProperties(config: FladleConfig): String = buildString {
     appendln("flank:")
 
+    // Fladle will fail in configuration phase if both maxTestShards and testShards are present
+    appendProperty(config.maxTestShards, name = "max-test-shards")
     appendProperty(config.testShards, name = "max-test-shards")
+
     appendProperty(config.shardTime, name = "shard-time")
     appendProperty(config.repeatTests, name = "num-test-runs")
     appendProperty(config.smartFlankGcsPath, name = "smart-flank-gcs-path")
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateExclusionUsage.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateExclusionUsage.kt
new file mode 100644
index 00000000..1db0b061
--- /dev/null
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateExclusionUsage.kt
@@ -0,0 +1,21 @@
+package com.osacky.flank.gradle.validation
+
+import com.osacky.flank.gradle.FladleConfig
+
+fun checkForExclusionUsage(config: FladleConfig) {
+  val usersProperties = config
+    .getPresentProperties()
+    .map { it.name }
+
+  exclusions.forEach {
+    if (usersProperties.contains(it.first) && usersProperties.contains(it.second)) {
+      throw IllegalStateException("Options ${it.first} and ${it.second} cannot be used together. Choose one of them.")
+    }
+  }
+}
+
+private val exclusions = listOf(
+  "testShards" to "maxTestShards",
+  "testShards" to "numUniformShards",
+  "maxTestShards" to "numUniformShards"
+)
diff --git a/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateOptions.kt b/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateOptions.kt
index 5eef0d7b..2833b9b6 100644
--- a/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateOptions.kt
+++ b/buildSrc/src/main/java/com/osacky/flank/gradle/validation/ValidateOptions.kt
@@ -1,22 +1,11 @@
 package com.osacky.flank.gradle.validation
 
 import com.osacky.flank.gradle.FladleConfig
-import org.gradle.api.provider.ListProperty
-import org.gradle.api.provider.MapProperty
-import org.gradle.api.provider.Property
 import org.gradle.util.VersionNumber
 import kotlin.reflect.full.memberProperties
 
-fun validateOptionsUsed(config: FladleConfig, flank: String) = config::class.memberProperties
-  .asSequence()
-  .filter {
-    when (val prop = it.call(config)) {
-      is Property<*> -> prop.isPresent
-      is MapProperty<*, *> -> prop.isPresent && prop.get().isNotEmpty()
-      is ListProperty<*> -> prop.isPresent && prop.get().isNotEmpty()
-      else -> false
-    }
-  }
+fun validateOptionsUsed(config: FladleConfig, flank: String) = config
+  .getPresentProperties()
   .mapNotNull { property -> properties[property.name]?.let { property to it } }
   .forEach { (property, version) ->
     val configFlankVersion = flank.toVersion()
diff --git a/docs/configuration.md b/docs/configuration.md
index f1c1cfb1..0da1f607 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -82,7 +82,7 @@ fladle {
         "- test: ${rootProject.buildDir}/database/module/outputs/apk/database-module-androidTest.apk"
     ]
     autoGoogleLogin = true
-    testShards = 5 //or numUniformShards=5 cannot use both
+    maxTestShards = 8 //or numUniformShards=5 cannot use both.
     shardTime = 120
     smartFlankGcsPath = "gs://tmp_flank/flank/test_app_android.xml"
     configs {
@@ -342,7 +342,8 @@ Environment variables are mirrored as extra options to the am instrument -e KEY1
     )
     ```
 ### testShards
-Overrides the number of automatically determined test shards for Flank to use. Uses Flanks default value when not specified.
+#### deprecated (use maxTestShards instead)
+The maximum number of shards. Fladle will throw an error when used together with [maxTestShards](./configuration.md#maxtestshards) or [numUniformShards](./configuration.md#numuniformshards). 
 
 === "Groovy"
     ``` groovy
@@ -353,6 +354,18 @@ Overrides the number of automatically determined test shards for Flank to use. U
     testShards.set(5)
     ```
 
+### maxTestShards
+The maximum number of shards. Fladle will throw an error when used together with [testShards](./configuration.md#testshards) or [numUniformShards](./configuration.md#numuniformshards).
+
+=== "Groovy"
+    ``` groovy
+    maxTestShards = 8
+    ```
+=== "Kotlin"
+    ``` kotlin
+    maxTestShards.set(8)
+    ```
+
 ### shardTime
 The amount of time tests within a shard should take.
 When set to > 0, the shard count is dynamically set based on time up to the maximum limit defined by maxTestShards
diff --git a/sample-kotlin/build.gradle.kts b/sample-kotlin/build.gradle.kts
index 5c76fa6f..dce5b4d4 100644
--- a/sample-kotlin/build.gradle.kts
+++ b/sample-kotlin/build.gradle.kts
@@ -23,6 +23,7 @@ fladle {
     projectId.set("flank-gradle")
     useOrchestrator.set(true)
     environmentVariables.set(project.provider { mapOf("clearPackageData" to "true") })
+    maxTestShards.set(10)
     testTargets.set(listOf(
             "class com.osacky.flank.gradle.sample.ExampleInstrumentedTest#seeView"
     ))


Test patch:
diff --git a/buildSrc/src/test/java/com/osacky/flank/gradle/YamlWriterTest.kt b/buildSrc/src/test/java/com/osacky/flank/gradle/YamlWriterTest.kt
index c49d1524..03deccaf 100644
--- a/buildSrc/src/test/java/com/osacky/flank/gradle/YamlWriterTest.kt
+++ b/buildSrc/src/test/java/com/osacky/flank/gradle/YamlWriterTest.kt
@@ -1301,6 +1301,15 @@ class YamlWriterTest {
     assertThat(properties).contains("fail-fast: true")
   }
 
+  @Test
+  fun writeMaxTestShardOption() {
+    val properties = emptyExtension {
+      maxTestShards.set(8)
+    }.toFlankProperties()
+
+    assertThat(properties).contains("max-test-shards: 8")
+  }
+
   private fun emptyExtension() = FlankGradleExtension(project.objects)
   private fun emptyExtension(block: FlankGradleExtension.() -> Unit) = emptyExtension().apply(block)
   private fun FlankGradleExtension.toFlankProperties() = yamlWriter.writeFlankProperties(this).trimIndent()
diff --git a/buildSrc/src/test/java/com/osacky/flank/gradle/validation/ValidateExclusionsTest.kt b/buildSrc/src/test/java/com/osacky/flank/gradle/validation/ValidateExclusionsTest.kt
new file mode 100644
index 00000000..63b73dac
--- /dev/null
+++ b/buildSrc/src/test/java/com/osacky/flank/gradle/validation/ValidateExclusionsTest.kt
@@ -0,0 +1,76 @@
+package com.osacky.flank.gradle.validation
+
+import com.google.common.truth.Truth.assertThat
+import com.osacky.flank.gradle.integration.gradleRunner
+import com.osacky.flank.gradle.integration.writeBuildDotGradle
+import org.junit.Rule
+import org.junit.Test
+import org.junit.rules.TemporaryFolder
+
+class ValidateExclusionsTest {
+  @get:Rule
+  var testProjectRoot = TemporaryFolder()
+
+  @Test
+  fun `should throw an error when mutually exclusive options used`() {
+    testProjectRoot.writeBuildDotGradle(
+      """
+      |plugins {
+      |  id "com.osacky.fladle"
+      |}
+      |
+      |fladle {
+      |  serviceAccountCredentials = layout.projectDirectory.file("flank-gradle-service.json")
+      |  debugApk = "foo.apk"
+      |  instrumentationApk = "test.apk"
+      |  testShards.set(1)
+      |  maxTestShards.set(2)
+      |  flankVersion.set("21.01.0")
+      |}
+    """.trimMargin()
+    )
+
+    val runner = testProjectRoot.gradleRunner()
+
+    runner.withArguments("printYml").buildAndFail().run {
+      assertThat(output).contains("FAILED")
+      assertThat(output).contains("Options testShards and maxTestShards cannot be used together. Choose one of them.")
+    }
+  }
+
+  @Test
+  fun `should throw an error when mutually exclusive options used -- inner config`() {
+    testProjectRoot.writeBuildDotGradle(
+      """
+      |plugins {
+      |  id "com.osacky.fladle"
+      |}
+      |
+      |fladle {
+      |  serviceAccountCredentials = layout.projectDirectory.file("flank-gradle-service.json")
+      |  debugApk = "foo.apk"
+      |  instrumentationApk = "test.apk"
+      |  testShards.set(1)
+      |  flankVersion.set("21.01.0")
+      |  configs {
+      |    newSharding {
+      |      maxTestShards.set(2)
+      |    }
+      |  }
+      |}
+    """.trimMargin()
+    )
+
+    val runner = testProjectRoot.gradleRunner()
+
+    runner.withArguments("printYml").buildAndFail().run {
+      assertThat(output).contains("FAILED")
+      assertThat(output).contains("Options testShards and maxTestShards cannot be used together. Choose one of them.")
+    }
+
+    runner.withArguments("printYmlNewSharding").buildAndFail().run {
+      assertThat(output).contains("FAILED")
+      assertThat(output).contains("Options testShards and maxTestShards cannot be used together. Choose one of them.")
+    }
+  }
+}