887 Update sharding limit #915

adamfilipow92 · 2020-07-24T15:29:51Z

Fixes #887

Test Plan

How do we know the code works?

For virtual devices, the limit should be set to 250.
For physical devices, the limit should be still 50.

In case when we have a virtual and physical device and shards limit set over the physical device limit, firebase returns an exception

Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 400 Bad Request
{
  "code" : 400,
  "errors" : [ {
    "domain" : "global",
    "message" : "Given number of shards of 60 shards exceeds limit of 50 shards.",
    "reason" : "badRequest"

Solution

If the limit exceeds the physical limit and we have physical and virtual devices we split matrices by device type. This solution allows us to use the maximum limit on every device type.

Checklist

Documented
Unit tested
release_notes.md updated
Split devices by type and run on different matrices

jan-goral · 2020-07-27T19:22:57Z

test_runner/src/main/kotlin/ftl/config/Device.kt

@@ -39,3 +40,10 @@ fun Map<String, String>.asDevice(android: Boolean) =
            orientation = getOrDefault("orientation", version)
        )
    }
+
+fun Device.isVirtual(projectId: String) = if (this.isVirtual == null) AndroidCatalog.isVirtualDevice(model, projectId).takeIf { it }?.apply {
+    isVirtual = this


this is Boolean in current scope so i guess also fits better then apply.

jan-goral · 2020-07-27T19:24:39Z

test_runner/src/main/kotlin/ftl/config/Device.kt

+} ?: false
+else this.isVirtual ?: false
+
+fun List<Device>.check(projectId: String) = forEach { it.isVirtual(projectId) }.let { this }


Suggested change

fun List<Device>.check(projectId: String) = forEach { it.isVirtual(projectId) }.let { this }

fun List<Device>.check(projectId: String) = apply { forEach { it.isVirtual(projectId) } }

jan-goral · 2020-07-27T19:25:46Z

test_runner/src/main/kotlin/ftl/config/Device.kt

+fun Device.isVirtual(projectId: String) = if (this.isVirtual == null) AndroidCatalog.isVirtualDevice(model, projectId).takeIf { it }?.apply {
+    isVirtual = this


Suggested change

fun Device.isVirtual(projectId: String) = if (this.isVirtual == null) AndroidCatalog.isVirtualDevice(model, projectId).takeIf { it }?.apply {

isVirtual = this

fun Device.isVirtual(projectId: String) = if (this.isVirtual == null) AndroidCatalog.isVirtualDevice(model, projectId).takeIf { it }?.also {

isVirtual = it

jan-goral · 2020-07-28T08:18:21Z

test_runner/src/main/kotlin/ftl/run/NewTestRun.kt

    }
+    return@withTimeoutOrNull this


Is there any reason for returning CoroutineScope?

No, this is redundant I should remove it. Thanks!

jan-goral · 2020-07-28T08:23:30Z

test_runner/src/main/kotlin/ftl/run/platform/RunAndroidTests.kt

    val testMatrices = mutableListOf<Deferred<TestMatrix>>()
    val allTestShardChunks = mutableListOf<List<String>>()
    val ignoredTestsShardChunks = mutableListOf<List<String>>()

    val history = GcToolResults.createToolResultsHistory(args)
    val otherGcsFiles = args.uploadOtherFiles(runGcsPath)
    val additionalApks = args.uploadAdditionalApks(runGcsPath)
+    val argsList = if (args.shouldSplitRuns()) args.splitConfigurationByDeviceType() else listOf(args)


argsList is a little bit generic in this context maybe deviceConfigurations or deviceArgs will fits better?

deviceArgs fits great, thanks!

jan-goral · 2020-07-29T11:20:02Z

test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt

+    }
+
+    @Test(expected = FlankFatalError::class)
+    fun `should throw when physical devices and maximum test shards limit exceed`() {


Suggested change

fun `should throw when physical devices and maximum test shards limit exceed`() {

fun `should throw when maximum test shards for physical devices limit exceeded`() {

jan-goral · 2020-07-29T11:27:12Z

test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt

+    }
+
+    @Test(expected = FlankFatalError::class)
+    fun `should throw when virtual devices and maximum test shards limit exceed`() {


Suggested change

fun `should throw when virtual devices and maximum test shards limit exceed`() {

fun `should throw when maximum test shards for virtual devices limit exceeded`() {

jan-goral · 2020-07-29T11:44:27Z

test_runner/src/main/kotlin/ftl/config/Device.kt

+    isVirtual = it
+} else isVirtual ?: false
+
+fun List<Device>.check(projectId: String) = apply { forEach { it.isVirtual(projectId) } }


Suggested change

fun List<Device>.check(projectId: String) = apply { forEach { it.isVirtual(projectId) } }

fun List<Device>.resolve(projectId: String) = apply { forEach { it.isVirtual(projectId) } }

piotradamczyk5 · 2020-07-29T14:48:00Z

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

@@ -23,6 +25,7 @@ data class AndroidArgs(
 ) : IArgs by commonArgs {
    companion object : AndroidArgsCompanion()

+    // override val maxTestShards: Int = calculateMaxTestShards(commonArgs.maxTestShards)


delete if unused

piotradamczyk5 · 2020-07-29T14:50:57Z

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

    xctestrunZip = gcloud.test!!.processFilePath("from test"),
    xctestrunFile = gcloud.xctestrunFile!!.processFilePath("from xctestrun-file"),
    xcodeVersion = gcloud.xcodeVersion,
    testTargets = flank.testTargets!!.filterNotNull()
 )
+
+private fun convertToShardCount(inputValue: Int): Int =


how about single line without : Int ?

piotradamczyk5 · 2020-07-29T14:52:43Z

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

+    if (!inVirtualRange) throwMaxTestShardsLimitExceeded()
+}
+
+private fun AndroidArgs.throwMaxTestShardsLimitExceeded() {


return type could be Nothing for self documentation

piotradamczyk5 · 2020-07-29T15:00:47Z

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

@@ -83,3 +86,17 @@ val AndroidArgs.isInstrumentationTest
 val AndroidArgs.isRoboTest
    get() = appApk != null &&
            (roboDirectives.isNotEmpty() || roboScript != null)
+
+fun AndroidArgs.containsVirtualDevices() = devices.any { it.isVirtual!! }


it.isVirtual ?: false is safer call. WDYT?

piotradamczyk5 · 2020-07-29T15:01:21Z

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

+
+fun AndroidArgs.containsPhysicalDevices() = devices.any { !it.isVirtual!! }
+
+fun AndroidArgs.shouldSplitRuns() = containsPhysicalDevices() && maxTestShards > 50


should not we use AVAILABLE_PHYSICAL_SHARD_COUNT_RANGE.last ?

piotradamczyk5 · 2020-07-29T15:03:29Z

test_runner/src/test/kotlin/ftl/fixtures/test_app_cases/flank-single-device-limit-success.yml

+flank:
+  disable-sharding: false
+  max-test-shards: 40
+


1 empty line could be skipped ;) please check other config files

piotradamczyk5 · 2020-07-29T15:05:37Z

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

@@ -1,6 +1,8 @@
 package ftl.args

 import ftl.args.yml.AppTestPair
+import ftl.config.resolve
+import ftl.config.isVirtual


I think that these imports are unused

jan-goral · 2020-07-31T10:01:04Z

test_runner/src/main/kotlin/ftl/config/Device.kt

@@ -12,9 +12,9 @@ data class Device(
    val model: String,
    val version: String,
    val locale: String = defaultLocale,
-    val orientation: String = defaultOrientation
+    val orientation: String = defaultOrientation,
+    val isVirtual: Boolean? = null


isVirtual shouldn't be nullable. We don't need nulls in later calculations.

Alright! Changed! 👍

…een virtual and physical devices

…fix tests, detekt

… virtual devices configured

jan-goral assigned adamfilipow92 Jul 27, 2020

jan-goral reviewed Jul 28, 2020

View reviewed changes

adamfilipow92 force-pushed the 887-Update-sharding-limit branch from a191b78 to 4cbfff1 Compare July 28, 2020 16:03

adamfilipow92 marked this pull request as ready for review July 28, 2020 16:08

adamfilipow92 requested review from jan-goral, pawelpasterz and piotradamczyk5 July 28, 2020 16:08

jan-goral reviewed Jul 29, 2020

View reviewed changes

piotradamczyk5 reviewed Jul 29, 2020

View reviewed changes

adamfilipow92 requested review from jan-goral and piotradamczyk5 July 31, 2020 09:04

jan-goral reviewed Jul 31, 2020

View reviewed changes

adamfilipow92 added 11 commits July 31, 2020 12:35

Separate validations between ios and android. Split shards limit betw…

38ff190

…een virtual and physical devices

Add spliting matrix by device type when max-shards > PHYSICAL LIMIT, …

5c0fb00

…fix tests, detekt

add tests

45b990c

Review changes

7e5a402

Add tests to check if maxTest shards right set

f42e994

Update release_notes.md

b25333f

Update Device.kt

9538026

Update RunAndroidTests.kt

63a82c3

Cr fixes

84bd1ec

Refractor devices, set behavior to use physical limit on physical and…

625c51b

… virtual devices configured

Update Device.kt

09c1664

adamfilipow92 force-pushed the 887-Update-sharding-limit branch from 48bc561 to 09c1664 Compare July 31, 2020 10:36

jan-goral approved these changes Jul 31, 2020

View reviewed changes

piotradamczyk5 approved these changes Jul 31, 2020

View reviewed changes

adamfilipow92 merged commit 052cd87 into master Aug 3, 2020

adamfilipow92 deleted the 887-Update-sharding-limit branch August 3, 2020 11:39

RainNapper mentioned this pull request Aug 20, 2020

Firebase throws error on 250 virtual shards, says limit is now 200 #1034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

887 Update sharding limit #915

887 Update sharding limit #915

adamfilipow92 commented Jul 24, 2020 •

edited

Loading

jan-goral Jul 27, 2020

jan-goral Jul 27, 2020

jan-goral Jul 27, 2020

jan-goral Jul 28, 2020

adamfilipow92 Jul 28, 2020

jan-goral Jul 28, 2020

adamfilipow92 Jul 28, 2020

jan-goral Jul 29, 2020

jan-goral Jul 29, 2020

jan-goral Jul 29, 2020

piotradamczyk5 Jul 29, 2020

piotradamczyk5 Jul 29, 2020 •

edited

Loading

piotradamczyk5 Jul 29, 2020

piotradamczyk5 Jul 29, 2020

piotradamczyk5 Jul 29, 2020

piotradamczyk5 Jul 29, 2020 •

edited

Loading

piotradamczyk5 Jul 29, 2020

jan-goral Jul 31, 2020

adamfilipow92 Jul 31, 2020

	fun List<Device>.check(projectId: String) = forEach { it.isVirtual(projectId) }.let { this }
	fun List<Device>.check(projectId: String) = apply { forEach { it.isVirtual(projectId) } }

		fun Device.isVirtual(projectId: String) = if (this.isVirtual == null) AndroidCatalog.isVirtualDevice(model, projectId).takeIf { it }?.apply {
		isVirtual = this

	fun `should throw when physical devices and maximum test shards limit exceed`() {
	fun `should throw when maximum test shards for physical devices limit exceeded`() {

	fun `should throw when virtual devices and maximum test shards limit exceed`() {
	fun `should throw when maximum test shards for virtual devices limit exceeded`() {

	fun List<Device>.check(projectId: String) = apply { forEach { it.isVirtual(projectId) } }
	fun List<Device>.resolve(projectId: String) = apply { forEach { it.isVirtual(projectId) } }


		fun AndroidArgs.containsPhysicalDevices() = devices.any { !it.isVirtual!! }

		fun AndroidArgs.shouldSplitRuns() = containsPhysicalDevices() && maxTestShards > 50

887 Update sharding limit #915

887 Update sharding limit #915

Conversation

adamfilipow92 commented Jul 24, 2020 • edited Loading

Test Plan

Solution

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotradamczyk5 Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotradamczyk5 Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamfilipow92 commented Jul 24, 2020 •

edited

Loading

piotradamczyk5 Jul 29, 2020 •

edited

Loading

piotradamczyk5 Jul 29, 2020 •

edited

Loading