[Webkit-unassigned] [Bug 162458] EWS should run JavaScriptCore tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162458

--- Comment #34 from Dean Johnson <dean_johnson at apple.com> ---
Comment on attachment 301069
  --> https://bugs.webkit.org/attachment.cgi?id=301069
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301069&action=review

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! :)

> Tools/Scripts/webkitpy/common/config/ports.py:167
> +        if (test_suite == "jsc"):

Nit: No need for parens.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:51
> +    def __init__(self, all_api_tests_passed, stress_test_failures):

Nit: __init__ should always be the first method defined for a class when not inherited.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:55
> +        self._test_results = stress_test_failures[:]

This should be renamed to self.test_results since it will be accessed outside of the class (see comments below).

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:61
> +               set(self._stress_test_failures) == set(other._stress_test_failures)

Python style discourages the use of \ to break lines. Instead, wrap the statement with parens ().

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:67
> +        return self._stress_test_failures

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.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:70
> +        return self._all_api_tests_passed

Ditto to "getter" comment.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:76
> +        return self.stress_test_failures()

Ditto to "getter" comment.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:79
> +        return self._test_results

Ditto to "getter" comment.

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:30
> +class JSCTestResultsReader(object):

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.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:143
> +            "Patch was not relevant")

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.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:159
> +            "Patch does not build")

Ditto to line-length and default args comment.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:175
> +            "Unable to build without patch")

Ditto to line-length and default args comment.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:213
> +            "Unable to pass tests without patch (tree is red?)")

Ditto to line-length and default args comment.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:258
> +        if hasattr(self._delegate, 'group') and self._delegate.group() == "jsc":

If make_option is used for self._delegate, I don't believe you need the hasattr(self._delegate, 'group') statement here.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:259
> +            first_results = self._delegate.test_results()

I'm unclear on what "first" and "second" are referring to -- can you rename the variables to be more accurate?

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:348
> +            return True

This function is very long... is there a way you can nicely put some of the logic into more isolated functions?

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:68
> +        class TestEWS(AbstractEarlyWarningSystem):

TestEWS should be defined outside of this function (at the top level of the file).

> Tools/Scripts/webkitpy/tool/steps/build.py:71
> +        if hasattr(self._options, 'group'):

group should always be defined given you have a default value defined (as None).

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:64
> +        if group == "jsc":

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)

> Tools/Scripts/webkitpy/tool/steps/checkpatchrelevance.py:72
> +        else:

This else is unnecessary since you return True in the alternative branch.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:61
> +        if hasattr(self._options, "group") and self._options.group == "jsc":

Ditto to hasattr being unnecessary.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170209/ea68bcaa/attachment.html>