Bug 174331 – Add setup script for Web Driver part of Benchmark Runner script

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Matthew Stewart]

Add setup script for Web Driver part of Benchmark Runner script

[Matthew Stewart]

*** Bug 174332 has been marked as a duplicate of this bug. ***

[Matthew Stewart]

*** Bug 174335 has been marked as a duplicate of this bug. ***

[Matthew Stewart]

Created attachment 315054 [details]
Patch

[Dean Johnson]

Comment on attachment 315054 [details]
Patch

Nice patch Matthew! I think you may have done a little more work than would be required for this, though. Have you had a chance to look into "autoinstalled" module in webkitpy? Essentially, you just define a very short function containing a URL to download a package from along with what the package name should be, and if you do `from webkitpy.autoinstalled.thirdparty import selenium` it would automatically download, install and source the import for you!

You can see an example of this running `git grep mechanize` from OpenSource/Tools/Scripts/webkitpy:
common/net/bugzilla/bugzilla.py:            from webkitpy.thirdparty.autoinstalled.mechanize import Browser.

Feel free to follow-up if you have any questions!

[Dean Johnson]

Comment on attachment 315054 [details]
Patch

Nice patch Matthew! I think you may have done a little more work than would be required for this, though. Have you had a chance to look into "autoinstalled" module in webkitpy? Essentially, you just define a very short function containing a URL to download a package from along with what the package name should be, and if you do `from webkitpy.autoinstalled.thirdparty import selenium` it would automatically download, install and source the import for you!

You can see an example of this running `git grep mechanize` from OpenSource/Tools/Scripts/webkitpy:
common/net/bugzilla/bugzilla.py:            from webkitpy.thirdparty.autoinstalled.mechanize import Browser.

Feel free to follow-up if you have any questions!

[Stephanie Lewis]

Comment on attachment 315054 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/__init__.py:29
>  

This needs to have a proper error message.

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:15
> +

Constants should be uppercase

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:39
> +        if subprocess.call(['curl', '--silent', '--location', '--remote-name', 'https://bootstrap.pypa.io/get-pip.py'], shell=False):

Check with Dean but I believe we should be installing pip using easy_install.  This won't work in the lab due to the firewalls.

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:52
> +        output = subprocess.check_output(['chromedriver', '--version']).strip()

Lets break the version check out into its own function.  Bonus points if firefox and chrome can use the same function.

> Tools/Scripts/webkitpy/benchmark_runner/setup.py:128
> +        print("Note: This script may not put the binaries in the correct place\nYou'll have to move them manually")

Where will it put them and where do they need to be.  Let's make these instructions detailed.

[Build Bot]

Comment on attachment 315054 [details]
Patch

Attachment 315054 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4096716

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315059 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Matthew Stewart]

Created attachment 315508 [details]
Patch

[Stephanie Lewis]

Comment on attachment 315508 [details]
Patch

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

Looks good

> Tools/Scripts/webkitpy/thirdparty/__init__.py:180
> +            open(os.path.join(directory, '__init__.py'), 'w+')

This probably needs to be closed

[Dean Johnson]

Comment on attachment 315508 [details]
Patch

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

Great work so far!

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:22
> +driver_executable = os.path.join(os.path.dirname(os.path.realpath(driver_init_file)), 'chromedriver')

Can you adopt this for geckodriver and chromedriver above, too? This seems very concise!

> Tools/Scripts/webkitpy/thirdparty/__init__.py:173
> +        version = subprocess.check_output(['curl', '--silent', CHROME_DRIVER_URL + "LATEST_RELEASE"]).strip()

We should use urllib2 here instead of subprocess. urllib2.urlopen(url).read() I believe should be all that's needed.

It may also be useful to have a function called install_binary (without a prepended _ to avoid getting captured in autoinstall_everything()), which you pass a url and it will download the binary and initialize it as a mock python object.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:174
> +        filename_postfix = self.get_driver_filename()[0]

Nit, instead of using indices, consider using _ as a placeholder like so:
filename_postfix, _ = self.get_driver_filename()

> Tools/Scripts/webkitpy/thirdparty/__init__.py:186
> +        filename_postfix = self.get_driver_filename()[1]

Ditto to _ comment.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:192
> +            open(os.path.join(directory, '__init__.py'), 'w+')

I think utilizing install_binary will reduce much of the duplication here.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:200
> +        import json

Nit: These imports can be done at the top of the file.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:202
> +        response = urlopen(json_url)

