run-android-on-specific-device by LearningDave · Pull Request #9568 · facebook/react-native

LearningDave · 2016-08-24T12:49:31Z

Summary

At the moment the run-android command from the react-native cli does run all available devices at once. However it would be extreme useful to have the option to choose only one specific device/simulator.
Therefore i've created a new flag for the command 'run-android': --deviceIdFromList 'deviceIdFromList' in order
to install and launch apps on a specific device/simulator from the command line.

'deviceIdFromList' is the id listed on the output of the command 'adb devices'.

Tests:

I've tested my code with the following commands:
react-native run-android --deviceIdFromList "Not existing id"
react-native run-android --deviceIdFromList
react-native run-android --deviceIdFromList "id of a simulator"
react-native run-android --deviceIdFromList "id of a device"

Output:

facebook-github-bot · 2016-08-24T12:49:57Z

By analyzing the blame information on this pull request, we identified @grabbou and @burgalon to be potential reviewers.

ghost · 2016-08-24T15:25:52Z

@LearningDave updated the pull request - view changes

ghost · 2016-08-24T15:28:26Z

@LearningDave updated the pull request - view changes

LearningDave · 2016-08-26T09:22:06Z

@grabbou @burgalon Please take a look. This PR would make creating automated tests using react-native much easier.

mkonicek · 2016-09-05T21:40:45Z

Nice! This is useful.

Looks like there is a lot of code in runAndroid.js related to this functionality:
https://github.com/facebook/react-native/blob/master/local-cli/runAndroid/runAndroid.js

e.g.:

 const cmd = process.platform.startsWith('win')
   ? 'gradlew.bat'
   : './gradlew';

Would it be possible to refactor the code? It looks like this PR adds some logic that's already in that file.

nit: Instead of --deviceIdFromList, what do you think about simple --deviceId? Does the fact that it's "from a list" add any info?

mkonicek · 2016-09-05T21:41:37Z

local-cli/runAndroid/runAndroid.js

Why Promise? Is the buildApk function async?

Hi, thanks for the feedback.
i called it '--deviceIdFromList' because its not the actually device id. It is just the id created by adb. -> https://developer.android.com/studio/command-line/adb.html#commandsummary
However '--deviceId' might still be better, since it is easier to use.

I will do some refactoring and remove the promise on buildApk.

ghost · 2016-09-06T09:52:43Z

@LearningDave updated the pull request - view changes

LearningDave · 2016-09-06T13:20:28Z

@mkonicek I've done some refactoring, changed the flag to '--deviceId' and removed the misplaced promise.

mkonicek · 2016-09-08T15:33:52Z

local-cli/runAndroid/runAndroid.js

nit (wording): given device id (listed by running "adb devices" on the command line).

(no space after the last .)

mkonicek · 2016-09-08T15:34:54Z

Thanks for refactoring this @LearningDave. It's a useful feature, it'll take me a while to review since it's quite a bit of code.

Have you run the test plan again after refactoring?

mkonicek · 2016-09-08T15:36:24Z

local-cli/runAndroid/runAndroid.js

nit: space after )

mkonicek · 2016-09-08T15:40:11Z

local-cli/runAndroid/runAndroid.js

Could you call adb.getDevices rather than reimplement it here?

react-native/local-cli/runAndroid/adb.js

Line 38 in b9396cd

function getDevices(): Array<string> {

mkonicek · 2016-09-08T15:45:19Z

OK I did a basic review :)

mroswald · 2016-10-24T09:16:06Z

@LearningDave could you please rebase your branch against current master? thanks!

hramos · 2016-11-07T20:12:39Z

@LearningDave are you still planning to ship this?

Summary: New flag for the command 'run-android': --deviceIdFromList <deviceIdFromList> in order to install and launch apps on a specific device/simulator from the command line. <deviceIdFromList> is the id listed on the output of the command 'adb devices'

LearningDave · 2016-11-23T17:52:06Z

@mroswald the branch is rebased now :)

@hramos yea i do. Sorry for the late response.

mkonicek · 2016-12-15T20:06:50Z

