<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:ap@webkit.org" title="Alexey Proskuryakov <ap@webkit.org>"> <span class="fn">Alexey Proskuryakov</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 #301667 Flags</td>
<td>review?
</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#c55">Comment # 55</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:ap@webkit.org" title="Alexey Proskuryakov <ap@webkit.org>"> <span class="fn">Alexey Proskuryakov</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=301667&action=diff" name="attach_301667" title="Patch">attachment 301667</a> <a href="attachment.cgi?id=301667&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=301667&action=review">https://bugs.webkit.org/attachment.cgi?id=301667&action=review</a>
<span class="quote">> > Tools/Scripts/webkitpy/common/config/ports.py:117
> > + command.append("--json-output=jsc_test_results.json")</span >
>
<span class="quote">> Can we write this as two arguments?</span >
There are other instances of passing arguments with '=' sign in this file, so I'm not sure if/why splitting is preferable.
I feel like AbstractTestResults is getting too skinny and may not be useful. But we can improve this in the future. We'll have a better idea of how the interface should look when working other test types, such as webkitpy.
I think that the next iteration should be ready to get an r+, looks pretty good to me.
<span class="quote">> Tools/ChangeLog:39
> + (JSCTestResults.failing_test_results): Getter. Some of these exist for interopability reasons.</span >
I believe that you got rid of this after Dan's review.
<span class="quote">> Tools/QueueStatusServer/handlers/statusbubble.py:194
> + return bool(status and not status.did_skip())</span >
This is a good pattern to use, but we should migrate other code to it (in a separate patch). Code above just compares to "Pass" and "Fail", so we are being inconsistent.
OK as is for now.
<span class="quote">> Tools/Scripts/webkitpy/common/config/ews.json:54
> + "JSC EWS": {</span >
We need to update status server to not change the case to "Jsc EWS". There is at least one instance of that on your test server. There is code that special cases some strings such as "EWS" for proper display.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:33
> + self.all_api_tests_passed = all_api_tests_passed</span >
all_api_tests_passed isn't used outside the class, so it should be prefixed with an underscore.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:34
> + self.stress_test_failures = stress_test_failures</span >
Ditto.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:36
> + self.results = stress_test_failures[:]</span >
Ditto.
But also, a better name would help here. This is a list of failing test names, so let's just call it _failing_test_names.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> + def test_results(self):
> + return self.results
> +
> + def failing_tests(self):
> + return self.results</span >
This can't be right. For LayoutTests, one function returns a list of test names, and another returns a list of TestResult objects. Making it so different for JSC tests is very confusing.
It seems like we can just remove test_results() from the interface, and have JSC code call failing_tests() where it now calls test_results().
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:38
> + except (IOError, KeyError): # File does not exist or can't be read.</span >
This comment doesn't seem useful.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:41
> + def _create_jsc_test_results(self):</span >
I don't think that extracting this into function helps, please inline it at call site.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:253
> + consistent_results = JSCTestResults.intersection(first_results, second_results)</span >
I think that this should be "consistently_failing_test_results"
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:339
> + if hasattr(self._delegate, 'group') and self._delegate.group() == "jsc":</span >
This is because commit queue doesn't have a group. This makes me wonder what to do with it - one might expect it to run JSC tests now.
Not something to fix right now.
<span class="quote">> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:95
> +class MockEarlyWarningSystemTaskForInconclusiveResults(EarlyWarningSystemTask):
> + def _test_patch(self):
> + self._test()
> + results = self._delegate.test_results()
> + return bool(results)
> +
> +
> +class MockAbstractEarlyWarningSystemForInconclusiveResults(AbstractEarlyWarningSystem):
> + def _create_task(self, patch):
> + task = MockEarlyWarningSystemTaskForInconclusiveResults(self, patch, self._options.run_tests)
> + return task</span >
These classes are only used for JSC, so we should have that in the name - otherwise it's difficult to tell which delegate is being invoked.
<span class="quote">> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:148
> + # This must use the Mock EWS classes defined above</span >
This comment basically says that one of the lines below is important. That's not useful - all of them are important, and if someone accidentally removes one, tests will catch that anyway.
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:71
> + _log.info("This patch has relevant changes.")</span >
I think that this would be too noisy. Processing status lines relies on there not being too many. Please remove this.
Logging just for the other case is sufficient.</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>