Nit: Usually only urllib2 functions all called from urllib2 rather than being imported directly. Example: urllib2.urlopen.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:208
> +    def get_driver_filename(self):

We should put this outside of this class since it's not dependent on self (except for get_os_info).

I would recommend rewriting this function for conciseness / flatness as follows:
def get_os_info():
  (os_name, os_type) = get_os_info()
  chrome_os, filefox_os = 'unsupported', 'unsupported'
  if os_name == 'Linux' and os_type == '64':
    chrome_os, firefox_os = 'linux64', 'linux64'
  elif os_name == 'Linux':
    chrome_os, firefox_os = 'linux32', 'linux32'
  elif os_name == 'Darwin':
    chrome_os, firefox_os = 'mac64', 'macos'
  return (chrome_os, firefox_os)

> Tools/Scripts/webkitpy/thirdparty/__init__.py:224
> +    def get_os_info(self):

Ditto to get_driver_filename. We should put this outside of this class.

[Matthew Stewart]

Created attachment 315857 [details]
Add functions to autoinstaller needed for Benchmark Runner script

Comment on attachment 315857 [details]
Add functions to autoinstaller needed for Benchmark Runner script

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/linux_chrome_driver.py:28
> +import webkitpy.thirdparty.autoinstalled.chromedriver

As we discussed in person, we should avoid installing those binaries every time we call run-benchmark script.
A lazy installation is preferred.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_chrome_driver.py:22
> +driver_executable = os.path.join(os.path.dirname(os.path.realpath(driver_init_file)), 'chromedriver')

Maybe worth abstract this function into utils.py since this has been called multiple times.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:33
> +import json

Nit: sort import by alphabet.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:170
> +        (url, url_subpath) = self.get_latest_pypi_url('selenium')

No need to change, but worth to mention that braces are not necessary here.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:226
> +def get_driver_filename():
> +    os_name, os_type = get_os_info()
> +    chrome_os, filefox_os = 'unsupported', 'unsupported'
> +    if os_name == 'Linux' and os_type == '64':
> +        chrome_os, firefox_os = 'linux64', 'linux64'
> +    elif os_name == 'Linux':
> +        chrome_os, firefox_os = 'linux32', 'linux32'
> +    elif os_name == 'Darwin':
> +        chrome_os, firefox_os = 'mac64', 'macos'
> +    return (chrome_os, firefox_os)

Would it better to become a function that takes an argument and returns the platform string base on the argument?
This could make it less coupled. Otherwise, we are making an assumption on the order of returned tuple.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:232
> +    return (os_name, os_type)

No need to change, but worth to mention that braces are not necessary here.

[Matthew Stewart]

Created attachment 316011 [details]
patch

Comment on attachment 316011 [details]
patch

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

> Tools/ChangeLog:6
> +        Reviewed by Dean Johnson.

r=me

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/browser_driver.py:38
> +    def get_webdriver_binary_path(self):
> +        return get_driver_binary_path(self.browser_name)

This looks good enough. It would even better if we use @property here since it makes the code cleaner. Then rather than calling it get_webdriver_binary_path, we can just call it webdriver_binary_path,

Comment on attachment 316011 [details]
patch

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

> Tools/Scripts/webkitpy/thirdparty/__init__.py:226
> +    return (chrome_os, firefox_os)

Using namedtuple would improve the readability of this code and get rid of the assumption we made on the indices of the return value.
DriverFilenameForBrowser = namedtuple('DriverFilenameForBrowser', ['chrome', 'firefox'])
return DriverFilenameForBrowser((chrome_os, firefox_os))
And update the use of this function's return value accordingly.

[Matthew Stewart]

Created attachment 316116 [details]
patch

[WebKit Commit Bot]

Comment on attachment 316116 [details]
patch

Rejecting attachment 316116 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 316116, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
tpy/benchmark_runner/browser_driver/osx_firefox_driver.py.rej
patching file Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py
patching file Tools/Scripts/webkitpy/benchmark_runner/utils.py
patching file Tools/Scripts/webkitpy/common/system/autoinstall.py
patching file Tools/Scripts/webkitpy/thirdparty/__init__.py

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Stephanie Lewis']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/4181297

[Matthew Stewart]

Created attachment 316333 [details]
patch

[WebKit Commit Bot]

Comment on attachment 316333 [details]
patch

Clearing flags on attachment: 316333

Committed r219853: <http://trac.webkit.org/changeset/219853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.