This is probably not the behavior we want (the user didn't specify "true"):

mkonicek · 2016-12-15T20:07:39Z

local-cli/runAndroid/runAndroid.js

 const fs = require('fs');
-const isPackagerRunning = require('../util/isPackagerRunning');
 const path = require('path');
+const isPackagerRunning = require('../util/isPackagerRunning');


alphasort these please

mkonicek · 2016-12-15T20:07:47Z

local-cli/runAndroid/runAndroid.js

 const path = require('path');
+const isPackagerRunning = require('../util/isPackagerRunning');
+const Promise = require('promise');
+const adb = require('./adb');


mkonicek · 2016-12-15T20:08:51Z

local-cli/runAndroid/runAndroid.js

-    adb.getDevices().map((device) => tryRunAdbReverse(device));
+    console.log(chalk.bold(
+      'Building the app...'
+    ));


This can be on single line.

mkonicek · 2016-12-15T20:09:31Z

local-cli/runAndroid/runAndroid.js

+    });
+  } catch (e) {
+    console.log(chalk.red(
+      'Could not build the app on the device, read the error above for details.\n'


We are not building the app on the device. We are either building the app, or installing it on device.

mkonicek · 2016-12-15T20:11:36Z

Just small nits, we can fix these as a followup. Thanks for working on this!

@facebook-github-bot shipit

facebook-github-bot · 2016-12-15T20:12:07Z

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mkonicek · 2016-12-16T14:01:30Z

@LearningDave are you up for sending a small followup PR for fixing the small nits above?

mroswald · 2016-12-19T10:16:11Z

@LearningDave if you want I can take over the improvements PR - will ping you on mail too

SandroMachado · 2016-12-20T11:05:20Z

local-cli/runAndroid/runAndroid.js

    command: '--flavor [string]',
    description: '--flavor has been deprecated. Use --variant instead',
-  }, {
-    command: '--configuration [string]',


Why did you removed the configuration flag? This is a breaking change.

Summary: As mkonicek suggested in [#9568](#9568 (comment)) I did some cleanup **Test plan (required)** Only one functional change: > Run `react-native run-android --deviceId` Before it was beginning to build the app and then failing because of the missing device "true" :-) Now it's showing a message and stopping the build: ``` ❯ react-native run-android --deviceId Starting JS server... Parameter missing (device id) ``` Closes #11703 Differential Revision: D4376615 Pulled By: ericvicenti fbshipit-source-id: 3c6e0f12220ab22539c7bc3d390367e02c96728a

Summary: At the moment the run-android command from the react-native cli does run all available devices at once. However it would be extreme useful to have the option to choose only one specific device/simulator. Therefore i've created a new flag for the command 'run-android': --deviceIdFromList 'deviceIdFromList' in order to install and launch apps on a specific device/simulator from the command line. 'deviceIdFromList' is the id listed on the output of the command 'adb devices'. I've tested my code with the following commands: react-native run-android --deviceIdFromList "Not existing id" react-native run-android --deviceIdFromList react-native run-android --deviceIdFromList "id of a simulator" react-native run-android --deviceIdFromList "id of a device" Output: ![not-existing-device](https://cloud.githubusercontent.com/assets/9102810/17931086/d843abc8-6a09-11e6-995d-8c737dd5ed5c.png) ![empty-flag](https://cloud.githubusercontent.com/assets/9102810/17931087/d8443930-6a09-11e6-94f3-d Closes facebook/react-native#9568 Differential Revision: D4335133 Pulled By: mkonicek fbshipit-source-id: a827628316be1b5751225851323b1131f451574c

Summary: As mkonicek suggested in [#9568](facebook/react-native#9568 (comment)) I did some cleanup **Test plan (required)** Only one functional change: > Run `react-native run-android --deviceId` Before it was beginning to build the app and then failing because of the missing device "true" :-) Now it's showing a message and stopping the build: ``` ❯ react-native run-android --deviceId Starting JS server... Parameter missing (device id) ``` Closes facebook/react-native#11703 Differential Revision: D4376615 Pulled By: ericvicenti fbshipit-source-id: 3c6e0f12220ab22539c7bc3d390367e02c96728a

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Aug 24, 2016

LearningDave force-pushed the feat/run-android-on-specific-device branch from ab5ff96 to 36f98ea Compare August 24, 2016 15:25

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 24, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 26, 2016

mkonicek reviewed Sep 5, 2016
View reviewed changes

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 6, 2016

mkonicek reviewed Sep 8, 2016
View reviewed changes

mkonicek self-assigned this Sep 8, 2016

mkonicek reviewed Sep 8, 2016
View reviewed changes

local-cli/runAndroid/runAndroid.js Outdated

Copy link

Contributor

mkonicek Sep 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space after )

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2016

mkonicek reviewed Sep 8, 2016
View reviewed changes

mkonicek added the GH Review: needs-revision label Sep 8, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 21, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 24, 2016

hramos added Needs Response from Author and removed GH Review: review-needed labels Nov 11, 2016

run-android-on-specific-device

1879a1c

Summary: New flag for the command 'run-android': --deviceIdFromList <deviceIdFromList> in order to install and launch apps on a specific device/simulator from the command line. <deviceIdFromList> is the id listed on the output of the command 'adb devices'

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 23, 2016

LearningDave force-pushed the feat/run-android-on-specific-device branch from adc2301 to 1879a1c Compare November 23, 2016 17:49

mkonicek reviewed Dec 15, 2016

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Dec 15, 2016

facebook-github-bot closed this in 85c8333 Dec 16, 2016

SandroMachado reviewed Dec 20, 2016

View reviewed changes

SandroMachado mentioned this pull request Dec 20, 2016

Improve the Android release documentation #11443

Closed

mroswald mentioned this pull request Jan 2, 2017

Followup of #9568, some cleanup #11703

Closed

Conversation

LearningDave commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests:

Uh oh!

facebook-github-bot commented Aug 24, 2016

Uh oh!

ghost commented Aug 24, 2016

Uh oh!

ghost commented Aug 24, 2016

Uh oh!

LearningDave commented Aug 26, 2016

Uh oh!

mkonicek commented Sep 5, 2016

Uh oh!

mkonicek Sep 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 6, 2016

Uh oh!

LearningDave commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Sep 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Sep 8, 2016

Uh oh!

mroswald commented Oct 24, 2016

Uh oh!

hramos commented Nov 7, 2016

Uh oh!

LearningDave commented Nov 23, 2016

Uh oh!

mkonicek commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Dec 15, 2016

Uh oh!

facebook-github-bot commented Dec 15, 2016

Uh oh!

mkonicek commented Dec 16, 2016

Uh oh!

mroswald commented Dec 19, 2016

Uh oh!

SandroMachado Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

LearningDave commented Aug 24, 2016 •

edited

Loading

mkonicek Sep 5, 2016 •

edited

Loading

mkonicek commented Dec 15, 2016 •

edited

Loading

SandroMachado Dec 20, 2016 •

edited

Loading