Flank / flank

:speedboat: Massively parallel Android and iOS test runner for Firebase Test Lab
https://firebase.community/
Apache License 2.0
680 stars 115 forks source link

Add support for device specification on config.properties #32

Closed thalescm closed 6 years ago

thalescm commented 7 years ago

As you can do with gcloud command, you can specify device ids and versions, or you can specify each device with it's own config (version, orientation, locale and model)

The point is to add support for this type of configuration as well on config.properties so you could have a specified device with API 19 and another with API 25.

Today when trying to do this you would end up with four configurations (both devices with API 19 and 25) instead of two.

bootstraponline commented 7 years ago

Is --device= what you're referring to?

https://cloud.google.com/sdk/gcloud/reference/beta/test/android/run

If so, I think this feature makes sense. We'd just want to ensure there are tests to verify config.properties is being parsed correctly.

thalescm commented 7 years ago

It's the --device=, sorry that I didn't make it clearer :D

abhagupta commented 7 years ago

Proposed Logic : if device is present in the config.properties file, then ignore deviceId .

thalescm commented 7 years ago

What about the other root parameters with deviceId? For example:

device= model=Nexus6,orientation=landscape,os=24
deviceIds=Nexus5,Nexus6p
os-version-ids=25
orientations=portrait
locales=pt_BR
shard-timeout=60
shard-duration=2400

Would it create Nexus6 devices with portrait and API 25 as well?

I'd not ignore deviceId, os-version-ids, orientations and locales when device is present. I'd stick with normal configuration, but wouldn't apply any of the root configurations (unless not present) on devices created with device tag.

E.g the example above would create: Nexus5 with API 25, portrait, pt_BR Nexus6p with API 25, portrait, pt_BR Nexus6 with API 24, landscape and pt_BR (got the root one as it was not provided in the device specification)

WDYT?

Now if only deviceId is present, it's safe to ignore it if device is also present

thalescm commented 7 years ago

I'm just proposing this, but actually I don't know what firebase would do. I'll take a look to see if I'm not saying impossible things

bootstraponline commented 7 years ago

I think for the sake of sanity, we should match whatever the gcloud CLI does in terms of behavior around options parsing.

thalescm commented 7 years ago
--device is now the preferred way to specify test devices and may not  
be used in conjunction with --devices-ids, --os-version-ids, --locales, or --orientations

Forget what I said

thalescm commented 6 years ago

@abhagupta Are you still working on this? I'm willing to open a PR for this as I'm needing this right now. Just don't wanna do duplicated work if this is about to be released.

renas commented 6 years ago

Thales please open a pr, abha is not working on it.

On Nov 28, 2017 8:04 AM, "Thales Machado" notifications@github.com wrote:

@abhagupta https://github.com/abhagupta Are you still working on this? I'm willing to open a PR for this as I'm needing this right now. Just don't wanna do duplicated work if this is about to be released.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/TestArmada/flank/issues/32#issuecomment-347571812, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKGpatjTL-vXabraFmUeYzMZgs2Ew2_ks5s7C8agaJpZM4N0IIX .

thalescm commented 6 years ago

@renas normally .properties file shouldn't have duplicated keys. In fact, the parser won't allow us the have multiple device keys. What do you suggest?

I thought in something like (so it's easy to parse):

devices=model:Nexus5X;version:23;orientation:portrait;locale:en,\
        model:Nexus6P;version:24,\
        model:pixel;version=26

WDYT?

thalescm commented 6 years ago

@renas whenever you can take a look pls https://github.com/TestArmada/flank/pull/83