<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827#c50">Comment # 50</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827">bug 159827</a>
from <span class="vcard"><a class="email" href="mailto:jbedard@apple.com" title="Jonathan Bedard <jbedard@apple.com>"> <span class="fn">Jonathan Bedard</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=285120&action=diff" name="attach_285120" title="Patch">attachment 285120</a> <a href="attachment.cgi?id=285120&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=285120&action=review">https://bugs.webkit.org/attachment.cgi?id=285120&action=review</a>
<span class="quote">>> Tools/Scripts/webkitpy/port/ios.py:423
>> + ], return_exit_code=True)
>
> Notice that Executive.run_command() only throws a ScriptError exception if return_exit_code := False. It is good programming practice to limit the scope of a try-except to the minimum code that can raise an exception to help make it straightforward to debug the cause of an exception. In <a href="show_bug.cgi?id=159827#c25">comment #25</a> I gave an example of one way we can write this code such that we limit the scope of the try-except block.</span >
This perhaps brings up another question, then, about sample_file_path(...), which calls filesystem.join(...). In the original sample_process code, finding the report was in the try catch block, which is why I kept it there after adding spindump. If filesystem.join(...) throws an exception (or results_directory(), actually), what is our desired functionality? I think for the time being, keeping the generation of the hang_report file path inside the try-except block is prudent, as the tests can continue if an error happens while attempting to generate either version of the crash report, and it would seem unhelpful to be to have two try-except blocks in this function.
I will look into this while refactoring the Mac and IOS ports, which will address some of the code duplication in this patch, if you think this is worth changing.</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>