Bug 160722 – Logging and other minor improvements to iOS webkitpy

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Logging and other minor improvements to iOS webkitpy

[Simon Fraser (smfr)]

Created attachment 285699 [details]
Patch

[Alexey Proskuryakov]

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?

[Simon Fraser (smfr)]

(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.

[Alexey Proskuryakov]

It seems cleaner and more reliable to re-create devices. But maybe there is a substantial performance benefit.

[Simon Fraser (smfr)]

(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).

[Simon Fraser (smfr)]

Created attachment 285732 [details]
Patch

[Daniel Bates]

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.

[Simon Fraser (smfr)]

https://trac.webkit.org/r204341