<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#c34">Comment # 34</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:dean_johnson@apple.com" title="Dean Johnson <dean_johnson@apple.com>"> <span class="fn">Dean Johnson</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>
As a heads up, I didn't do much reviewing of the unit tests. Really happy to see the labor put towards this come to fruition! :)
<span class="quote">> Tools/Scripts/webkitpy/common/config/ports.py:167
> + if (test_suite == "jsc"):</span >
Nit: No need for parens.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:51
> + def __init__(self, all_api_tests_passed, stress_test_failures):</span >
Nit: __init__ should always be the first method defined for a class when not inherited.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:55
> + self._test_results = stress_test_failures[:]</span >
This should be renamed to self.test_results since it will be accessed outside of the class (see comments below).
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:61
> + set(self._stress_test_failures) == set(other._stress_test_failures)</span >
Python style discourages the use of \ to break lines. Instead, wrap the statement with parens ().
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:67
> + return self._stress_test_failures</span >
This is the logical equivalent of a "getter" and because Python doesn't use public/private scopes you should remove this function and do direct variable access. To do this, rename _stress_test_failures => stress_test_failures and call <some_instantiated_JSCTestResultsobject>.stress_test_failures to retrieve the value.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:70
> + return self._all_api_tests_passed</span >
Ditto to "getter" comment.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> + return self.stress_test_failures()</span >
Ditto to "getter" comment.
<span class="quote">> Tools/Scripts/webkitpy/common/net/jsctestresults.py:79
> + return self._test_results</span >
Ditto to "getter" comment.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:30
> +class JSCTestResultsReader(object):</span >
Is this entire class really necessary? All it does it open a file and read the contents then pass it through to JSCTestResults. It seems like it'd be better to define an @classmethod that took a results_file_path and maybe had a name like "results_from_file." But if this is the only entrypoint to JSCResults, it might be even better just to merge the functionality of this class and the @classmethod from JSCTestResults::results_from_string.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:143
> + "Patch was not relevant")</span >
It doesn't seem necessary for line-length concerns to put each argument on its own line. Also, if self._run_command has default arguments then using them like: `return self._run_command(args, success_text="Checked relevance of patch", failure_text="Patch was not relevant")` would be more clear.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:159
> + "Patch does not build")</span >
Ditto to line-length and default args comment.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:175
> + "Unable to build without patch")</span >
Ditto to line-length and default args comment.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:213
> + "Unable to pass tests without patch (tree is red?)")</span >
Ditto to line-length and default args comment.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:258
> + if hasattr(self._delegate, 'group') and self._delegate.group() == "jsc":</span >
If make_option is used for self._delegate, I don't believe you need the hasattr(self._delegate, 'group') statement here.
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:259
> + first_results = self._delegate.test_results()</span >
I'm unclear on what "first" and "second" are referring to -- can you rename the variables to be more accurate?
<span class="quote">> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:348
> + return True</span >
This function is very long... is there a way you can nicely put some of the logic into more isolated functions?
<span class="quote">> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:68
> + class TestEWS(AbstractEarlyWarningSystem):</span >
TestEWS should be defined outside of this function (at the top level of the file).
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/build.py:71
> + if hasattr(self._options, 'group'):</span >
group should always be defined given you have a default value defined (as None).
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:64
> + if group == "jsc":</span >
Given we know we're going to be adding many more projects here, I recommend you do something like this:
# Define group_to_path_mapping at the same scope as jsc_paths
group_to_paths_mapping = {
'jsc': jsc_paths,
}
def run(self, state)
.....
relevant_paths = self.group_to_paths_mapping.get(group, [])
relevant = self._does_contain_change_in_paths(change_list, relevant_paths)
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:72
> + else:</span >
This else is unnecessary since you return True in the alternative branch.
<span class="quote">> Tools/Scripts/webkitpy/tool/steps/runtests.py:61
> + if hasattr(self._options, "group") and self._options.group == "jsc":</span >
Ditto to hasattr being unnecessary.</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>