<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#c37">Comment # 37</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:svijayaraghavan@apple.com" title="Sri Vijayaraghavan <svijayaraghavan@apple.com>"> <span class="fn">Sri Vijayaraghavan</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=162458#c36">comment #36</a>)
<span class="quote">> 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>
>
> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:32
> > + intersection_api_tests_passed = first._all_api_tests_passed or second._all_api_tests_passed
>
> please verify whether 'or' or 'and' should be used for intersection.</span >
I know it looks funny, but it should be 'or'. If there are some API test failures the first time, but they all passed the second time, those failures are flaky results, so the reported value of api_tests_passed should be True. There are some examples in the unittests at webkitpy/common/net/jsctestresults_unittest.py.
<span class="quote">> > Tools/Scripts/webkitpy/common/net/jsctestresults.py:44
> > + return None
>
> might be a good idea to log a warning as well.</span >
I'll do this.
<span class="quote">> > Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:31
> > + empty_string = ""
>
> might consider renaming empty_string to empty_json to make it more
> consistent.</span >
I'll do this.
<span class="quote">> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:45
> > + self.failure_status_id = 42
>
> any specific reason to choose 42? would it make more sense to keep failure
> id as -1?</span >
I'll change it to zero. This becomes a part of the URL to the test results, so it can't be -1.
<span class="quote">> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:52
> > + else:
>
> 'else' is unnecessary since if returns a value.</span >
I'll fix this.
<span class="quote">> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:99
> > + self.assertEqual(return_value, True)
>
> should use assertTrue(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:119
> > + self.assertEqual(return_value, True)
>
> same, should use assertTrue(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:129
> > + self.assertEqual(return_value, True)
>
> same, should use assertTrue(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:139
> > + self.assertEqual(return_value, True)
>
> same, should use assertTrue(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:159
> > + self.assertEqual(return_value, False)
>
> same, should use assertFalse(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:169
> > + self.assertEqual(return_value, False)
>
> same, should use assertFalse(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:179
> > + self.assertEqual(return_value, False)
>
> same, should use assertFalse(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:188
> > + self.assertEqual(return_value, True)
>
> same, should use assertTrue(return_value)
>
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:198
> > + self.assertEqual(return_value, True)
>
> same, should use assertTrue(return_value)</span >
>
<span class="quote">> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:218
> > + self.assertEqual(return_value, True)
>
> same, should use assertTrue(return_value)</span >
I'll fix this set of items.
<span class="quote">> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:209
> > + self.assertEqual(task.test_run_count(), 3)
>
> don't we need to assert the return_value here?</span >
There should be no return value in this case, since we expect the function call to raise an exception and not return. For this case, I'll change "return_value = task._test_patch()" to simply "task._test_patch()" since return_value shouldn't be set anyway.
<span class="quote">> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:40
> > + jsc_paths = [
>
> can consider renaming it to jsc_relevant_paths</span >
I'll do this.
<span class="quote">> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:47
> > + def _does_contain_change_in_paths(self, files, patterns):
>
> _does_contain_change_in_paths is misleading name (we are not searching for
> change in path here)</span >
I'll rename the method.
<span class="quote">> > Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:48
> > + for item in files:
>
> renaming item to file would make it more readable.</span >
I'll do this.</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>