<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</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 #301437 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#c45">Comment # 45</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:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=301437&action=diff" name="attach_301437" title="Patch">attachment 301437</a> <a href="attachment.cgi?id=301437&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=301437&action=review">https://bugs.webkit.org/attachment.cgi?id=301437&action=review</a>
r-; with the exception of my second remark in <a href="show_bug.cgi?id=162458#c42">comment 42</a> all other remarks in <a href="show_bug.cgi?id=162458#c42">comment 42</a> were neither addressed in this patch nor replied to with a reason for disagreement. Please either address all of my remarks in <a href="show_bug.cgi?id=162458#c42">comment 42</a> in an updated patch or reply to them with your reason(s) for disagreement. I have some more questions with this iteration of the patch.
<span class="quote">> Tools/QueueStatusServer/handlers/statusbubble.py:190
> + # EWS queues that weren't skipped are also shown when complete.</span >
I do not feel that this comment adds much value. If you want to keep it then at least move it below the if-statement because the code before the if-statement and the if-statement itself are for all queues, and non-EWS queues, respectively. The code that is applicable for EWS queues is after the if-statement (lines 193 thru 195).
<span class="quote">> Tools/Scripts/webkitpy/common/config/ports.py:111
> + if build_style == "debug":
> + command.append("--debug")
> + if build_style == "release":
> + command.append("--release")</span >
I know that you are just mimicking the code from build_webkit_command/run_webkit_tests_command. We should not duplicate this code. We should define a static private helper function, say _append_build_style_flag(), that takes a list and a build style and appends the appropriate build flag. When we move this code into _append_build_style_flag() we should take this opportunity to clean it up and write it using an elif to avoid an unnecessary string comparison when build_style == 'debug'. Then we can write build_jsc_command, build_webkit_command, and run_webkit_tests_command (and run_javascriptcore_tests_command?) in terms of _append_build_style_flag().
<span class="quote">> Tools/Scripts/webkitpy/common/config/ports.py:117
> + command.append("--json-output=jsc_test_results.json")</span >
Can we write this as two arguments?
<span class="quote">> Tools/Scripts/webkitpy/common/config/ports.py:119
> + if (build_style == "debug"):
> + command.append("--debug")</span >
How did you come to the decision to only support a debug build style?
<span class="quote">> Tools/Scripts/webkitpy/common/config/ports.py:169
> + def run_webkit_tests_command(self, build_style=None, test_suite=None):
> + if test_suite == "jsc":
> + return super(MacPort, self).run_javascriptcore_tests_command(build_style)
> +</span >
Where are we invoking run_webkit_tests_command() with a non-None test_suite?
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:38
> + except (IOError, KeyError): # File does not exist or can't be read.</span >
When is the exception KeyError raised? Unit tests?
<span class="quote">> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> + jsc_results_directory = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))</span >
We should not be burying this directory path here. Similar to how we have a Port.result_directory() function, please add a function to the port base class and/or derived classes that returns the appropriate path tot he JSC results directory.</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>