Logging and other minor improvements to iOS webkitpy
Created attachment 285699 [details] Patch
Comment on attachment 285699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285699&action=review > Tools/ChangeLog:11 > + Have clean_up_test_run() reset the simulator device rather than deleting it, and Why?
(In reply to comment #2) > Comment on attachment 285699 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285699&action=review > > > Tools/ChangeLog:11 > > + Have clean_up_test_run() reset the simulator device rather than deleting it, and > > Why? It seemed sensible, but isn't required to fix a bug.
It seems cleaner and more reliable to re-create devices. But maybe there is a substantial performance benefit.
(In reply to comment #4) > It seems cleaner and more reliable to re-create devices. But maybe there is > a substantial performance benefit. I think creating devices is pretty fast. Deleting them is slower (has to delete a potentially large directory hierarchy).
Created attachment 285732 [details] Patch
Comment on attachment 285732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285732&action=review > Tools/Scripts/webkitpy/port/ios.py:248 > + _log.debug("\"{0} {1} {2}\"".format(self.LSREGISTER_PATH, "-u", simulator_path)) Notice that logger.debug() takes a format string by <https://docs.python.org/2/library/logging.html#logging.debug>. I also suggest we use a single quoted format string to avoid the need to escape the double quote characters. So, this can be simplified to: _log.debug('"%s %s %s"', self.LSREGISTER_PATH, "-u", simulator_path) > Tools/Scripts/webkitpy/port/ios.py:249 > + subprocess.call([self.LSREGISTER_PATH, "-u", simulator_path]) We should be using self._executive.run_command() instead of subprocess.call() here. > Tools/Scripts/webkitpy/port/ios.py:251 > + _log.debug("\"rmtree {0}".format(simulator_path)) Similarly we can simplify this line. > Tools/Scripts/webkitpy/port/ios.py:254 > + logs_path = os.path.join(os.path.expanduser("~"), "Library/Logs/CoreSimulator/", device_udid) We should be using self._filesystem.join() and self._filesystem.expanduser() instead of os.path.join() and os.path.expanduser(), respectively. > Tools/Scripts/webkitpy/port/ios.py:255 > + _log.debug("\"rmtree {0}".format(logs_path)) See my above remark for line 248 on how we can simplify this line. > Tools/Scripts/webkitpy/port/ios.py:256 > + shutil.rmtree(logs_path, ignore_errors=True) Would "xcrun simctl delete" delete these logs? If so, I suggest we use it instead of deleting them by hand. > Tools/Scripts/webkitpy/port/ios.py:258 > + saved_state_path = os.path.join(os.path.expanduser("~"), "Library/Saved Application State/", self.SIMULATOR_BUNDLE_ID + str(i) + ".savedState") We should be using self._filesystem.join() and self._filesystem.expanduser() instead of os.path.join() and os.path.expanduser(), respectively. > Tools/Scripts/webkitpy/port/ios.py:259 > + _log.debug("\"rmtree {0}".format(saved_state_path)) See my above remark for line 248 on how we can simplify this line. > Tools/Scripts/webkitpy/port/ios.py:260 > + shutil.rmtree(saved_state_path, ignore_errors=True) We should be using self._filesystem.rmtree(saved_state_path) instead of calling shutil.rmtree() directly. Notice that Filesystem.rmtree() passes ignore_errors=True to shutil.rmtree(). > Tools/Scripts/webkitpy/xcode/simulator.py:216 > + _log.debug("\"xcrun simctl create {0} {1} {2}\" returned {3}".format(name, device_type.identifier, runtime.identifier, device_udid)) See my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:227 > + _log.debug("deleting device {}\"".format(udid)) Ditto. > Tools/Scripts/webkitpy/xcode/simulator.py:230 > + _log.debug("xcrun simctl delete {}".format(udid)) Ditto. > Tools/Scripts/webkitpy/xcode/simulator.py:233 > + raise RuntimeError('\"xcrun simctl delete\" failed: device state is {}'.format(Simulator.device_state(udid))) No need to escape the double quote characters as they are inside a single quoted string literal. > Tools/Scripts/webkitpy/xcode/simulator.py:242 > + _log.debug("resetting device {}\"".format(udid)) Did you intend to have a trailing "? Also see my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:245 > + _log.debug("xcrun simctl erase {}".format(udid)) See my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:248 > + raise RuntimeError('\"xcrun simctl erase\" failed: device state is {}'.format(Simulator.device_state(udid))) No need to escape the double quote characters as they are inside a single quoted string literal. > Tools/Scripts/webkitpy/xcode/simulator.py:306 > + _log.debug("\"xcrun simctl spawn {0}\" returned {1}".format(udid, state)) No need to escape the double quote characters as they are inside a single quoted string literal. Also see my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:318 > + _log.debug("waiting for device {0} to enter state {1} with timeout {2}".format(udid, wait_until_state, timeout_seconds)) See my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:323 > + _log.debug(" device state {0}".format(device_state)) Did you intend to have a leading space? See my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:551 > + _log.debug("lookup_or_create_device {0} {1} {2} found {3}".format(name, device_type, runtime, testing_device.name)) See my above remark for line 248 in ios.py on how we can simplify this line. > Tools/Scripts/webkitpy/xcode/simulator.py:554 > + _log.debug("lookup_or_create_device {0} {1} {2} created {3}".format(name, device_type, runtime, testing_device.name)) Ditto.
https://trac.webkit.org/r204341