[webkit-reviews] review granted: [Bug 218440] Use FileDownload to fetch built product from master : [Attachment 413160] Patch

[bugzilla-daemon at webkit.org]

Aakash Jain <aakash_jain at apple.com> has granted Angelos Oikonomopoulos
<angelos at igalia.com>'s request for review:
Bug 218440: Use FileDownload to fetch built product from master
https://bugs.webkit.org/show_bug.cgi?id=218440

Attachment 413160: Patch

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




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

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

Patch looks good. various minor comments.

Also you can remove EWS_BUILD_URL variable, it's unused after this patch.

> Tools/CISupport/ews-build/steps.py:2443
> +class DownloadBuiltProductFromMaster(transfer.FileDownload):

Please add step name, e.g.:
name = 'download-built-product-from-master'

> Tools/CISupport/ews-build/steps.py:2458
>      def getResultSummary(self):

Nit: missing empty line before method.

> Tools/CISupport/ews-build/steps_unittest.py:211
> +def downloadFileRecordingContents(recorder):

Maybe it's better to keep this method inside TestDownloadBuiltProductFromMaster
class since it's used only there.

> Tools/CISupport/ews-build/steps_unittest.py:2601
> +	   self.setupStep(step)

Can write this as one line:
self.setupStep(DownloadBuiltProductFromMaster(mastersrc=__file__))

> Tools/CISupport/ews-build/steps_unittest.py:2621
> +	   buf = b''.join(buf)

is 'b' required in b''?

> Tools/CISupport/ews-build/steps_unittest.py:2622
> +	   self.assertEqual(len(buf), 1000)

Can store this 1000 in a variable and re-use that variable.

> Tools/CISupport/ews-build/steps_unittest.py:2623
> +	   with open(__file__, 'rb') as mf:

can improve the variable name for 'mf'