Skip to content
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

Fix run-ios not turning on Simulator app #18721

Closed
wants to merge 1 commit into from
Closed

Conversation

grabbou
Copy link
Contributor

@grabbou grabbou commented Apr 6, 2018

Right now, run-ios will "boot" Simulator in the headless mode in the background, as long as the Simulator.app is not running. It is exactly what "detox" does - it executes your test suite in the background.

It seems to me that the author of this change was testing it with Simulator.app open.

In order to fix this behavior, we have to make sure it runs before we attempt booting.

This passed through the release process since we don't use run-ios there (it recommends turning on XCode and testing manually which is what I have done recently too).

CC: @kelset @koenpunt @hramos

Related: #18681

@facebook-github-bot 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 Apr 6, 2018
@react-native-bot react-native-bot added Missing Test Plan This PR appears to be missing a test plan. Core Team Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Platform: iOS iOS applications. labels Apr 6, 2018
@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Apr 6, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@lebedev
Copy link
Contributor

lebedev commented Apr 6, 2018

@hramos If this is landed together with #18711 (which fixed the same issue this PR addresses and has already been landed by @grabbou in c661057), it could bring unexpected regressions into master. Maybe you can stop it?

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Apr 7, 2018
@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@koenpunt
Copy link
Contributor

koenpunt commented Apr 7, 2018

I believe I tested it with simulator not running as well, but apparently there was an oversight. Anyway, nice there's a solution already.

* it will not boot the "default" device, but the one we set. If the app is already running,
* this flag has no effect.
*/
child_process.execFileSync('open', [
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe open -a Simulator should launch the simulator as well, without specifying a full path. Although not sure if you're then able to pass it additional flags

Copy link
Contributor

Choose a reason for hiding this comment

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

After further investigation, I think you should be able to use;

open -a Simulator --args -CurrentDeviceUDID ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is nice!

@grabbou
Copy link
Contributor Author

grabbou commented Apr 9, 2018

@angly-cat @koenpunt I am wondering whether we should stick to what we already have in master or keep using this approach.

Any ideas? This one uses sort of dedicated and less hacky API for launching the simulator (unfortunately, in headless mode so hack for Simulator app is still needed).

@koenpunt
Copy link
Contributor

koenpunt commented Apr 9, 2018

I think if we use the open -a Simulator --args -CurrentDeviceUDID ... approach, the xcrun simctl boot ... is no longer necessary, since the simulator will be booted already, right?

@grabbou
Copy link
Contributor Author

grabbou commented Apr 9, 2018 via email

@koenpunt
Copy link
Contributor

koenpunt commented Apr 9, 2018

Ah, I see, yeah that's true.

@hramos
Copy link
Contributor

hramos commented Sep 7, 2018

Should I try to land this again?

@hramos
Copy link
Contributor

hramos commented Dec 11, 2018

CLI has been moved.

@hramos hramos closed this Dec 11, 2018
@zpao zpao deleted the grabbou-patch-2 branch February 2, 2019 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Missing Test Plan This PR appears to be missing a test plan. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants