Bug 160722 - Logging and other minor improvements to iOS webkitpy
Summary: Logging and other minor improvements to iOS webkitpy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-09 17:19 PDT by Simon Fraser (smfr)
Modified: 2016-08-10 13:03 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.41 KB, patch)
2016-08-09 17:27 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (11.34 KB, patch)
2016-08-10 10:26 PDT, Simon Fraser (smfr)
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-08-09 17:19:32 PDT
Logging and other minor improvements to iOS webkitpy
Comment 1 Simon Fraser (smfr) 2016-08-09 17:27:10 PDT
Created attachment 285699 [details]
Patch
Comment 2 Alexey Proskuryakov 2016-08-09 20:02:52 PDT
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?
Comment 3 Simon Fraser (smfr) 2016-08-10 07:57:29 PDT
(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.
Comment 4 Alexey Proskuryakov 2016-08-10 09:03:27 PDT
It seems cleaner and more reliable to re-create devices. But maybe there is a substantial performance benefit.
Comment 5 Simon Fraser (smfr) 2016-08-10 10:07:19 PDT
(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).
Comment 6 Simon Fraser (smfr) 2016-08-10 10:26:29 PDT
Created attachment 285732 [details]
Patch
Comment 7 Daniel Bates 2016-08-10 10:48:05 PDT
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.
Comment 8 Simon Fraser (smfr) 2016-08-10 13:01:38 PDT
https://trac.webkit.org/r204341