<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c36">Comment # 36</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=301069&action=diff" name="attach_301069" title="Patch">attachment 301069</a> <a href="attachment.cgi?id=301069&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=301069&action=review">https://bugs.webkit.org/attachment.cgi?id=301069&action=review</a>
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:32
> + intersection_api_tests_passed = first._all_api_tests_passed or second._all_api_tests_passed</span >
please verify whether 'or' or 'and' should be used for intersection.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:44
> + return None</span >
might be a good idea to log a warning as well.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:31
> + empty_string = ""</span >
might consider renaming empty_string to empty_json to make it more consistent.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:45
> + self.failure_status_id = 42</span >
any specific reason to choose 42? would it make more sense to keep failure id as -1?
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:52
> + else:</span >
'else' is unnecessary since if returns a value.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:99
> + self.assertEqual(return_value, True)</span >
should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:119
> + self.assertEqual(return_value, True)</span >
same, should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:129
> + self.assertEqual(return_value, True)</span >
same, should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:139
> + self.assertEqual(return_value, True)</span >
same, should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:159
> + self.assertEqual(return_value, False)</span >
same, should use assertFalse(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:169
> + self.assertEqual(return_value, False)</span >
same, should use assertFalse(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:179
> + self.assertEqual(return_value, False)</span >
same, should use assertFalse(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:188
> + self.assertEqual(return_value, True)</span >
same, should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:198
> + self.assertEqual(return_value, True)</span >
same, should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:209
> + self.assertEqual(task.test_run_count(), 3)</span >
don't we need to assert the return_value here?
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:218
> + self.assertEqual(return_value, True)</span >
same, should use assertTrue(return_value)
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:40
> + jsc_paths = [</span >
can consider renaming it to jsc_relevant_paths
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:47
> + def _does_contain_change_in_paths(self, files, patterns):</span >
_does_contain_change_in_paths is misleading name (we are not searching for change in path here)
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:48
> + for item in files:</span >
renaming item to file would make it more readable.</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>