Updating the `react-native run-ios` command to use UDID #6043

icodethings · 2016-02-20T02:48:24Z

Updating the react-native run-ios command to launch the simulator based off the UUID and to retrieve the list of simulators with the --json flag and therefore removing the reliance on regex parsing and the local-cli/runIOS/parseIOSSimulatorsList.js.

This solves a few problems:

When trying to launch an iOS simulator with just the name when you have other simulators that are similarly named.
For example, you may have a simulator called 'iPhone 6' and one called 'iPhone 6 + 38mm Apple Watch'.
If you try and run xcrun instruments -w "iPhone 6" you will get an error Instruments Usage Error : Ambiguous device name/identifier 'iPhone 6'.
If there is a simulator with brackets in it's name (e.g. "iPhone 6 (test 2)") the regex will parse the contents of the contents of the brackets as the UDID.
This isn't an issue prior to this change for launching the simulator, if that was the only simulator starting with 'iPhone6' as the (test 2) would be stripped from the name. The UDID is used for building the project so it would have failed there too.
It won't launch unavailable simulators. Because we can accurately get the status of each simulator, we can filter out ones that are not in the available state.
This could be extended to check if they are already booted, or to use the default one as the booted one.

All in all, this feels like a must more robust way of parsing the output of the list of simulators.

Tests
I have tested this with the following setup of simulators:

"iPhone 4s" (9.2)
"iPhone 5" (9.2)
"iPhone 6" (9.2)
"iPhone 6" (9.2) + Apple Watch - 38mm
"iPhone 6s" (9.2)
"iPhone 6 (Plus)" (9.2)

This would previously not work because running xcrun instruments -w "iPhone 6" would match all of the above that start with "iPhone 6" and throw an error.

facebook-github-bot · 2016-02-20T02:49:20Z

By analyzing the blame information on this pull request, we identified @frantic to be a potential reviewer.

grabbou · 2016-02-22T00:38:41Z

CC: @mkonicek @frantic going to test it & leave the feedback since I can reproduce it already in my projects.

facebook-github-bot · 2016-02-23T20:21:24Z

@icodethings updated the pull request.

mkonicek · 2016-02-25T23:48:04Z

Why remove the test (parseIOSSimulatorsList-test.js)? Since you updated the parsing code can you also update the test please?

icodethings · 2016-02-26T00:02:14Z

@mkonicek I've also removed the parseIOSSimulatorsList.js. Hence removing parseIOSSimulatorsList-test.js.

icodethings · 2016-02-26T00:03:05Z

The command that gets the simulator list now returns JSON. We don't need to use regex (inside parseIOSSimulatorsList.js)

mkonicek · 2016-02-26T00:36:24Z

Sorry for the lack of clarity. I meant adding a test for the new parsing code.

facebook-github-bot · 2016-02-26T02:25:28Z

@icodethings updated the pull request.

icodethings · 2016-02-26T02:25:55Z

I've moved the code that picks the simulator to a separate file and added tests for the function.

mkonicek · 2016-03-01T19:08:11Z

local-cli/runIOS/findMatchingSimulator.js

+        };
+      }
+
+      if(simulatorName === null && !match){


nit: space after if (everywhere, for consistency with the rest of this file and the codebase)

mkonicek · 2016-03-01T19:12:42Z

Cool, thanks for adding the tests. Good idea with using the --json arg. Just nits now, almost good to go.

facebook-github-bot · 2016-03-02T00:14:16Z

@icodethings updated the pull request.

icodethings · 2016-03-02T00:18:03Z

@mkonicek I've fixed those things up

grabbou · 2016-03-03T17:52:39Z

There still seem to be quite a few styling issues here - would be so cool to get that shipped soon since its' finally going to allow running on iPhone 6s (currently there's naming collision)

icodethings · 2016-03-03T18:02:00Z

Is there a link to a code style guide??

mkonicek · 2016-03-03T18:28:52Z

Tests are green on master now. I restarted the build to make sure this passes all tests:
https://travis-ci.org/facebook/react-native/builds/113004514

