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

[bugzilla-daemon at webkit.org]

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

--- Comment #42 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 301117
  --> https://bugs.webkit.org/attachment.cgi?id=301117
Patch

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

> Tools/QueueStatusServer/config/queues.py:2
> -# Copyright (C) 2014 Apple Inc. All rights reserved.
> +# Copyright (C) 2014, 2017 Apple Inc. All rights reserved.
>  # Copyright (C) 2013 Google Inc. All rights reserved.

This patch is sufficient large. Towards making it straightforward to review this code for correctness I would appreciate if you would split this patch into three sub-patches: one patch for the changes to QueueStatusServer, one patch for non-test webkitpy changes, and one patch for the webkitpy unit tests. When it comes time to land the patches we can choose to land each sub-patch or combine the sub-patches into one mega patch.

> Tools/QueueStatusServer/handlers/statusbubble.py:198
>              return True
> -        # EWS queues are also shown when complete.
> -        return bool(queue.is_ews() and attachment.status_for_queue(queue))
> +
> +        # EWS queues are also shown when complete
> +        # But JSC EWS bubble is not shown if the patch is not relevant to JSC.
> +        if not queue.is_ews():
> +            return False
> +
> +        status = attachment.status_for_queue(queue)
> +        if queue.name() == "jsc-ews":
> +            return not status.message.startswith("Patch not applicable")
> +        return bool(status)

This is error prone because it depends on the formatting of the status message that the EWS code submits and the EWS code lives in a different directory hierarchy (Tools/Scripts/webkitpy) from this code. I also do not feel that this "do not show bubble if patch is not applicable" needs to be specific to the JSC EWS. Such functionality could be beneficial for other queues so as make the status bubble listing less noisy. Towards making this code less error prone, we need to keep this status message in sync between this code and the code in Tools/Scripts/webkitpy. I suggest that we do the following:

QueueStatusServer changes:

1. Define a dedicated status message constant, say skip_status or non_applicable_status, in Tools/QueueStatusServer/config/messages.py and a similarly named class variable in AbstractQueue (e.g. _skip_status).
2. Add a public member function to QueueStatus, say did_skip(), that returns true if and only if self.message ==  messages.skip_status

webkitpy changes:

1. Implement AbstractPatchQueue._did_skip() similar to how we implemented AbstractPatchQueue._did_pass() substituting self._skip_status for self._pass_status.
2. Modify AbstractEarlyWarningSystem.review_patch() to call AbstractPatchQueue._did_skip() and then return False when it catches the exception PatchIsNotApplicable.

Then we can update this code (in _should_show_bubble_for()) to read:

if not queue.is_ews():
    return False

status = attachment.status_for_queue(queue)
return bool(status and not status.did_skip())

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:29
> +class JSCTestResults(object):

This class needs to conform to the same interface as LayoutTestResults. What is the preferred way to do this in Python? One idea is to define an AbstractTestResults class that implements stub functions that raise an exception. Then have this class and LayoutTestResults extend AbstractTestResults and override the applicable/all methods.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:52
> +            _log.error("Invalid JSON results")

This patch alternatives between using double-quoted string literals and single-quoted string literals. From my understanding we prefer to use single-quoted string literals unless the string literal contains a single quote (then we use a double-quoted string literal). Regardless, we should pick one quoting style and stick with it throughout this patch.

> Tools/Scripts/webkitpy/common/net/jsctestresults.py:77
> +    def failing_test_results(self):
> +        return self.results
> +
> +    def test_results(self):
> +        return self.results
> +
> +    def failing_tests(self):
> +        return self.results

How did you come to the decision to return the same result for all of these functions?I know that we designed JSCTestResults to conform to the interface of LayoutTestResults but if methods are not applicable for the JSC EWS and later changes ensure we do not call these functions then I suggest we omit such functions. If we really need these functions then it seems weird that they are return the same result.

-- 
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/20170211/42eb60d5/attachment-0001.html>