[Webkit-unassigned] [Bug 147800] Make run-benchmark cleanup more robust and minor code cleaning

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=147800

--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 258645
  --> https://bugs.webkit.org/attachment.cgi?id=258645
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:72
> +                    result = None

Can we extract the code inside the inner try into a separate helper function?
Six levels of indentation below is just too hard to read.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:83
> +                    _log.error('No result or the server crashed. Something went wrong. Will skip current benchmark.\nError: {error}, Server return code: {return_code}, result: {result}'

This comment is inaccurate.  We're not skipping the current benchmark, we're exiting early.
I don't think we need to say anything about that at all.
In fact, I don't think we want to catch exceptions here at all since that'll allow the exception to propagate all the way up and exit the program anyway.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:87
> +                    if self._browser_driver:

How can self._browser_driver be ever False'y?  Since this is initialized in __init__, this should never be falsey.
If it could, e.g. factory.create could return None, then we should check that condition upfront and bail out
instead of trying to run the test and blowing up later.

> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:90
> +                    if self._http_server_driver:

Ditto.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150810/4d996b47/attachment.html>