<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:aakash_jain@apple.com" title="Aakash Jain <aakash_jain@apple.com>"> <span class="fn">Aakash Jain</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - EWS should run JavaScriptCore tests"
href="https://bugs.webkit.org/show_bug.cgi?id=162458">bug 162458</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #297551 Flags</td>
<td>
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - EWS should run JavaScriptCore tests"
href="https://bugs.webkit.org/show_bug.cgi?id=162458#c7">Comment # 7</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - EWS should run JavaScriptCore tests"
href="https://bugs.webkit.org/show_bug.cgi?id=162458">bug 162458</a>
from <span class="vcard"><a class="email" href="mailto:aakash_jain@apple.com" title="Aakash Jain <aakash_jain@apple.com>"> <span class="fn">Aakash Jain</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=297551&action=diff" name="attach_297551" title="Patch">attachment 297551</a> <a href="attachment.cgi?id=297551&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=297551&action=review">https://bugs.webkit.org/attachment.cgi?id=297551&action=review</a>
overall looks fine, couple of comments below inline. should add tests.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:34
> + parsed_results = json.loads(string)</span >
is string guaranteed to be a valid json? if not use try/except accordingly.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:36
> + return cls(parsed_results['allApiTestsPassed'], parsed_results['stressTestFailures'])</span >
can consider checking if parsed_results contains 'allApiTestsPassed' and 'stressTestFailures'.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:42
> + self._test_results = stress_test_failures</span >
does test_results variable indicate test results or test failures? maybe consider renaming it
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:34
> + api_test_failures_string = '{"allApiTestsPassed": false, "stressTestFailures":[]}'</span >
can consider making it a separate unit-test for better readability.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:25
> +from webkitpy.common.net.jsctestresults import JSCTestResults</span >
probably need to add a newline before 'from'. check with other files in webkitpy to confirm.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:135
> + args.append("--group=%s" % self._delegate.group())</span >
instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:151
> + args.append("--group=%s" % self._delegate.group())</span >
instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:170
> + args.append("--group=%s" % self._delegate.group())</span >
instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:189
> + args.append("--group=%s" % self._delegate.group())</span >
instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:243
> + else:</span >
'else' keyword is not required here since if statement has a return.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:244
> + # TODO <a href="mailto:svijayaraghavan@apple.com">svijayaraghavan@apple.com</a>: Comment this, and uncomment the below lines so it actually builds without patch</span >
please fix the TODO.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:255
> </span >
Question: Do we need to check for flakiness of the tests similar to layout tests, and report to flakiness dashboard (if dashboard supports these results)? Maybe something to consider for future.
<span class="quote">> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> + dir = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))</span >
dir should be renamed to more readable name to indicate whether it is for results or logs. Also we try to avoid abbreviations in variable names, see: <a href="https://webkit.org/code-style-guidelines/#names-full-words">https://webkit.org/code-style-guidelines/#names-full-words</a>
<span class="quote">> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:139
> + else:</span >
'else' can be removed.
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/build.py:73
> + group = None</span >
can consider moving this line above 'if' statement and removing 'else'</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>