Bug 184920 – Added Support for a Build Binaries Fetcher in Run WebKit Tests via a new flag `--download-binaries`

• Component: Tools / Tests
• Status: NEW

[Amal Hussein]

Added a Build Binaries Fetcher in Run WebKit Tests via a new flag `--download-binaries`

[Amal Hussein]

Created attachment 338656 [details]
Patch

[Amal Hussein]

unit tests are coming - I still need to make some changes to them.

[Alexey Proskuryakov]

Comment on attachment 338656 [details]
Patch

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

> Tools/ChangeLog:3
> +        Added a Build Binaries Fetcher in Run WebKit Tests via a new flag `--download-binaries`

What is the rationale for having this feature? Having many ways to achieve the same result is not great, as it increases potential for confusion.

[Jonathan Bedard]

I would prefer if we just made download-built-product (or a script like it) fit for human usage and move it to scripts.

[Amal Hussein]

(In reply to Jonathan Bedard from comment #4)
> I would prefer if we just made download-built-product (or a script like it)
> fit for human usage and move it to scripts.

Thanks for the feedback Jonathan. Your suggestion of moving this to a standalone script under Tools/Scripts makes sense. 

There is value in integrating the script with the test runner via a flag, so I assume that hook would stay there but reference the renamed standalone script.

Is that also what you were thinking?

[Alexey Proskuryakov]

No, the suggestion is not not have the feature exposed through run-webkit-tests at all. You say that there is value in integrating support, but what is this value? Do you also intend to add such support to run-javascriptcore-tests, run-api-tests, and to other test scripts?

[youenn fablet]

(In reply to Alexey Proskuryakov from comment #6)
> No, the suggestion is not not have the feature exposed through
> run-webkit-tests at all. You say that there is value in integrating support,
> but what is this value? Do you also intend to add such support to
> run-javascriptcore-tests, run-api-tests, and to other test scripts?

That is indeed something that we could consider.
The fact that this is encapsulated in BuildBinariesFetcher should help that.

We need BuildBinariesFetcher to easily rebase WPT tests.
For this specific purpose, there is no hard requirement to add it to run-webkit-tests since we need to do other stuff before and after.

I believe though that this would be a handy option for users of run-webkit-tests.

[Alexey Proskuryakov]

Comment on attachment 338656 [details]
Patch

I think that I'm going to say r- at this point. It doesn't sound like there is any huge reason to create a toaster/fridge combo here.

[Amal Hussein]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 338656 [details]
> Patch
> 
> I think that I'm going to say r- at this point. It doesn't sound like there
> is any huge reason to create a toaster/fridge combo here.

Alexey - can you elaborate on your points of concern?

[Amal Hussein]

Will be moving this to a standalone script here https://bugs.webkit.org/show_bug.cgi?id=185045

[Alexey Proskuryakov]

> Alexey - can you elaborate on your points of concern?

I think that I already did.