Bug 177205 – webkitpy: Ignore failure to get updated selenium version

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

When running unit tests, we update all required packages before running tests.  In automation, however, this has become problematic because asking pypi for an updated version every time we run unit tests is resulting in periodic network errors.  We should ignore these errors if selenium is already installed.

[Radar WebKit Bug Importer]

<rdar://problem/34531669>

[Jonathan Bedard]

Created attachment 321261 [details]
Patch

[Alexey Proskuryakov]

Is selenium the only package that is problematic?

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #3)
> Is selenium the only package that is problematic?

The short answer is yes.

The longer answer is that for other packages which we constantly update (Gecko and Chrome drivers, for example) we don't ask PyPi for the newest version, we check other URLs.  I presume that if we had other packages which checked PyPi for the newest version, we would encounter a similar problem.

[Alexey Proskuryakov]

Besides that fact that we don't know the root cause for these failures, here are a few things I'm unsure of:

1. What is the concrete reason to special case selenium failures here? If we were to add another package using the newest version, would we need to add it here? Having to list packages in two places is fragile.

2. Do we actually need to have selenium use the newest version? Why is it different?

3. It seems acceptable to ignore failures when the package is already installed, but what if the failure occurs on the first run, when it's not? Shouldn't we retry?

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #5)
> Besides that fact that we don't know the root cause for these failures, here
> are a few things I'm unsure of:
> 
> 1. What is the concrete reason to special case selenium failures here? If we
> were to add another package using the newest version, would we need to add
> it here? Having to list packages in two places is fragile.

Depends on how we are querying it's newest version.  At the moment, Selenium is the only package where we query PyPi for the latest version.  In other cases, we are consulting the project sites for the most recent version.  These sites aren't having the same flakiness problems that PyPI is having.
 
> 2. Do we actually need to have selenium use the newest version? Why is it
> different?

My understanding after talking with Stephanie about this is we are using Selenium to compare between browsers, we don't want to use an old version of Selenium because we want to be comparing the current behavior of browsers, rather than accidentally comparing historical behavior.
 
> 3. It seems acceptable to ignore failures when the package is already
> installed, but what if the failure occurs on the first run, when it's not?
> Shouldn't we retry?

We could retry, although, this will be a pretty rare case given that we would only trigger the retry logic the very first time a machine runs test-webkitpy.  I don't think a retry (in this case) is worth the trouble.

[Lucas Forschler]

Comment on attachment 321261 [details]
Patch

collecting irc comments:
Falling back to an existing version could mean we fall back to a version which is not supported. Instead, we should enforce a minimum version, and bail/warn if the installed version does not meet the requirements.

[Lucas Forschler]

Comment on attachment 321261 [details]
Patch

also, shouldn't the 'selenium' target be a directory, and not a file?  Maybe we should use os.path.isdir()

[Jonathan Bedard]

(In reply to Lucas Forschler from comment #8)
> Comment on attachment 321261 [details]
> Patch
> 
> also, shouldn't the 'selenium' target be a directory, and not a file?  Maybe
> we should use os.path.isdir()

I am checking for selenium/__init__.py.  In any case, that code will be changing to address what you mentioned in comment 7.

[Jonathan Bedard]

Created attachment 321453 [details]
Patch

[Lucas Forschler]

Comment on attachment 321453 [details]
Patch

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

> Tools/Scripts/webkitpy/thirdparty/__init__.py:182
> +                        break

We should consider writing a version check function, which would allow us to use this logic for other components, if we needed similar logic in the future. (one may already exist elsewhere in webkitpy)

> Tools/Scripts/webkitpy/thirdparty/__init__.py:185
> +            url = 'https://pypi.python.org/packages/ac/d7/1928416439d066c60f26c87a8d1b78a8edd64c7d05a0aa917fa97a8ee02d/selenium-3.5.0.tar.gz'

The other URLs include the hash, let's try to be consistent. 
Example: installer.install(url="http://pypi.python.org/packages/source/J/Jinja2/Jinja2-2.6.tar.gz#md5=1c49a8825c993bfdcf55bb36897d28a2",
                                                url_subpath="Jinja2-2.6/jinja2")

> Tools/Scripts/webkitpy/thirdparty/__init__.py:186
> +            url_subpath = '{}/selenium'.format(minimum_version)

Can you include the '-' here, so the subpath follows existing convention (selenium-3.5.0)

[Jonathan Bedard]

Created attachment 321478 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 321478 [details]
Patch for landing

Clearing flags on attachment: 321478

Committed r222352: <http://trac.webkit.org/changeset/222352>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.