Bug 171293 – webkitpy: Teardown iOS Simulators on exit if managed Simulators are still running

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

[Jonathan Bedard]

Currently, if an exception is thrown while booting the simulators, the simulators will not be torn down.  Perform simulator tear-down at exit to avoid this issue.

[Radar WebKit Bug Importer]

<rdar://problem/31821689>

[Jonathan Bedard]

Created attachment 308149 [details]
Patch

[Aakash Jain]

Comment on attachment 308149 [details]
Patch

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

Overall looks fine. Couple of comments below.

> Tools/Scripts/webkitpy/port/ios_simulator.py:187
> +    def _teardown_managed_simulators():

Please consider adding a comment that this method will be called multiple times in a run. or add a log.debug statement.

> Tools/Scripts/webkitpy/port/ios_simulator.py:224
> +            atexit.register(IOSSimulatorPort._teardown_managed_simulators)

You can consider moving this register() one line above. What if _createSimulatorApps() failed (with exception) after creating couple of simulators. We still want the cleanup.

> Tools/Scripts/webkitpy/port/ios_simulator.py:-240
> -            return

I would prefer to keep the check for _using_dedicated_simulators inside the method (_quit_ios_simulator), instead of adding the responsibility to the callers. It is very easy to miss the check for the callers.

> Tools/Scripts/webkitpy/port/ios_simulator.py:-255
> -                pass

Please separate this change in another patch as this is unrelated to other changes.

> Tools/Scripts/webkitpy/port/ios_simulator.py:343
> +            # Maybe this should delete all devices that we've created?

we shouldn't need this change after reverting _quit_ios_simulator behavior.

[Jonathan Bedard]

Created attachment 308273 [details]
Patch

[Aakash Jain]

Comment on attachment 308273 [details]
Patch

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

looks good to me.

> Tools/ChangeLog:9
> +        thrown while booting.  Make some IOSSimulatorPort functions into static methods and register

Nit: two spaces.

> Tools/Scripts/webkitpy/port/ios_simulator.py:221
>              self._createSimulatorApps()

unrelated, but we can consider removing _createSimulatorApps() and moving its code here. We don't need two methods _create_simulators() and _createSimulatorApps().

[Jonathan Bedard]

Created attachment 308277 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 308277 [details]
Patch for landing

Clearing flags on attachment: 308277

Committed r215824: <http://trac.webkit.org/changeset/215824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Remove FIFO code in <https://bugs.webkit.org/show_bug.cgi?id=171891>