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

[bugzilla-daemon at webkit.org]

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

Aakash Jain <aakash_jain at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #297551|                            |review-
              Flags|                            |

--- Comment #7 from Aakash Jain <aakash_jain at apple.com> ---
Comment on attachment 297551
  --> https://bugs.webkit.org/attachment.cgi?id=297551
Patch

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

overall looks fine, couple of comments below inline. should add tests.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:34
> +        parsed_results = json.loads(string)

is string guaranteed to be a valid json? if not use try/except accordingly.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:36
> +        return cls(parsed_results['allApiTestsPassed'], parsed_results['stressTestFailures'])

can consider checking if parsed_results contains 'allApiTestsPassed' and 'stressTestFailures'.

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

does test_results variable indicate test results or test failures? maybe consider renaming it

> Tools/Scripts/webkitpy/common/net/jsctestresults_unittest.py:34
> +        api_test_failures_string = '{"allApiTestsPassed": false, "stressTestFailures":[]}'

can consider making it a separate unit-test for better readability.

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:25
> +from webkitpy.common.net.jsctestresults import JSCTestResults

probably need to add a newline before 'from'. check with other files in webkitpy to confirm.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:135
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:151
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:170
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:189
> +            args.append("--group=%s" % self._delegate.group())

instead of try/finally, it would be better to use something like hasattr(self._delegate, 'group') to check if group is present and then append to args accordingly.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:243
> +            else:

'else' keyword is not required here since if statement has a return.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:244
> +                # TODO svijayaraghavan at apple.com: Comment this, and uncomment the below lines so it actually builds without patch

please fix the TODO.

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:255
>  

Question: Do we need to check for flakiness of the tests similar to layout tests, and report to flakiness dashboard (if dashboard supports these results)? Maybe something to consider for future.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:61
> +            dir = os.path.dirname(os.path.dirname(os.path.dirname(self._port.results_directory())))

dir should be renamed to more readable name to indicate whether it is for results or logs. Also we try to avoid abbreviations in variable names, see: https://webkit.org/code-style-guidelines/#names-full-words

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:139
> +        else:

'else' can be removed.

> Tools/Scripts/webkitpy/tool/steps/build.py:73
> +            group = None

can consider moving this line above 'if' statement and removing 'else'

-- 
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/20161221/73554cbb/attachment-0001.html>