<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Make run-benchmark cleanup more robust and minor code cleaning"
href="https://bugs.webkit.org/show_bug.cgi?id=147800#c9">Comment # 9</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Make run-benchmark cleanup more robust and minor code cleaning"
href="https://bugs.webkit.org/show_bug.cgi?id=147800">bug 147800</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=258645&action=diff" name="attach_258645" title="Patch">attachment 258645</a> <a href="attachment.cgi?id=258645&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=258645&action=review">https://bugs.webkit.org/attachment.cgi?id=258645&action=review</a>
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:72
> + result = None</span >
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.
<span class="quote">> 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}'</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:87
> + if self._browser_driver:</span >
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.
<span class="quote">> Tools/Scripts/webkitpy/benchmark_runner/benchmark_runner.py:90
> + if self._http_server_driver:</span >
Ditto.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>