[webkit-reviews] review granted: [Bug 201321] run-webkit-tests: Report results archive to results.webkit.org : [Attachment 378246] Patch

[bugzilla-daemon at webkit.org]

Aakash Jain <aakash_jain at apple.com> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 201321: run-webkit-tests: Report results archive to results.webkit.org
https://bugs.webkit.org/show_bug.cgi?id=201321

Attachment 378246: Patch

https://bugs.webkit.org/attachment.cgi?id=378246&action=review




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

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

r+ after the comments are addressed.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:294
> +		   for host_name in self._options.report_urls:

Should report_url also renamed to report_hostnames? Maybe in a separate patch.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:295
> +		       self._printer.write_update('Uploading to {}
...'.format(host_name))

should it be host_name or hostname?

> Tools/Scripts/webkitpy/results/upload.py:35
> +    ARCHIVE_UPLOAD_ENDPOINT = '/api/upload/archive'.format(UPLOAD_ENDPOINT)

seems like you missed removing .format

> Tools/Scripts/webkitpy/results/upload.py:170
> +    def upload(self, host, log_line_func=lambda val: sys.stdout.write(val +
'\n')):

what is better, host or hostname, maybe hostname to maintain consistency?

> Tools/Scripts/webkitpy/results/upload.py:171
>	   try:

we should probably check here that host is not None/empty, and print proper
error message, rather than not-so-user-friendly error about failed upload.

> Tools/Scripts/webkitpy/results/upload.py:188
> +    def upload_archive(self, host, archive, log_line_func=lambda val:
sys.stdout.write(val + '\n')):

Ditto for host vs hostname and validation.

> Tools/Scripts/webkitpy/results/upload_unittest.py:141
>  

We should probably add a unit-test with empty URL, after adding the
error-checking in the upload() method.