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

[bugzilla-daemon at webkit.org]

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

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #301437|review?                     |review-
              Flags|                            |

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

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

r-; with the exception of my second remark in comment 42 all other remarks in comment 42 were neither addressed in this patch nor replied to with a reason for disagreement. Please either address all of my remarks in comment 42 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.

> Tools/QueueStatusServer/handlers/statusbubble.py:190
> +        # EWS queues that weren't skipped are also shown when complete.

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).

> Tools/Scripts/webkitpy/common/config/ports.py:111
> +        if build_style == "debug":
> +            command.append("--debug")
> +        if build_style == "release":
> +            command.append("--release")

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().

> Tools/Scripts/webkitpy/common/config/ports.py:117
> +        command.append("--json-output=jsc_test_results.json")

Can we write this as two arguments?

> Tools/Scripts/webkitpy/common/config/ports.py:119
> +        if (build_style == "debug"):
> +            command.append("--debug")

How did you come to the decision to only support a debug build style?

> 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)
> +

Where are we invoking run_webkit_tests_command() with a non-None test_suite?

> Tools/Scripts/webkitpy/tool/bot/jsctestresultsreader.py:38
> +        except (IOError, KeyError):  # File does not exist or can't be read.

When is the exception KeyError raised? Unit tests?

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

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.

-- 
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/20170214/d01ecb9e/attachment.html>