Bug 218440

Summary:

Use FileDownload to fetch built product from master

Product:

WebKit

Reporter:

Angelos Oikonomopoulos <angelos>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aakash_jain, jbedard, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Angelos Oikonomopoulos 2020-11-02 05:37:06 PST

Use FileDownload to fetch built product from master

Comment 1 Angelos Oikonomopoulos 2020-11-02 05:39:06 PST

Created attachment 412907 [details]
Patch

Comment 2 Aakash Jain 2020-11-02 06:36:17 PST

unit-tests in TestDownloadBuiltProductFromMaster would need to be updated, it would become similar to TestUploadBuiltProduct.

Comment 3 Angelos Oikonomopoulos 2020-11-04 07:40:43 PST

Created attachment 413160 [details]
Patch

Comment 4 Radar WebKit Bug Importer 2020-11-09 05:38:12 PST

<rdar://problem/71187403>

Comment 5 Aakash Jain 2020-11-09 18:21:51 PST

Comment on attachment 413160 [details]
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'

Comment 6 Angelos Oikonomopoulos 2020-11-11 06:51:51 PST

Created attachment 413818 [details]
Patch

Comment 7 EWS 2020-11-11 07:53:23 PST

Committed r269683: <https://trac.webkit.org/changeset/269683>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413818 [details].