The style guide is very simple, assumed if (foo) { is obvious, we should probably add it or make ESLint catch it:
https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md#style-guide

frantic · 2016-03-05T20:53:52Z

local-cli/runIOS/__tests__/findMatchingSimulator-test.js

+describe('findMatchingSimulator', () => {
+  it('should find simulator', () => {
+    expect(findMatchingSimulator({
+        "devices": {


maybe move this JSON to a const? This way the test cases will be much more readable.

@frantic Each one of these JSON blobs are unique to each test. I was thinking it'd be easier to read if each of them were inside the test. Thoughts?

Personally I find it very hard to read this test file. I believe it will make it easier if you use same inputs for each test – currently the search token ('iPhone 6') and expected result are deeply burrowed inside the test function, and the IDs are very random.

facebook-github-bot · 2016-03-15T17:59:27Z

@icodethings updated the pull request.

facebook-github-bot · 2016-03-15T21:23:17Z

@icodethings updated the pull request.

icodethings · 2016-03-15T21:24:01Z

@mkonicek There was one thing around moving the JSON blobs in the test to the top of the document that you can see in the changed files.

Other than that, I've just done a rebase and pushed so we should see test come green soon!

mkonicek · 2016-03-16T17:44:47Z

local-cli/runIOS/findMatchingSimulator.js

+          version
+        };
+      }
+


nit: Can you kill all the empty lines inside this method?

mkonicek · 2016-03-16T17:46:13Z

cc @frantic For final review. Looks good to me in general, also has a test now.

Updating the `react-native run-ios` command to launch the simulator based off the UUID and to retrieve the list of simulators with the --json flag and therefor removing the reliance on regex parsing and the `local-cli/runIOS/parseIOSSimulatorsList.js`. This solves a few problems: 1) When trying to launch an iOS simulator with just the name when you have other simulators that are similarly named. For example, you may have a simulator called 'iPhone 6' and one called iPhone 6 + 38mm Apple Watch'. If you try and run `xcrun instruments -w "iPhone 6"` you will get an error Instruments Usage Error : Ambiguous device name/identifier 'iPhone 6'. 2) If there is a simulator with brackets in it's name (e.g. "iPhone 6 (test 2)") the regex will parse the contents of the contents of the brackets as the UDID. This isn't an issue prior to this change for launching the simulator, but the UDID is used for building the project so it would have failed there. 3) It won't launch unavailable simulators. Because we can accurately get the status of each simulator, we can filter out ones that are not in the available state. This could be extended to check if they are already booted, or to use the default one as the booted one. All in all, this feels like a must more robust way of parsing the output of the list of simulators. I've also removed the default of 'iPhone 6' for the currently booted simulator, or the first in the list. If there is a simulator that is booted, we can't boot any others.

facebook-github-bot · 2016-03-17T04:13:16Z

@icodethings updated the pull request.

facebook-github-bot · 2016-03-18T23:39:45Z

@icodethings updated the pull request.

frantic · 2016-03-18T23:40:00Z

local-cli/runIOS/findMatchingSimulator.js

+    return null;
+  }
+  const devices = simulators.devices;
+  var match;


nit: use let for consistency?

frantic · 2016-03-18T23:42:36Z

local-cli/runIOS/findMatchingSimulator.js

+ * @param Object simulators a parsed list from `xcrun simctl list --json devices` command
+ * @param String|null simulatorName the string with the name of desired simulator. If null, it will use the currently
+ *        booted simulator, or if none are booted, the first in the list.
+ * @returns {Object} {udid, name, version}


Prefer using @flow, doctype comment types are not typechecked

ghost · 2016-04-06T17:00:19Z

@icodethings do you have any updates for this pull request? It's been a while so we wanted to check in and see if you've looked at the requested changes.

jsierles · 2016-04-27T05:57:03Z

@icodethings Just checking if you have time to update this PR. It would be great to ship this!

mbraude · 2016-08-24T18:14:07Z

@icodethings @jsierles any way we can get some traction on this so we can merge it in?

mkonicek · 2016-09-09T09:52:23Z

It looks like the last comments are mostly nits and a comment about the readability of the test.

I'll merge this because quite a few people need the feature.

@facebook-github-bot shipit

ghost · 2016-09-09T09:52:53Z

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

mkonicek · 2016-09-09T09:59:15Z

Looks like this has now merge conflicts and needs to be rebased, sorry! @icodethings, @mbraude, @jsierles, @grabbou would you be up for sending a new PR which is just this PR rebased, and using Flow instead of types in JS docs as frantic suggested?

I'll close this PR just so it doesn't stay open indefinitely (there have been no updates from author in a while). A new PR is definitely welcome, this fixes a bug many people will run into!

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 Feb 20, 2016

icodethings mentioned this pull request Feb 20, 2016

Added UDID to the simulator identifier in react-native run-ios #5978

Closed

mkonicek added GH Review: needs-revision and removed GH Review: review-needed labels Feb 25, 2016

This was referenced Feb 26, 2016

react-native run-ios fails with ambiguous simulator targets #5850

Closed

Running on iPhone 6 simulator may fail microsoft/vscode-react-native#71

Closed

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

mkonicek reviewed Mar 1, 2016
View reviewed changes

mkonicek added the GH Review: needs-revision label Mar 1, 2016

frantic reviewed Mar 5, 2016
View reviewed changes

mkonicek reviewed Mar 16, 2016
View reviewed changes

local-cli/runIOS/findMatchingSimulator.js

version

};

}

Copy link

Contributor

mkonicek Mar 16, 2016

Choose a reason for hiding this comment

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

nit: Can you kill all the empty lines inside this method?

mkonicek assigned frantic Mar 16, 2016

frantic reviewed Mar 18, 2016
View reviewed changes

frantic removed the GH Review: review-needed label Mar 18, 2016

frantic reviewed Mar 18, 2016
View reviewed changes

satya164 added Needs Response from Author and removed GH Review: review-needed labels May 24, 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 Jul 12, 2016

LearningDave mentioned this pull request Aug 16, 2016

cli run-ios on device #9414

Closed

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

mkonicek closed this Sep 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating the `react-native run-ios` command to use UDID #6043

Updating the `react-native run-ios` command to use UDID #6043

icodethings commented Feb 20, 2016

facebook-github-bot commented Feb 20, 2016

grabbou commented Feb 22, 2016

facebook-github-bot commented Feb 23, 2016

mkonicek commented Feb 25, 2016

icodethings commented Feb 26, 2016

icodethings commented Feb 26, 2016

mkonicek commented Feb 26, 2016

facebook-github-bot commented Feb 26, 2016

icodethings commented Feb 26, 2016

mkonicek Mar 1, 2016

mkonicek commented Mar 1, 2016

facebook-github-bot commented Mar 2, 2016

icodethings commented Mar 2, 2016

grabbou commented Mar 3, 2016

icodethings commented Mar 3, 2016

mkonicek commented Mar 3, 2016

frantic Mar 5, 2016

icodethings Mar 13, 2016

frantic Mar 18, 2016

facebook-github-bot commented Mar 15, 2016

facebook-github-bot commented Mar 15, 2016

icodethings commented Mar 15, 2016

mkonicek Mar 16, 2016

mkonicek commented Mar 16, 2016

facebook-github-bot commented Mar 17, 2016

facebook-github-bot commented Mar 18, 2016

frantic Mar 18, 2016

frantic Mar 18, 2016

ghost commented Apr 6, 2016

jsierles commented Apr 27, 2016

mbraude commented Aug 24, 2016

mkonicek commented Sep 9, 2016

ghost commented Sep 9, 2016

mkonicek commented Sep 9, 2016 •

edited

Loading

Updating the react-native run-ios command to use UDID #6043

Updating the react-native run-ios command to use UDID #6043

Conversation

icodethings commented Feb 20, 2016

facebook-github-bot commented Feb 20, 2016

grabbou commented Feb 22, 2016

facebook-github-bot commented Feb 23, 2016

mkonicek commented Feb 25, 2016

icodethings commented Feb 26, 2016

icodethings commented Feb 26, 2016

mkonicek commented Feb 26, 2016

facebook-github-bot commented Feb 26, 2016

icodethings commented Feb 26, 2016

mkonicek Mar 1, 2016

Choose a reason for hiding this comment

mkonicek commented Mar 1, 2016

facebook-github-bot commented Mar 2, 2016

icodethings commented Mar 2, 2016

grabbou commented Mar 3, 2016

icodethings commented Mar 3, 2016

mkonicek commented Mar 3, 2016

frantic Mar 5, 2016

Choose a reason for hiding this comment

icodethings Mar 13, 2016

Choose a reason for hiding this comment

frantic Mar 18, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Mar 15, 2016

facebook-github-bot commented Mar 15, 2016

icodethings commented Mar 15, 2016

mkonicek Mar 16, 2016

Choose a reason for hiding this comment

mkonicek commented Mar 16, 2016

facebook-github-bot commented Mar 17, 2016

facebook-github-bot commented Mar 18, 2016

frantic Mar 18, 2016

Choose a reason for hiding this comment

frantic Mar 18, 2016

Choose a reason for hiding this comment

ghost commented Apr 6, 2016

jsierles commented Apr 27, 2016

mbraude commented Aug 24, 2016

mkonicek commented Sep 9, 2016

ghost commented Sep 9, 2016

mkonicek commented Sep 9, 2016 • edited Loading

Updating the `react-native run-ios` command to use UDID #6043

Updating the `react-native run-ios` command to use UDID #6043

mkonicek commented Sep 9, 2016 •

edited

Loading