Bug 171293 - webkitpy: Teardown iOS Simulators on exit if managed Simulators are still running
Summary: webkitpy: Teardown iOS Simulators on exit if managed Simulators are still run...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-25 14:46 PDT by Jonathan Bedard
Modified: 2017-05-09 15:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.86 KB, patch)
2017-04-25 14:48 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (6.90 KB, patch)
2017-04-26 12:29 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (6.88 KB, patch)
2017-04-26 13:08 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2017-04-25 14:46:56 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-04-25 14:48:19 PDT
<rdar://problem/31821689>
Comment 2 Jonathan Bedard 2017-04-25 14:48:59 PDT
Created attachment 308149 [details]
Patch
Comment 3 Aakash Jain 2017-04-26 12:04:01 PDT
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.
Comment 4 Jonathan Bedard 2017-04-26 12:29:30 PDT
Created attachment 308273 [details]
Patch
Comment 5 Aakash Jain 2017-04-26 12:44:07 PDT
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().
Comment 6 Jonathan Bedard 2017-04-26 13:08:56 PDT
Created attachment 308277 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-04-26 13:23:35 PDT
Comment on attachment 308277 [details]
Patch for landing

Clearing flags on attachment: 308277

Committed r215824: <http://trac.webkit.org/changeset/215824>
Comment 8 WebKit Commit Bot 2017-04-26 13:23:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Jonathan Bedard 2017-05-09 15:53:51 PDT
Remove FIFO code in <https://bugs.webkit.org/show_bug.cgi?id=171891>