Bug 148197 – iOS Simulator layout-tests fail to start while cleaning a directory structure if simulator is already running

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

see rdar://22334382

[Aakash Jain]

Created attachment 259414 [details]
proposed patch

The failure happens when a simulator is already running (due to previous run or some other reasons). Due to that the directory cannot be deleted since the simulator is writing some files to it. 
In this patch, we are quitting all the running simulator before attempting to delete the logs directory. Tested on local setup.

[Daniel Bates]

Comment on attachment 259414 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259414&action=review

> Tools/Scripts/webkitpy/port/ios.py:387
> +        self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit'])
> +        quit_delay = 5
> +        _log.debug('Waiting {seconds} seconds for any old iOS Simulator instance to quit ...'.format(seconds=quit_delay))
> +        time.sleep(quit_delay)
>          if os.path.isdir(data_path):
>              shutil.rmtree(data_path)

Is it necessary to remove the data directory? I mean, check_sys_deps() will conditionally erase the simulator device using "simctl erase", which will delete the data directory. If it is not necessary then I suggest we have this function be empty (except for the presence of the pass keyword - since Python does not allow a function to have an empty body). If it is necessary to remove the data directory here then can we make use of Simulator.check_simulator_device_and_erase_if_needed() or extract the logic from this function into a shared function that can be called here and in Simulator.check_simulator_device_and_erase_if_needed()? Can we make use of Simulator.wait_until_device_is_in_state() to wait until the booted simulator device is shutdown instead of waiting a hardcoded 5 seconds.

[Daniel Bates]

Comment on attachment 259414 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259414&action=review

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=148197

Please include the radar URL, <rdar://problem/22334382>, under the Bugzilla bug URL.

[Aakash Jain]

Created attachment 259560 [details]
Proposed patch

- Quit the simulator after the testing is done, so that it is not already running during the next build.
 
- Instead of quitting the simulator during the setup through the code, use kill-old-processes to kill the Simulator (will need another patch for buildbot).

[Daniel Bates]

Comment on attachment 259560 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259560&action=review

> Tools/BuildSlaveSupport/kill-old-processes:88
> +        "Simulator"  #FIXME: Consider moving iOS-Specific tasks to their own list

Please put a space character after the '#', substitute "specific" for "Specific" and add a period to the end of this sentence.

> Tools/Scripts/webkitpy/port/ios.py:-248
> -        # testing_device will fail to boot if it is already booted. We assume that if testing_device
> -        # is booted that it was booted by the iOS Simulator app (as opposed to simctl). So, quit the
> -        # iOS Simulator app to shutdown testing_device.
> -        # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator
> -        self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit'])
> -        Simulator.wait_until_device_is_in_state(testing_device.udid, Simulator.DeviceState.SHUTDOWN)
> -

Actually, we need this code to handle the scenario where a person launched the iOS Simulator with simulator device testing_device and then subsequently runs run-webkit-test --ios-simulator (this is implied by the comment on lines 242 - 244). So, I suggest that we extract the common code into shared function, say _quitIOSSimulator, that we call from both here and clean_up_test_run(). Maybe something like:

def _quitIOSSimulator(self):
    # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator
    self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit'])

[Daniel Bates]

(In reply to comment #5)
> [...]
> def _quitIOSSimulator(self):
>     # FIXME: <rdar://problem/20916140> Switch to using
> CoreSimulator.framework for launching and quitting iOS Simulator
>     self._executive.run_command(['osascript', '-e', 'tell application id
> "com.apple.iphonesimulator" to quit'])

We may also want to take to this opportunity to extract the bundle identifier "com.apple.iphonesimulator" into a constant that can be shared by both the quit simulator logic (say, in IOSSimulatorPort._quitIOSSimulator()) and IOSSimulatorPort.setup_test_run().

[Daniel Bates]

(In reply to comment #5)
> > Tools/Scripts/webkitpy/port/ios.py:-248
> > -        # testing_device will fail to boot if it is already booted. We assume that if testing_device
> > -        # is booted that it was booted by the iOS Simulator app (as opposed to simctl). So, quit the
> > -        # iOS Simulator app to shutdown testing_device.
> > -        # FIXME: <rdar://problem/20916140> Switch to using CoreSimulator.framework for launching and quitting iOS Simulator
> > -        self._executive.run_command(['osascript', '-e', 'tell application id "com.apple.iphonesimulator" to quit'])
> > -        Simulator.wait_until_device_is_in_state(testing_device.udid, Simulator.DeviceState.SHUTDOWN)
> > -
> 
> Actually, we need this code to handle the scenario where a person launched
> the iOS Simulator with simulator device testing_device and then subsequently
> runs run-webkit-test --ios-simulator (this is implied by the comment on
> lines 242 - 244). So, I suggest that we extract the common code into shared
> function, say _quitIOSSimulator, that we call from both here and
> clean_up_test_run().

Disregard this comment. We also need to move the logic for quitting the simulator from IOSSimulatorPort.check_sys_deps() to IOSSimulatorPort.reset_preferences(). Otherwise, we will hit this bug when a person launches the testing device in iOS Simulator and subsequently runs run-webkit-tests --ios-simulator.

[Aakash Jain]

Created attachment 259884 [details]
Proposed patch

Made the changes as discussed.

Quitting the simulator both during the cleanup as well as at the beginning of layout_tests (during the reset_preferences).

[Daniel Bates]

Comment on attachment 259884 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259884&action=review

> Tools/ChangeLog:12
> +        (IOSSimulatorPort._quitIOSSimulator): common function to quit IOSSimulator.

Nit: common => Common
IOSSimulator => iOS Simulator

> Tools/ChangeLog:14
> +        (IOSSimulatorPort.check_sys_deps): No need to quit the simulator here as its now being quit in reset_preferences.

Nit: reset_preferences => reset_preferences()

> Tools/Scripts/webkitpy/port/ios.py:80
> +    SIMULATOR_BUNDLE_ID = "com.apple.iphonesimulator"

Nit: ' (single quote) => " (double quote)

> Tools/Scripts/webkitpy/port/ios.py:213
> +    def _quitIOSSimulator(self):

I didn't meant to steer you wrong on the naming of this function. We name functions with underscores between words. So, this function should be named _quit_ios_simulator.

[Aakash Jain]

Created attachment 259893 [details]
Proposed patch

[Daniel Bates]

Comment on attachment 259893 [details]
Proposed patch

Thank you Aakash for updating the patch.

[WebKit Commit Bot]

Comment on attachment 259893 [details]
Proposed patch

Clearing flags on attachment: 259893

Committed r188942: <http://trac.webkit.org/changeset/188942>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.