-
Notifications
You must be signed in to change notification settings - Fork 119
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
#835 Added option to print available devices and OS version to test against #882
Conversation
938458b
to
dd9175d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is to copy the gcloud layout so we can easily track upstream changes when new features are added.
https://cloud.google.com/sdk/gcloud/reference/alpha/firebase/test/android/
The available devices command would then be something like:
gcloud alpha firebase test android models list
https://cloud.google.com/sdk/gcloud/reference/alpha/firebase/test/android/models/list
because there is no --available-devices
flag for android run.
https://cloud.google.com/sdk/gcloud/reference/alpha/firebase/test/android/run
Ideally each new command would be an individual ticket/pull request to keep the overall pull request size small. Small PRs are easier to review.
Changed as requested |
I think we should split the PR, even if it's more work. It'll be good to build the habit of following engineering best practices. |
@@ -322,7 +321,7 @@ class IosRunCommandTest { | |||
|
|||
@Test | |||
fun `obfuscate parse`() { | |||
val cmd = AndroidRunCommand() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
private fun List<AndroidModel>.createTestEnvironmentInfo() = | ||
fold(mutableMapOf<String, MutableList<String>>()) { devicesInfo, androidDevice -> | ||
devicesInfo.apply { | ||
getOrCreateList(MODEL_ID).add(androidDevice.codename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codename
, manufacturer
, name
, from
etc ... generally properties of androidDevice
all are nullable values (java ... ). Sometimes there is no big harm but in other cases it may end up with NPE.
I am wondering if we could protect logic from null
s and make flank more robust. I am aware it should not happen but I am also sure, sooner or later this will blow.
Above is applicable to ListAndroidSofwareVersions.kt
, ListIOsDevices.kt
, ListIOsSofwareVersions.kt
as well. (I just don't want to duplicate comment :) )
Let me know what do you think
private fun StringBuilder.newLine() { | ||
append(System.lineSeparator()) | ||
} | ||
fun TableColumn.applyColorsUsing(mapper: (String) -> SystemOutColor) = copy(dataColor = data.map(mapper)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about making this inline?
@bootstraponline @Pasterzp this PR is split into smaller parts as suggested by @bootstraponline. |
Fixes #835
Test Plan
Use on of 3 new CLI option to test this PR:
flank firebase test android|ios models list
orflank android|ios models list
- print Android/iOS available devices to test againstflank firebase test android|ios versions list
orflank android|ios versions list
- print Android/iOS available os software versions to test againstflank firebase test android|ios test-environment
orflank android|ios test-environment
- print both available devices and os software versions to test againstSample Android output:
Sample iOS output:
More listing options of the test environment will be added in the next tasks after research.
The new options should be printed together when using
--available-environment
optionChecklist